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

[P J P]

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