Re: [PATCH] wifi: mt76: mt7921s: fix slab-out-of-bounds access in sdio host

["lorenzo@kernel.org" <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2554 bytes --]

> Hi Lore,
> 
> On Wed, 2022-11-30 at 15:46 +0100, Lorenzo Bianconi wrote:
> > > SDIO may need addtional 512 bytes to align bus operation. If the
> > > tailroom
> > > of this skb is not big enough, we would access invalid memory
> > > region.
> > > For low level operation, take xmit_buf instead of skb to keep valid
> > > memory
> > > access in SDIO.
> > > Note: xmit_buf is big enough for single skb size
> > > 
> > > Fixes: 764dee47e2c1 ("mt76: sdio: move common code in mt76_sdio
> > > module")
> > > Tested-by: YN Chen <YN.Chen@mediatek.com>
> > > Signed-off-by: Deren Wu <deren.wu@mediatek.com>
> > > ---
> > >  drivers/net/wireless/mediatek/mt76/sdio_txrx.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> > > b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> > > index bfc4de50a4d2..ebea5c4e8da5 100644
> > > --- a/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> > > +++ b/drivers/net/wireless/mediatek/mt76/sdio_txrx.c
> > > @@ -254,7 +254,8 @@ static int mt76s_tx_run_queue(struct mt76_dev
> > > *dev, struct mt76_queue *q)
> > >  
> > >  		if (!test_bit(MT76_STATE_MCU_RUNNING, &dev->phy.state)) 
> > > {
> > >  			__skb_put_zero(e->skb, 4);
> > > -			err = __mt76s_xmit_queue(dev, e->skb->data,
> > > +			memcpy(sdio->xmit_buf, e->skb->data, e->skb-
> > > >len);
> > 
> > (even if it is not critical for performance) iirc the skb from the
> > mcu is
> > always linear, I guess we can use __skb_grow() instead. What do you
> > think?
> > 
> > Regards,
> > Lorenzo
> > 
> 
> _skb_grow() looks good for me. It's a balance solution for this case.
> If you have no concern about the patch below, I will post v2 after UT.
> :)
> 
>         if (!test_bit(MT76_STATE_MCU_RUNNING, &dev->phy.state)) {
>                 __skb_put_zero(e->skb, 4);
> +               err = __skb_grow(e->skb, roundup(e->skb->len,
> +                                                sdio->func-
> >cur_blksize));

can we merge __skb_put_zero() and __skb_grow()? Does sdio chip require the 4
last bytes to be 0?

Regards,
Lorenzo

> +               if (err)
> +                       return err;
>                 err = __mt76s_xmit_queue(dev, e->skb->data,
>                                                  e->skb->len);
> 
> 
> Regards,
> Deren
> 
> > > +			err = __mt76s_xmit_queue(dev, sdio->xmit_buf,
> > >  						 e->skb->len);
> > >  			if (err)
> > >  				return err;
> > > -- 
> > > 2.18.0
> > > 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]