On 2/9/21 9:28 AM, Bin Meng wrote: > Hi Philippe, > > On Tue, Feb 9, 2021 at 3:34 AM Philippe Mathieu-Daudé <[email protected]> wrote: >> >> Per the "SD Host Controller Simplified Specification Version 2.00" >> spec. 'Table 2-4 : Block Size Register': >> >> Transfer Block Size [...] can be accessed only if no >> transaction is executing (i.e., after a transaction has stopped). >> Read operations during transfers may return an invalid value, >> and write operations shall be ignored. >> >> Transactions will update 'data_count', so do not modify 'blksize' >> and 'blkcnt' when 'data_count' is used. This fixes: >> >> $ cat << EOF | qemu-system-x86_64 -qtest stdio -monitor none \ >> -nographic -serial none -M pc-q35-5.0 \ >> -device sdhci-pci,sd-spec-version=3 \ >> -device sd-card,drive=mydrive \ >> -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive >> outl 0xcf8 0x80001810 >> outl 0xcfc 0xe1068000 >> outl 0xcf8 0x80001814 > > Is this command needed?
My guess is this makes the northbridge somehow map the device PCI space. Probably not needed in machines where SDHCI is MMIO mapped. > >> outl 0xcf8 0x80001804 >> outw 0xcfc 0x7 >> outl 0xcf8 0x8000fa20 > > and this one? Ditto. > >> write 0xe106802c 0x1 0x0f >> write 0xe1068004 0xc 0x2801d10101fffffbff28a384 > > Are these fuzzy data? Yes, I didn't try to understand what this does, as often non-sense operations. But this is what would craft a malicious attacker. > >> write 0xe106800c 0x1f >> 0x9dacbbcad9e8f7061524334251606f7e8d9cabbac9d8e7f60514233241505f >> write 0xe1068003 0x28 >> 0x80d000251480d000252280d000253080d000253e80d000254c80d000255a80d000256880d0002576 >> write 0xe1068003 0x1 0xfe >> EOF >> ================================================================= >> ==2686219==ERROR: AddressSanitizer: heap-buffer-overflow on address >> 0x61500003bb00 at pc 0x55ab469f456c bp 0x7ffee71be330 sp 0x7ffee71bdae0 >> WRITE of size 4 at 0x61500003bb00 thread T0 >> #0 0x55ab469f456b in __asan_memcpy (qemu-system-i386+0x1cea56b) >> #1 0x55ab483dc396 in stl_he_p include/qemu/bswap.h:353:5 >> #2 0x55ab483af5e4 in stn_he_p include/qemu/bswap.h:546:1 >> #3 0x55ab483aeb4b in flatview_read_continue softmmu/physmem.c:2839:13 >> #4 0x55ab483b0705 in flatview_read softmmu/physmem.c:2877:12 >> #5 0x55ab483b028e in address_space_read_full softmmu/physmem.c:2890:18 >> #6 0x55ab483b1294 in address_space_rw softmmu/physmem.c:2918:16 >> #7 0x55ab479374a2 in dma_memory_rw_relaxed include/sysemu/dma.h:88:12 >> #8 0x55ab47936f50 in dma_memory_rw include/sysemu/dma.h:127:12 >> #9 0x55ab4793665f in dma_memory_read include/sysemu/dma.h:145:12 >> #10 0x55ab4792f176 in sdhci_sdma_transfer_multi_blocks >> hw/sd/sdhci.c:639:13 >> #11 0x55ab4793dc9d in sdhci_write hw/sd/sdhci.c:1129:17 >> #12 0x55ab483f8db8 in memory_region_write_accessor >> softmmu/memory.c:491:5 >> #13 0x55ab483f868a in access_with_adjusted_size softmmu/memory.c:552:18 >> #14 0x55ab483f6da5 in memory_region_dispatch_write >> softmmu/memory.c:1501:16 >> #15 0x55ab483c3b11 in flatview_write_continue softmmu/physmem.c:2774:23 >> #16 0x55ab483b0eb6 in flatview_write softmmu/physmem.c:2814:14 >> #17 0x55ab483b0a3e in address_space_write softmmu/physmem.c:2906:18 >> #18 0x55ab48465c56 in qtest_process_command softmmu/qtest.c:654:9 >> >> 0x61500003bb00 is located 0 bytes to the right of 512-byte region >> [0x61500003b900,0x61500003bb00) >> allocated by thread T0 here: >> #0 0x55ab469f58a7 in calloc (qemu-system-i386+0x1ceb8a7) >> #1 0x7f21d678f9b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x589b0) >> #2 0x55ab479530ed in sdhci_pci_realize hw/sd/sdhci-pci.c:36:5 >> #3 0x55ab476f102a in pci_qdev_realize hw/pci/pci.c:2108:9 >> #4 0x55ab48baaad2 in device_set_realized hw/core/qdev.c:761:13 >> >> SUMMARY: AddressSanitizer: heap-buffer-overflow >> (qemu-system-i386+0x1cea56b) in __asan_memcpy >> Shadow bytes around the buggy address: >> 0x0c2a7ffff710: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa >> 0x0c2a7ffff720: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >> 0x0c2a7ffff730: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >> 0x0c2a7ffff740: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >> 0x0c2a7ffff750: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >> =>0x0c2a7ffff760:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa >> 0x0c2a7ffff770: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd >> 0x0c2a7ffff780: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd >> 0x0c2a7ffff790: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd >> 0x0c2a7ffff7a0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd >> 0x0c2a7ffff7b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa >> Shadow byte legend (one shadow byte represents 8 application bytes): >> Addressable: 00 >> Heap left redzone: fa >> Freed heap region: fd >> ==2686219==ABORTING >> >> Fixes: CVE-2020-17380 >> Fixes: CVE-2020-25085 >> Signed-off-by: Philippe Mathieu-Daudé <[email protected]> >> --- >> Cc: Mauro Matteo Cascella <[email protected]> >> Cc: Alexander Bulekov <[email protected]> >> Cc: Alistair Francis <[email protected]> >> Cc: Prasad J Pandit <[email protected]> >> Cc: Bandan Das <[email protected]> >> >> RFC because missing Reported-by tags, launchpad/bugzilla links and >> qtest reproducer. Sending for review meanwhile. >> --- >> hw/sd/sdhci.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c >> index 8ffa53999d8..7ac7d9af9e4 100644 >> --- a/hw/sd/sdhci.c >> +++ b/hw/sd/sdhci.c >> @@ -1133,6 +1133,12 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t >> val, unsigned size) >> } >> break; >> case SDHC_BLKSIZE: >> + if (s->data_count) { >> + qemu_log_mask(LOG_GUEST_ERROR, >> + "%s: Can not update blksize when" >> + " transaction is executing\n", __func__); >> + break; >> + } >> if (!TRANSFERRING_DATA(s->prnsts)) { > > I am not sure I get the whole picture here. The problem is out of bound access on fifo_buffer. > Isn't write to s->blksize and s->blkcnt already protected in this if > () statement? I tried this code but it didn't work: -- >8 -- diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index 8ffa53999d8..182641ae98a 100644 --- a/hw/sd/sdhci.c +++ b/hw/sd/sdhci.c @@ -584,6 +584,11 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s) uint32_t boundary_chk = 1 << (((s->blksize & ~BLOCK_SIZE_MASK) >> 12) + 12); uint32_t boundary_count = boundary_chk - (s->sdmasysad % boundary_chk); + if (TRANSFERRING_DATA(s->prnsts)) { + qemu_log_mask(LOG_GUEST_ERROR, + "%s: Transfer already in progress", __func__); + return; + } if (!(s->trnmod & SDHC_TRNS_BLK_CNT_EN) || !s->blkcnt) { qemu_log_mask(LOG_UNIMP, "infinite transfer is not supported\n"); return; --- Do you think we need both? Maybe we miss to set a bit in s->prnsts somewhere...
