Re: [PATCH] fdc: check drive block device before usage (CVE-2021-20196)

[John Snow]

On 5/18/21 5:01 AM, P J P wrote:
>    Hello John,
>
> +-- On Mon, 17 May 2021, John Snow wrote --+
> | >       /* Selected drive */
> | > -    fdctrl->cur_drv = value & FD_DOR_SELMASK;
> | > +    if (fdctrl->drives[value & FD_DOR_SELMASK].blk) {
> | > +        fdctrl->cur_drv = value & FD_DOR_SELMASK;
> | > +    }
> |
> | I don't think this is correct. If you look at get_cur_drv(), it uses the
> | TDR_BOOTSEL bit to change the logical mappings of "drive 0" or "drive 1" to be
> | reversed. You don't check that bit here, so you might be checking the wrong
> | drive.
> |
> | Plus, the TDR bit can change later, so I think you shouldn't actually protect
> | the register write like this. Just delete this bit of code. We ought to
> | protect the drives when we go to use them instead of preventing the registers
> | from getting "the wrong values".
>
> * I see.
>
> | >       cur_drv = get_cur_drv(fdctrl);
> | > +    if (!cur_drv->blk) {
> | > +        FLOPPY_DPRINTF("No drive connected\n");
> | > +        return 0;
> | > +    }
> |
> | This seems fine ... or at least not worse than the other error handling we
> | already have here. (Which seems to be ... basically, none. We just ignore the
> | write and do nothing, which seems wrong. I guess it's better than a crash...
> | but I don't have the time to do a proper audit of what this is SUPPOSED to do
> | in this case.)
> |
> | > -            if (blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
> | > +            if (cur_drv->blk == NULL
> | > +                || blk_pwrite(cur_drv->blk, fd_offset(cur_drv),
> | > fdctrl->fifo,
> |
> | Seems fine, but if we had a drive for the earlier check, will we really be in
> | a situation where we don't have one now?
> |
> | Ignore the bit I sent earlier about the qtest reproducer not correlating to
> | this patch -- it does, I was experiencing an unrelated crash.
>
> * Okay.
>
>
> | On 5/17/21 7:12 AM, P J P wrote:
> | > Do we need a revised patch[-series] including a qtest? OR it can be done at
> | > merge time?
> |
> | If you have the time to write a qtest reproducer, you can send it separately
> | and I'll pick it up if everything looks correct.
>
> * Yes, that seems better, I'll try to create a qtest, but it may take time.

I minified alexander's reproducer, which uses as few writes and as few 
bits as possible to achieve the same crash. It makes it easier to see 
what's going on with the DPRINTF/traces a little better.

outb 0x3f2 0x04
outb 0x3f4 0x03

outb 0x3f5 0x25
outb 0x3f5 0x01
outb 0x3f5 0x0
outb 0x3f5 0x0
outb 0x3f5 0x0
outb 0x3f5 0x0
outb 0x3f5 0x00
outb 0x3f5 0x00
outb 0x3f5 0x01

outb 0x3f3 0x04
outb 0x3f5 0x0


Annotated:

# fdctrl->cur_drv starts at 0x00
# fdctrl->dor starts at 0x0c (DMA, RESET#)
# fdctrl->dsr starts at 0x00

> outb 0x3f2 0x04
fdc_ioport_write write reg 0x02 [DOR] Digital Output Register val 0x04
  DOR changed from default after SeaBIOS init from 0x0c to 0x04
  DMA GATE# (0x08) set from 1 --> 0
  DMA GATE# appears needed to coerce fdc into a "non-dma transfer".
  +RESET# remains on. Needed to avoid engaging RESET routine.

> outb 0x3f4 0x03
fdc_ioport_write write reg 0x04 [DSR] Date Rate Select Register val 0x03
  DSR: +DRATE SEL1
  DSR: +DRATE SEL0
  Needed to prevent "data rate mismatch" error handling by write cmd.

The next 9 bytes (all to 0x3f5) set up the write command.

0x25 selects the "Write (BeOS)" command.
0x01 selects drive1.
...
0x01 appears to say that a sector is "1 byte", but oddly enough no other 
value seems to trigger this crash. Not sure why. Recommend investigating 
if you have time. Could be transfer length calculation bug.

> outb 0x3f3 0x04
fdc_ioport_write write reg 0x03 [TDR] Tape Drive Register val 0x04
    TDR: +BOOTSEL
    This changes the meaning of cur_drv and flips selection (as far as 
I can tell) back to drive0 instead of the command's programmed drive1.

> outb 0x3f5 0x00
fdc_ioport_write write reg 0x05 [FIFO] Data val 0x00
    write is attempted on "drv1" which due to BOOTSEL maps back to 
"drv0", which is undefined.


This should (I hope) help guide to write a more targeted patch and a 
good qtest case.

--js

> * I'll check and revise the patch with above details asap.
>
>
> Thank you.
> --
>   - P J P
> 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D