Re: [PATCH-for-9.0] hw/sd/sdhci: Discard excess of data written to Buffer Data Port register

[Peter Maydell]

On Mon, 8 Apr 2024 at 13:34, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 4 Apr 2024 at 09:56, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> > index c5e0bc018b..2dd88fa139 100644
> > --- a/hw/sd/sdhci.c
> > +++ b/hw/sd/sdhci.c
> > @@ -552,7 +552,7 @@ static void sdhci_write_block_to_card(SDHCIState *s)
> >   * register */
> >  static void sdhci_write_dataport(SDHCIState *s, uint32_t value, unsigned 
> > size)
> >  {
> > -    unsigned i;
> > +    unsigned i, available;
> >
> >      /* Check that there is free space left in a buffer */
> >      if (!(s->prnsts & SDHC_SPACE_AVAILABLE)) {
> > @@ -560,6 +560,14 @@ static void sdhci_write_dataport(SDHCIState *s, 
> > uint32_t value, unsigned size)
> >          return;
> >      }
> >
> > +    available = s->buf_maxsz - s->data_count;
> > +    if (size > available) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "SDHC buffer data full (size: 
> > %"PRIu32")"
> > +                                       " discarding %u byte%s\n",
> > +                                       s->buf_maxsz, size - available,
> > +                                       size - available > 1 ? "s" : "");
> > +        size = available; /* Excess data of the last write is ignored. */
> > +    }
> >      for (i = 0; i < size; i++) {
> >          s->fifo_buffer[s->data_count] = value & 0xFF;
> >          s->data_count++;
>
> So, this will definitely avoid the buffer overrun, and the
> quoted text also suggests that we should not be doing the
> "if sdhci_write_block_to_card() writes the data then keep
> going with the rest of the bytes in the value for the start
> of the new block". (With this change we could move the
> "if (s->data_count >= (s->blksize & BLOCK_SIZE_MASK)) ..."
> out of the for() loop and down to the bottom of the function.)
>
> But I'm not sure it fixes the underlying cause of the problem,
> because the repro case isn't writing a multi-byte value, it's
> only writing a single byte.
>
> It looks from the code like if there's no space in the
> buffer then SDHC_SPACE_AVAILABLE should be clear in the
> present-status register, but that has somehow got out of sync.
> The way the repro from the fuzzer toggles the device in and
> out of DMA mode looks suspicious about how that out-of-sync
> situation might have come about.

It looks like the spec says that TRNMOD writes are supposed to
be ignored while the Command Inhibit (DAT) bit is set, which
would forbid this kind of "turn DMA off in the middle of a
DMA transfer" silliness:

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index c5e0bc018b0..9cd887b7f30 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1208,6 +1208,12 @@ sdhci_write(void *opaque, hwaddr offset,
uint64_t val, unsigned size)
         if (!(s->capareg & R_SDHC_CAPAB_SDMA_MASK)) {
             value &= ~SDHC_TRNS_DMA;
         }
+
+        /* TRNMOD writes are inhibited while Command Inhibit (DAT) is true */
+        if (s->prnsts & SDHC_DATA_INHIBIT) {
+            mask |= 0xffff;
+        }
+
         MASKED_WRITE(s->trnmod, mask, value & SDHC_TRNMOD_MASK);
         MASKED_WRITE(s->cmdreg, mask >> 16, value >> 16);

And if you forbid that TRNMOD write that turns off the DMA
then the write to BDATA is no longer an overrun.

So another approach here would be to do that, plus an assert()
at the point of writing into fifo_buffer[] in case there are
other ways to get there with a bogus SPACE_AVAILABLE setting.
(That way any further bug is only "badly behaved code makes
QEMU assert" rather than an overrun.)

The spec is annoyingly vague about the exact conditions
when writing to the used-in-dma registers like TRNMOD
and the block-size register. Currently we use the
TRANSFERRING_DATA() condition to block writes to BLKSIZE,
and you might have thought TRNMOD would be the same, but
apparently not.

thanks
-- PMM