|
| 1 | +From 9e4b27ca6bf4974f169bbca7f3dca117b1208b6f Mon Sep 17 00:00:00 2001 |
| 2 | +From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= <philmd@linaro.org> |
| 3 | +Date: Tue, 9 Apr 2024 16:19:27 +0200 |
| 4 | +Subject: [PATCH] hw/sd/sdhci: Do not update TRNMOD when Command Inhibit (DAT) |
| 5 | + is set |
| 6 | +MIME-Version: 1.0 |
| 7 | +Content-Type: text/plain; charset=UTF-8 |
| 8 | +Content-Transfer-Encoding: 8bit |
| 9 | + |
| 10 | +Per "SD Host Controller Standard Specification Version 3.00": |
| 11 | + |
| 12 | + * 2.2.5 Transfer Mode Register (Offset 00Ch) |
| 13 | + |
| 14 | + Writes to this register shall be ignored when the Command |
| 15 | + Inhibit (DAT) in the Present State register is 1. |
| 16 | + |
| 17 | +Do not update the TRNMOD register when Command Inhibit (DAT) |
| 18 | +bit is set to avoid the present-status register going out of |
| 19 | +sync, leading to malicious guest using DMA mode and overflowing |
| 20 | +the FIFO buffer: |
| 21 | + |
| 22 | + $ cat << EOF | qemu-system-i386 \ |
| 23 | + -display none -nographic -nodefaults \ |
| 24 | + -machine accel=qtest -m 512M \ |
| 25 | + -device sdhci-pci,sd-spec-version=3 \ |
| 26 | + -device sd-card,drive=mydrive \ |
| 27 | + -drive if=none,index=0,file=null-co://,format=raw,id=mydrive \ |
| 28 | + -qtest stdio |
| 29 | + outl 0xcf8 0x80001013 |
| 30 | + outl 0xcfc 0x91 |
| 31 | + outl 0xcf8 0x80001001 |
| 32 | + outl 0xcfc 0x06000000 |
| 33 | + write 0x9100002c 0x1 0x05 |
| 34 | + write 0x91000058 0x1 0x16 |
| 35 | + write 0x91000005 0x1 0x04 |
| 36 | + write 0x91000028 0x1 0x08 |
| 37 | + write 0x16 0x1 0x21 |
| 38 | + write 0x19 0x1 0x20 |
| 39 | + write 0x9100000c 0x1 0x01 |
| 40 | + write 0x9100000e 0x1 0x20 |
| 41 | + write 0x9100000f 0x1 0x00 |
| 42 | + write 0x9100000c 0x1 0x00 |
| 43 | + write 0x91000020 0x1 0x00 |
| 44 | + EOF |
| 45 | + |
| 46 | +Stack trace (part): |
| 47 | +================================================================= |
| 48 | +==89993==ERROR: AddressSanitizer: heap-buffer-overflow on address |
| 49 | +0x615000029900 at pc 0x55d5f885700d bp 0x7ffc1e1e9470 sp 0x7ffc1e1e9468 |
| 50 | +WRITE of size 1 at 0x615000029900 thread T0 |
| 51 | + #0 0x55d5f885700c in sdhci_write_dataport hw/sd/sdhci.c:564:39 |
| 52 | + #1 0x55d5f8849150 in sdhci_write hw/sd/sdhci.c:1223:13 |
| 53 | + #2 0x55d5fa01db63 in memory_region_write_accessor system/memory.c:497:5 |
| 54 | + #3 0x55d5fa01d245 in access_with_adjusted_size system/memory.c:573:18 |
| 55 | + #4 0x55d5fa01b1a9 in memory_region_dispatch_write system/memory.c:1521:16 |
| 56 | + #5 0x55d5fa09f5c9 in flatview_write_continue system/physmem.c:2711:23 |
| 57 | + #6 0x55d5fa08f78b in flatview_write system/physmem.c:2753:12 |
| 58 | + #7 0x55d5fa08f258 in address_space_write system/physmem.c:2860:18 |
| 59 | + ... |
| 60 | +0x615000029900 is located 0 bytes to the right of 512-byte region |
| 61 | +[0x615000029700,0x615000029900) allocated by thread T0 here: |
| 62 | + #0 0x55d5f7237b27 in __interceptor_calloc |
| 63 | + #1 0x7f9e36dd4c50 in g_malloc0 |
| 64 | + #2 0x55d5f88672f7 in sdhci_pci_realize hw/sd/sdhci-pci.c:36:5 |
| 65 | + #3 0x55d5f844b582 in pci_qdev_realize hw/pci/pci.c:2092:9 |
| 66 | + #4 0x55d5fa2ee74b in device_set_realized hw/core/qdev.c:510:13 |
| 67 | + #5 0x55d5fa325bfb in property_set_bool qom/object.c:2358:5 |
| 68 | + #6 0x55d5fa31ea45 in object_property_set qom/object.c:1472:5 |
| 69 | + #7 0x55d5fa332509 in object_property_set_qobject om/qom-qobject.c:28:10 |
| 70 | + #8 0x55d5fa31f6ed in object_property_set_bool qom/object.c:1541:15 |
| 71 | + #9 0x55d5fa2e2948 in qdev_realize hw/core/qdev.c:292:12 |
| 72 | + #10 0x55d5f8eed3f1 in qdev_device_add_from_qdict system/qdev-monitor.c:719:10 |
| 73 | + #11 0x55d5f8eef7ff in qdev_device_add system/qdev-monitor.c:738:11 |
| 74 | + #12 0x55d5f8f211f0 in device_init_func system/vl.c:1200:11 |
| 75 | + #13 0x55d5fad0877d in qemu_opts_foreach util/qemu-option.c:1135:14 |
| 76 | + #14 0x55d5f8f0df9c in qemu_create_cli_devices system/vl.c:2638:5 |
| 77 | + #15 0x55d5f8f0db24 in qmp_x_exit_preconfig system/vl.c:2706:5 |
| 78 | + #16 0x55d5f8f14dc0 in qemu_init system/vl.c:3737:9 |
| 79 | + ... |
| 80 | +SUMMARY: AddressSanitizer: heap-buffer-overflow hw/sd/sdhci.c:564:39 |
| 81 | +in sdhci_write_dataport |
| 82 | + |
| 83 | +Add assertions to ensure the fifo_buffer[] is not overflowed by |
| 84 | +malicious accesses to the Buffer Data Port register. |
| 85 | + |
| 86 | +Fixes: CVE-2024-3447 |
| 87 | +Cc: qemu-stable@nongnu.org |
| 88 | +Fixes: d7dfca0807 ("hw/sdhci: introduce standard SD host controller") |
| 89 | +Buglink: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=58813 |
| 90 | +Reported-by: Alexander Bulekov <alxndr@bu.edu> |
| 91 | +Reported-by: Chuhong Yuan <hslester96@gmail.com> |
| 92 | +Signed-off-by: Peter Maydell <peter.maydell@linaro.org> |
| 93 | +Message-Id: <CAFEAcA9iLiv1XGTGKeopgMa8Y9+8kvptvsb8z2OBeuy+5=NUfg@mail.gmail.com> |
| 94 | +Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> |
| 95 | +Message-Id: <20240409145524.27913-1-philmd@linaro.org> |
| 96 | +--- |
| 97 | + hw/sd/sdhci.c | 8 ++++++++ |
| 98 | + 1 file changed, 8 insertions(+) |
| 99 | + |
| 100 | +diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c |
| 101 | +index c5e0bc018b0..27673e1c70e 100644 |
| 102 | +--- a/hw/sd/sdhci.c |
| 103 | ++++ b/hw/sd/sdhci.c |
| 104 | +@@ -473,6 +473,7 @@ static uint32_t sdhci_read_dataport(SDHCIState *s, unsigned size) |
| 105 | + } |
| 106 | + |
| 107 | + for (i = 0; i < size; i++) { |
| 108 | ++ assert(s->data_count < s->buf_maxsz); |
| 109 | + value |= s->fifo_buffer[s->data_count] << i * 8; |
| 110 | + s->data_count++; |
| 111 | + /* check if we've read all valid data (blksize bytes) from buffer */ |
| 112 | +@@ -561,6 +562,7 @@ static void sdhci_write_dataport(SDHCIState *s, uint32_t value, unsigned size) |
| 113 | + } |
| 114 | + |
| 115 | + for (i = 0; i < size; i++) { |
| 116 | ++ assert(s->data_count < s->buf_maxsz); |
| 117 | + s->fifo_buffer[s->data_count] = value & 0xFF; |
| 118 | + s->data_count++; |
| 119 | + value >>= 8; |
| 120 | +@@ -1208,6 +1210,12 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size) |
| 121 | + if (!(s->capareg & R_SDHC_CAPAB_SDMA_MASK)) { |
| 122 | + value &= ~SDHC_TRNS_DMA; |
| 123 | + } |
| 124 | ++ |
| 125 | ++ /* TRNMOD writes are inhibited while Command Inhibit (DAT) is true */ |
| 126 | ++ if (s->prnsts & SDHC_DATA_INHIBIT) { |
| 127 | ++ mask |= 0xffff; |
| 128 | ++ } |
| 129 | ++ |
| 130 | + MASKED_WRITE(s->trnmod, mask, value & SDHC_TRNMOD_MASK); |
| 131 | + MASKED_WRITE(s->cmdreg, mask >> 16, value >> 16); |
| 132 | + |
| 133 | +-- |
| 134 | +GitLab |
| 135 | + |
0 commit comments