diff mbox series

[v2,1/6] hw/sd: sdhci: Don't transfer any data when command time out

Message ID 1613447214-81951-2-git-send-email-bmeng.cn@gmail.com
State Superseded
Headers show
Series hw/sd: sdhci: Fixes to CVE-2020-17380, CVE-2020-25085, CVE-2021-3409 | expand

Commit Message

Bin Meng Feb. 16, 2021, 3:46 a.m. UTC 
At the end of sdhci_send_command(), it starts a data transfer if the
command register indicates data is associated. But the data transfer
should only be initiated when the command execution has succeeded.

With this fix, the following reproducer:

outl 0xcf8 0x80001810
outl 0xcfc 0xe1068000
outl 0xcf8 0x80001804
outw 0xcfc 0x7
write 0xe106802c 0x1 0x0f
write 0xe1068004 0xc 0x2801d10101fffffbff28a384
write 0xe106800c 0x1f 0x9dacbbcad9e8f7061524334251606f7e8d9cabbac9d8e7f60514233241505f
write 0xe1068003 0x28 0x80d000251480d000252280d000253080d000253e80d000254c80d000255a80d000256880d0002576
write 0xe1068003 0x1 0xfe

cannot be reproduced with the following QEMU command line:

$ qemu-system-x86_64 -nographic -M pc-q35-5.0 \
      -device sdhci-pci,sd-spec-version=3 \
      -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
      -device sd-card,drive=mydrive \
      -monitor none -serial none -qtest stdio

Cc: qemu-stable@nongnu.org
Fixes: CVE-2020-17380
Fixes: CVE-2020-25085
Fixes: CVE-2021-3409
Fixes: d7dfca0807a0 ("hw/sdhci: introduce standard SD host controller")
Reported-by: Alexander Bulekov <alxndr@bu.edu>
Reported-by: Cornelius Aschermann (Ruhr-University Bochum)
Reported-by: Muhammad Ramdhan
Reported-by: Sergej Schumilo (Ruhr-University Bochum)
Reported-by: Simon Wrner (Ruhr-University Bochum)
Buglink: https://bugs.launchpad.net/qemu/+bug/1892960
Buglink: https://bugs.launchpad.net/qemu/+bug/1909418
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1928146
Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
Tested-by: Alexander Bulekov <alxndr@bu.edu>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---

(no changes since v1)

 hw/sd/sdhci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Philippe Mathieu-Daudé Feb. 18, 2021, 4:25 p.m. UTC | #1 
On 2/16/21 4:46 AM, Bin Meng wrote:
> At the end of sdhci_send_command(), it starts a data transfer if the
> command register indicates data is associated. But the data transfer
> should only be initiated when the command execution has succeeded.
> 
> With this fix, the following reproducer:
> 
> outl 0xcf8 0x80001810
> outl 0xcfc 0xe1068000
> outl 0xcf8 0x80001804
> outw 0xcfc 0x7
> write 0xe106802c 0x1 0x0f
> write 0xe1068004 0xc 0x2801d10101fffffbff28a384
> write 0xe106800c 0x1f 0x9dacbbcad9e8f7061524334251606f7e8d9cabbac9d8e7f60514233241505f
> write 0xe1068003 0x28 0x80d000251480d000252280d000253080d000253e80d000254c80d000255a80d000256880d0002576
> write 0xe1068003 0x1 0xfe
> 
> cannot be reproduced with the following QEMU command line:
> 
> $ qemu-system-x86_64 -nographic -M pc-q35-5.0 \
>       -device sdhci-pci,sd-spec-version=3 \
>       -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
>       -device sd-card,drive=mydrive \
>       -monitor none -serial none -qtest stdio

Can you directly add the reproducer in tests/qtest/fuzz-sdhci-test.c
instead, similarly to tests/qtest/fuzz-test.c?

> Cc: qemu-stable@nongnu.org
> Fixes: CVE-2020-17380
> Fixes: CVE-2020-25085
> Fixes: CVE-2021-3409
> Fixes: d7dfca0807a0 ("hw/sdhci: introduce standard SD host controller")
> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Reported-by: Cornelius Aschermann (Ruhr-University Bochum)
> Reported-by: Muhammad Ramdhan
> Reported-by: Sergej Schumilo (Ruhr-University Bochum)
> Reported-by: Simon Wrner (Ruhr-University Bochum)
> Buglink: https://bugs.launchpad.net/qemu/+bug/1892960
> Buglink: https://bugs.launchpad.net/qemu/+bug/1909418
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1928146
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> Acked-by: Alistair Francis <alistair.francis@wdc.com>
> Tested-by: Alexander Bulekov <alxndr@bu.edu>
> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> 
> (no changes since v1)
> 
>  hw/sd/sdhci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 8ffa539..1c5ab26 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -326,6 +326,7 @@ static void sdhci_send_command(SDHCIState *s)
>      SDRequest request;
>      uint8_t response[16];
>      int rlen;
> +    bool timeout = false;
>  
>      s->errintsts = 0;
>      s->acmd12errsts = 0;
> @@ -349,6 +350,7 @@ static void sdhci_send_command(SDHCIState *s)
>              trace_sdhci_response16(s->rspreg[3], s->rspreg[2],
>                                     s->rspreg[1], s->rspreg[0]);
>          } else {
> +            timeout = true;
>              trace_sdhci_error("timeout waiting for command response");
>              if (s->errintstsen & SDHC_EISEN_CMDTIMEOUT) {
>                  s->errintsts |= SDHC_EIS_CMDTIMEOUT;
> @@ -369,7 +371,7 @@ static void sdhci_send_command(SDHCIState *s)
>  
>      sdhci_update_irq(s);
>  
> -    if (s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
> +    if (!timeout && s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {

No need to add 'timeout':

       if ((s->errintsts & SDHC_EIS_CMDTIMEOUT)
               && s->blksize
               && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {

>          s->data_count = 0;
>          sdhci_data_transfer(s);
>      }
>
Philippe Mathieu-Daudé Feb. 18, 2021, 4:46 p.m. UTC | #2 
On 2/18/21 5:25 PM, Philippe Mathieu-Daudé wrote:
> On 2/16/21 4:46 AM, Bin Meng wrote:
>> At the end of sdhci_send_command(), it starts a data transfer if the
>> command register indicates data is associated. But the data transfer
>> should only be initiated when the command execution has succeeded.
>>
>> With this fix, the following reproducer:
>>
>> outl 0xcf8 0x80001810
>> outl 0xcfc 0xe1068000
>> outl 0xcf8 0x80001804
>> outw 0xcfc 0x7
>> write 0xe106802c 0x1 0x0f
>> write 0xe1068004 0xc 0x2801d10101fffffbff28a384
>> write 0xe106800c 0x1f 0x9dacbbcad9e8f7061524334251606f7e8d9cabbac9d8e7f60514233241505f
>> write 0xe1068003 0x28 0x80d000251480d000252280d000253080d000253e80d000254c80d000255a80d000256880d0002576
>> write 0xe1068003 0x1 0xfe
>>
>> cannot be reproduced with the following QEMU command line:
>>
>> $ qemu-system-x86_64 -nographic -M pc-q35-5.0 \
>>       -device sdhci-pci,sd-spec-version=3 \
>>       -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
>>       -device sd-card,drive=mydrive \
>>       -monitor none -serial none -qtest stdio
> 
> Can you directly add the reproducer in tests/qtest/fuzz-sdhci-test.c
> instead, similarly to tests/qtest/fuzz-test.c?

Hold on, Alexander will send a RFC series to have that conversion
done automatically.
Bin Meng Feb. 18, 2021, 11:33 p.m. UTC | #3 
Hi Philippe,

On Fri, Feb 19, 2021 at 12:25 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 2/16/21 4:46 AM, Bin Meng wrote:
> > At the end of sdhci_send_command(), it starts a data transfer if the
> > command register indicates data is associated. But the data transfer
> > should only be initiated when the command execution has succeeded.
> >
> > With this fix, the following reproducer:
> >
> > outl 0xcf8 0x80001810
> > outl 0xcfc 0xe1068000
> > outl 0xcf8 0x80001804
> > outw 0xcfc 0x7
> > write 0xe106802c 0x1 0x0f
> > write 0xe1068004 0xc 0x2801d10101fffffbff28a384
> > write 0xe106800c 0x1f 0x9dacbbcad9e8f7061524334251606f7e8d9cabbac9d8e7f60514233241505f
> > write 0xe1068003 0x28 0x80d000251480d000252280d000253080d000253e80d000254c80d000255a80d000256880d0002576
> > write 0xe1068003 0x1 0xfe
> >
> > cannot be reproduced with the following QEMU command line:
> >
> > $ qemu-system-x86_64 -nographic -M pc-q35-5.0 \
> >       -device sdhci-pci,sd-spec-version=3 \
> >       -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
> >       -device sd-card,drive=mydrive \
> >       -monitor none -serial none -qtest stdio
>
> Can you directly add the reproducer in tests/qtest/fuzz-sdhci-test.c
> instead, similarly to tests/qtest/fuzz-test.c?
>
> > Cc: qemu-stable@nongnu.org
> > Fixes: CVE-2020-17380
> > Fixes: CVE-2020-25085
> > Fixes: CVE-2021-3409
> > Fixes: d7dfca0807a0 ("hw/sdhci: introduce standard SD host controller")
> > Reported-by: Alexander Bulekov <alxndr@bu.edu>
> > Reported-by: Cornelius Aschermann (Ruhr-University Bochum)
> > Reported-by: Muhammad Ramdhan
> > Reported-by: Sergej Schumilo (Ruhr-University Bochum)
> > Reported-by: Simon Wrner (Ruhr-University Bochum)
> > Buglink: https://bugs.launchpad.net/qemu/+bug/1892960
> > Buglink: https://bugs.launchpad.net/qemu/+bug/1909418
> > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1928146
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > Acked-by: Alistair Francis <alistair.francis@wdc.com>
> > Tested-by: Alexander Bulekov <alxndr@bu.edu>
> > Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> >
> > (no changes since v1)
> >
> >  hw/sd/sdhci.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> > index 8ffa539..1c5ab26 100644
> > --- a/hw/sd/sdhci.c
> > +++ b/hw/sd/sdhci.c
> > @@ -326,6 +326,7 @@ static void sdhci_send_command(SDHCIState *s)
> >      SDRequest request;
> >      uint8_t response[16];
> >      int rlen;
> > +    bool timeout = false;
> >
> >      s->errintsts = 0;
> >      s->acmd12errsts = 0;
> > @@ -349,6 +350,7 @@ static void sdhci_send_command(SDHCIState *s)
> >              trace_sdhci_response16(s->rspreg[3], s->rspreg[2],
> >                                     s->rspreg[1], s->rspreg[0]);
> >          } else {
> > +            timeout = true;
> >              trace_sdhci_error("timeout waiting for command response");
> >              if (s->errintstsen & SDHC_EISEN_CMDTIMEOUT) {
> >                  s->errintsts |= SDHC_EIS_CMDTIMEOUT;
> > @@ -369,7 +371,7 @@ static void sdhci_send_command(SDHCIState *s)
> >
> >      sdhci_update_irq(s);
> >
> > -    if (s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
> > +    if (!timeout && s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
>
> No need to add 'timeout':

But s->errintsts only gets updated if (s->errintstsen & SDHC_EISEN_CMDTIMEOUT).

>
>        if ((s->errintsts & SDHC_EIS_CMDTIMEOUT)
>                && s->blksize
>                && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
>
> >          s->data_count = 0;
> >          sdhci_data_transfer(s);
> >      }

Regards,
Bin
diff mbox series

Patch

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 8ffa539..1c5ab26 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -326,6 +326,7 @@  static void sdhci_send_command(SDHCIState *s)
     SDRequest request;
     uint8_t response[16];
     int rlen;
+    bool timeout = false;
 
     s->errintsts = 0;
     s->acmd12errsts = 0;
@@ -349,6 +350,7 @@  static void sdhci_send_command(SDHCIState *s)
             trace_sdhci_response16(s->rspreg[3], s->rspreg[2],
                                    s->rspreg[1], s->rspreg[0]);
         } else {
+            timeout = true;
             trace_sdhci_error("timeout waiting for command response");
             if (s->errintstsen & SDHC_EISEN_CMDTIMEOUT) {
                 s->errintsts |= SDHC_EIS_CMDTIMEOUT;
@@ -369,7 +371,7 @@  static void sdhci_send_command(SDHCIState *s)
 
     sdhci_update_irq(s);
 
-    if (s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
+    if (!timeout && s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
         s->data_count = 0;
         sdhci_data_transfer(s);
     }