Re: Is this a out-of-bounds issue?

Re: Is this a out-of-bounds issue?

[Lorenzo Bianconi]

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

> Hi,
> 
> The code in drivers/net/wireless/mediatek/mt76/mt7925/mcu.c may have a
> out-of-bounds issue:
> 
> 638         for (offset = 0; offset < len; offset += le32_to_cpu(clc->len)) {
> 639                 clc = (const struct mt7925_clc *)(clc_base + offset);
> 640
> 641                 if (clc->idx > ARRAY_SIZE(phy->clc)) <-
> 642                         break;
> 643
> 644                 /* do not init buf again if chip reset triggered */
> 645                 if (phy->clc[clc->idx])
> 646                         continue;
> 647
> 648                 phy->clc[clc->idx] = devm_kmemdup(mdev->dev, clc,
> 649                                                   le32_to_cpu(clc->len),
> 650                                                   GFP_KERNEL);
> 651
> 652                 if (!phy->clc[clc->idx]) {
> 653                         ret = -ENOMEM;
> 654                         goto out;
> 655                 }
> 656         }
> 
> Let's say the array size of "phy->clc" is 2, then the valid index is 0 and 1.
> If "clc->idx" is 2, "clc->idx > ARRAY_SIZE(phy->clc)" must be false, the "break"
> statement won't be executed, and "phy->clc[2]" may access illegal memory address.
> 
> So, should we modify the code like this?
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7925/mcu.c b/drivers/net/wireless/mediatek/mt76/mt7925/mcu.c
> index 748ea6adbc6b..0c2a2337c313 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7925/mcu.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt7925/mcu.c
> @@ -638,7 +638,7 @@ static int mt7925_load_clc(struct mt792x_dev *dev, const char *fw_name)
>  	for (offset = 0; offset < len; offset += le32_to_cpu(clc->len)) {
>  		clc = (const struct mt7925_clc *)(clc_base + offset);
>  
> -		if (clc->idx > ARRAY_SIZE(phy->clc))
> +		if (clc->idx >= ARRAY_SIZE(phy->clc))
>  			break;
>  
>  		/* do not init buf again if chip reset triggered */
> 
> -- 
> Best,
> Qianqiang Li 
> 

Hi,

I think this is already fixed here:
https://patchwork.kernel.org/project/linux-wireless/patch/84bf5dd2-2fe3-4410-a7af-ae841e41082a@stanley.mountain/

Regards,
Lornzo

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

Re: Is this a out-of-bounds issue?

[Qianqiang Liu]

> Hi,
> 
> I think this is already fixed here:
> https://patchwork.kernel.org/project/linux-wireless/patch/84bf5dd2-2fe3-4410-a7af-ae841e41082a@stanley.mountain/
> 
> Regards,
> Lornzo

Thanks!

-- 
Best,
Qianqiang Liu

Is this a out-of-bounds issue?

[Qianqiang Liu]

Hi,

The code in drivers/net/wireless/mediatek/mt76/mt7925/mcu.c may have a
out-of-bounds issue:

638         for (offset = 0; offset < len; offset += le32_to_cpu(clc->len)) {
639                 clc = (const struct mt7925_clc *)(clc_base + offset);
640
641                 if (clc->idx > ARRAY_SIZE(phy->clc)) <-
642                         break;
643
644                 /* do not init buf again if chip reset triggered */
645                 if (phy->clc[clc->idx])
646                         continue;
647
648                 phy->clc[clc->idx] = devm_kmemdup(mdev->dev, clc,
649                                                   le32_to_cpu(clc->len),
650                                                   GFP_KERNEL);
651
652                 if (!phy->clc[clc->idx]) {
653                         ret = -ENOMEM;
654                         goto out;
655                 }
656         }

Let's say the array size of "phy->clc" is 2, then the valid index is 0 and 1.
If "clc->idx" is 2, "clc->idx > ARRAY_SIZE(phy->clc)" must be false, the "break"
statement won't be executed, and "phy->clc[2]" may access illegal memory address.

So, should we modify the code like this?

diff --git a/drivers/net/wireless/mediatek/mt76/mt7925/mcu.c b/drivers/net/wireless/mediatek/mt76/mt7925/mcu.c
index 748ea6adbc6b..0c2a2337c313 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7925/mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7925/mcu.c
@@ -638,7 +638,7 @@ static int mt7925_load_clc(struct mt792x_dev *dev, const char *fw_name)
 	for (offset = 0; offset < len; offset += le32_to_cpu(clc->len)) {
 		clc = (const struct mt7925_clc *)(clc_base + offset);
 
-		if (clc->idx > ARRAY_SIZE(phy->clc))
+		if (clc->idx >= ARRAY_SIZE(phy->clc))
 			break;
 
 		/* do not init buf again if chip reset triggered */

-- 
Best,
Qianqiang Li