Hi Bin, On 2/16/21 4:46 AM, Bin Meng wrote: > If the block size is programmed to a different value from the > previous one, reset the data pointer of s->fifo_buffer[] so that > s->fifo_buffer[] can be filled in using the new block size in > the next transfer. > > With this fix, the following reproducer: > > outl 0xcf8 0x80001010 > outl 0xcfc 0xe0000000 > outl 0xcf8 0x80001001 > outl 0xcfc 0x06000000 > write 0xe000002c 0x1 0x05 > write 0xe0000005 0x1 0x02 > write 0xe0000007 0x1 0x01 > write 0xe0000028 0x1 0x10 > write 0x0 0x1 0x23 > write 0x2 0x1 0x08 > write 0xe000000c 0x1 0x01 > write 0xe000000e 0x1 0x20 > write 0xe000000f 0x1 0x00 > write 0xe000000c 0x1 0x32 > write 0xe0000004 0x2 0x0200 > write 0xe0000028 0x1 0x00 > write 0xe0000003 0x1 0x40 > > cannot be reproduced with the following QEMU command line: > > $ qemu-system-x86_64 -nographic -machine accel=qtest -m 512M \ > -nodefaults -device sdhci-pci,sd-spec-version=3 \ > -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \ > -device sd-card,drive=mydrive -qtest stdio > > 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> > > --- > > Changes in v2: > - new patch: sdhci: Reset the data pointer of s->fifo_buffer[] when a > different block size is programmed > > hw/sd/sdhci.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c > index d0c8e29..5b86781 100644 > --- a/hw/sd/sdhci.c > +++ b/hw/sd/sdhci.c > @@ -1140,6 +1140,8 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, > unsigned size) > break; > case SDHC_BLKSIZE: > if (!TRANSFERRING_DATA(s->prnsts)) { > + uint16_t blksize = s->blksize; > + > MASKED_WRITE(s->blksize, mask, extract32(value, 0, 12)); > MASKED_WRITE(s->blkcnt, mask >> 16, value >> 16); > > @@ -1151,6 +1153,16 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, > unsigned size) > > s->blksize = deposit32(s->blksize, 0, 12, s->buf_maxsz); > } > + > + /* > + * If the block size is programmed to a different value from > + * the previous one, reset the data pointer of s->fifo_buffer[] > + * so that s->fifo_buffer[] can be filled in using the new block > + * size in the next transfer. > + */ > + if (blksize != s->blksize) { > + s->data_count = 0;
I doubt the hardware works that way. Shouldn't we reset the FIFO each time BLKSIZE is accessed, regardless of its previous value? > + } > } > > break; >