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

[Mauro Matteo Cascella]

On Mon, Nov 7, 2022 at 8:12 PM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> 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.

Note that a similar thing is done in sdhci_{read,write}_dataport. But
it's true that this is probably not enough (e.g. no update of
s->prnsts). Thank you for sending
https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg01161.html.

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

--
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0