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

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

[Deren Wu]

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

Error message:
[69.951] BUG: KASAN: slab-out-of-bounds in sg_copy_buffer+0xe9/0x1a0
[69.951] Read of size 64 at addr ffff88811c9cf000 by task kworker/u16:7/451
[69.951] CPU: 4 PID: 451 Comm: kworker/u16:7 Tainted: G W  OE  6.1.0-rc5 #1
[69.951] Workqueue: kvub300c vub300_cmndwork_thread [vub300]
[69.951] Call Trace:
[69.951]  <TASK>
[69.952]  dump_stack_lvl+0x49/0x63
[69.952]  print_report+0x171/0x4a8
[69.952]  kasan_report+0xb4/0x130
[69.952]  kasan_check_range+0x149/0x1e0
[69.952]  memcpy+0x24/0x70
[69.952]  sg_copy_buffer+0xe9/0x1a0
[69.952]  sg_copy_to_buffer+0x12/0x20
[69.952]  __command_write_data.isra.0+0x23c/0xbf0 [vub300]
[69.952]  vub300_cmndwork_thread+0x17f3/0x58b0 [vub300]
[69.952]  process_one_work+0x7ee/0x1320
[69.952]  worker_thread+0x53c/0x1240
[69.952]  kthread+0x2b8/0x370
[69.952]  ret_from_fork+0x1f/0x30
[69.952]  </TASK>

[69.952] Allocated by task 854:
[69.952]  kasan_save_stack+0x26/0x50
[69.952]  kasan_set_track+0x25/0x30
[69.952]  kasan_save_alloc_info+0x1b/0x30
[69.952]  __kasan_kmalloc+0x87/0xa0
[69.952]  __kmalloc_node_track_caller+0x63/0x150
[69.952]  kmalloc_reserve+0x31/0xd0
[69.952]  __alloc_skb+0xfc/0x2b0
[69.952]  __mt76_mcu_msg_alloc+0xbf/0x230 [mt76]
[69.952]  mt76_mcu_send_and_get_msg+0xab/0x110 [mt76]
[69.952]  __mt76_mcu_send_firmware.cold+0x94/0x15d [mt76]
[69.952]  mt76_connac_mcu_send_ram_firmware+0x415/0x54d [mt76_connac_lib]
[69.952]  mt76_connac2_load_ram.cold+0x118/0x4bc [mt76_connac_lib]
[69.952]  mt7921_run_firmware.cold+0x2e9/0x405 [mt7921_common]
[69.952]  mt7921s_mcu_init+0x45/0x80 [mt7921s]
[69.953]  mt7921_init_work+0xe1/0x2a0 [mt7921_common]
[69.953]  process_one_work+0x7ee/0x1320
[69.953]  worker_thread+0x53c/0x1240
[69.953]  kthread+0x2b8/0x370
[69.953]  ret_from_fork+0x1f/0x30
[69.953] The buggy address belongs to the object at ffff88811c9ce800
             which belongs to the cache kmalloc-2k of size 2048
[69.953] The buggy address is located 0 bytes to the right of
             2048-byte region [ffff88811c9ce800, ffff88811c9cf000)

[69.953] Memory state around the buggy address:
[69.953]  ffff88811c9cef00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[69.953]  ffff88811c9cef80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[69.953] >ffff88811c9cf000: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[69.953]                    ^
[69.953]  ffff88811c9cf080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[69.953]  ffff88811c9cf100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc

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);
+			err = __mt76s_xmit_queue(dev, sdio->xmit_buf,
 						 e->skb->len);
 			if (err)
 				return err;
-- 
2.18.0

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

[Lorenzo Bianconi]

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

> 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
> 
> Error message:
> [69.951] BUG: KASAN: slab-out-of-bounds in sg_copy_buffer+0xe9/0x1a0
> [69.951] Read of size 64 at addr ffff88811c9cf000 by task kworker/u16:7/451
> [69.951] CPU: 4 PID: 451 Comm: kworker/u16:7 Tainted: G W  OE  6.1.0-rc5 #1
> [69.951] Workqueue: kvub300c vub300_cmndwork_thread [vub300]
> [69.951] Call Trace:
> [69.951]  <TASK>
> [69.952]  dump_stack_lvl+0x49/0x63
> [69.952]  print_report+0x171/0x4a8
> [69.952]  kasan_report+0xb4/0x130
> [69.952]  kasan_check_range+0x149/0x1e0
> [69.952]  memcpy+0x24/0x70
> [69.952]  sg_copy_buffer+0xe9/0x1a0
> [69.952]  sg_copy_to_buffer+0x12/0x20
> [69.952]  __command_write_data.isra.0+0x23c/0xbf0 [vub300]
> [69.952]  vub300_cmndwork_thread+0x17f3/0x58b0 [vub300]
> [69.952]  process_one_work+0x7ee/0x1320
> [69.952]  worker_thread+0x53c/0x1240
> [69.952]  kthread+0x2b8/0x370
> [69.952]  ret_from_fork+0x1f/0x30
> [69.952]  </TASK>
> 
> [69.952] Allocated by task 854:
> [69.952]  kasan_save_stack+0x26/0x50
> [69.952]  kasan_set_track+0x25/0x30
> [69.952]  kasan_save_alloc_info+0x1b/0x30
> [69.952]  __kasan_kmalloc+0x87/0xa0
> [69.952]  __kmalloc_node_track_caller+0x63/0x150
> [69.952]  kmalloc_reserve+0x31/0xd0
> [69.952]  __alloc_skb+0xfc/0x2b0
> [69.952]  __mt76_mcu_msg_alloc+0xbf/0x230 [mt76]
> [69.952]  mt76_mcu_send_and_get_msg+0xab/0x110 [mt76]
> [69.952]  __mt76_mcu_send_firmware.cold+0x94/0x15d [mt76]
> [69.952]  mt76_connac_mcu_send_ram_firmware+0x415/0x54d [mt76_connac_lib]
> [69.952]  mt76_connac2_load_ram.cold+0x118/0x4bc [mt76_connac_lib]
> [69.952]  mt7921_run_firmware.cold+0x2e9/0x405 [mt7921_common]
> [69.952]  mt7921s_mcu_init+0x45/0x80 [mt7921s]
> [69.953]  mt7921_init_work+0xe1/0x2a0 [mt7921_common]
> [69.953]  process_one_work+0x7ee/0x1320
> [69.953]  worker_thread+0x53c/0x1240
> [69.953]  kthread+0x2b8/0x370
> [69.953]  ret_from_fork+0x1f/0x30
> [69.953] The buggy address belongs to the object at ffff88811c9ce800
>              which belongs to the cache kmalloc-2k of size 2048
> [69.953] The buggy address is located 0 bytes to the right of
>              2048-byte region [ffff88811c9ce800, ffff88811c9cf000)
> 
> [69.953] Memory state around the buggy address:
> [69.953]  ffff88811c9cef00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> [69.953]  ffff88811c9cef80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> [69.953] >ffff88811c9cf000: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> [69.953]                    ^
> [69.953]  ffff88811c9cf080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> [69.953]  ffff88811c9cf100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> 
> 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

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

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

[Deren Wu (武德仁)]

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

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

[lorenzo]

[-- 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 --]

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

[Deren Wu (武德仁)]

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