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

[Eero Tamminen <oak@xxxxxxxxxxxxxx>]

Hi,

On 10/13/18 11:35 AM, Uwe Seimet wrote:
> 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.

It needs to be checked also in write case. They can crash Hatari
if write comes from an address that isn't mapped by Hatari to
emulated Atari RAM (address that isn't mapped to Hatari's process
at all).


	- Eero

> 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