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