[PATCH v4 0/3] hw/block/fdc: Fix CVE-2021-20196

[PATCH v4 0/3] hw/block/fdc: Fix CVE-2021-20196

[Philippe Mathieu-Daudé]

Since v3:
- Preliminary extract blk_create_empty_drive()
- qtest checks qtest_check_clang_sanitizer() enabled
- qtest uses null-co:// driver instead of file

Philippe Mathieu-Daudé (3):
  hw/block/fdc: Extract blk_create_empty_drive()
  hw/block/fdc: Kludge missing floppy drive to fix CVE-2021-20196
  tests/qtest/fdc-test: Add a regression test for CVE-2021-20196

 hw/block/fdc.c         | 23 ++++++++++++++++++++---
 tests/qtest/fdc-test.c | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 3 deletions(-)

-- 
2.33.1

[PATCH v4 1/3] hw/block/fdc: Extract blk_create_empty_drive()

[Philippe Mathieu-Daudé]

We are going to re-use this code in the next commit,
so extract it as a new blk_create_empty_drive() function.

Inspired-by: Hanna Reitz <hreitz@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/fdc.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index fa933cd3263..1dbf3f6028f 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -61,6 +61,12 @@
     } while (0)
 
 
+/* Anonymous BlockBackend for empty drive */
+static BlockBackend *blk_create_empty_drive(void)
+{
+    return blk_new(qemu_get_aio_context(), 0, BLK_PERM_ALL);
+}
+
 /********************************************************/
 /* qdev floppy bus                                      */
 
@@ -486,8 +492,7 @@ static void floppy_drive_realize(DeviceState *qdev, Error **errp)
     }
 
     if (!dev->conf.blk) {
-        /* Anonymous BlockBackend for an empty drive */
-        dev->conf.blk = blk_new(qemu_get_aio_context(), 0, BLK_PERM_ALL);
+        dev->conf.blk = blk_create_empty_drive();
         ret = blk_attach_dev(dev->conf.blk, qdev);
         assert(ret == 0);
 
-- 
2.33.1

Re: [PATCH v4 1/3] hw/block/fdc: Extract blk_create_empty_drive()

[Hanna Reitz]

On 24.11.21 17:15, Philippe Mathieu-Daudé wrote:
> We are going to re-use this code in the next commit,
> so extract it as a new blk_create_empty_drive() function.
>
> Inspired-by: Hanna Reitz <hreitz@redhat.com>

:)

> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   hw/block/fdc.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)

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

[PATCH v4 2/3] hw/block/fdc: Kludge missing floppy drive to fix CVE-2021-20196

[Philippe Mathieu-Daudé]

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>
Reviewed-by: Hanna Reitz <hreitz@redhat.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 1dbf3f6028f..21d18ac2e36 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1166,7 +1166,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_create_empty_drive();
+    }
+    return cur_drv;
 }
 
 /* Status A register : 0x00 (read-only) */
-- 
2.33.1

[PATCH v4 3/3] tests/qtest/fdc-test: Add a regression test for CVE-2021-20196

[Philippe Mathieu-Daudé]

Without the previous commit, when running 'make check-qtest-i386'
with QEMU configured with '--enable-sanitizers' we get:

  AddressSanitizer:DEADLYSIGNAL
  =================================================================
  ==287878==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000344
  ==287878==The signal is caused by a WRITE memory access.
  ==287878==Hint: address points to the zero page.
      #0 0x564b2e5bac27 in blk_inc_in_flight block/block-backend.c:1346:5
      #1 0x564b2e5bb228 in blk_pwritev_part block/block-backend.c:1317:5
      #2 0x564b2e5bcd57 in blk_pwrite block/block-backend.c:1498:11
      #3 0x564b2ca1cdd3 in fdctrl_write_data hw/block/fdc.c:2221:17
      #4 0x564b2ca1b2f7 in fdctrl_write hw/block/fdc.c:829:9
      #5 0x564b2dc49503 in portio_write softmmu/ioport.c:201:9

Add the reproducer for CVE-2021-20196.

Suggested-by: Alexander Bulekov <alxndr@bu.edu>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/qtest/fdc-test.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/tests/qtest/fdc-test.c b/tests/qtest/fdc-test.c
index 26b69f7c5cd..8f6eee84a47 100644
--- a/tests/qtest/fdc-test.c
+++ b/tests/qtest/fdc-test.c
@@ -32,6 +32,9 @@
 /* TODO actually test the results and get rid of this */
 #define qmp_discard_response(...) qobject_unref(qmp(__VA_ARGS__))
 
+#define DRIVE_FLOPPY_BLANK \
+    "-drive if=floppy,file=null-co://,file.read-zeroes=on,format=raw,size=1440k"
+
 #define TEST_IMAGE_SIZE 1440 * 1024
 
 #define FLOPPY_BASE 0x3f0
@@ -546,6 +549,40 @@ static void fuzz_registers(void)
     }
 }
 
+static bool qtest_check_clang_sanitizer(void)
+{
+#if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
+    return true;
+#else
+    g_test_skip("QEMU not configured using --enable-sanitizers");
+    return false;
+#endif
+}
+static void test_cve_2021_20196(void)
+{
+    QTestState *s;
+
+    if (!qtest_check_clang_sanitizer()) {
+        return;
+    }
+
+    s = qtest_initf("-nographic -m 32M -nodefaults " DRIVE_FLOPPY_BLANK);
+
+    qtest_outw(s, 0x3f4, 0x0500);
+    qtest_outb(s, 0x3f5, 0x00);
+    qtest_outb(s, 0x3f5, 0x00);
+    qtest_outw(s, 0x3f4, 0x0000);
+    qtest_outb(s, 0x3f5, 0x00);
+    qtest_outw(s, 0x3f1, 0x0400);
+    qtest_outw(s, 0x3f4, 0x0000);
+    qtest_outw(s, 0x3f4, 0x0000);
+    qtest_outb(s, 0x3f5, 0x00);
+    qtest_outb(s, 0x3f5, 0x01);
+    qtest_outw(s, 0x3f1, 0x0500);
+    qtest_outb(s, 0x3f5, 0x00);
+    qtest_quit(s);
+}
+
 int main(int argc, char **argv)
 {
     int fd;
@@ -576,6 +613,7 @@ int main(int argc, char **argv)
     qtest_add_func("/fdc/read_no_dma_18", test_read_no_dma_18);
     qtest_add_func("/fdc/read_no_dma_19", test_read_no_dma_19);
     qtest_add_func("/fdc/fuzz-registers", fuzz_registers);
+    qtest_add_func("/fdc/fuzz/cve_2021_20196", test_cve_2021_20196);
 
     ret = g_test_run();
 
-- 
2.33.1

Re: [PATCH v4 3/3] tests/qtest/fdc-test: Add a regression test for CVE-2021-20196

[Hanna Reitz]

On 24.11.21 17:15, Philippe Mathieu-Daudé wrote:
> Without the previous commit, when running 'make check-qtest-i386'
> with QEMU configured with '--enable-sanitizers' we get:
>
>    AddressSanitizer:DEADLYSIGNAL
>    =================================================================
>    ==287878==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000344
>    ==287878==The signal is caused by a WRITE memory access.
>    ==287878==Hint: address points to the zero page.
>        #0 0x564b2e5bac27 in blk_inc_in_flight block/block-backend.c:1346:5
>        #1 0x564b2e5bb228 in blk_pwritev_part block/block-backend.c:1317:5
>        #2 0x564b2e5bcd57 in blk_pwrite block/block-backend.c:1498:11
>        #3 0x564b2ca1cdd3 in fdctrl_write_data hw/block/fdc.c:2221:17
>        #4 0x564b2ca1b2f7 in fdctrl_write hw/block/fdc.c:829:9
>        #5 0x564b2dc49503 in portio_write softmmu/ioport.c:201:9
>
> Add the reproducer for CVE-2021-20196.
>
> Suggested-by: Alexander Bulekov <alxndr@bu.edu>
> Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   tests/qtest/fdc-test.c | 38 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 38 insertions(+)
>
> diff --git a/tests/qtest/fdc-test.c b/tests/qtest/fdc-test.c
> index 26b69f7c5cd..8f6eee84a47 100644
> --- a/tests/qtest/fdc-test.c
> +++ b/tests/qtest/fdc-test.c
> @@ -32,6 +32,9 @@
>   /* TODO actually test the results and get rid of this */
>   #define qmp_discard_response(...) qobject_unref(qmp(__VA_ARGS__))
>   
> +#define DRIVE_FLOPPY_BLANK \
> +    "-drive if=floppy,file=null-co://,file.read-zeroes=on,format=raw,size=1440k"
> +
>   #define TEST_IMAGE_SIZE 1440 * 1024
>   
>   #define FLOPPY_BASE 0x3f0
> @@ -546,6 +549,40 @@ static void fuzz_registers(void)
>       }
>   }
>   
> +static bool qtest_check_clang_sanitizer(void)
> +{
> +#if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
> +    return true;
> +#else
> +    g_test_skip("QEMU not configured using --enable-sanitizers");
> +    return false;
> +#endif
> +}
> +static void test_cve_2021_20196(void)
> +{
> +    QTestState *s;
> +
> +    if (!qtest_check_clang_sanitizer()) {
> +        return;
> +    }
> +
> +    s = qtest_initf("-nographic -m 32M -nodefaults " DRIVE_FLOPPY_BLANK);
> +
> +    qtest_outw(s, 0x3f4, 0x0500);
> +    qtest_outb(s, 0x3f5, 0x00);
> +    qtest_outb(s, 0x3f5, 0x00);
> +    qtest_outw(s, 0x3f4, 0x0000);
> +    qtest_outb(s, 0x3f5, 0x00);
> +    qtest_outw(s, 0x3f1, 0x0400);
> +    qtest_outw(s, 0x3f4, 0x0000);
> +    qtest_outw(s, 0x3f4, 0x0000);
> +    qtest_outb(s, 0x3f5, 0x00);
> +    qtest_outb(s, 0x3f5, 0x01);
> +    qtest_outw(s, 0x3f1, 0x0500);
> +    qtest_outb(s, 0x3f5, 0x00);
> +    qtest_quit(s);
> +}
> +

Now this works as a reproducer for me, but... this is a completely 
different I/O sequence now, right?

Can’t complain, though, I didn’t understand the previous one, I can’t 
claim I need to understand this one or why they’re different.

All the rest looks good to me, so all in all:

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

Re: [PATCH v4 3/3] tests/qtest/fdc-test: Add a regression test for CVE-2021-20196

[Philippe Mathieu-Daudé]

On 11/25/21 12:57, Hanna Reitz wrote:
> On 24.11.21 17:15, Philippe Mathieu-Daudé wrote:
>> Without the previous commit, when running 'make check-qtest-i386'
>> with QEMU configured with '--enable-sanitizers' we get:
>>
>>    AddressSanitizer:DEADLYSIGNAL
>>    =================================================================
>>    ==287878==ERROR: AddressSanitizer: SEGV on unknown address
>> 0x000000000344
>>    ==287878==The signal is caused by a WRITE memory access.
>>    ==287878==Hint: address points to the zero page.
>>        #0 0x564b2e5bac27 in blk_inc_in_flight
>> block/block-backend.c:1346:5
>>        #1 0x564b2e5bb228 in blk_pwritev_part block/block-backend.c:1317:5
>>        #2 0x564b2e5bcd57 in blk_pwrite block/block-backend.c:1498:11
>>        #3 0x564b2ca1cdd3 in fdctrl_write_data hw/block/fdc.c:2221:17
>>        #4 0x564b2ca1b2f7 in fdctrl_write hw/block/fdc.c:829:9
>>        #5 0x564b2dc49503 in portio_write softmmu/ioport.c:201:9
>>
>> Add the reproducer for CVE-2021-20196.
>>
>> Suggested-by: Alexander Bulekov <alxndr@bu.edu>
>> Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   tests/qtest/fdc-test.c | 38 ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 38 insertions(+)
>>
>> diff --git a/tests/qtest/fdc-test.c b/tests/qtest/fdc-test.c
>> index 26b69f7c5cd..8f6eee84a47 100644
>> --- a/tests/qtest/fdc-test.c
>> +++ b/tests/qtest/fdc-test.c
>> @@ -32,6 +32,9 @@
>>   /* TODO actually test the results and get rid of this */
>>   #define qmp_discard_response(...) qobject_unref(qmp(__VA_ARGS__))
>>   +#define DRIVE_FLOPPY_BLANK \
>> +    "-drive
>> if=floppy,file=null-co://,file.read-zeroes=on,format=raw,size=1440k"
>> +
>>   #define TEST_IMAGE_SIZE 1440 * 1024
>>     #define FLOPPY_BASE 0x3f0
>> @@ -546,6 +549,40 @@ static void fuzz_registers(void)
>>       }
>>   }
>>   +static bool qtest_check_clang_sanitizer(void)
>> +{
>> +#if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
>> +    return true;
>> +#else
>> +    g_test_skip("QEMU not configured using --enable-sanitizers");
>> +    return false;
>> +#endif
>> +}
>> +static void test_cve_2021_20196(void)
>> +{
>> +    QTestState *s;
>> +
>> +    if (!qtest_check_clang_sanitizer()) {
>> +        return;
>> +    }
>> +
>> +    s = qtest_initf("-nographic -m 32M -nodefaults "
>> DRIVE_FLOPPY_BLANK);
>> +
>> +    qtest_outw(s, 0x3f4, 0x0500);
>> +    qtest_outb(s, 0x3f5, 0x00);
>> +    qtest_outb(s, 0x3f5, 0x00);
>> +    qtest_outw(s, 0x3f4, 0x0000);
>> +    qtest_outb(s, 0x3f5, 0x00);
>> +    qtest_outw(s, 0x3f1, 0x0400);
>> +    qtest_outw(s, 0x3f4, 0x0000);
>> +    qtest_outw(s, 0x3f4, 0x0000);
>> +    qtest_outb(s, 0x3f5, 0x00);
>> +    qtest_outb(s, 0x3f5, 0x01);
>> +    qtest_outw(s, 0x3f1, 0x0500);
>> +    qtest_outb(s, 0x3f5, 0x00);
>> +    qtest_quit(s);
>> +}
>> +
> 
> Now this works as a reproducer for me, but... this is a completely
> different I/O sequence now, right?

The patch Alexander sent [*] was indeed not working, but I could
manually reproduce, then I figure while the commit *description*
was working, the patch *content* was not accurate. This patch uses
the commit description.

[1] https://www.mail-archive.com/qemu-block@nongnu.org/msg82825.html

> Can’t complain, though, I didn’t understand the previous one, I can’t
> claim I need to understand this one or why they’re different.

Same here =)

> All the rest looks good to me, so all in all:
> 
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Thank you!

Re: [PATCH v4 0/3] hw/block/fdc: Fix CVE-2021-20196

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 1025 bytes --]

On Wed, Nov 24, 2021 at 11:15 AM Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:

> Since v3:
> - Preliminary extract blk_create_empty_drive()
> - qtest checks qtest_check_clang_sanitizer() enabled
> - qtest uses null-co:// driver instead of file
>
> Philippe Mathieu-Daudé (3):
>   hw/block/fdc: Extract blk_create_empty_drive()
>   hw/block/fdc: Kludge missing floppy drive to fix CVE-2021-20196
>   tests/qtest/fdc-test: Add a regression test for CVE-2021-20196
>
>  hw/block/fdc.c         | 23 ++++++++++++++++++++---
>  tests/qtest/fdc-test.c | 38 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 58 insertions(+), 3 deletions(-)
>
> --
> 2.33.1
>
>
I'm testing this now. I'm going to take your word for it. If Hanna is fine
with the block-layer components of the fix, I'll probably take it, but I
will be sending a patch to remove myself as maintainer in the process,
since I don't have the time to do the "proper fix" for these devices, and
haven't for quite some time.

--js

[-- Attachment #2: Type: text/html, Size: 1441 bytes --]

Re: [PATCH v4 0/3] hw/block/fdc: Fix CVE-2021-20196

[Kevin Wolf]

Am 24.11.2021 um 17:15 hat Philippe Mathieu-Daudé geschrieben:
> Since v3:
> - Preliminary extract blk_create_empty_drive()
> - qtest checks qtest_check_clang_sanitizer() enabled
> - qtest uses null-co:// driver instead of file
> 
> Philippe Mathieu-Daudé (3):
>   hw/block/fdc: Extract blk_create_empty_drive()
>   hw/block/fdc: Kludge missing floppy drive to fix CVE-2021-20196
>   tests/qtest/fdc-test: Add a regression test for CVE-2021-20196

If I may ask a meta question: No doubt that this is a bug and it's good
that we fixed it, but why was it assigned a CVE?

Any guest can legitimately shut down and we don't consider that a denial
of service. This bug was essentially just another undocumented way for
the guest kernel to shut down, as unprivileged users in the guest can't
normally access the I/O ports of the floppy controller. I don't think we
generally consider guests killing themselves a security problem as long
as it requires kernel or root privileges in the guest.

Kevin

Re: [PATCH v4 0/3] hw/block/fdc: Fix CVE-2021-20196

[Philippe Mathieu-Daudé]

On 12/10/21 14:42, Kevin Wolf wrote:
> Am 24.11.2021 um 17:15 hat Philippe Mathieu-Daudé geschrieben:
>> Since v3:
>> - Preliminary extract blk_create_empty_drive()
>> - qtest checks qtest_check_clang_sanitizer() enabled
>> - qtest uses null-co:// driver instead of file
>>
>> Philippe Mathieu-Daudé (3):
>>   hw/block/fdc: Extract blk_create_empty_drive()
>>   hw/block/fdc: Kludge missing floppy drive to fix CVE-2021-20196
>>   tests/qtest/fdc-test: Add a regression test for CVE-2021-20196
> 
> If I may ask a meta question: No doubt that this is a bug and it's good
> that we fixed it, but why was it assigned a CVE?

No clue, I suppose this is audited and handled by qemu-security@ team
members. Cc'ing them.

> Any guest can legitimately shut down and we don't consider that a denial
> of service. This bug was essentially just another undocumented way for
> the guest kernel to shut down, as unprivileged users in the guest can't
> normally access the I/O ports of the floppy controller. I don't think we
> generally consider guests killing themselves a security problem as long
> as it requires kernel or root privileges in the guest.

Agreed.