Message ID | 1613447214-81951-2-git-send-email-bmeng.cn@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | hw/sd: sdhci: Fixes to CVE-2020-17380, CVE-2020-25085, CVE-2021-3409 | expand |
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); > } >
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.
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 --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); }