From bb08b4b7b586fa9648ae62e3b411d48b7f3d9cf9 Mon Sep 17 00:00:00 2001
From: Jonne Ransijn <jonne@yyny.dev>
Date: Sat, 7 Jun 2025 13:26:01 +0200
Subject: [PATCH] Add bounds checking to `.System/expansion` commands

Unchecked `.System/expansion` commands can cause segmentation faults.

We should of course reject banks >= `RAM_PAGES`, but we also have to account
for addresses pointing past the end of the (last) bank. For this, we have
several options:

 - Clamp the address to the end of the memory.
 - Clamp the address to the end of the bank.
 - Wrap the address to stay in the same bank.

I chose wrapping to make expansion banks easiest to implement on
16-bit machines and when memory banks are not contiguous.
Wrapping is also how `STA` and `LDA` are implemented.

Since out-of-bounds banks are now ignored. ROMs can write a byte to a
memory bank and read it back to detect if the bank exists.

This also fixes an unintuitive off-by-one in the `cpyr` command that caused
the `system.expansion.tal` example to break.
---
 src/devices/system.c | 41 +++++++++++++++++++++++++++--------------
 src/uxn.h            |  1 +
 2 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/src/devices/system.c b/src/devices/system.c
index e41daa4..17101e6 100644
--- a/src/devices/system.c
+++ b/src/devices/system.c
@@ -14,6 +14,8 @@ THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
 WITH REGARD TO THIS SOFTWARE.
 */
 
+#define PAGE_INDEX(bank, addr) ((bank) * PAGE_SIZE + ((addr) & PAGE_MASK))
+
 char *boot_path;
 
 static void
@@ -84,26 +86,37 @@ system_deo(Uint8 port)
 {
 	switch(port) {
 	case 0x3: {
-		Uint16 value;
 		Uint16 addr = PEEK2(uxn.dev + 2);
 		Uint8 *aptr = uxn.ram + addr;
 		Uint16 length = PEEK2(aptr + 1);
 		if(uxn.ram[addr] == 0x0) {
-			unsigned int a = PEEK2(aptr + 3) * PAGE_SIZE + PEEK2(aptr + 5);
-			unsigned int b = a + length;
-			value = uxn.ram[addr + 7];
-			for(; a < b; uxn.ram[a++] = value);
+			unsigned int src_bank = PEEK2(aptr + 3);
+			unsigned int src_addr = PEEK2(aptr + 5);
+			Uint16 value = uxn.ram[addr + 7];
+			if (src_bank < RAM_PAGES) {
+				unsigned int a = src_addr;
+				unsigned int b = a + length;
+				for(; a < b; uxn.ram[PAGE_INDEX(src_bank, a++)] = value);
+			}
 		} else if(uxn.ram[addr] == 0x1) {
-			unsigned int a = PEEK2(aptr + 3) * PAGE_SIZE + PEEK2(aptr + 5);
-			unsigned int b = a + length;
-			unsigned int c = PEEK2(aptr + 7) * PAGE_SIZE + PEEK2(aptr + 9);
-			for(; a < b; uxn.ram[c++] = uxn.ram[a++]);
+			unsigned int src_bank = PEEK2(aptr + 3);
+			unsigned int src_addr = PEEK2(aptr + 5);
+			unsigned int dst_bank = PEEK2(aptr + 7);
+			unsigned int dst_addr = PEEK2(aptr + 9);
+			if (src_bank < RAM_PAGES && dst_bank < RAM_PAGES) {
+                unsigned int src_last = src_addr + length;
+				for(; src_addr < src_last; uxn.ram[PAGE_INDEX(dst_bank, dst_addr++)] = uxn.ram[PAGE_INDEX(src_bank, src_addr++)]);
+			}
 		} else if(uxn.ram[addr] == 0x2) {
-			unsigned int a = PEEK2(aptr + 3) * PAGE_SIZE + PEEK2(aptr + 5);
-			unsigned int b = a + length;
-			unsigned int c = PEEK2(aptr + 7) * PAGE_SIZE + PEEK2(aptr + 9);
-			unsigned int d = c + length;
-			for(; b >= a; uxn.ram[--d] = uxn.ram[--b]);
+			unsigned int src_bank = PEEK2(aptr + 3);
+			unsigned int src_addr = PEEK2(aptr + 5);
+			unsigned int dst_bank = PEEK2(aptr + 7);
+			unsigned int dst_addr = PEEK2(aptr + 9);
+			if (src_bank < RAM_PAGES && dst_bank < RAM_PAGES) {
+				unsigned int src_last = src_addr + length;
+				unsigned int dst_last = dst_addr + length;
+				for(; src_last > src_addr; uxn.ram[PAGE_INDEX(dst_bank, --dst_last)] = uxn.ram[PAGE_INDEX(src_bank, --src_last)]);
+			}
 		} else
 			fprintf(stderr, "Unknown Expansion Command 0x%02x\n", uxn.ram[addr]);
 		break;
diff --git a/src/uxn.h b/src/uxn.h
index 1b7e660..a8b174d 100644
--- a/src/uxn.h
+++ b/src/uxn.h
@@ -19,6 +19,7 @@ WITH REGARD TO THIS SOFTWARE.
 #define STEP_MAX 0x80000000
 #define PAGE_PROGRAM 0x0100
 #define PAGE_SIZE 0x10000
+#define PAGE_MASK 0xffff
 
 typedef unsigned char Uint8;
 typedef signed char Sint8;