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);
+    }
+    return cur_drv;
 }
 
 /* Status A register : 0x00 (read-only) */
-- 
2.31.1

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) */

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!