Re: [hatari-devel] Cycle exact setting affects nf_scsidrv

[Uwe Seimet <Uwe.Seimet@xxxxxxxxx>]

Hi,

Please see the attachment. This is the result in case of an invalid RAM
address when reading:

scsidrv_inout: handle=0, dir=0, cmd_len=10, buffer=0x5632a9f5c9e0,
               transfer_len=65536, sense_buffer=0x5632a91dbfb0, timeout=10000,
               cmd=$28:$00:$00:$00:$00:$00:$00:$00:$80:$00ERROR: pBank flags mismatch: 0x2 & 0x1 (RAM = 0x1)
WARN : scsidrv_interface_fetures: Invalid RAM range 0xe00000+65536
 -> -1

Note that writing is allowed, it does not have to be a RAM address in
this case.

Best regards

Uwe

> Hi,
> 
> On 10/12/18 12:25 AM, Uwe Seimet wrote:
> >> Is st_buffer data something that could be program code read from
> >> the SCSI disk?  If yes, all caches would need to be flushed for that.
> > 
> > Yes, you are probably right. Just checked that I'm also flushing both in
> > HDDRIVER. One more updated patch attached. The sense data buffer and
> > the buffer for the bus name do not need flushing the instruction cache
> > because you cannot transfer program code this way.
> 
> Thanks, I pushed you patch:
> https://hg.tuxfamily.org/mercurialroot/hatari/hatari/rev/d6355bb2dc25
> 
> When checking that, I noticed that nf_scsidrv.c doesn't validate
> that the memory addresses specified through the NF API are valid,
> like GEMDOS HD driver does:
> -----------------------------
>          /* Check that read is to valid memory area */
>          if ( !STMemory_CheckAreaType ( Addr, Size, ABFLAG_RAM ) )
>          {
>                  Log_Printf(LOG_WARN, "GEMDOS Fread() failed due to 
> invalid RAM range 0x%x+%i\n", Addr, Size);
>                  Regs[REG_D0] = GEMDOS_ERANGE;
>                  return true;
>          }
> -----------------------------
> 
> Could you send also a (tested) patch that adds those checks?
> 
> 
> 	- Eero
> 
> 

diff -r 07fb0f2a18e0 src/nf_scsidrv.c
--- a/src/nf_scsidrv.c	Sat Oct 13 01:12:47 2018 +0300
+++ b/src/nf_scsidrv.c	Sat Oct 13 09:14:02 2018 +0200
@@ -179,6 +179,12 @@
 	Uint32 features = read_stack_long(&stack);
 	Uint32 transferLen = read_stack_long(&stack);
 
+        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);
@@ -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,22 @@
 		}
 	}
 
+        if ( !dir && !STMemory_CheckAreaType ( st_buffer, transfer_len, ABFLAG_RAM ) )
+        {
+            Log_Printf(LOG_WARN, "scsidrv_interface_features: 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)
 	{