Re: [PATCH] hw/sd/sdhci: reset data count in sdhci_buff_access_is_sequential()

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 7/11/22 11:35, Mauro Matteo Cascella wrote:
> Make sure to reset data_count if it's equal to (or exceeds) block_size.
> This prevents an off-by-one read / write when accessing s->fifo_buffer
> in sdhci_read_dataport / sdhci_write_dataport, both called right after
> sdhci_buff_access_is_sequential.
> 
> Fixes: CVE-2022-3872
> Reported-by: RivenDell <XRivenDell@outlook.com>
> Reported-by: Siqi Chen <coc.cyqh@gmail.com>
> Reported-by: ningqiang <ningqiang1@huawei.com>
> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> ---
>   hw/sd/sdhci.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 306070c872..aa2fd79df2 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -978,6 +978,10 @@ static bool sdhci_can_issue_command(SDHCIState *s)
>   static inline bool
>   sdhci_buff_access_is_sequential(SDHCIState *s, unsigned byte_num)
>   {
> +    if (s->data_count >= (s->blksize & BLOCK_SIZE_MASK)) {
> +        s->data_count = 0;
> +    }

You avoid an off-by-one but now the model doesn't work per the spec.
Not really what the best fix IMHO.

>       if ((s->data_count & 0x3) != byte_num) {
>           trace_sdhci_error("Non-sequential access to Buffer Data Port register"
>                             "is prohibited\n");

I wonder why sdhci_data_transfer() indiscriminately sets
SDHC_SPACE_AVAILABLE in the write path (at least without
clearing the FIFO first).

The fix could be:

-- >8 --
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
@@ -955,5 +955,5 @@ static void sdhci_data_transfer(void *opaque)
          } else {
              s->prnsts |= SDHC_DOING_WRITE | SDHC_DAT_LINE_ACTIVE |
-                    SDHC_SPACE_AVAILABLE | SDHC_DATA_INHIBIT;
+                                           SDHC_DATA_INHIBIT;
              sdhci_write_block_to_card(s);
          }
---

Bin, what do you think?

Regards,

Phil.