Re: [PATCH v2 6/6] hw/sd: sdhci: Reset the data pointer of s->fifo_buffer[] when a different block size is programmed

[Bin Meng <bmeng.cn@gmail.com>]

Hi Philippe,

On Fri, Feb 19, 2021 at 2:06 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> 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-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>
> >
> > ---
> >
> > 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.

Me too, because s->data_count is not exposed by the hardware as a
register or descriptor, so it's purely our internal implementation. A
hardware might implement like that, but we really don't know unless
some hardware guys who designed a SDHC could jump out and comment :)

> Shouldn't we reset the FIFO each time BLKSIZE is accessed, regardless of its previous value?

If we do that, we will end up rewriting the logic of the data transfer
functions. I looked at the current implementation and I think there
are some spec violations about handling page boundaries, and that part
is related to sd->data-count. But like I said in the cover letter
these should be addressed in future patches.

>
> > +            }
> >          }
> >
> >          break;
> >

Regards,
Bin