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

["Deren Wu (武德仁)" <Deren.Wu@mediatek.com>]

On Wed, 2022-11-30 at 18:09 +0100, lorenzo@kernel.org wrote:
> > 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

The chip need padding 4 zero to align bus operation and we cannnot
ignore the process here. I think we still need both of them.

Regards,
Deren

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