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.

(I know this is extremely backwards from how good code ought to be 
written where we centralize protecting sane values from becoming object 
state.)

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

(Which, to be clear, is 100% OK by me for this patch.)

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

Understand. Maybe I can help. The fuzzer reproducer is a great first 
step, but just needs to be "back-translated" into the logical operations 
it is performing so that the test code is readable.

I started doodling a tracer patch similar to the IDE one I checked in 
some ages ago to give symbolic names to the registers on read/write, 
which makes "reading" Alexander's fuzzing reproducers a bit easier.

I'll go work on that for a little while.

> * I'll check and revise the patch with above details asap.

OK; on your own schedule. I will try to leap on the patches as soon as I 
get them before the FDC code falls out of my head again.

If at all possible, I wouldn't mind seeing a series bundled with the 
other FDC fixes outstanding aggregated together. It will be easier (for 
me) to make sure I have everything up to date and together. (If it isn't 
too much hassle for you.)

AFAIK there's one for reads and one for writes that are very similar -- 
they protect against null BLK reads when we do not have a floppy drive 
present.

Thank you,
--js

> Thank you.
> --
>   - P J P
> 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D