diff mbox series

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

Message ID 631e6a06fb640ec4f81c92b57d31eb0f7b23c351.1669814212.git.deren.wu@mediatek.com (mailing list archive)
State Changes Requested
Delegated to: Felix Fietkau
Headers show
Series wifi: mt76: mt7921s: fix slab-out-of-bounds access in sdio host | expand

Commit Message

Deren Wu Nov. 30, 2022, 1:33 p.m. UTC
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(-)

Comments

Lorenzo Bianconi Nov. 30, 2022, 2:46 p.m. UTC | #1
> 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
>
Deren Wu Nov. 30, 2022, 3:44 p.m. UTC | #2
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
> >
Lorenzo Bianconi Nov. 30, 2022, 5:09 p.m. UTC | #3
> 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
> > >
Deren Wu Dec. 1, 2022, 2:02 a.m. UTC | #4
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
> > > >
diff mbox series

Patch

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;