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

[Hanna Reitz]

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.

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.

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

> +    }
> +    return cur_drv;
>   }
>   
>   /* Status A register : 0x00 (read-only) */