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

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

[Ahmad Fatoum]

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

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

[PATCH net 2/2] net: dsa: realtek: fix missing new lines in error messages

[Ahmad Fatoum]

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>
---
 drivers/net/dsa/realtek/rtl8365mb.c    | 2 +-
 drivers/net/dsa/realtek/rtl8366-core.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
index da31d8b839ac..33d28951f461 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -2068,7 +2068,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
 
 	if (!mb->chip_info) {
 		dev_err(priv->dev,
-			"unrecognized switch (id=0x%04x, ver=0x%04x)", chip_id,
+			"unrecognized switch (id=0x%04x, ver=0x%04x)\n", chip_id,
 			chip_ver);
 		return -ENODEV;
 	}
diff --git a/drivers/net/dsa/realtek/rtl8366-core.c b/drivers/net/dsa/realtek/rtl8366-core.c
index dc5f75be3017..f353483b281b 100644
--- a/drivers/net/dsa/realtek/rtl8366-core.c
+++ b/drivers/net/dsa/realtek/rtl8366-core.c
@@ -329,7 +329,7 @@ int rtl8366_vlan_add(struct dsa_switch *ds, int port,
 
 	ret = rtl8366_set_vlan(priv, vlan->vid, member, untag, 0);
 	if (ret) {
-		dev_err(priv->dev, "failed to set up VLAN %04x", vlan->vid);
+		dev_err(priv->dev, "failed to set up VLAN %04x\n", vlan->vid);
 		return ret;
 	}
 
@@ -338,7 +338,7 @@ int rtl8366_vlan_add(struct dsa_switch *ds, int port,
 
 	ret = rtl8366_set_pvid(priv, port, vlan->vid);
 	if (ret) {
-		dev_err(priv->dev, "failed to set PVID on port %d to VLAN %04x",
+		dev_err(priv->dev, "failed to set PVID on port %d to VLAN %04x\n",
 			port, vlan->vid);
 		return ret;
 	}
-- 
2.30.2

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

[Linus Walleij]

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

Re: [PATCH net 2/2] net: dsa: realtek: fix missing new lines in error messages

[Linus Walleij]

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

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

[Alvin Šipraga]

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
>

Re: [PATCH net 2/2] net: dsa: realtek: fix missing new lines in error messages

[Alvin Šipraga]

On Wed, Mar 15, 2023 at 02:09:16PM +0100, Ahmad Fatoum 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: Alvin Šipraga <alsi@bang-olufsen.dk>

>  drivers/net/dsa/realtek/rtl8365mb.c    | 2 +-
>  drivers/net/dsa/realtek/rtl8366-core.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> index da31d8b839ac..33d28951f461 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> @@ -2068,7 +2068,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
>  
>  	if (!mb->chip_info) {
>  		dev_err(priv->dev,
> -			"unrecognized switch (id=0x%04x, ver=0x%04x)", chip_id,
> +			"unrecognized switch (id=0x%04x, ver=0x%04x)\n", chip_id,
>  			chip_ver);
>  		return -ENODEV;
>  	}
> diff --git a/drivers/net/dsa/realtek/rtl8366-core.c b/drivers/net/dsa/realtek/rtl8366-core.c
> index dc5f75be3017..f353483b281b 100644
> --- a/drivers/net/dsa/realtek/rtl8366-core.c
> +++ b/drivers/net/dsa/realtek/rtl8366-core.c
> @@ -329,7 +329,7 @@ int rtl8366_vlan_add(struct dsa_switch *ds, int port,
>  
>  	ret = rtl8366_set_vlan(priv, vlan->vid, member, untag, 0);
>  	if (ret) {
> -		dev_err(priv->dev, "failed to set up VLAN %04x", vlan->vid);
> +		dev_err(priv->dev, "failed to set up VLAN %04x\n", vlan->vid);
>  		return ret;
>  	}
>  
> @@ -338,7 +338,7 @@ int rtl8366_vlan_add(struct dsa_switch *ds, int port,
>  
>  	ret = rtl8366_set_pvid(priv, port, vlan->vid);
>  	if (ret) {
> -		dev_err(priv->dev, "failed to set PVID on port %d to VLAN %04x",
> +		dev_err(priv->dev, "failed to set PVID on port %d to VLAN %04x\n",
>  			port, vlan->vid);
>  		return ret;
>  	}
> -- 
> 2.30.2
>

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

[Andrew Lunn]

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

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

[Ahmad Fatoum]

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
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Andrew Lunn]

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

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

[Ahmad Fatoum]

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
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Florian Fainelli]

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

Re: [PATCH net 2/2] net: dsa: realtek: fix missing new lines in error messages

[Florian Fainelli]

On 3/15/23 06:09, Ahmad Fatoum 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: Florian Fainelli <f.fainelli@gmail.com>

Not sure this deserves a fixes tag, but I guess why not.
-- 
Florian

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

[Luiz Angelo Daros de Luca]

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

Re: [PATCH net 2/2] net: dsa: realtek: fix missing new lines in error messages

[Jakub Kicinski]

On Wed, 15 Mar 2023 14:09:16 +0100 Ahmad Fatoum wrote:
> Some error messages lack a new line, add them.

I thought printk() and friends automatically add a new line these days,
unless continuation is specifically requested. Is that not the case?
Have you seen these prints actually getting mangled?

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

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

[Jakub Kicinski]

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.

Re: [PATCH net 2/2] net: dsa: realtek: fix missing new lines in error messages

[Ahmad Fatoum]

On 17.03.23 05:05, Jakub Kicinski wrote:
> On Wed, 15 Mar 2023 14:09:16 +0100 Ahmad Fatoum wrote:
>> Some error messages lack a new line, add them.
> 
> I thought printk() and friends automatically add a new line these days,
> unless continuation is specifically requested. Is that not the case?
> Have you seen these prints actually getting mangled?

I did not. I had noticed the asymmetry with other error prints in the
same file and assumed this to be unintended. Good to know this no
longer causes an issue nowadays.

Sorry for the noise and please disregard this patch.

Thanks,
Ahmad

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

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Ahmad Fatoum]

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



> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Jakub Kicinski]

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