Re: [PATCH-for-6.2 v3 1/2] hw/block/fdc: Kludge missing floppy drive to fix CVE-2021-20196

[Philippe Mathieu-Daudé]

On 11/23/21 14:33, Hanna Reitz wrote:
> On 18.11.21 13:06, Philippe Mathieu-Daudé wrote:
>> Guest might select another drive on the bus by setting the
>> DRIVE_SEL bit of the DIGITAL OUTPUT REGISTER (DOR).
>> The current controller model doesn't expect a BlockBackend
>> to be NULL. A simple way to fix CVE-2021-20196 is to create
>> an empty BlockBackend when it is missing. All further
>> accesses will be safely handled, and the controller state
>> machines keep behaving correctly.
>>
>> Cc: qemu-stable@nongnu.org
>> Fixes: CVE-2021-20196
>> Reported-by: Gaoning Pan (Ant Security Light-Year Lab) <pgn@zju.edu.cn>
>> BugLink: https://bugs.launchpad.net/qemu/+bug/1912780
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/338
>> Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   hw/block/fdc.c | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>> index d21b717b7d6..6f94b6a6daa 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -1161,7 +1161,19 @@ static FDrive *get_drv(FDCtrl *fdctrl, int unit)
>>     static FDrive *get_cur_drv(FDCtrl *fdctrl)
>>   {
>> -    return get_drv(fdctrl, fdctrl->cur_drv);
>> +    FDrive *cur_drv = get_drv(fdctrl, fdctrl->cur_drv);
>> +
>> +    if (!cur_drv->blk) {
>> +        /*
>> +         * Kludge: empty drive line selected. Create an anonymous
>> +         * BlockBackend to avoid NULL deref with various BlockBackend
>> +         * API calls within this model (CVE-2021-20196).
>> +         * Due to the controller QOM model limitations, we don't
>> +         * attach the created to the controller device.
>> +         */
>> +        cur_drv->blk = blk_new(qemu_get_aio_context(), 0, BLK_PERM_ALL);
> 
> So to me this looks basically like a mini version of
> floppy_drive_realize(), and I was wondering what else we might want to
> use from that function.  fd_init() and fd_revalidate() look interesting,
> but it appears that fdctrl_realize_common() already did that for all
> drives so we should be good.

How the controller / bus / floppy / drive are connected is a bit
confusing. Did you ever try to hot-remove the magnetic medium from
the floppy plastic enclosure while the controller rotates it?

> Then again, fd_revalidate() behaves differently for the initial drv->blk
> == NULL (drv->drive is set to TYPE_NONE, and last_sect and max_track are
> set to 0) and for then later !blk_is_inserted() (drv->drive not changed
> (so I guess it stays TYPE_NONE?), but last_sect and max_track are set to
> 0xff).  Not sure if that’s a problem.  Probably not, given that I think
> drv->disk and drv->drive both stay TYPE_NONE.

I'm not sure about the future plans for this device model...

> Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Thanks!