[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] hw/sd/sdhci: Do not modify BlockSizeRegister if transaction
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH] hw/sd/sdhci: Do not modify BlockSizeRegister if transaction in progress |
Date: |
Thu, 11 Feb 2021 20:45:38 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 |
Hi Alexander,
On 2/11/21 6:04 PM, Alexander Bulekov wrote:
> On 210208 2034, Philippe Mathieu-Daudé 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
>> outl 0xcf8 0x80001804
>> outw 0xcfc 0x7
>> outl 0xcf8 0x8000fa20
>> write 0xe106802c 0x1 0x0f
>> write 0xe1068004 0xc 0x2801d10101fffffbff28a384
>> 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é <f4bug@amsat.org>
>
> I applied this along with
> <1612868085-72809-1-git-send-email-bmeng.cn@gmail.com>
> "hw/sd: sdhci: Do not transfer any data when command fails"
>
> I ran through the entire OSS-Fuzz corpus, and could not reproduce the
> crash.
Thanks for your testing, it is helpful!
However I am a bit confused, because Bin's patch is supposed to
replace mine. Are you saying Bin's patch doesn't fix the problem?
Could you test this patch without Bin's one?
>
> Tested-by: Alexander Bulekov <alxndr@bu.edu>
> Thanks
Re: [PATCH] hw/sd/sdhci: Do not modify BlockSizeRegister if transaction in progress, Alexander Bulekov, 2021/02/11
- Re: [PATCH] hw/sd/sdhci: Do not modify BlockSizeRegister if transaction in progress,
Philippe Mathieu-Daudé <=