Hi Philippe, On Fri, Feb 19, 2021 at 12:25 AM Philippe Mathieu-Daudé <f4...@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-sta...@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 <alx...@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...@gmail.com> > > Acked-by: Alistair Francis <alistair.fran...@wdc.com> > > Tested-by: Alexander Bulekov <alx...@bu.edu> > > Tested-by: Philippe Mathieu-Daudé <f4...@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