[PATCH] sd: sdhci: check data_count is within fifo_buffer

[PATCH] sd: sdhci: check data_count is within fifo_buffer

[P J P]

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

Re: [PATCH] sd: sdhci: check data_count is within fifo_buffer

[Alexander Bulekov]

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
> 
> 

Re: [PATCH] sd: sdhci: check data_count is within fifo_buffer

[P J P]

+-- 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

Re: [PATCH] sd: sdhci: check data_count is within fifo_buffer

[Philippe Mathieu-Daudé]

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>

Re: [PATCH] sd: sdhci: check data_count is within fifo_buffer

[P J P]

[-- Attachment #1: Type: text/plain, Size: 509 bytes --]

+-- 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

Re: [PATCH] sd: sdhci: check data_count is within fifo_buffer

[Philippe Mathieu-Daudé]

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;
> 

Re: [PATCH] sd: sdhci: check data_count is within fifo_buffer

[P J P]

[-- Attachment #1: Type: text/plain, Size: 901 bytes --]

+-- 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

Re: [PATCH] sd: sdhci: check data_count is within fifo_buffer

[Philippe Mathieu-Daudé]

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
>