Re: [PATCH-for-9.0] hw/sd/sdhci: Discard excess of data written to Buffer Data Port register

[Mauro Matteo Cascella]

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.

-- 
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0