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>
---
Cc: Mauro Matteo Cascella <mcascell@redhat.com>
Cc: Alexander Bulekov <alxndr@bu.edu>
Cc: Alistair Francis <alistair.francis@wdc.com>
Cc: Prasad J Pandit <ppandit@redhat.com>
Cc: Bandan Das <bsd@redhat.com>
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)) {
MASKED_WRITE(s->blksize, mask, extract32(value, 0, 12));
MASKED_WRITE(s->blkcnt, mask >> 16, value >> 16);
--
2.26.2
On Mon, Feb 8, 2021 at 8:35 PM Philippe Mathieu-Daudé <f4bug@amsat.org> 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>
> ---
> Cc: Mauro Matteo Cascella <mcascell@redhat.com>
> Cc: Alexander Bulekov <alxndr@bu.edu>
> Cc: Alistair Francis <alistair.francis@wdc.com>
> Cc: Prasad J Pandit <ppandit@redhat.com>
> Cc: Bandan Das <bsd@redhat.com>
>
> 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)) {
> MASKED_WRITE(s->blksize, mask, extract32(value, 0, 12));
> MASKED_WRITE(s->blkcnt, mask >> 16, value >> 16);
> --
> 2.26.2
>
For the above CVEs:
Tested-by: Mauro Matteo Cascella <mcascell@redhat.com>
Thanks a lot,
--
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0
On Mon, Feb 8, 2021 at 8:59 PM Mauro Matteo Cascella <mcascell@redhat.com> wrote: > On Mon, Feb 8, 2021 at 8:35 PM Philippe Mathieu-Daudé <f4bug@amsat.org> 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. > > ... > > > > Fixes: CVE-2020-17380 > > Fixes: CVE-2020-25085 > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > --- > > Cc: Mauro Matteo Cascella <mcascell@redhat.com> > > Cc: Alexander Bulekov <alxndr@bu.edu> > > Cc: Alistair Francis <alistair.francis@wdc.com> > > Cc: Prasad J Pandit <ppandit@redhat.com> > > Cc: Bandan Das <bsd@redhat.com> > > > > RFC because missing Reported-by tags, launchpad/bugzilla links and > > qtest reproducer. Sending for review meanwhile. ... > For the above CVEs: > Tested-by: Mauro Matteo Cascella <mcascell@redhat.com> Thanks Mauro for testing. Do you know what tags I should add for the credits? Phil.
On Mon, Feb 8, 2021 at 9:26 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > On Mon, Feb 8, 2021 at 8:59 PM Mauro Matteo Cascella > <mcascell@redhat.com> wrote: > > On Mon, Feb 8, 2021 at 8:35 PM Philippe Mathieu-Daudé <f4bug@amsat.org> 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. > > > > ... > > > > > > Fixes: CVE-2020-17380 > > > Fixes: CVE-2020-25085 > > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > > --- > > > Cc: Mauro Matteo Cascella <mcascell@redhat.com> > > > Cc: Alexander Bulekov <alxndr@bu.edu> > > > Cc: Alistair Francis <alistair.francis@wdc.com> > > > Cc: Prasad J Pandit <ppandit@redhat.com> > > > Cc: Bandan Das <bsd@redhat.com> > > > > > > RFC because missing Reported-by tags, launchpad/bugzilla links and > > > qtest reproducer. Sending for review meanwhile. > ... > > For the above CVEs: > > Tested-by: Mauro Matteo Cascella <mcascell@redhat.com> > > Thanks Mauro for testing. Do you know what tags I should add for the credits? > > Phil. > I think the credit should go to Alexander for reporting [1] as well as people from Ruhr-University Bochum for CVE-2020-25085 (I don't know about their emails, though): Reported-by: Alexander Bulekov <alxndr@bu.edu> Reported-by: Sergej Schumilo (Ruhr-University Bochum) Reported-by: Cornelius Aschermann (Ruhr-University Bochum) Reported-by: Simon Wrner (Ruhr-University Bochum) [1] https://bugs.launchpad.net/qemu/+bug/1892960 -- Mauro Matteo Cascella Red Hat Product Security PGP-Key ID: BB3410B0
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.
Tested-by: Alexander Bulekov <alxndr@bu.edu>
Thanks
> ---
> Cc: Mauro Matteo Cascella <mcascell@redhat.com>
> Cc: Alexander Bulekov <alxndr@bu.edu>
> Cc: Alistair Francis <alistair.francis@wdc.com>
> Cc: Prasad J Pandit <ppandit@redhat.com>
> Cc: Bandan Das <bsd@redhat.com>
>
> 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)) {
> MASKED_WRITE(s->blksize, mask, extract32(value, 0, 12));
> MASKED_WRITE(s->blkcnt, mask >> 16, value >> 16);
> --
> 2.26.2
>
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
On 210211 2045, Philippe Mathieu-Daudé wrote:
> 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?
Ah I misunderstood.
>
> Could you test this patch without Bin's one?
>
Wow - Applying only this one, there is still a crash. Applying only
Bin's, there is still a crash. Applying both - no crash ..
Here's a reproducer for crashing with this patch applied. I'll post the
other reproducer in the thread for Bin's patch.
-Alex
cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest -nographic \
-m 512M -nodefaults -device sdhci-pci,sd-spec-version=3 \
-drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
-device sd-card,drive=mydrive -qtest stdio
outl 0xcf8 0x80001013
outl 0xcfc 0x5a
outl 0xcf8 0x80001001
outl 0xcfc 0x06000000
write 0x5a00002c 0x1 0x05
write 0x5a00000f 0x1 0x37
write 0x5a00000a 0x1 0x01
write 0x5a00000f 0x1 0x29
outl 0xcf8 0x80001012
outb 0xcfc 0x1
write 0x5a01000f 0x1 0x02
write 0x5a01000f 0x1 0x03
outl 0xcfc 0x5a55
write 0x5a550005 0x1 0x02
write 0x5a550028 0x1 0x10
write 0x0 0x1 0x21
write 0x6 0x1 0x55
write 0x7 0x1 0x5a
write 0x5a55000a 0x1 0x20
write 0x5a55000c 0x1 0x11
write 0x5a55000e 0x1 0x20
write 0x5a55000f 0x1 0x06
EOF
==768748==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x615000031880 at pc 0x5651b619d4e8 bp 0x7ffff8e7acb0 sp 0x7ffff8e7aca8
WRITE of size 1 at 0x615000031880 thread T0
#0 0x5651b619d4e7 in sdhci_write_dataport /home/alxndr/Development/qemu/build/../hw/sd/sdhci.c:560:39
#1 0x5651b619d4e7 in sdhci_write /home/alxndr/Development/qemu/build/../hw/sd/sdhci.c:1178:13
#2 0x5651b6d6a46c in memory_region_write_accessor /home/alxndr/Development/qemu/build/../softmmu/memory.c:491:5
#3 0x5651b6d69f6b in access_with_adjusted_size /home/alxndr/Development/qemu/build/../softmmu/memory.c:552:18
#4 0x5651b6d69770 in memory_region_dispatch_write /home/alxndr/Development/qemu/build/../softmmu/memory.c
#5 0x5651b71385cb in flatview_write_continue /home/alxndr/Development/qemu/build/../softmmu/physmem.c:2776:23
#6 0x5651b712dd1b in flatview_write /home/alxndr/Development/qemu/build/../softmmu/physmem.c:2816:14
#7 0x5651b712dd1b in address_space_write /home/alxndr/Development/qemu/build/../softmmu/physmem.c:2908:18
#8 0x5651b618d042 in dma_memory_rw_relaxed /home/alxndr/Development/qemu/include/sysemu/dma.h:88:12
#9 0x5651b618d042 in dma_memory_rw /home/alxndr/Development/qemu/include/sysemu/dma.h:127:12
#10 0x5651b618d042 in dma_memory_write /home/alxndr/Development/qemu/include/sysemu/dma.h:163:12
#11 0x5651b618d042 in sdhci_do_adma /home/alxndr/Development/qemu/build/../hw/sd/sdhci.c:783:21
#12 0x5651b6185251 in sdhci_data_transfer /home/alxndr/Development/qemu/build/../hw/sd/sdhci.c
#13 0x5651b6199a31 in sdhci_send_command /home/alxndr/Development/qemu/build/../hw/sd/sdhci.c:374:9
#14 0x5651b6199a31 in sdhci_write /home/alxndr/Development/qemu/build/../hw/sd/sdhci.c:1174:9
#15 0x5651b6d6a46c in memory_region_write_accessor /home/alxndr/Development/qemu/build/../softmmu/memory.c:491:5
...
> >
> > Tested-by: Alexander Bulekov <alxndr@bu.edu>
> > Thanks
Hi Philippe,
On Tue, Feb 9, 2021 at 3:34 AM Philippe Mathieu-Daudé <f4bug@amsat.org> 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?
> outl 0xcf8 0x80001804
> outw 0xcfc 0x7
> outl 0xcf8 0x8000fa20
and this one?
> write 0xe106802c 0x1 0x0f
> write 0xe1068004 0xc 0x2801d10101fffffbff28a384
Are these fuzzy data?
> 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>
> ---
> Cc: Mauro Matteo Cascella <mcascell@redhat.com>
> Cc: Alexander Bulekov <alxndr@bu.edu>
> Cc: Alistair Francis <alistair.francis@wdc.com>
> Cc: Prasad J Pandit <ppandit@redhat.com>
> Cc: Bandan Das <bsd@redhat.com>
>
> 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.
Isn't write to s->blksize and s->blkcnt already protected in this if
() statement?
> MASKED_WRITE(s->blksize, mask, extract32(value, 0, 12));
> MASKED_WRITE(s->blkcnt, mask >> 16, value >> 16);
> --
Regards,
Bin
On 2/9/21 9:28 AM, Bin Meng wrote:
> Hi Philippe,
>
> On Tue, Feb 9, 2021 at 3:34 AM Philippe Mathieu-Daudé <f4bug@amsat.org> 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é <f4bug@amsat.org>
>> ---
>> Cc: Mauro Matteo Cascella <mcascell@redhat.com>
>> Cc: Alexander Bulekov <alxndr@bu.edu>
>> Cc: Alistair Francis <alistair.francis@wdc.com>
>> Cc: Prasad J Pandit <ppandit@redhat.com>
>> Cc: Bandan Das <bsd@redhat.com>
>>
>> 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...
Hi Philippe,
On Tue, Feb 9, 2021 at 5:38 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 2/9/21 9:28 AM, Bin Meng wrote:
> > Hi Philippe,
> >
> > On Tue, Feb 9, 2021 at 3:34 AM Philippe Mathieu-Daudé <f4bug@amsat.org> 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.
I think this is not needed. Writing only the CFG_ADDR
>
> >
> >> 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é <f4bug@amsat.org>
> >> ---
> >> Cc: Mauro Matteo Cascella <mcascell@redhat.com>
> >> Cc: Alexander Bulekov <alxndr@bu.edu>
> >> Cc: Alistair Francis <alistair.francis@wdc.com>
> >> Cc: Prasad J Pandit <ppandit@redhat.com>
> >> Cc: Bandan Das <bsd@redhat.com>
> >>
> >> 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...
Oops, hitting "send" by mistake ...
On Tue, Feb 9, 2021 at 5:42 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Philippe,
>
> On Tue, Feb 9, 2021 at 5:38 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >
> > On 2/9/21 9:28 AM, Bin Meng wrote:
> > > Hi Philippe,
> > >
> > > On Tue, Feb 9, 2021 at 3:34 AM Philippe Mathieu-Daudé <f4bug@amsat.org> 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.
>
> I think this is not needed. Writing only the CFG_ADDR
I think this is not needed. Writing only the CFG_ADDR without wring
CFG_DATA does not take any effect.
>
> >
> > >
> > >> 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é <f4bug@amsat.org>
> > >> ---
> > >> Cc: Mauro Matteo Cascella <mcascell@redhat.com>
> > >> Cc: Alexander Bulekov <alxndr@bu.edu>
> > >> Cc: Alistair Francis <alistair.francis@wdc.com>
> > >> Cc: Prasad J Pandit <ppandit@redhat.com>
> > >> Cc: Bandan Das <bsd@redhat.com>
> > >>
> > >> 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 to follow the CVE link from
https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-25085 which
gave me:
https://bugs.launchpad.net/qemu/+bug/1892960
The above page says this CVE is already fixed, so I am lost.
> >
> > 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...
Probably, but I need to take a further look.
Regards,
Bin
On 210209 1745, Bin Meng wrote:
> Oops, hitting "send" by mistake ...
>
> On Tue, Feb 9, 2021 at 5:42 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Philippe,
> >
> > On Tue, Feb 9, 2021 at 5:38 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > >
> > > On 2/9/21 9:28 AM, Bin Meng wrote:
> > > > Hi Philippe,
> > > >
> > > > On Tue, Feb 9, 2021 at 3:34 AM Philippe Mathieu-Daudé <f4bug@amsat.org> 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.
> >
> > I think this is not needed. Writing only the CFG_ADDR
>
> I think this is not needed. Writing only the CFG_ADDR without wring
> CFG_DATA does not take any effect.
>
Ran it through scripts/oss-fuzz/minimize_qtest_trace.py , though that's
probably not very useful now:
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 0x80001804
outw 0xcfc 0x7
write 0xe106802c 0x1 0x0f
write 0xe1068004 0x1 0x20
write 0xe1068005 0x1 0x01
write 0xe1068007 0x1 0x01
write 0xe106800c 0x1 0x33
write 0xe106800e 0x1 0x20
write 0xe106800f 0x1 0x0
write 0xe106800c 0x1 0x0
write 0xe106802a 0x1 0x11
write 0xe1068003 0x1 0x0
write 0xe1068005 0x1 0x00
write 0xe106800c 0x1 0x22
write 0xe106802a 0x1 0x12
write 0xe1068003 0x1 0x10
EOF
> >
> > >
> > > >
> > > >> 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é <f4bug@amsat.org>
> > > >> ---
> > > >> Cc: Mauro Matteo Cascella <mcascell@redhat.com>
> > > >> Cc: Alexander Bulekov <alxndr@bu.edu>
> > > >> Cc: Alistair Francis <alistair.francis@wdc.com>
> > > >> Cc: Prasad J Pandit <ppandit@redhat.com>
> > > >> Cc: Bandan Das <bsd@redhat.com>
> > > >>
> > > >> 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 to follow the CVE link from
> https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-25085 which
> gave me:
> https://bugs.launchpad.net/qemu/+bug/1892960
>
> The above page says this CVE is already fixed, so I am lost.
>
> > >
> > > 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...
>
> Probably, but I need to take a further look.
>
> Regards,
> Bin
© 2016 - 2024 Red Hat, Inc.