| Message ID | 20240404085549.16987-1-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCH-for-9.0] hw/sd/sdhci: Discard excess of data written to Buffer Data Port register | expand |
On Thu, Apr 4, 2024 at 10:55 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > Per "SD Host Controller Standard Specification Version 3.00": > > * 1.7 Buffer Control > > - 1.7.1 Control of Buffer Pointer > > (3) Buffer Control with Block Size > > In case of write operation, the buffer accumulates the data > written through the Buffer Data Port register. When the buffer > pointer reaches the block size, Buffer Write Enable in the > Present State register changes 1 to 0. It means no more data > can be written to the buffer. Excess data of the last write is > ignored. For example, if just lower 2 bytes data can be written > to the buffer and a 32-bit (4-byte) block of data is written to > the Buffer Data Port register, the lower 2 bytes of data is > written to the buffer and the upper 2 bytes is ignored. > > Discard the excess of data to avoid overflow reported by fuzzer: > > $ cat << EOF | qemu-system-i386 \ > -display none -nodefaults \ > -machine accel=qtest -m 512M \ > -device sdhci-pci,sd-spec-version=3 \ > -device sd-card,drive=mydrive \ > -drive if=none,index=0,file=null-co://,format=raw,id=mydrive -nographic \ > -qtest stdio > outl 0xcf8 0x80001013 > outl 0xcfc 0x91 > outl 0xcf8 0x80001001 > outl 0xcfc 0x06000000 > write 0x9100002c 0x1 0x05 > write 0x91000058 0x1 0x16 > write 0x91000005 0x1 0x04 > write 0x91000028 0x1 0x08 > write 0x16 0x1 0x21 > write 0x19 0x1 0x20 > write 0x9100000c 0x1 0x01 > write 0x9100000e 0x1 0x20 > write 0x9100000f 0x1 0x00 > write 0x9100000c 0x1 0x00 > write 0x91000020 0x1 0x00 > EOF > > Stack trace (part): > ================================================================= > ==89993==ERROR: AddressSanitizer: heap-buffer-overflow on address > 0x615000029900 at pc 0x55d5f885700d bp 0x7ffc1e1e9470 sp 0x7ffc1e1e9468 > WRITE of size 1 at 0x615000029900 thread T0 > #0 0x55d5f885700c in sdhci_write_dataport hw/sd/sdhci.c:564:39 > #1 0x55d5f8849150 in sdhci_write hw/sd/sdhci.c:1223:13 > #2 0x55d5fa01db63 in memory_region_write_accessor system/memory.c:497:5 > #3 0x55d5fa01d245 in access_with_adjusted_size system/memory.c:573:18 > #4 0x55d5fa01b1a9 in memory_region_dispatch_write system/memory.c:1521:16 > #5 0x55d5fa09f5c9 in flatview_write_continue system/physmem.c:2711:23 > #6 0x55d5fa08f78b in flatview_write system/physmem.c:2753:12 > #7 0x55d5fa08f258 in address_space_write system/physmem.c:2860:18 > ... > 0x615000029900 is located 0 bytes to the right of 512-byte region > [0x615000029700,0x615000029900) allocated by thread T0 here: > #0 0x55d5f7237b27 in __interceptor_calloc > #1 0x7f9e36dd4c50 in g_malloc0 > #2 0x55d5f88672f7 in sdhci_pci_realize hw/sd/sdhci-pci.c:36:5 > #3 0x55d5f844b582 in pci_qdev_realize hw/pci/pci.c:2092:9 > #4 0x55d5fa2ee74b in device_set_realized hw/core/qdev.c:510:13 > #5 0x55d5fa325bfb in property_set_bool qom/object.c:2358:5 > #6 0x55d5fa31ea45 in object_property_set qom/object.c:1472:5 > #7 0x55d5fa332509 in object_property_set_qobject om/qom-qobject.c:28:10 > #8 0x55d5fa31f6ed in object_property_set_bool qom/object.c:1541:15 > #9 0x55d5fa2e2948 in qdev_realize hw/core/qdev.c:292:12 > #10 0x55d5f8eed3f1 in qdev_device_add_from_qdict system/qdev-monitor.c:719:10 > #11 0x55d5f8eef7ff in qdev_device_add system/qdev-monitor.c:738:11 > #12 0x55d5f8f211f0 in device_init_func system/vl.c:1200:11 > #13 0x55d5fad0877d in qemu_opts_foreach util/qemu-option.c:1135:14 > #14 0x55d5f8f0df9c in qemu_create_cli_devices system/vl.c:2638:5 > #15 0x55d5f8f0db24 in qmp_x_exit_preconfig system/vl.c:2706:5 > #16 0x55d5f8f14dc0 in qemu_init system/vl.c:3737:9 > ... > SUMMARY: AddressSanitizer: heap-buffer-overflow hw/sd/sdhci.c:564:39 > in sdhci_write_dataport > > Cc: qemu-stable@nongnu.org > Fixes: d7dfca0807 ("hw/sdhci: introduce standard SD host controller") > Buglink: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=58813 > Reported-by: Alexander Bulekov <alxndr@bu.edu> > Reported-by: Chuhong Yuan <hslester96@gmail.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > hw/sd/sdhci.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c > index c5e0bc018b..2dd88fa139 100644 > --- a/hw/sd/sdhci.c > +++ b/hw/sd/sdhci.c > @@ -552,7 +552,7 @@ static void sdhci_write_block_to_card(SDHCIState *s) > * register */ > static void sdhci_write_dataport(SDHCIState *s, uint32_t value, unsigned size) > { > - unsigned i; > + unsigned i, available; > > /* Check that there is free space left in a buffer */ > if (!(s->prnsts & SDHC_SPACE_AVAILABLE)) { > @@ -560,6 +560,14 @@ static void sdhci_write_dataport(SDHCIState *s, uint32_t value, unsigned size) > return; > } > > + available = s->buf_maxsz - s->data_count; > + if (size > available) { > + qemu_log_mask(LOG_GUEST_ERROR, "SDHC buffer data full (size: %"PRIu32")" > + " discarding %u byte%s\n", > + s->buf_maxsz, size - available, > + size - available > 1 ? "s" : ""); > + size = available; /* Excess data of the last write is ignored. */ > + } > for (i = 0; i < size; i++) { > s->fifo_buffer[s->data_count] = value & 0xFF; > s->data_count++; > -- > 2.41.0 > Thank you Philippe. This was assigned CVE-2024-3447.
On Thu, 4 Apr 2024 at 09:56, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > Per "SD Host Controller Standard Specification Version 3.00": > > * 1.7 Buffer Control > > - 1.7.1 Control of Buffer Pointer > > (3) Buffer Control with Block Size > > In case of write operation, the buffer accumulates the data > written through the Buffer Data Port register. When the buffer > pointer reaches the block size, Buffer Write Enable in the > Present State register changes 1 to 0. It means no more data > can be written to the buffer. Excess data of the last write is > ignored. For example, if just lower 2 bytes data can be written > to the buffer and a 32-bit (4-byte) block of data is written to > the Buffer Data Port register, the lower 2 bytes of data is > written to the buffer and the upper 2 bytes is ignored. > > Discard the excess of data to avoid overflow reported by fuzzer: > > $ cat << EOF | qemu-system-i386 \ > -display none -nodefaults \ > -machine accel=qtest -m 512M \ > -device sdhci-pci,sd-spec-version=3 \ > -device sd-card,drive=mydrive \ > -drive if=none,index=0,file=null-co://,format=raw,id=mydrive -nographic \ > -qtest stdio > outl 0xcf8 0x80001013 > outl 0xcfc 0x91 > outl 0xcf8 0x80001001 > outl 0xcfc 0x06000000 > write 0x9100002c 0x1 0x05 > write 0x91000058 0x1 0x16 > write 0x91000005 0x1 0x04 > write 0x91000028 0x1 0x08 > write 0x16 0x1 0x21 > write 0x19 0x1 0x20 > write 0x9100000c 0x1 0x01 > write 0x9100000e 0x1 0x20 > write 0x9100000f 0x1 0x00 > write 0x9100000c 0x1 0x00 > write 0x91000020 0x1 0x00 > EOF > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c > index c5e0bc018b..2dd88fa139 100644 > --- a/hw/sd/sdhci.c > +++ b/hw/sd/sdhci.c > @@ -552,7 +552,7 @@ static void sdhci_write_block_to_card(SDHCIState *s) > * register */ > static void sdhci_write_dataport(SDHCIState *s, uint32_t value, unsigned size) > { > - unsigned i; > + unsigned i, available; > > /* Check that there is free space left in a buffer */ > if (!(s->prnsts & SDHC_SPACE_AVAILABLE)) { > @@ -560,6 +560,14 @@ static void sdhci_write_dataport(SDHCIState *s, uint32_t value, unsigned size) > return; > } > > + available = s->buf_maxsz - s->data_count; > + if (size > available) { > + qemu_log_mask(LOG_GUEST_ERROR, "SDHC buffer data full (size: %"PRIu32")" > + " discarding %u byte%s\n", > + s->buf_maxsz, size - available, > + size - available > 1 ? "s" : ""); > + size = available; /* Excess data of the last write is ignored. */ > + } > for (i = 0; i < size; i++) { > s->fifo_buffer[s->data_count] = value & 0xFF; > s->data_count++; So, this will definitely avoid the buffer overrun, and the quoted text also suggests that we should not be doing the "if sdhci_write_block_to_card() writes the data then keep going with the rest of the bytes in the value for the start of the new block". (With this change we could move the "if (s->data_count >= (s->blksize & BLOCK_SIZE_MASK)) ..." out of the for() loop and down to the bottom of the function.) But I'm not sure it fixes the underlying cause of the problem, because the repro case isn't writing a multi-byte value, it's only writing a single byte. It looks from the code like if there's no space in the buffer then SDHC_SPACE_AVAILABLE should be clear in the present-status register, but that has somehow got out of sync. The way the repro from the fuzzer toggles the device in and out of DMA mode looks suspicious about how that out-of-sync situation might have come about. thanks -- PMM
On Mon, 8 Apr 2024 at 13:34, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Thu, 4 Apr 2024 at 09:56, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c > > index c5e0bc018b..2dd88fa139 100644 > > --- a/hw/sd/sdhci.c > > +++ b/hw/sd/sdhci.c > > @@ -552,7 +552,7 @@ static void sdhci_write_block_to_card(SDHCIState *s) > > * register */ > > static void sdhci_write_dataport(SDHCIState *s, uint32_t value, unsigned size) > > { > > - unsigned i; > > + unsigned i, available; > > > > /* Check that there is free space left in a buffer */ > > if (!(s->prnsts & SDHC_SPACE_AVAILABLE)) { > > @@ -560,6 +560,14 @@ static void sdhci_write_dataport(SDHCIState *s, uint32_t value, unsigned size) > > return; > > } > > > > + available = s->buf_maxsz - s->data_count; > > + if (size > available) { > > + qemu_log_mask(LOG_GUEST_ERROR, "SDHC buffer data full (size: %"PRIu32")" > > + " discarding %u byte%s\n", > > + s->buf_maxsz, size - available, > > + size - available > 1 ? "s" : ""); > > + size = available; /* Excess data of the last write is ignored. */ > > + } > > for (i = 0; i < size; i++) { > > s->fifo_buffer[s->data_count] = value & 0xFF; > > s->data_count++; > > So, this will definitely avoid the buffer overrun, and the > quoted text also suggests that we should not be doing the > "if sdhci_write_block_to_card() writes the data then keep > going with the rest of the bytes in the value for the start > of the new block". (With this change we could move the > "if (s->data_count >= (s->blksize & BLOCK_SIZE_MASK)) ..." > out of the for() loop and down to the bottom of the function.) > > But I'm not sure it fixes the underlying cause of the problem, > because the repro case isn't writing a multi-byte value, it's > only writing a single byte. > > It looks from the code like if there's no space in the > buffer then SDHC_SPACE_AVAILABLE should be clear in the > present-status register, but that has somehow got out of sync. > The way the repro from the fuzzer toggles the device in and > out of DMA mode looks suspicious about how that out-of-sync > situation might have come about. It looks like the spec says that TRNMOD writes are supposed to be ignored while the Command Inhibit (DAT) bit is set, which would forbid this kind of "turn DMA off in the middle of a DMA transfer" silliness: diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index c5e0bc018b0..9cd887b7f30 100644 --- a/hw/sd/sdhci.c +++ b/hw/sd/sdhci.c @@ -1208,6 +1208,12 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size) if (!(s->capareg & R_SDHC_CAPAB_SDMA_MASK)) { value &= ~SDHC_TRNS_DMA; } + + /* TRNMOD writes are inhibited while Command Inhibit (DAT) is true */ + if (s->prnsts & SDHC_DATA_INHIBIT) { + mask |= 0xffff; + } + MASKED_WRITE(s->trnmod, mask, value & SDHC_TRNMOD_MASK); MASKED_WRITE(s->cmdreg, mask >> 16, value >> 16); And if you forbid that TRNMOD write that turns off the DMA then the write to BDATA is no longer an overrun. So another approach here would be to do that, plus an assert() at the point of writing into fifo_buffer[] in case there are other ways to get there with a bogus SPACE_AVAILABLE setting. (That way any further bug is only "badly behaved code makes QEMU assert" rather than an overrun.) The spec is annoyingly vague about the exact conditions when writing to the used-in-dma registers like TRNMOD and the block-size register. Currently we use the TRANSFERRING_DATA() condition to block writes to BLKSIZE, and you might have thought TRNMOD would be the same, but apparently not. thanks -- PMM
On Mon, 8 Apr 2024 at 17:42, Peter Maydell <peter.maydell@linaro.org> wrote: > So another approach here would be... That said, this is all quite complicated looking, so for 9.0 and backports at least this patch is fine. Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
On 9/4/24 13:35, Peter Maydell wrote: > On Mon, 8 Apr 2024 at 17:42, Peter Maydell <peter.maydell@linaro.org> wrote: >> So another approach here would be... > > That said, this is all quite complicated looking, so > for 9.0 and backports at least this patch is fine. Your patch looks like the correct fix, and doesn't seem that complicated (nor hard to backport), so I'll send a v2 in case. > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > thanks > -- PMM
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index c5e0bc018b..2dd88fa139 100644 --- a/hw/sd/sdhci.c +++ b/hw/sd/sdhci.c @@ -552,7 +552,7 @@ static void sdhci_write_block_to_card(SDHCIState *s) * register */ static void sdhci_write_dataport(SDHCIState *s, uint32_t value, unsigned size) { - unsigned i; + unsigned i, available; /* Check that there is free space left in a buffer */ if (!(s->prnsts & SDHC_SPACE_AVAILABLE)) { @@ -560,6 +560,14 @@ static void sdhci_write_dataport(SDHCIState *s, uint32_t value, unsigned size) return; } + available = s->buf_maxsz - s->data_count; + if (size > available) { + qemu_log_mask(LOG_GUEST_ERROR, "SDHC buffer data full (size: %"PRIu32")" + " discarding %u byte%s\n", + s->buf_maxsz, size - available, + size - available > 1 ? "s" : ""); + size = available; /* Excess data of the last write is ignored. */ + } for (i = 0; i < size; i++) { s->fifo_buffer[s->data_count] = value & 0xFF; s->data_count++;
Per "SD Host Controller Standard Specification Version 3.00": * 1.7 Buffer Control - 1.7.1 Control of Buffer Pointer (3) Buffer Control with Block Size In case of write operation, the buffer accumulates the data written through the Buffer Data Port register. When the buffer pointer reaches the block size, Buffer Write Enable in the Present State register changes 1 to 0. It means no more data can be written to the buffer. Excess data of the last write is ignored. For example, if just lower 2 bytes data can be written to the buffer and a 32-bit (4-byte) block of data is written to the Buffer Data Port register, the lower 2 bytes of data is written to the buffer and the upper 2 bytes is ignored. Discard the excess of data to avoid overflow reported by fuzzer: $ cat << EOF | qemu-system-i386 \ -display none -nodefaults \ -machine accel=qtest -m 512M \ -device sdhci-pci,sd-spec-version=3 \ -device sd-card,drive=mydrive \ -drive if=none,index=0,file=null-co://,format=raw,id=mydrive -nographic \ -qtest stdio outl 0xcf8 0x80001013 outl 0xcfc 0x91 outl 0xcf8 0x80001001 outl 0xcfc 0x06000000 write 0x9100002c 0x1 0x05 write 0x91000058 0x1 0x16 write 0x91000005 0x1 0x04 write 0x91000028 0x1 0x08 write 0x16 0x1 0x21 write 0x19 0x1 0x20 write 0x9100000c 0x1 0x01 write 0x9100000e 0x1 0x20 write 0x9100000f 0x1 0x00 write 0x9100000c 0x1 0x00 write 0x91000020 0x1 0x00 EOF Stack trace (part): ================================================================= ==89993==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x615000029900 at pc 0x55d5f885700d bp 0x7ffc1e1e9470 sp 0x7ffc1e1e9468 WRITE of size 1 at 0x615000029900 thread T0 #0 0x55d5f885700c in sdhci_write_dataport hw/sd/sdhci.c:564:39 #1 0x55d5f8849150 in sdhci_write hw/sd/sdhci.c:1223:13 #2 0x55d5fa01db63 in memory_region_write_accessor system/memory.c:497:5 #3 0x55d5fa01d245 in access_with_adjusted_size system/memory.c:573:18 #4 0x55d5fa01b1a9 in memory_region_dispatch_write system/memory.c:1521:16 #5 0x55d5fa09f5c9 in flatview_write_continue system/physmem.c:2711:23 #6 0x55d5fa08f78b in flatview_write system/physmem.c:2753:12 #7 0x55d5fa08f258 in address_space_write system/physmem.c:2860:18 ... 0x615000029900 is located 0 bytes to the right of 512-byte region [0x615000029700,0x615000029900) allocated by thread T0 here: #0 0x55d5f7237b27 in __interceptor_calloc #1 0x7f9e36dd4c50 in g_malloc0 #2 0x55d5f88672f7 in sdhci_pci_realize hw/sd/sdhci-pci.c:36:5 #3 0x55d5f844b582 in pci_qdev_realize hw/pci/pci.c:2092:9 #4 0x55d5fa2ee74b in device_set_realized hw/core/qdev.c:510:13 #5 0x55d5fa325bfb in property_set_bool qom/object.c:2358:5 #6 0x55d5fa31ea45 in object_property_set qom/object.c:1472:5 #7 0x55d5fa332509 in object_property_set_qobject om/qom-qobject.c:28:10 #8 0x55d5fa31f6ed in object_property_set_bool qom/object.c:1541:15 #9 0x55d5fa2e2948 in qdev_realize hw/core/qdev.c:292:12 #10 0x55d5f8eed3f1 in qdev_device_add_from_qdict system/qdev-monitor.c:719:10 #11 0x55d5f8eef7ff in qdev_device_add system/qdev-monitor.c:738:11 #12 0x55d5f8f211f0 in device_init_func system/vl.c:1200:11 #13 0x55d5fad0877d in qemu_opts_foreach util/qemu-option.c:1135:14 #14 0x55d5f8f0df9c in qemu_create_cli_devices system/vl.c:2638:5 #15 0x55d5f8f0db24 in qmp_x_exit_preconfig system/vl.c:2706:5 #16 0x55d5f8f14dc0 in qemu_init system/vl.c:3737:9 ... SUMMARY: AddressSanitizer: heap-buffer-overflow hw/sd/sdhci.c:564:39 in sdhci_write_dataport Cc: qemu-stable@nongnu.org Fixes: d7dfca0807 ("hw/sdhci: introduce standard SD host controller") Buglink: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=58813 Reported-by: Alexander Bulekov <alxndr@bu.edu> Reported-by: Chuhong Yuan <hslester96@gmail.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/sd/sdhci.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)