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

[Eero Tamminen <oak@xxxxxxxxxxxxxx>]

Hi,

On 10/13/18 12:33 PM, Uwe Seimet wrote:
> I see. I'm not sure, though, whether I should allow read access to ROM
> or not. Of course, trying to read something from a device and store it in
> a ROM address does not make sense. But in practice you can nevertheless
> do it. Should I allow reading data to ROM addresses in order to be more
> compatible with real hardware? I would expect a crash (of TOS, not
> Hatari), but this is what also would most likely happen with real
> hardware.

For example, a TOS ROM saving application:
* Should be able to read Atari ROM memory area and save it to hard disk
* should not be able to read ROM file from disk and overwrite ROM memory
  area (as that's not writable on real device).

I.e. memory check for disk writes should allow both RAM & ROM area
to be given as address, but for disk reads only RAM access is allowed.


	- Eero

> Best regards
>
> Uwe
>
> > 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