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

[Eero Tamminen <oak@xxxxxxxxxxxxxx>]

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