Re: [hatari-devel] Disk driver memory access checks

[Uwe Seimet <Uwe.Seimet@xxxxxxxxx>]

Hi,

> The main thing is to protect the emulation process i.e. prevent
> Hatari itself doing invalid memory access.
> 
> What else driver does in such a corner case is less of a concern
> from my point of view.  For now, you could log an error and
> later once it's checked on real HW, add proper error response.

Thinking more about this, even with real hardware there is no
well-defined behavior. Even with SCSI/ACSI DMA you often also need PIO,
or data transferred via DMA have to be copied to their final address, e.g.
for SCSI transfers to/from ST-RAM and for transfers to/from odd addresses.
Only for IDE it's always PIO, at least on original Atari machines. The
Milan supports IDE DMAE, but there is no DMA-capable IDE driver for it.

The attached patch allows writes from RAM and ROM and reads to RAM only.
This should be fine, at least it does not contract any documented behavior.

Best regards

Uwe

diff -r 956ad0c30a6e src/nf_scsidrv.c
--- a/src/nf_scsidrv.c	Sat Oct 13 12:12:53 2018 +0300
+++ b/src/nf_scsidrv.c	Sat Oct 13 19:02:29 2018 +0200
@@ -179,13 +179,19 @@
 	Uint32 features = read_stack_long(&stack);
 	Uint32 transferLen = read_stack_long(&stack);
 
+	LOG_TRACE(TRACE_SCSIDRV, "scsidrv_interface_features: busName=%s, features=$%04x, transferLen=%d", BUS_NAME, BUS_FEATURES, BUS_TRANSFER_LEN);
+
+        if ( !STMemory_CheckAreaType ( st_bus_name, 20, ABFLAG_RAM ) )
+        {
+            Log_Printf(LOG_WARN, "scsidrv_interface_features: Invalid RAM range 0x%x+%i\n", st_bus_name, 20);
+            return -1;
+        }
+
 	strncpy(busName, BUS_NAME, 20);
         M68000_Flush_Data_Cache(st_bus_name, 20);
 	write_word(features, BUS_FEATURES);
 	write_long(transferLen, BUS_TRANSFER_LEN);
 
-	LOG_TRACE(TRACE_SCSIDRV, "scsidrv_interface_features: busName=%s, features=$%04x, transferLen=%d", busName, BUS_FEATURES, BUS_TRANSFER_LEN);
-
 	return 0;
 }
 
@@ -296,15 +302,9 @@
 	Uint32 transfer_len = read_stack_long(&stack);
         Uint32 st_sense_buffer = STMemory_ReadLong(stack);
 	unsigned char *sense_buffer = read_stack_pointer(&stack);
-	Uint32 timeout;
+	Uint32 timeout = read_stack_long(&stack);
 	int status;
 
-	if (sense_buffer)
-	{
-		memset(sense_buffer, 0, 18);
-	}
-	timeout = read_stack_long(&stack);
-
 	if (LOG_TRACE_LEVEL(TRACE_SCSIDRV))
 	{
 		LOG_TRACE_PRINT(
@@ -323,11 +323,24 @@
 		}
 	}
 
+        // Writing is allowed with a RAM or ROM address,
+        // reading requires a RAM address
+        if ( !STMemory_CheckAreaType ( st_buffer, transfer_len, dir ? ABFLAG_RAM | ABFLAG_ROM : ABFLAG_RAM ) )
+        {
+            Log_Printf(LOG_WARN, "scsidrv_inout: Invalid RAM range 0x%x+%i\n", st_buffer, transfer_len);
+            return -1;
+        }
+
 	if (handle >= SCSI_MAX_HANDLES || !handle_meta_data[handle].fd)
 	{
 		return GEMDOS_ENHNDL;
 	}
 
+	if (sense_buffer)
+	{
+		memset(sense_buffer, 0, 18);
+	}
+
 	// No explicit LUN support, the SG driver maps LUNs to device files
 	if (cmd[1] & 0xe0)
 	{