| Message ID | 20200827115336.1851276-1-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sd: sdhci: check data_count is within fifo_buffer | expand |
Here's a qtest reproducer for this one: cat << EOF |./i386-softmmu/qemu-system-i386 -nodefaults \ -device sdhci-pci -device sd-card,drive=mydrive \ -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \ -nographic -accel qtest -qtest stdio -nographic outl 0xcf8 0x80001001 outl 0xcfc 0x7e6f25b7 outl 0xcf8 0x80001012 outl 0xcfc 0x842b1212 writeb 0x12120005 0xff writeq 0x12120027 0x5e32b7120584125e write 0x0 0x1 0x21 write 0x8 0x1 0x21 write 0x10 0x1 0x21 write 0x18 0x1 0x21 write 0x20 0x1 0x21 write 0x23 0x1 0x2b writeq 0x1212000c 0x123a0584052da3ab writeq 0x12120000 0xcfff000000000002 writeq 0x12120027 0x5c04c1c9c100005e clock_step EOF Is it related to this https://bugs.launchpad.net/qemu/+bug/1892960 ? -Alex On 200827 1723, P J P wrote: > From: Prasad J Pandit <pjp@fedoraproject.org> > > While doing multi block SDMA, transfer block size may exceed > the 's->fifo_buffer[s->buf_maxsz]' size. It may leave the > current element pointer 's->data_count' pointing out of bounds. > Leading the subsequent DMA r/w operation to OOB access issue. > Add check to avoid it. > > -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Fsdhci_oob_write1 > ==1459837==ERROR: AddressSanitizer: heap-buffer-overflow > WRITE of size 54722048 at 0x61500001e280 thread T3 > #0 __interceptor_memcpy (/lib64/libasan.so.6+0x3a71d) > #1 flatview_read_continue ../exec.c:3245 > #2 flatview_read ../exec.c:3278 > #3 address_space_read_full ../exec.c:3291 > #4 address_space_rw ../exec.c:3319 > #5 dma_memory_rw_relaxed ../include/sysemu/dma.h:87 > #6 dma_memory_rw ../include/sysemu/dma.h:110 > #7 dma_memory_read ../include/sysemu/dma.h:116 > #8 sdhci_sdma_transfer_multi_blocks ../hw/sd/sdhci.c:629 > #9 sdhci_write ../hw/sd/sdhci.c:1097 > #10 memory_region_write_accessor ../softmmu/memory.c:483 > ... > > Reported-by: Ruhr-University <bugs-syssec@rub.de> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- > hw/sd/sdhci.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c > index 1785d7e1f7..155e25ceee 100644 > --- a/hw/sd/sdhci.c > +++ b/hw/sd/sdhci.c > @@ -604,6 +604,9 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s) > s->blkcnt--; > } > } > + if (s->data_count <= begin || s->data_count > s->buf_maxsz) { > + break; > + } > dma_memory_write(s->dma_as, s->sdmasysad, > &s->fifo_buffer[begin], s->data_count - begin); > s->sdmasysad += s->data_count - begin; > @@ -626,6 +629,9 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s) > s->data_count = block_size; > boundary_count -= block_size - begin; > } > + if (s->data_count <= begin || s->data_count > s->buf_maxsz) { > + break; > + } > dma_memory_read(s->dma_as, s->sdmasysad, > &s->fifo_buffer[begin], s->data_count - begin); > s->sdmasysad += s->data_count - begin; > -- > 2.26.2 > >
+-- On Sun, 30 Aug 2020, Alexander Bulekov wrote --+
| Here's a qtest reproducer for this one:
|
| cat << EOF |./i386-softmmu/qemu-system-i386 -nodefaults \
| -device sdhci-pci -device sd-card,drive=mydrive \
| -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
| -nographic -accel qtest -qtest stdio -nographic
| outl 0xcf8 0x80001001
| outl 0xcfc 0x7e6f25b7
| outl 0xcf8 0x80001012
| outl 0xcfc 0x842b1212
| writeb 0x12120005 0xff
| writeq 0x12120027 0x5e32b7120584125e
| write 0x0 0x1 0x21
| write 0x8 0x1 0x21
| write 0x10 0x1 0x21
| write 0x18 0x1 0x21
| write 0x20 0x1 0x21
| write 0x23 0x1 0x2b
| writeq 0x1212000c 0x123a0584052da3ab
| writeq 0x12120000 0xcfff000000000002
| writeq 0x12120027 0x5c04c1c9c100005e
| clock_step
| EOF
|
| Is it related to this https://bugs.launchpad.net/qemu/+bug/1892960 ?
Yes, it's same. This patch fixes it.
| > +++ b/hw/sd/sdhci.c
| > @@ -604,6 +604,9 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
| > }
| > + if (s->data_count <= begin || s->data_count > s->buf_maxsz) {
| > + break;
| > + }
| > dma_memory_write(s->dma_as, s->sdmasysad,
| > &s->fifo_buffer[begin], s->data_count - begin);
| ...
| > + if (s->data_count <= begin || s->data_count > s->buf_maxsz) {
| > + break;
| > + }
| > dma_memory_read(s->dma_as, s->sdmasysad,
| > &s->fifo_buffer[begin], s->data_count - begin);
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
On 8/27/20 1:53 PM, P J P wrote: > From: Prasad J Pandit <pjp@fedoraproject.org> > > While doing multi block SDMA, transfer block size may exceed > the 's->fifo_buffer[s->buf_maxsz]' size. It may leave the > current element pointer 's->data_count' pointing out of bounds. > Leading the subsequent DMA r/w operation to OOB access issue. > Add check to avoid it. > > -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Fsdhci_oob_write1 This directory is 3 months old, I can't find it on the list... Did I missed that or did the list eat the report? > ==1459837==ERROR: AddressSanitizer: heap-buffer-overflow > WRITE of size 54722048 at 0x61500001e280 thread T3 > #0 __interceptor_memcpy (/lib64/libasan.so.6+0x3a71d) > #1 flatview_read_continue ../exec.c:3245 > #2 flatview_read ../exec.c:3278 > #3 address_space_read_full ../exec.c:3291 > #4 address_space_rw ../exec.c:3319 > #5 dma_memory_rw_relaxed ../include/sysemu/dma.h:87 > #6 dma_memory_rw ../include/sysemu/dma.h:110 > #7 dma_memory_read ../include/sysemu/dma.h:116 > #8 sdhci_sdma_transfer_multi_blocks ../hw/sd/sdhci.c:629 > #9 sdhci_write ../hw/sd/sdhci.c:1097 > #10 memory_region_write_accessor ../softmmu/memory.c:483 > ... > > Reported-by: Ruhr-University <bugs-syssec@rub.de> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
Hi Prasad, On 8/27/20 1:53 PM, P J P wrote: > From: Prasad J Pandit <pjp@fedoraproject.org> > > While doing multi block SDMA, transfer block size may exceed > the 's->fifo_buffer[s->buf_maxsz]' size. It may leave the > current element pointer 's->data_count' pointing out of bounds. > Leading the subsequent DMA r/w operation to OOB access issue. > Add check to avoid it. > > -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Fsdhci_oob_write1 > ==1459837==ERROR: AddressSanitizer: heap-buffer-overflow > WRITE of size 54722048 at 0x61500001e280 thread T3 > #0 __interceptor_memcpy (/lib64/libasan.so.6+0x3a71d) > #1 flatview_read_continue ../exec.c:3245 > #2 flatview_read ../exec.c:3278 > #3 address_space_read_full ../exec.c:3291 > #4 address_space_rw ../exec.c:3319 > #5 dma_memory_rw_relaxed ../include/sysemu/dma.h:87 > #6 dma_memory_rw ../include/sysemu/dma.h:110 > #7 dma_memory_read ../include/sysemu/dma.h:116 > #8 sdhci_sdma_transfer_multi_blocks ../hw/sd/sdhci.c:629 > #9 sdhci_write ../hw/sd/sdhci.c:1097 > #10 memory_region_write_accessor ../softmmu/memory.c:483 > ... > > Reported-by: Ruhr-University <bugs-syssec@rub.de> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- > hw/sd/sdhci.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c > index 1785d7e1f7..155e25ceee 100644 > --- a/hw/sd/sdhci.c > +++ b/hw/sd/sdhci.c > @@ -604,6 +604,9 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s) > s->blkcnt--; > } > } > + if (s->data_count <= begin || s->data_count > s->buf_maxsz) { > + break; > + } Thanks for your patch. Note however this kind of security fix hides the bug in the model, furthermore it makes the model behaves differently that the real hardware (which we aim to model). Can you replace by an assert() call instead? Since this should never happen. I posted a different fix for this problem (fixing the model bug): https://www.mail-archive.com/qemu-devel@nongnu.org/msg735715.html (you already reviewed it, thank you - I still comment it for the other reviewers). Regards, Phil. > dma_memory_write(s->dma_as, s->sdmasysad, > &s->fifo_buffer[begin], s->data_count - begin); > s->sdmasysad += s->data_count - begin; > @@ -626,6 +629,9 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s) > s->data_count = block_size; > boundary_count -= block_size - begin; > } > + if (s->data_count <= begin || s->data_count > s->buf_maxsz) { > + break; > + } > dma_memory_read(s->dma_as, s->sdmasysad, > &s->fifo_buffer[begin], s->data_count - begin); > s->sdmasysad += s->data_count - begin; >
+-- On Wed, 2 Sep 2020, Philippe Mathieu-Daudé wrote --+
| > + if (s->data_count <= begin || s->data_count > s->buf_maxsz) {
| > + break;
| > + }
|
| Thanks for your patch. Note however this kind of security fix hides
| the bug in the model, furthermore it makes the model behaves differently
| that the real hardware (which we aim to model).
Right, got it.
| I posted a different fix for this problem (fixing the model bug):
| https://www.mail-archive.com/qemu-devel@nongnu.org/msg735715.html
| (you already reviewed it, thank you - I still comment it for the
| other reviewers).
|
| Can you replace by an assert() call instead? Since this should never
| happen.
Replace above check with an assert() call? Even with your revised fix above?
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
+-- On Tue, 1 Sep 2020, Philippe Mathieu-Daudé wrote --+ | > -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Fsdhci_oob_write1 | | This directory is 3 months old, I can't find it on the list... | Did I missed that or did the list eat the report? No, it was reported to [qemu-security] contacts. These are few last remaining old issues. We shall have better response time now. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
On 9/2/20 6:46 PM, P J P wrote: > +-- On Wed, 2 Sep 2020, Philippe Mathieu-Daudé wrote --+ > | > + if (s->data_count <= begin || s->data_count > s->buf_maxsz) { > | > + break; > | > + } > | > | Thanks for your patch. Note however this kind of security fix hides > | the bug in the model, furthermore it makes the model behaves differently > | that the real hardware (which we aim to model). > > Right, got it. > > | I posted a different fix for this problem (fixing the model bug): > | https://www.mail-archive.com/qemu-devel@nongnu.org/msg735715.html > | (you already reviewed it, thank you - I still comment it for the > | other reviewers). > | > | Can you replace by an assert() call instead? Since this should never > | happen. > > Replace above check with an assert() call? Even with your revised fix above? Well, there might be other bugs leading there... > > > Thank you. > -- > Prasad J Pandit / Red Hat Product Security Team > 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D >
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index 1785d7e1f7..155e25ceee 100644 --- a/hw/sd/sdhci.c +++ b/hw/sd/sdhci.c @@ -604,6 +604,9 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s) s->blkcnt--; } } + if (s->data_count <= begin || s->data_count > s->buf_maxsz) { + break; + } dma_memory_write(s->dma_as, s->sdmasysad, &s->fifo_buffer[begin], s->data_count - begin); s->sdmasysad += s->data_count - begin; @@ -626,6 +629,9 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s) s->data_count = block_size; boundary_count -= block_size - begin; } + if (s->data_count <= begin || s->data_count > s->buf_maxsz) { + break; + } dma_memory_read(s->dma_as, s->sdmasysad, &s->fifo_buffer[begin], s->data_count - begin); s->sdmasysad += s->data_count - begin;