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

[Peter Maydell]

On Thu, 4 Apr 2024 at 09:56, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Per "SD Host Controller Standard Specification Version 3.00":
>
>   * 1.7 Buffer Control
>
>   - 1.7.1 Control of Buffer Pointer
>
>     (3) Buffer Control with Block Size
>
>     In case of write operation, the buffer accumulates the data
>     written through the Buffer Data Port register. When the buffer
>     pointer reaches the block size, Buffer Write Enable in the
>     Present State register changes 1 to 0. It means no more data
>     can be written to the buffer. Excess data of the last write is
>     ignored. For example, if just lower 2 bytes data can be written
>     to the buffer and a 32-bit (4-byte) block of data is written to
>     the Buffer Data Port register, the lower 2 bytes of data is
>     written to the buffer and the upper 2 bytes is ignored.
>
> Discard the excess of data to avoid overflow reported by fuzzer:
>
>   $ cat << EOF | qemu-system-i386 \
>                      -display none -nodefaults \
>                      -machine accel=qtest -m 512M \
>                      -device sdhci-pci,sd-spec-version=3 \
>                      -device sd-card,drive=mydrive \
>                      -drive 
> if=none,index=0,file=null-co://,format=raw,id=mydrive -nographic \
>                      -qtest stdio
>   outl 0xcf8 0x80001013
>   outl 0xcfc 0x91
>   outl 0xcf8 0x80001001
>   outl 0xcfc 0x06000000
>   write 0x9100002c 0x1 0x05
>   write 0x91000058 0x1 0x16
>   write 0x91000005 0x1 0x04
>   write 0x91000028 0x1 0x08
>   write 0x16 0x1 0x21
>   write 0x19 0x1 0x20
>   write 0x9100000c 0x1 0x01
>   write 0x9100000e 0x1 0x20
>   write 0x9100000f 0x1 0x00
>   write 0x9100000c 0x1 0x00
>   write 0x91000020 0x1 0x00
>   EOF

> 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.

thanks
-- PMM