[PATCH-for-6.2 0/2] hw/block/fdc: Fix CVE-2021-3507

[PATCH-for-6.2 0/2] hw/block/fdc: Fix CVE-2021-3507

[Philippe Mathieu-Daudé]

Trivial fix for CVE-2021-3507.

Philippe Mathieu-Daudé (2):
  hw/block/fdc: Prevent end-of-track overrun (CVE-2021-3507)
  tests/qtest/fdc-test: Add a regression test for CVE-2021-3507

 hw/block/fdc.c         |  8 ++++++++
 tests/qtest/fdc-test.c | 20 ++++++++++++++++++++
 2 files changed, 28 insertions(+)

-- 
2.31.1

[PATCH-for-6.2 1/2] hw/block/fdc: Prevent end-of-track overrun (CVE-2021-3507)

[Philippe Mathieu-Daudé]

Per the 82078 datasheet, if the end-of-track (EOT byte in
the FIFO) is more than the number of sectors per side, the
command is terminated unsuccessfully:

* 5.2.5 DATA TRANSFER TERMINATION

  The 82078 supports terminal count explicitly through
  the TC pin and implicitly through the underrun/over-
  run and end-of-track (EOT) functions. For full sector
  transfers, the EOT parameter can define the last
  sector to be transferred in a single or multisector
  transfer. If the last sector to be transferred is a par-
  tial sector, the host can stop transferring the data in
  mid-sector, and the 82078 will continue to complete
  the sector as if a hardware TC was received. The
  only difference between these implicit functions and
  TC is that they return "abnormal termination" result
  status. Such status indications can be ignored if they
  were expected.

* 6.1.3 READ TRACK

  This command terminates when the EOT specified
  number of sectors have been read. If the 82078
  does not find an I D Address Mark on the diskette
  after the second· occurrence of a pulse on the
  INDX# pin, then it sets the IC code in Status Regis-
  ter 0 to "01" (Abnormal termination), sets the MA bit
  in Status Register 1 to "1", and terminates the com-
  mand.

* 6.1.6 VERIFY

  Refer to Table 6-6 and Table 6-7 for information
  concerning the values of MT and EC versus SC and
  EOT value.

* Table 6·6. Result Phase Table

* Table 6-7. Verify Command Result Phase Table

Fix by aborting the transfer when EOT > # Sectors Per Side.

Cc: qemu-stable@nongnu.org
Cc: Hervé Poussineau <hpoussin@reactos.org>
Fixes: baca51faff0 ("floppy driver: disk geometry auto detect")
Reported-by: Alexander Bulekov <alxndr@bu.edu>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/339
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/fdc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index fa933cd3263..d21b717b7d6 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1512,6 +1512,14 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
         int tmp;
         fdctrl->data_len = 128 << (fdctrl->fifo[5] > 7 ? 7 : fdctrl->fifo[5]);
         tmp = (fdctrl->fifo[6] - ks + 1);
+        if (tmp < 0) {
+            FLOPPY_DPRINTF("invalid EOT: %d\n", tmp);
+            fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
+            fdctrl->fifo[3] = kt;
+            fdctrl->fifo[4] = kh;
+            fdctrl->fifo[5] = ks;
+            return;
+        }
         if (fdctrl->fifo[0] & 0x80)
             tmp += fdctrl->fifo[6];
         fdctrl->data_len *= tmp;
-- 
2.31.1

Re: [PATCH-for-6.2 1/2] hw/block/fdc: Prevent end-of-track overrun (CVE-2021-3507)

[Hanna Reitz]

On 18.11.21 12:57, Philippe Mathieu-Daudé wrote:
> Per the 82078 datasheet, if the end-of-track (EOT byte in
> the FIFO) is more than the number of sectors per side, the
> command is terminated unsuccessfully:

Patch looks OK to me (can’t believe I’ve looked into the spec...), just 
one question (note from later self: I really believed this, and now I 
ended up with this wall of text...):

Isn’t the problem that the EOT is smaller than the start sector index?  
AFAIU fifo[6] is the EOT, ks (fifo[4]) is the start sector number, and 
so the problem occurs if fifo[6] < fifo[4] - 1 (EOT < start sector index).

I don’t think there is any problem if the EOT exceeds the number of 
sectors per anything (e.g. sectors per track or per cylinder). Well, the 
spec might say it should cause an error, but in that case tmp (and thus 
data_len) just become very large.  AFAIU fd_seek() checks that the start 
sector is in bounds, so in fact passing an EOT that is larger than 
cur_drv->last_sect would never be negative, and so never trigger the tmp 
< 0 condition.

The explanation below however seems to be precisely for the case where 
EOT would be larger than last_sect (say 18), and so e.g. sector 19 can’t 
be found (“second occurrence of a pulse on the INDX# pin”).  Contrarily, 
the spec doesn’t seem to say anything for the case where EOT is in 
bounds but less than the start sector index. It doesn’t even seem to say 
how the EOT is evaluated; is it a “read until sector >= EOT”?  (I.e. EOT 
< start sector would yield a zero-byte read)  Or is it a “read until 
sector == EOT”?  (I.e. EOT < start sector would yield an error because 
that condition is never reached, and we go beyond last_sect, yielding 
the same error as if EOT were out of bounds.)

Now that raises another question to me.  Say EOT is out of bounds; 
wouldn’t the FDC still read all sectors until the end of the 
track/cylinder?  And only then raise an error that the sector beyond 
doesn’t exist?

I have no idea if any of that made sense and even if it did, whether I’m 
right.  But even if I am, I don’t think it’s a problem.  Our problem at 
hand is the `tmp` underflow, and it’s good to handle that by returning 
some form of error to the guest instead of crashing. It’s just that I’m 
not sure this solution is actually what the spec requires (because I 
have no idea what the spec requires in that case), and the explanation 
given in the commit message to me seems to define an error for a very 
different case (where tmp would not be negative) that we just execute 
without an error.

When I look into this I see many more things that look like they aren’t 
according to spec (like not checking the sector size, or that we honor 
fifo[0] & 0x80 for READ TRACK even though the spec says no multi-track 
operations for READ TRACK), so I don’t personally care.  It’s good that 
this patch handles the `tmp` underflow, because that’s important, 
compliance with the spec isn’t.

(Yes, the entire reason for this wall of text is that I wonder why the 
commit message goes into so much detail quoting the spec, while I can’t 
find it applies.)

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

> * 5.2.5 DATA TRANSFER TERMINATION
>
>    The 82078 supports terminal count explicitly through
>    the TC pin and implicitly through the underrun/over-
>    run and end-of-track (EOT) functions. For full sector
>    transfers, the EOT parameter can define the last
>    sector to be transferred in a single or multisector
>    transfer. If the last sector to be transferred is a par-
>    tial sector, the host can stop transferring the data in
>    mid-sector, and the 82078 will continue to complete
>    the sector as if a hardware TC was received. The
>    only difference between these implicit functions and
>    TC is that they return "abnormal termination" result
>    status. Such status indications can be ignored if they
>    were expected.
>
> * 6.1.3 READ TRACK
>
>    This command terminates when the EOT specified
>    number of sectors have been read. If the 82078
>    does not find an I D Address Mark on the diskette
>    after the second· occurrence of a pulse on the
>    INDX# pin, then it sets the IC code in Status Regis-
>    ter 0 to "01" (Abnormal termination), sets the MA bit
>    in Status Register 1 to "1", and terminates the com-
>    mand.
>
> * 6.1.6 VERIFY
>
>    Refer to Table 6-6 and Table 6-7 for information
>    concerning the values of MT and EC versus SC and
>    EOT value.
>
> * Table 6·6. Result Phase Table
>
> * Table 6-7. Verify Command Result Phase Table
>
> Fix by aborting the transfer when EOT > # Sectors Per Side.
>
> Cc: qemu-stable@nongnu.org
> Cc: Hervé Poussineau <hpoussin@reactos.org>
> Fixes: baca51faff0 ("floppy driver: disk geometry auto detect")
> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/339
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   hw/block/fdc.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index fa933cd3263..d21b717b7d6 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -1512,6 +1512,14 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
>           int tmp;
>           fdctrl->data_len = 128 << (fdctrl->fifo[5] > 7 ? 7 : fdctrl->fifo[5]);
>           tmp = (fdctrl->fifo[6] - ks + 1);
> +        if (tmp < 0) {
> +            FLOPPY_DPRINTF("invalid EOT: %d\n", tmp);
> +            fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
> +            fdctrl->fifo[3] = kt;
> +            fdctrl->fifo[4] = kh;
> +            fdctrl->fifo[5] = ks;
> +            return;
> +        }
>           if (fdctrl->fifo[0] & 0x80)
>               tmp += fdctrl->fifo[6];
>           fdctrl->data_len *= tmp;

[PATCH-for-6.2 2/2] tests/qtest/fdc-test: Add a regression test for CVE-2021-3507

[Philippe Mathieu-Daudé]

Add the reproducer from https://gitlab.com/qemu-project/qemu/-/issues/339

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

  ==4028352==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x619000062a00 at pc 0x5626d03c491a bp 0x7ffdb4199410 sp 0x7ffdb4198bc0
  READ of size 786432 at 0x619000062a00 thread T0
      #0 0x5626d03c4919 in __asan_memcpy (qemu-system-i386+0x1e65919)
      #1 0x5626d1c023cc in flatview_write_continue softmmu/physmem.c:2787:13
      #2 0x5626d1bf0c0f in flatview_write softmmu/physmem.c:2822:14
      #3 0x5626d1bf0798 in address_space_write softmmu/physmem.c:2914:18
      #4 0x5626d1bf0f37 in address_space_rw softmmu/physmem.c:2924:16
      #5 0x5626d1bf14c8 in cpu_physical_memory_rw softmmu/physmem.c:2933:5
      #6 0x5626d0bd5649 in cpu_physical_memory_write include/exec/cpu-common.h:82:5
      #7 0x5626d0bd0a07 in i8257_dma_write_memory hw/dma/i8257.c:452:9
      #8 0x5626d09f825d in fdctrl_transfer_handler hw/block/fdc.c:1616:13
      #9 0x5626d0a048b4 in fdctrl_start_transfer hw/block/fdc.c:1539:13
      #10 0x5626d09f4c3e in fdctrl_write_data hw/block/fdc.c:2266:13
      #11 0x5626d09f22f7 in fdctrl_write hw/block/fdc.c:829:9
      #12 0x5626d1c20bc5 in portio_write softmmu/ioport.c:207:17

  0x619000062a00 is located 0 bytes to the right of 512-byte region [0x619000062800,0x619000062a00)
  allocated by thread T0 here:
      #0 0x5626d03c66ec in posix_memalign (qemu-system-i386+0x1e676ec)
      #1 0x5626d2b988d4 in qemu_try_memalign util/oslib-posix.c:210:11
      #2 0x5626d2b98b0c in qemu_memalign util/oslib-posix.c:226:27
      #3 0x5626d09fbaf0 in fdctrl_realize_common hw/block/fdc.c:2341:20
      #4 0x5626d0a150ed in isabus_fdc_realize hw/block/fdc-isa.c:113:5
      #5 0x5626d2367935 in device_set_realized hw/core/qdev.c:531:13

  SUMMARY: AddressSanitizer: heap-buffer-overflow (qemu-system-i386+0x1e65919) in __asan_memcpy
  Shadow bytes around the buggy address:
    0x0c32800044f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
    0x0c3280004500: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0x0c3280004510: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0x0c3280004520: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0x0c3280004530: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  =>0x0c3280004540:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
    0x0c3280004550: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
    0x0c3280004560: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
    0x0c3280004570: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
    0x0c3280004580: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
    0x0c3280004590: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  Shadow byte legend (one shadow byte represents 8 application bytes):
    Addressable:           00
    Heap left redzone:       fa
    Freed heap region:       fd
  ==4028352==ABORTING

Reported-by: Alexander Bulekov <alxndr@bu.edu>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/qtest/fdc-test.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/tests/qtest/fdc-test.c b/tests/qtest/fdc-test.c
index 26b69f7c5cd..f164d972d10 100644
--- a/tests/qtest/fdc-test.c
+++ b/tests/qtest/fdc-test.c
@@ -546,6 +546,25 @@ static void fuzz_registers(void)
     }
 }
 
+static void test_cve_2021_3507(void)
+{
+    QTestState *s;
+
+    s = qtest_initf("-nographic -m 32M -nodefaults "
+                    "-drive file=%s,format=raw,if=floppy", test_image);
+    qtest_outl(s, 0x9, 0x0a0206);
+    qtest_outw(s, 0x3f4, 0x1600);
+    qtest_outw(s, 0x3f4, 0x0000);
+    qtest_outw(s, 0x3f4, 0x0000);
+    qtest_outw(s, 0x3f4, 0x0000);
+    qtest_outw(s, 0x3f4, 0x0200);
+    qtest_outw(s, 0x3f4, 0x0200);
+    qtest_outw(s, 0x3f4, 0x0000);
+    qtest_outw(s, 0x3f4, 0x0000);
+    qtest_outw(s, 0x3f4, 0x0000);
+    qtest_quit(s);
+}
+
 int main(int argc, char **argv)
 {
     int fd;
@@ -576,6 +595,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_3507", test_cve_2021_3507);
 
     ret = g_test_run();
 
-- 
2.31.1

Re: [PATCH-for-6.2 2/2] tests/qtest/fdc-test: Add a regression test for CVE-2021-3507

[Alexander Bulekov]

On 211118 1257, Philippe Mathieu-Daudé wrote:
> Add the reproducer from https://gitlab.com/qemu-project/qemu/-/issues/339
> 
> Without the previous commit, when running 'make check-qtest-i386'
> with QEMU configured with '--enable-sanitizers' we get:
> 
>   ==4028352==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x619000062a00 at pc 0x5626d03c491a bp 0x7ffdb4199410 sp 0x7ffdb4198bc0
>   READ of size 786432 at 0x619000062a00 thread T0
>       #0 0x5626d03c4919 in __asan_memcpy (qemu-system-i386+0x1e65919)
>       #1 0x5626d1c023cc in flatview_write_continue softmmu/physmem.c:2787:13
>       #2 0x5626d1bf0c0f in flatview_write softmmu/physmem.c:2822:14
>       #3 0x5626d1bf0798 in address_space_write softmmu/physmem.c:2914:18
>       #4 0x5626d1bf0f37 in address_space_rw softmmu/physmem.c:2924:16
>       #5 0x5626d1bf14c8 in cpu_physical_memory_rw softmmu/physmem.c:2933:5
>       #6 0x5626d0bd5649 in cpu_physical_memory_write include/exec/cpu-common.h:82:5
>       #7 0x5626d0bd0a07 in i8257_dma_write_memory hw/dma/i8257.c:452:9
>       #8 0x5626d09f825d in fdctrl_transfer_handler hw/block/fdc.c:1616:13
>       #9 0x5626d0a048b4 in fdctrl_start_transfer hw/block/fdc.c:1539:13
>       #10 0x5626d09f4c3e in fdctrl_write_data hw/block/fdc.c:2266:13
>       #11 0x5626d09f22f7 in fdctrl_write hw/block/fdc.c:829:9
>       #12 0x5626d1c20bc5 in portio_write softmmu/ioport.c:207:17
> 
>   0x619000062a00 is located 0 bytes to the right of 512-byte region [0x619000062800,0x619000062a00)
>   allocated by thread T0 here:
>       #0 0x5626d03c66ec in posix_memalign (qemu-system-i386+0x1e676ec)
>       #1 0x5626d2b988d4 in qemu_try_memalign util/oslib-posix.c:210:11
>       #2 0x5626d2b98b0c in qemu_memalign util/oslib-posix.c:226:27
>       #3 0x5626d09fbaf0 in fdctrl_realize_common hw/block/fdc.c:2341:20
>       #4 0x5626d0a150ed in isabus_fdc_realize hw/block/fdc-isa.c:113:5
>       #5 0x5626d2367935 in device_set_realized hw/core/qdev.c:531:13
> 
>   SUMMARY: AddressSanitizer: heap-buffer-overflow (qemu-system-i386+0x1e65919) in __asan_memcpy
>   Shadow bytes around the buggy address:
>     0x0c32800044f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>     0x0c3280004500: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>     0x0c3280004510: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>     0x0c3280004520: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>     0x0c3280004530: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   =>0x0c3280004540:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>     0x0c3280004550: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>     0x0c3280004560: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>     0x0c3280004570: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>     0x0c3280004580: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>     0x0c3280004590: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>   Shadow byte legend (one shadow byte represents 8 application bytes):
>     Addressable:           00
>     Heap left redzone:       fa
>     Freed heap region:       fd
>   ==4028352==ABORTING
> 
> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  tests/qtest/fdc-test.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/tests/qtest/fdc-test.c b/tests/qtest/fdc-test.c
> index 26b69f7c5cd..f164d972d10 100644
> --- a/tests/qtest/fdc-test.c
> +++ b/tests/qtest/fdc-test.c
> @@ -546,6 +546,25 @@ static void fuzz_registers(void)
>      }
>  }
>  
> +static void test_cve_2021_3507(void)
> +{
> +    QTestState *s;
> +
> +    s = qtest_initf("-nographic -m 32M -nodefaults "
> +                    "-drive file=%s,format=raw,if=floppy", test_image);

Does it make sense to run this with -snapshot ?

Reviewed-by: Alexander Bulekov <alxndr@bu.edu>

> +    qtest_outl(s, 0x9, 0x0a0206);
> +    qtest_outw(s, 0x3f4, 0x1600);
> +    qtest_outw(s, 0x3f4, 0x0000);
> +    qtest_outw(s, 0x3f4, 0x0000);
> +    qtest_outw(s, 0x3f4, 0x0000);
> +    qtest_outw(s, 0x3f4, 0x0200);
> +    qtest_outw(s, 0x3f4, 0x0200);
> +    qtest_outw(s, 0x3f4, 0x0000);
> +    qtest_outw(s, 0x3f4, 0x0000);
> +    qtest_outw(s, 0x3f4, 0x0000);
> +    qtest_quit(s);
> +}
> +
>  int main(int argc, char **argv)
>  {
>      int fd;
> @@ -576,6 +595,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_3507", test_cve_2021_3507);
>  
>      ret = g_test_run();
>  
> -- 
> 2.31.1
> 

Re: [PATCH-for-6.2 2/2] tests/qtest/fdc-test: Add a regression test for CVE-2021-3507

[Hanna Reitz]

On 18.11.21 12:57, Philippe Mathieu-Daudé wrote:
> Add the reproducer from https://gitlab.com/qemu-project/qemu/-/issues/339
>
> Without the previous commit, when running 'make check-qtest-i386'
> with QEMU configured with '--enable-sanitizers' we get:
>
>    ==4028352==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x619000062a00 at pc 0x5626d03c491a bp 0x7ffdb4199410 sp 0x7ffdb4198bc0
>    READ of size 786432 at 0x619000062a00 thread T0
>        #0 0x5626d03c4919 in __asan_memcpy (qemu-system-i386+0x1e65919)
>        #1 0x5626d1c023cc in flatview_write_continue softmmu/physmem.c:2787:13
>        #2 0x5626d1bf0c0f in flatview_write softmmu/physmem.c:2822:14
>        #3 0x5626d1bf0798 in address_space_write softmmu/physmem.c:2914:18
>        #4 0x5626d1bf0f37 in address_space_rw softmmu/physmem.c:2924:16
>        #5 0x5626d1bf14c8 in cpu_physical_memory_rw softmmu/physmem.c:2933:5
>        #6 0x5626d0bd5649 in cpu_physical_memory_write include/exec/cpu-common.h:82:5
>        #7 0x5626d0bd0a07 in i8257_dma_write_memory hw/dma/i8257.c:452:9
>        #8 0x5626d09f825d in fdctrl_transfer_handler hw/block/fdc.c:1616:13
>        #9 0x5626d0a048b4 in fdctrl_start_transfer hw/block/fdc.c:1539:13
>        #10 0x5626d09f4c3e in fdctrl_write_data hw/block/fdc.c:2266:13
>        #11 0x5626d09f22f7 in fdctrl_write hw/block/fdc.c:829:9
>        #12 0x5626d1c20bc5 in portio_write softmmu/ioport.c:207:17
>
>    0x619000062a00 is located 0 bytes to the right of 512-byte region [0x619000062800,0x619000062a00)
>    allocated by thread T0 here:
>        #0 0x5626d03c66ec in posix_memalign (qemu-system-i386+0x1e676ec)
>        #1 0x5626d2b988d4 in qemu_try_memalign util/oslib-posix.c:210:11
>        #2 0x5626d2b98b0c in qemu_memalign util/oslib-posix.c:226:27
>        #3 0x5626d09fbaf0 in fdctrl_realize_common hw/block/fdc.c:2341:20
>        #4 0x5626d0a150ed in isabus_fdc_realize hw/block/fdc-isa.c:113:5
>        #5 0x5626d2367935 in device_set_realized hw/core/qdev.c:531:13
>
>    SUMMARY: AddressSanitizer: heap-buffer-overflow (qemu-system-i386+0x1e65919) in __asan_memcpy
>    Shadow bytes around the buggy address:
>      0x0c32800044f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>      0x0c3280004500: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>      0x0c3280004510: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>      0x0c3280004520: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>      0x0c3280004530: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>    =>0x0c3280004540:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>      0x0c3280004550: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>      0x0c3280004560: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>      0x0c3280004570: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>      0x0c3280004580: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>      0x0c3280004590: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>    Shadow byte legend (one shadow byte represents 8 application bytes):
>      Addressable:           00
>      Heap left redzone:       fa
>      Freed heap region:       fd
>    ==4028352==ABORTING
>
> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   tests/qtest/fdc-test.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
>
> diff --git a/tests/qtest/fdc-test.c b/tests/qtest/fdc-test.c
> index 26b69f7c5cd..f164d972d10 100644
> --- a/tests/qtest/fdc-test.c
> +++ b/tests/qtest/fdc-test.c
> @@ -546,6 +546,25 @@ static void fuzz_registers(void)
>       }
>   }
>   
> +static void test_cve_2021_3507(void)
> +{
> +    QTestState *s;
> +
> +    s = qtest_initf("-nographic -m 32M -nodefaults "
> +                    "-drive file=%s,format=raw,if=floppy", test_image);
> +    qtest_outl(s, 0x9, 0x0a0206);
> +    qtest_outw(s, 0x3f4, 0x1600);
> +    qtest_outw(s, 0x3f4, 0x0000);
> +    qtest_outw(s, 0x3f4, 0x0000);
> +    qtest_outw(s, 0x3f4, 0x0000);
> +    qtest_outw(s, 0x3f4, 0x0200);
> +    qtest_outw(s, 0x3f4, 0x0200);
> +    qtest_outw(s, 0x3f4, 0x0000);
> +    qtest_outw(s, 0x3f4, 0x0000);
> +    qtest_outw(s, 0x3f4, 0x0000);

No idea what this does (looks like a 256-byte sector read read from 
sector 2 with EOT=0), but hits the problem and reproduces for me.

Unfortunately, I have the exact same problem with test_image as I did in 
the other series.

Hanna

> +    qtest_quit(s);
> +}
> +
>   int main(int argc, char **argv)
>   {
>       int fd;
> @@ -576,6 +595,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_3507", test_cve_2021_3507);
>   
>       ret = g_test_run();
>   

Re: [PATCH-for-6.2 2/2] tests/qtest/fdc-test: Add a regression test for CVE-2021-3507

[John Snow]

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

On Tue, Nov 23, 2021 at 11:08 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 18.11.21 12:57, Philippe Mathieu-Daudé wrote:
> > Add the reproducer from
> https://gitlab.com/qemu-project/qemu/-/issues/339
> >
> > Without the previous commit, when running 'make check-qtest-i386'
> > with QEMU configured with '--enable-sanitizers' we get:
> >
> >    ==4028352==ERROR: AddressSanitizer: heap-buffer-overflow on address
> 0x619000062a00 at pc 0x5626d03c491a bp 0x7ffdb4199410 sp 0x7ffdb4198bc0
> >    READ of size 786432 at 0x619000062a00 thread T0
> >        #0 0x5626d03c4919 in __asan_memcpy (qemu-system-i386+0x1e65919)
> >        #1 0x5626d1c023cc in flatview_write_continue
> softmmu/physmem.c:2787:13
> >        #2 0x5626d1bf0c0f in flatview_write softmmu/physmem.c:2822:14
> >        #3 0x5626d1bf0798 in address_space_write softmmu/physmem.c:2914:18
> >        #4 0x5626d1bf0f37 in address_space_rw softmmu/physmem.c:2924:16
> >        #5 0x5626d1bf14c8 in cpu_physical_memory_rw
> softmmu/physmem.c:2933:5
> >        #6 0x5626d0bd5649 in cpu_physical_memory_write
> include/exec/cpu-common.h:82:5
> >        #7 0x5626d0bd0a07 in i8257_dma_write_memory hw/dma/i8257.c:452:9
> >        #8 0x5626d09f825d in fdctrl_transfer_handler
> hw/block/fdc.c:1616:13
> >        #9 0x5626d0a048b4 in fdctrl_start_transfer hw/block/fdc.c:1539:13
> >        #10 0x5626d09f4c3e in fdctrl_write_data hw/block/fdc.c:2266:13
> >        #11 0x5626d09f22f7 in fdctrl_write hw/block/fdc.c:829:9
> >        #12 0x5626d1c20bc5 in portio_write softmmu/ioport.c:207:17
> >
> >    0x619000062a00 is located 0 bytes to the right of 512-byte region
> [0x619000062800,0x619000062a00)
> >    allocated by thread T0 here:
> >        #0 0x5626d03c66ec in posix_memalign (qemu-system-i386+0x1e676ec)
> >        #1 0x5626d2b988d4 in qemu_try_memalign util/oslib-posix.c:210:11
> >        #2 0x5626d2b98b0c in qemu_memalign util/oslib-posix.c:226:27
> >        #3 0x5626d09fbaf0 in fdctrl_realize_common hw/block/fdc.c:2341:20
> >        #4 0x5626d0a150ed in isabus_fdc_realize hw/block/fdc-isa.c:113:5
> >        #5 0x5626d2367935 in device_set_realized hw/core/qdev.c:531:13
> >
> >    SUMMARY: AddressSanitizer: heap-buffer-overflow
> (qemu-system-i386+0x1e65919) in __asan_memcpy
> >    Shadow bytes around the buggy address:
> >      0x0c32800044f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> >      0x0c3280004500: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >      0x0c3280004510: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >      0x0c3280004520: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >      0x0c3280004530: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >    =>0x0c3280004540:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> >      0x0c3280004550: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> >      0x0c3280004560: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> >      0x0c3280004570: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> >      0x0c3280004580: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> >      0x0c3280004590: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> >    Shadow byte legend (one shadow byte represents 8 application bytes):
> >      Addressable:           00
> >      Heap left redzone:       fa
> >      Freed heap region:       fd
> >    ==4028352==ABORTING
> >
> > Reported-by: Alexander Bulekov <alxndr@bu.edu>
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> >   tests/qtest/fdc-test.c | 20 ++++++++++++++++++++
> >   1 file changed, 20 insertions(+)
> >
> > diff --git a/tests/qtest/fdc-test.c b/tests/qtest/fdc-test.c
> > index 26b69f7c5cd..f164d972d10 100644
> > --- a/tests/qtest/fdc-test.c
> > +++ b/tests/qtest/fdc-test.c
> > @@ -546,6 +546,25 @@ static void fuzz_registers(void)
> >       }
> >   }
> >
> > +static void test_cve_2021_3507(void)
> > +{
> > +    QTestState *s;
> > +
> > +    s = qtest_initf("-nographic -m 32M -nodefaults "
> > +                    "-drive file=%s,format=raw,if=floppy", test_image);
> > +    qtest_outl(s, 0x9, 0x0a0206);
> > +    qtest_outw(s, 0x3f4, 0x1600);
> > +    qtest_outw(s, 0x3f4, 0x0000);
> > +    qtest_outw(s, 0x3f4, 0x0000);
> > +    qtest_outw(s, 0x3f4, 0x0000);
> > +    qtest_outw(s, 0x3f4, 0x0200);
> > +    qtest_outw(s, 0x3f4, 0x0200);
> > +    qtest_outw(s, 0x3f4, 0x0000);
> > +    qtest_outw(s, 0x3f4, 0x0000);
> > +    qtest_outw(s, 0x3f4, 0x0000);
>
> No idea what this does (looks like a 256-byte sector read read from
> sector 2 with EOT=0), but hits the problem and reproduces for me.
>
> Unfortunately, I have the exact same problem with test_image as I did in
> the other series.
>
> Hanna
>
> > +    qtest_quit(s);
> > +}
> > +
> >   int main(int argc, char **argv)
> >   {
> >       int fd;
> > @@ -576,6 +595,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_3507", test_cve_2021_3507);
> >
> >       ret = g_test_run();
> >
>

OK, I won't pull this one if there's a question over the suitability of the
test. I'm going to be away for the holiday until Monday, though. If the two
of you can agree that the fix and the test are good, though, I definitely
won't care if someone sends a PR for it.

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

Re: [PATCH-for-6.2 0/2] hw/block/fdc: Fix CVE-2021-3507

[Philippe Mathieu-Daudé]

ping for 6.2?

On 11/18/21 12:57, Philippe Mathieu-Daudé wrote:
> Trivial fix for CVE-2021-3507.
> 
> Philippe Mathieu-Daudé (2):
>   hw/block/fdc: Prevent end-of-track overrun (CVE-2021-3507)
>   tests/qtest/fdc-test: Add a regression test for CVE-2021-3507
> 
>  hw/block/fdc.c         |  8 ++++++++
>  tests/qtest/fdc-test.c | 20 ++++++++++++++++++++
>  2 files changed, 28 insertions(+)
> 

Re: [PATCH-for-6.2 0/2] hw/block/fdc: Fix CVE-2021-3507

[Jon Maloy]

On 11/18/21 06:57, Philippe Mathieu-Daudé wrote:
> Trivial fix for CVE-2021-3507.
>
> Philippe Mathieu-Daudé (2):
>    hw/block/fdc: Prevent end-of-track overrun (CVE-2021-3507)
>    tests/qtest/fdc-test: Add a regression test for CVE-2021-3507
>
>   hw/block/fdc.c         |  8 ++++++++
>   tests/qtest/fdc-test.c | 20 ++++++++++++++++++++
>   2 files changed, 28 insertions(+)
>
Series
Acked-by: Jon Maloy <jmaloy@redhat.com>

Re: [PATCH-for-6.2 0/2] hw/block/fdc: Fix CVE-2021-3507

[John Snow]

On Thu, Jan 27, 2022 at 3:11 PM Jon Maloy <jmaloy@redhat.com> wrote:
>
>
> On 11/18/21 06:57, Philippe Mathieu-Daudé wrote:
> > Trivial fix for CVE-2021-3507.
> >
> > Philippe Mathieu-Daudé (2):
> >    hw/block/fdc: Prevent end-of-track overrun (CVE-2021-3507)
> >    tests/qtest/fdc-test: Add a regression test for CVE-2021-3507
> >
> >   hw/block/fdc.c         |  8 ++++++++
> >   tests/qtest/fdc-test.c | 20 ++++++++++++++++++++
> >   2 files changed, 28 insertions(+)
> >
> Series
> Acked-by: Jon Maloy <jmaloy@redhat.com>
>

I could have sworn that Philippe said that this patch was incomplete
and to not merge it for 6.2, but maybe I mistook that for a different
series.

I seem to recall that this series didn't apply correctly in
conjunction with the fix for 2021-20196, but if there was a followup,
I missed it.

--js

Re: [PATCH-for-6.2 0/2] hw/block/fdc: Fix CVE-2021-3507

[Jon Maloy]

On 1/27/22 15:14, Jon Maloy wrote:
>
> On 11/18/21 06:57, Philippe Mathieu-Daudé wrote:
>> Trivial fix for CVE-2021-3507.
>>
>> Philippe Mathieu-Daudé (2):
>>    hw/block/fdc: Prevent end-of-track overrun (CVE-2021-3507)
>>    tests/qtest/fdc-test: Add a regression test for CVE-2021-3507
>>
>>   hw/block/fdc.c         |  8 ++++++++
>>   tests/qtest/fdc-test.c | 20 ++++++++++++++++++++
>>   2 files changed, 28 insertions(+)
>>
> Series
> Acked-by: Jon Maloy <jmaloy@redhat.com>

Philippe,
I hear from other sources that you earlier have qualified this one as 
"incomplete".
I am of course aware that this one, just like my own patch, is just a 
mitigation and not a complete correction of the erroneous calculation.
Or did you have anything else in mind?

Regards
///jon

Re: [PATCH-for-6.2 0/2] hw/block/fdc: Fix CVE-2021-3507

[Jon Maloy]

Trying again with correct email address.
///jon

On 2/6/22 14:15, Jon Maloy wrote:
>
>
> On 1/27/22 15:14, Jon Maloy wrote:
>>
>> On 11/18/21 06:57, Philippe Mathieu-Daudé wrote:
>>> Trivial fix for CVE-2021-3507.
>>>
>>> Philippe Mathieu-Daudé (2):
>>>    hw/block/fdc: Prevent end-of-track overrun (CVE-2021-3507)
>>>    tests/qtest/fdc-test: Add a regression test for CVE-2021-3507
>>>
>>>   hw/block/fdc.c         |  8 ++++++++
>>>   tests/qtest/fdc-test.c | 20 ++++++++++++++++++++
>>>   2 files changed, 28 insertions(+)
>>>
>> Series
>> Acked-by: Jon Maloy <jmaloy@redhat.com>
>
> Philippe,
> I hear from other sources that you earlier have qualified this one as 
> "incomplete".
> I am of course aware that this one, just like my own patch, is just a 
> mitigation and not a complete correction of the erroneous calculation.
> Or did you have anything else in mind?
>
> Regards
> ///jon
>

Re: [PATCH-for-6.2 0/2] hw/block/fdc: Fix CVE-2021-3507

[Thomas Huth]

On 06/02/2022 20.19, Jon Maloy wrote:
> Trying again with correct email address.
> ///jon
> 
> On 2/6/22 14:15, Jon Maloy wrote:
>>
>>
>> On 1/27/22 15:14, Jon Maloy wrote:
>>>
>>> On 11/18/21 06:57, Philippe Mathieu-Daudé wrote:
>>>> Trivial fix for CVE-2021-3507.
>>>>
>>>> Philippe Mathieu-Daudé (2):
>>>>    hw/block/fdc: Prevent end-of-track overrun (CVE-2021-3507)
>>>>    tests/qtest/fdc-test: Add a regression test for CVE-2021-3507
>>>>
>>>>   hw/block/fdc.c         |  8 ++++++++
>>>>   tests/qtest/fdc-test.c | 20 ++++++++++++++++++++
>>>>   2 files changed, 28 insertions(+)
>>>>
>>> Series
>>> Acked-by: Jon Maloy <jmaloy@redhat.com>
>>
>> Philippe,
>> I hear from other sources that you earlier have qualified this one as 
>> "incomplete".
>> I am of course aware that this one, just like my own patch, is just a 
>> mitigation and not a complete correction of the erroneous calculation.
>> Or did you have anything else in mind?

Any news on this one? It would be nice to get the CVE fixed for 7.0 ?

  Thomas

Re: [PATCH-for-6.2 0/2] hw/block/fdc: Fix CVE-2021-3507

[Jon Maloy]

On 3/10/22 12:14, Thomas Huth wrote:
> On 06/02/2022 20.19, Jon Maloy wrote:
>> Trying again with correct email address.
>> ///jon
>>
>> On 2/6/22 14:15, Jon Maloy wrote:
>>>
>>>
>>> On 1/27/22 15:14, Jon Maloy wrote:
>>>>
>>>> On 11/18/21 06:57, Philippe Mathieu-Daudé wrote:
>>>>> Trivial fix for CVE-2021-3507.
>>>>>
>>>>> Philippe Mathieu-Daudé (2):
>>>>>    hw/block/fdc: Prevent end-of-track overrun (CVE-2021-3507)
>>>>>    tests/qtest/fdc-test: Add a regression test for CVE-2021-3507
>>>>>
>>>>>   hw/block/fdc.c         |  8 ++++++++
>>>>>   tests/qtest/fdc-test.c | 20 ++++++++++++++++++++
>>>>>   2 files changed, 28 insertions(+)
>>>>>
>>>> Series
>>>> Acked-by: Jon Maloy <jmaloy@redhat.com>
>>>
>>> Philippe,
>>> I hear from other sources that you earlier have qualified this one 
>>> as "incomplete".
>>> I am of course aware that this one, just like my own patch, is just 
>>> a mitigation and not a complete correction of the erroneous 
>>> calculation.
>>> Or did you have anything else in mind?
>
> Any news on this one? It would be nice to get the CVE fixed for 7.0 ?
>
>  Thomas
>
The ball is currently with John Snow, as I understand it.
The concern is that this fix may not take the driver back to a 
consistent state, so that we may have other problems later.
Maybe Philippe can chip in with a comment here?

///jon

Re: [PATCH-for-6.2 0/2] hw/block/fdc: Fix CVE-2021-3507

[Thomas Huth]

On 10/03/2022 18.53, Jon Maloy wrote:
> 
> On 3/10/22 12:14, Thomas Huth wrote:
>> On 06/02/2022 20.19, Jon Maloy wrote:
>>> Trying again with correct email address.
>>> ///jon
>>>
>>> On 2/6/22 14:15, Jon Maloy wrote:
>>>>
>>>>
>>>> On 1/27/22 15:14, Jon Maloy wrote:
>>>>>
>>>>> On 11/18/21 06:57, Philippe Mathieu-Daudé wrote:
>>>>>> Trivial fix for CVE-2021-3507.
>>>>>>
>>>>>> Philippe Mathieu-Daudé (2):
>>>>>>    hw/block/fdc: Prevent end-of-track overrun (CVE-2021-3507)
>>>>>>    tests/qtest/fdc-test: Add a regression test for CVE-2021-3507
>>>>>>
>>>>>>   hw/block/fdc.c         |  8 ++++++++
>>>>>>   tests/qtest/fdc-test.c | 20 ++++++++++++++++++++
>>>>>>   2 files changed, 28 insertions(+)
>>>>>>
>>>>> Series
>>>>> Acked-by: Jon Maloy <jmaloy@redhat.com>
>>>>
>>>> Philippe,
>>>> I hear from other sources that you earlier have qualified this one as 
>>>> "incomplete".
>>>> I am of course aware that this one, just like my own patch, is just a 
>>>> mitigation and not a complete correction of the erroneous calculation.
>>>> Or did you have anything else in mind?
>>
>> Any news on this one? It would be nice to get the CVE fixed for 7.0 ?
>>
>>  Thomas
>>
> The ball is currently with John Snow, as I understand it.
> The concern is that this fix may not take the driver back to a consistent 
> state, so that we may have other problems later.
> Maybe Philippe can chip in with a comment here?

John, Philippe, any ideas how to move this forward?

  Thomas

Re: [PATCH-for-6.2 0/2] hw/block/fdc: Fix CVE-2021-3507

[John Snow]

On Fri, Mar 18, 2022 at 2:50 PM Thomas Huth <thuth@redhat.com> wrote:
>
> On 10/03/2022 18.53, Jon Maloy wrote:
> >
> > On 3/10/22 12:14, Thomas Huth wrote:
> >> On 06/02/2022 20.19, Jon Maloy wrote:
> >>> Trying again with correct email address.
> >>> ///jon
> >>>
> >>> On 2/6/22 14:15, Jon Maloy wrote:
> >>>>
> >>>>
> >>>> On 1/27/22 15:14, Jon Maloy wrote:
> >>>>>
> >>>>> On 11/18/21 06:57, Philippe Mathieu-Daudé wrote:
> >>>>>> Trivial fix for CVE-2021-3507.
> >>>>>>
> >>>>>> Philippe Mathieu-Daudé (2):
> >>>>>>    hw/block/fdc: Prevent end-of-track overrun (CVE-2021-3507)
> >>>>>>    tests/qtest/fdc-test: Add a regression test for CVE-2021-3507
> >>>>>>
> >>>>>>   hw/block/fdc.c         |  8 ++++++++
> >>>>>>   tests/qtest/fdc-test.c | 20 ++++++++++++++++++++
> >>>>>>   2 files changed, 28 insertions(+)
> >>>>>>
> >>>>> Series
> >>>>> Acked-by: Jon Maloy <jmaloy@redhat.com>
> >>>>
> >>>> Philippe,
> >>>> I hear from other sources that you earlier have qualified this one as
> >>>> "incomplete".
> >>>> I am of course aware that this one, just like my own patch, is just a
> >>>> mitigation and not a complete correction of the erroneous calculation.
> >>>> Or did you have anything else in mind?
> >>
> >> Any news on this one? It would be nice to get the CVE fixed for 7.0 ?
> >>
> >>  Thomas
> >>
> > The ball is currently with John Snow, as I understand it.
> > The concern is that this fix may not take the driver back to a consistent
> > state, so that we may have other problems later.
> > Maybe Philippe can chip in with a comment here?
>
> John, Philippe, any ideas how to move this forward?
>
>   Thomas
>

The ball is indeed in my court. I need to audit this properly and get
the patch re-applied, and get tests passing.

As a personal favor: Could you please ping me on IRC tomorrow about
this? (Well, later today, for you.)

Re: [PATCH-for-6.2 0/2] hw/block/fdc: Fix CVE-2021-3507

[Kevin Wolf]

Am 23.03.2022 um 03:25 hat John Snow geschrieben:
> On Fri, Mar 18, 2022 at 2:50 PM Thomas Huth <thuth@redhat.com> wrote:
> >
> > On 10/03/2022 18.53, Jon Maloy wrote:
> > >
> > > On 3/10/22 12:14, Thomas Huth wrote:
> > >> On 06/02/2022 20.19, Jon Maloy wrote:
> > >>> Trying again with correct email address.
> > >>> ///jon
> > >>>
> > >>> On 2/6/22 14:15, Jon Maloy wrote:
> > >>>>
> > >>>>
> > >>>> On 1/27/22 15:14, Jon Maloy wrote:
> > >>>>>
> > >>>>> On 11/18/21 06:57, Philippe Mathieu-Daudé wrote:
> > >>>>>> Trivial fix for CVE-2021-3507.
> > >>>>>>
> > >>>>>> Philippe Mathieu-Daudé (2):
> > >>>>>>    hw/block/fdc: Prevent end-of-track overrun (CVE-2021-3507)
> > >>>>>>    tests/qtest/fdc-test: Add a regression test for CVE-2021-3507
> > >>>>>>
> > >>>>>>   hw/block/fdc.c         |  8 ++++++++
> > >>>>>>   tests/qtest/fdc-test.c | 20 ++++++++++++++++++++
> > >>>>>>   2 files changed, 28 insertions(+)
> > >>>>>>
> > >>>>> Series
> > >>>>> Acked-by: Jon Maloy <jmaloy@redhat.com>
> > >>>>
> > >>>> Philippe,
> > >>>> I hear from other sources that you earlier have qualified this one as
> > >>>> "incomplete".
> > >>>> I am of course aware that this one, just like my own patch, is just a
> > >>>> mitigation and not a complete correction of the erroneous calculation.
> > >>>> Or did you have anything else in mind?
> > >>
> > >> Any news on this one? It would be nice to get the CVE fixed for 7.0 ?
> > >>
> > >>  Thomas
> > >>
> > > The ball is currently with John Snow, as I understand it.
> > > The concern is that this fix may not take the driver back to a consistent
> > > state, so that we may have other problems later.
> > > Maybe Philippe can chip in with a comment here?
> >
> > John, Philippe, any ideas how to move this forward?
> >
> >   Thomas
> >
> 
> The ball is indeed in my court. I need to audit this properly and get
> the patch re-applied, and get tests passing.
> 
> As a personal favor: Could you please ping me on IRC tomorrow about
> this? (Well, later today, for you.)

Going through old patches... Is this one still open?

Kevin

Re: [PATCH-for-6.2 0/2] hw/block/fdc: Fix CVE-2021-3507

[Jon Maloy]

On 5/3/22 05:59, Kevin Wolf wrote:
> Am 23.03.2022 um 03:25 hat John Snow geschrieben:
>> On Fri, Mar 18, 2022 at 2:50 PM Thomas Huth <thuth@redhat.com> wrote:
>>> On 10/03/2022 18.53, Jon Maloy wrote:
>>>> On 3/10/22 12:14, Thomas Huth wrote:
>>>>> On 06/02/2022 20.19, Jon Maloy wrote:
>>>>>> Trying again with correct email address.
>>>>>> ///jon
>>>>>>
>>>>>> On 2/6/22 14:15, Jon Maloy wrote:
>>>>>>>
>>>>>>> On 1/27/22 15:14, Jon Maloy wrote:
>>>>>>>> On 11/18/21 06:57, Philippe Mathieu-Daudé wrote:
>>>>>>>>> Trivial fix for CVE-2021-3507.
>>>>>>>>>
>>>>>>>>> Philippe Mathieu-Daudé (2):
>>>>>>>>>     hw/block/fdc: Prevent end-of-track overrun (CVE-2021-3507)
>>>>>>>>>     tests/qtest/fdc-test: Add a regression test for CVE-2021-3507
>>>>>>>>>
>>>>>>>>>    hw/block/fdc.c         |  8 ++++++++
>>>>>>>>>    tests/qtest/fdc-test.c | 20 ++++++++++++++++++++
>>>>>>>>>    2 files changed, 28 insertions(+)
>>>>>>>>>
>>>>>>>> Series
>>>>>>>> Acked-by: Jon Maloy <jmaloy@redhat.com>
>>>>>>> Philippe,
>>>>>>> I hear from other sources that you earlier have qualified this one as
>>>>>>> "incomplete".
>>>>>>> I am of course aware that this one, just like my own patch, is just a
>>>>>>> mitigation and not a complete correction of the erroneous calculation.
>>>>>>> Or did you have anything else in mind?
>>>>> Any news on this one? It would be nice to get the CVE fixed for 7.0 ?
>>>>>
>>>>>   Thomas
>>>>>
>>>> The ball is currently with John Snow, as I understand it.
>>>> The concern is that this fix may not take the driver back to a consistent
>>>> state, so that we may have other problems later.
>>>> Maybe Philippe can chip in with a comment here?
>>> John, Philippe, any ideas how to move this forward?
>>>
>>>    Thomas
>>>
>> The ball is indeed in my court. I need to audit this properly and get
>> the patch re-applied, and get tests passing.
>>
>> As a personal favor: Could you please ping me on IRC tomorrow about
>> this? (Well, later today, for you.)
> Going through old patches... Is this one still open?
>
> Kevin
>
Yes, it is.

///jon

Re: [PATCH-for-6.2 0/2] hw/block/fdc: Fix CVE-2021-3507

[Kevin Wolf]

Am 03.05.2022 um 18:21 hat Jon Maloy geschrieben:
> 
> 
> On 5/3/22 05:59, Kevin Wolf wrote:
> > Am 23.03.2022 um 03:25 hat John Snow geschrieben:
> > > On Fri, Mar 18, 2022 at 2:50 PM Thomas Huth <thuth@redhat.com> wrote:
> > > > On 10/03/2022 18.53, Jon Maloy wrote:
> > > > > On 3/10/22 12:14, Thomas Huth wrote:
> > > > > > On 06/02/2022 20.19, Jon Maloy wrote:
> > > > > > > Trying again with correct email address.
> > > > > > > ///jon
> > > > > > > 
> > > > > > > On 2/6/22 14:15, Jon Maloy wrote:
> > > > > > > > 
> > > > > > > > On 1/27/22 15:14, Jon Maloy wrote:
> > > > > > > > > On 11/18/21 06:57, Philippe Mathieu-Daudé wrote:
> > > > > > > > > > Trivial fix for CVE-2021-3507.
> > > > > > > > > > 
> > > > > > > > > > Philippe Mathieu-Daudé (2):
> > > > > > > > > >     hw/block/fdc: Prevent end-of-track overrun (CVE-2021-3507)
> > > > > > > > > >     tests/qtest/fdc-test: Add a regression test for CVE-2021-3507
> > > > > > > > > > 
> > > > > > > > > >    hw/block/fdc.c         |  8 ++++++++
> > > > > > > > > >    tests/qtest/fdc-test.c | 20 ++++++++++++++++++++
> > > > > > > > > >    2 files changed, 28 insertions(+)
> > > > > > > > > > 
> > > > > > > > > Series
> > > > > > > > > Acked-by: Jon Maloy <jmaloy@redhat.com>
> > > > > > > > Philippe,
> > > > > > > > I hear from other sources that you earlier have qualified this one as
> > > > > > > > "incomplete".
> > > > > > > > I am of course aware that this one, just like my own patch, is just a
> > > > > > > > mitigation and not a complete correction of the erroneous calculation.
> > > > > > > > Or did you have anything else in mind?
> > > > > > Any news on this one? It would be nice to get the CVE fixed for 7.0 ?
> > > > > > 
> > > > > >   Thomas
> > > > > > 
> > > > > The ball is currently with John Snow, as I understand it.
> > > > > The concern is that this fix may not take the driver back to a consistent
> > > > > state, so that we may have other problems later.
> > > > > Maybe Philippe can chip in with a comment here?
> > > > John, Philippe, any ideas how to move this forward?
> > > > 
> > > >    Thomas
> > > > 
> > > The ball is indeed in my court. I need to audit this properly and get
> > > the patch re-applied, and get tests passing.
> > > 
> > > As a personal favor: Could you please ping me on IRC tomorrow about
> > > this? (Well, later today, for you.)
> > Going through old patches... Is this one still open?
> > 
> > Kevin
> > 
> Yes, it is.

I was hoping that John would get back to it after my ping, but doesn't
look like it.

So this may not be the perfect fix and the perfect test, but it's
certainly better than having nothing for multiple releases. I fixed up
the test with the snapshot=on that Alexander suggested (this also fixes
the file locking problem Hanna had and that I saw, too) and applied it
to my block branch.

Kevin