[PULL,02/19] hw/sd/sdcard: Avoid OOB in sd_read_byte() during unexpected CMD switch

Commit Message

Philippe Mathieu-Daudé June 24, 2024, 1:14 p.m. UTC

For multi-bytes commands, our implementation uses the @data_start
and @data_offset fields to track byte access. We initialize the
command start/offset in buffer once. Malicious guest might abuse
by switching command while staying in the 'transfer' state, switching
command buffer size, and our implementation can access out of buffer
boundary. For example, CMD17 (READ_SINGLE_BLOCK) allows to read up to
512 bytes, and CMD13 (SEND_STATUS) up to 64 bytes. By switching from
CMD17 to CMD13 (see reproducer below), bytes [64-511] are out of the
'status' buffer.

Our implementation return R0 status code for unexpected commands.
Such in-transaction command switch is unexpected and returns R0.
This is a good place to reset the start/offset fields to avoid
malicious accesses.

Can be reproduced running:

  $ export UBSAN_OPTIONS=print_stacktrace=1:halt_on_error=1
  $ cat << EOF | qemu-system-i386 \
                     -display none -nographic \
                     -machine accel=qtest -m 512M \
                     -nodefaults \
                     -device sdhci-pci,sd-spec-version=3 \
                     -device sd-card,drive=mydrive \
                     -drive if=none,index=0,file=null-co://,format=raw,id=mydrive \
                     -qtest stdio -trace sd\* -trace -sdbus_read
  outl 0xcf8 0x80001010
  outl 0xcfc 0xe0000000
  outl 0xcf8 0x80001004
  outw 0xcfc 0x02
  write 0xe000002c 0x1 0x05
  write 0xe000000f 0x1 0x37
  write 0xe000000a 0x1 0x01
  write 0xe000000f 0x1 0x29
  write 0xe000000f 0x1 0x02
  write 0xe000000f 0x1 0x03
  write 0xe000000c 0x1 0x32
  write 0xe000000f 0x1 0x06
  write 0xe0000005 0x1 0x01
  write 0xe0000007 0x1 0x01
  write 0xe0000003 0x1 0x00
  write 0xe000000f 0x1 0x11
  write 0xe000002a 0x1 0x01
  write 0xe000002a 0x1 0x02
  write 0xe000000f 0x1 0x0d
  write 0xe000002a 0x1 0x01
  write 0xe000002a 0x1 0x02
  EOF
  hw/sd/sd.c:1984:15: runtime error: index 256 out of bounds for type 'uint8_t [64]'
  #0 sd_read_byte hw/sd/sd.c:1984:15
  #1 sdbus_read_data hw/sd/core.c:157:23
  #2 sdhci_read_block_from_card hw/sd/sdhci.c:423:9
  #3 sdhci_blkgap_write hw/sd/sdhci.c:1074:13
  #4 sdhci_write hw/sd/sdhci.c:1195:13
  #5 memory_region_write_accessor softmmu/memory.c:492:5
  #6 access_with_adjusted_size softmmu/memory.c:554:18
  #7 memory_region_dispatch_write softmmu/memory.c
  #8 flatview_write_continue softmmu/physmem.c:2778:23
  #9 flatview_write softmmu/physmem.c:2818:14
  #10 address_space_write softmmu/physmem.c:2910:18
  SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior hw/sd/sd.c:1984:15

Reported-by: Alexander Bulekov <[e-mail]>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/487
Buglink: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=36240
Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>
Message-Id: <[e-mail]>
---
 hw/sd/sd.c | 7 +++++++
 1 file changed, 7 insertions(+)

Patch

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 807b5d3de3..6a7a10501b 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1826,6 +1826,13 @@  send_response:
         break;
 
     case sd_r0:
+        /*
+         * Invalid state transition, reset implementation
+         * fields to avoid OOB abuse.
+         */
+        sd->data_start = 0;
+        sd->data_offset = 0;
+        /* fall-through */
     case sd_illegal:
         rsplen = 0;
         break;