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;
+            }
         }
 
         break;
-- 
2.7.4

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. Shouldn't we reset the FIFO
each time BLKSIZE is accessed, regardless of its previous value?

> +            }
>          }
>  
>          break;
> 

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