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

[Uwe Seimet <Uwe.Seimet@xxxxxxxxx>]

Hi,

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.

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
> >>>>
> >>>>
> >>
> >>
> > 
> > 
> > 
> 
>