diff mbox series

[net,1/2] net: dsa: realtek: fix out-of-bounds access

Message ID 20230315130917.3633491-1-a.fatoum@pengutronix.de
State New
Headers
Series [net,1/2] net: dsa: realtek: fix out-of-bounds access |

Commit Message

Ahmad Fatoum March 15, 2023, 1:09 p.m. UTC 
  The probe function sets priv->chip_data to (void *)priv + sizeof(*priv)
with the expectation that priv has enough trailing space.

However, only realtek-smi actually allocated this chip_data space.
Do likewise in realtek-mdio to fix out-of-bounds accesses.

Fixes: aac94001067d ("net: dsa: realtek: add new mdio interface for drivers")
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/net/dsa/realtek/realtek-mdio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Linus Walleij March 15, 2023, 1:15 p.m. UTC | #1 
On Wed, Mar 15, 2023 at 2:09 PM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:

> The probe function sets priv->chip_data to (void *)priv + sizeof(*priv)
> with the expectation that priv has enough trailing space.
>
> However, only realtek-smi actually allocated this chip_data space.
> Do likewise in realtek-mdio to fix out-of-bounds accesses.
>
> Fixes: aac94001067d ("net: dsa: realtek: add new mdio interface for drivers")
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

That this worked for so long is kind of scary, and the reason why we run Kasan
over so much code, I don't know if Kasan would have found this one.

Rewriting the whole world in Rust will fix this problem, but it will
take a while...

Yours,
Linus Walleij
Linus Walleij March 15, 2023, 1:16 p.m. UTC | #2 
On Wed, Mar 15, 2023 at 2:09 PM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:

> Some error messages lack a new line, add them.
>
> Fixes: d40f607c181f ("net: dsa: realtek: rtl8365mb: add RTL8367S support")
> Fixes: d8652956cf37 ("net: dsa: realtek-smi: Add Realtek SMI driver")
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
Alvin Šipraga March 15, 2023, 1:21 p.m. UTC | #3 
On Wed, Mar 15, 2023 at 02:09:15PM +0100, Ahmad Fatoum wrote:
> The probe function sets priv->chip_data to (void *)priv + sizeof(*priv)
> with the expectation that priv has enough trailing space.
> 
> However, only realtek-smi actually allocated this chip_data space.
> Do likewise in realtek-mdio to fix out-of-bounds accesses.
> 
> Fixes: aac94001067d ("net: dsa: realtek: add new mdio interface for drivers")
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---

Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>

Makes me wonder how the switches worked over MDIO all this time... I
guess it was dumb luck.

>  drivers/net/dsa/realtek/realtek-mdio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> index 3e54fac5f902..3b22d3f7ad99 100644
> --- a/drivers/net/dsa/realtek/realtek-mdio.c
> +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> @@ -152,7 +152,7 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev)
>  	if (!var)
>  		return -EINVAL;
>  
> -	priv = devm_kzalloc(&mdiodev->dev, sizeof(*priv), GFP_KERNEL);
> +	priv = devm_kzalloc(&mdiodev->dev, sizeof(*priv) + var->chip_data_sz, GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
>  
> -- 
> 2.30.2
>
Andrew Lunn March 15, 2023, 1:30 p.m. UTC | #4 
On Wed, Mar 15, 2023 at 02:09:15PM +0100, Ahmad Fatoum wrote:
> The probe function sets priv->chip_data to (void *)priv + sizeof(*priv)
> with the expectation that priv has enough trailing space.
> 
> However, only realtek-smi actually allocated this chip_data space.
> Do likewise in realtek-mdio to fix out-of-bounds accesses.

Hi Ahmad

It is normal to include a patch 0/X which gives the big picture, what
does this patch set do as a whole.

Please try to remember this for the next set you post.

       Andrew
Ahmad Fatoum March 15, 2023, 1:35 p.m. UTC | #5 
Hello Andrew,

On 15.03.23 14:30, Andrew Lunn wrote:
> On Wed, Mar 15, 2023 at 02:09:15PM +0100, Ahmad Fatoum wrote:
>> The probe function sets priv->chip_data to (void *)priv + sizeof(*priv)
>> with the expectation that priv has enough trailing space.
>>
>> However, only realtek-smi actually allocated this chip_data space.
>> Do likewise in realtek-mdio to fix out-of-bounds accesses.
> 
> Hi Ahmad
> 
> It is normal to include a patch 0/X which gives the big picture, what
> does this patch set do as a whole.
> 
> Please try to remember this for the next set you post.

Ok, will do next time. I didn't include one this time, because there's
no big picture here; Just two fixes for the same driver.

Cheers,
Ahmad

> 
>        Andrew
>
Andrew Lunn March 15, 2023, 1:45 p.m. UTC | #6 
On Wed, Mar 15, 2023 at 02:35:34PM +0100, Ahmad Fatoum wrote:
> Hello Andrew,
> 
> On 15.03.23 14:30, Andrew Lunn wrote:
> > On Wed, Mar 15, 2023 at 02:09:15PM +0100, Ahmad Fatoum wrote:
> >> The probe function sets priv->chip_data to (void *)priv + sizeof(*priv)
> >> with the expectation that priv has enough trailing space.
> >>
> >> However, only realtek-smi actually allocated this chip_data space.
> >> Do likewise in realtek-mdio to fix out-of-bounds accesses.
> > 
> > Hi Ahmad
> > 
> > It is normal to include a patch 0/X which gives the big picture, what
> > does this patch set do as a whole.
> > 
> > Please try to remember this for the next set you post.
> 
> Ok, will do next time. I didn't include one this time, because there's
> no big picture here; Just two fixes for the same driver.

Fine. You could just say that. Patch 0/X is then used as the merge
commit messages.

       Andrew
Ahmad Fatoum March 15, 2023, 3:34 p.m. UTC | #7 
Hi Linus,

On 15.03.23 14:15, Linus Walleij wrote:
> On Wed, Mar 15, 2023 at 2:09 PM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> 
>> The probe function sets priv->chip_data to (void *)priv + sizeof(*priv)
>> with the expectation that priv has enough trailing space.
>>
>> However, only realtek-smi actually allocated this chip_data space.
>> Do likewise in realtek-mdio to fix out-of-bounds accesses.
>>
>> Fixes: aac94001067d ("net: dsa: realtek: add new mdio interface for drivers")
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thanks for the review.
 
> That this worked for so long is kind of scary, and the reason why we run Kasan
> over so much code, I don't know if Kasan would have found this one.

It still worked. I looked into it some more and for some reason, struct realtek_priv
has a char buf[4096] member that's unused. I assume it caused kmalloc to return a 8K
slab, where the out-of-bound writes didn't overwrite anything of value. That buffer
ought to be removed, but that's for net-next.

I just checked with KASAN and it does detect the OOB on ARM64. I first noticed the
bug on barebox though, which has a near verbatim port of the Linux driver, but a
TLSF allocator, which fits allocations more tightly, hence it crashes not long
after driver probe unlike Linux.

> Rewriting the whole world in Rust will fix this problem, but it will
> take a while...

^^'.

Cheers,
Ahmad



> 
> Yours,
> Linus Walleij
>
Florian Fainelli March 15, 2023, 8:22 p.m. UTC | #8 
On 3/15/23 06:09, Ahmad Fatoum wrote:
> The probe function sets priv->chip_data to (void *)priv + sizeof(*priv)
> with the expectation that priv has enough trailing space.
> 
> However, only realtek-smi actually allocated this chip_data space.
> Do likewise in realtek-mdio to fix out-of-bounds accesses.
> 
> Fixes: aac94001067d ("net: dsa: realtek: add new mdio interface for drivers")
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Luiz Angelo Daros de Luca March 16, 2023, 5:41 p.m. UTC | #9 
> The probe function sets priv->chip_data to (void *)priv + sizeof(*priv)
> with the expectation that priv has enough trailing space.
>
> However, only realtek-smi actually allocated this chip_data space.
> Do likewise in realtek-mdio to fix out-of-bounds accesses.
>
> Fixes: aac94001067d ("net: dsa: realtek: add new mdio interface for drivers")
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

My bad. Thanks for the fix, Ahmad.

Reviewed-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
Jakub Kicinski March 17, 2023, 4:07 a.m. UTC | #10 
On Wed, 15 Mar 2023 14:09:15 +0100 Ahmad Fatoum wrote:
> -	priv = devm_kzalloc(&mdiodev->dev, sizeof(*priv), GFP_KERNEL);
> +	priv = devm_kzalloc(&mdiodev->dev, sizeof(*priv) + var->chip_data_sz, GFP_KERNEL);

size_add() ?
Otherwise some static checker is going to soon send us a patch saying
this can overflow. Let's save ourselves the hassle.
Ahmad Fatoum March 22, 2023, 3:51 p.m. UTC | #11 
On 17.03.23 05:07, Jakub Kicinski wrote:
> On Wed, 15 Mar 2023 14:09:15 +0100 Ahmad Fatoum wrote:
>> -	priv = devm_kzalloc(&mdiodev->dev, sizeof(*priv), GFP_KERNEL);
>> +	priv = devm_kzalloc(&mdiodev->dev, sizeof(*priv) + var->chip_data_sz, GFP_KERNEL);
> 
> size_add() ?
> Otherwise some static checker is going to soon send us a patch saying
> this can overflow. Let's save ourselves the hassle.

The exact same line is already in realtek-smi. Would you prefer I send
a follow-up patch for net-next which switches over both files to size_add
or should I send a v2?

Cheers,
Ahmad



>
Jakub Kicinski March 22, 2023, 6:21 p.m. UTC | #12 
On Wed, 22 Mar 2023 16:51:00 +0100 Ahmad Fatoum wrote:
> On 17.03.23 05:07, Jakub Kicinski wrote:
> > On Wed, 15 Mar 2023 14:09:15 +0100 Ahmad Fatoum wrote:  
> >> -	priv = devm_kzalloc(&mdiodev->dev, sizeof(*priv), GFP_KERNEL);
> >> +	priv = devm_kzalloc(&mdiodev->dev, sizeof(*priv) + var->chip_data_sz, GFP_KERNEL);  
> > 
> > size_add() ?
> > Otherwise some static checker is going to soon send us a patch saying
> > this can overflow. Let's save ourselves the hassle.  
> 
> The exact same line is already in realtek-smi. Would you prefer I send
> a follow-up patch for net-next which switches over both files to size_add
> or should I send a v2?

We can leave the existing code be, but use the helper in the new code
for v2
diff mbox series

Patch

diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
index 3e54fac5f902..3b22d3f7ad99 100644
--- a/drivers/net/dsa/realtek/realtek-mdio.c
+++ b/drivers/net/dsa/realtek/realtek-mdio.c
@@ -152,7 +152,7 @@  static int realtek_mdio_probe(struct mdio_device *mdiodev)
 	if (!var)
 		return -EINVAL;
 
-	priv = devm_kzalloc(&mdiodev->dev, sizeof(*priv), GFP_KERNEL);
+	priv = devm_kzalloc(&mdiodev->dev, sizeof(*priv) + var->chip_data_sz, GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;