Re: [hatari-devel] Disk driver memory access checks (was: Cycle exact setting affects nf_scsidrv)

[Uwe Seimet <Uwe.Seimet@xxxxxxxxx>]

Hi,

I'm not sure whether a modified patch is required. This is the patched
code:

        if ( !dir && !STMemory_CheckAreaType ( st_buffer, transfer_len, ABFLAG_RAM ) )
        {
 

!dir means read access, i.e. in case of a write operation nothing is checked at
all, meaning any source address is allowed for writes.

Best regards

Uwe

> Hi,
> 
> On 10/13/18 10:15 AM, Uwe Seimet wrote:
> > 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
> 
> Thanks!
> 
> 
> > Note that writing is allowed, it does not have to be a RAM address in
> > this case.
> 
> If you mean that write could also be from ROM area, you need to use:
> 	ABFLAG_RAM | ABFLAG_ROM
> 
> (see what the other disk drivers do.)
> 
> New patch?
> 
> 
> 	- Eero
> 
> Ps. Thomas, I didn't see Hatari IDE driver doing these checks, but on
> quick check, I didn't find it doing any buffer transfers, only
> byte/word/long register reads & writes...?
> 
> > 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
> >>
> >>
> 
>