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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Wed, Mar 15, 2023 at 2:09 PM Ahmad Fatoum <[email protected]> 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 <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
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
On Wed, Mar 15, 2023 at 2:09 PM Ahmad Fatoum <[email protected]> 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 <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
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 <[email protected]>
> ---
Reviewed-by: Alvin Šipraga <[email protected]>
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
>
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 <[email protected]>
> ---
Reviewed-by: Alvin Šipraga <[email protected]>
> 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
>
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
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 |
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
Hi Linus,
On 15.03.23 14:15, Linus Walleij wrote:
> On Wed, Mar 15, 2023 at 2:09 PM Ahmad Fatoum <[email protected]> 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 <[email protected]>
>
> Reviewed-by: Linus Walleij <[email protected]>
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 |
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 <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
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 <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
Not sure this deserves a fixes tag, but I guess why not.
--
Florian
> 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 <[email protected]>
My bad. Thanks for the fix, Ahmad.
Reviewed-by: Luiz Angelo Daros de Luca <[email protected]>
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 <[email protected]>
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.
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 <[email protected]>
>
--
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 |
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 |
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