| Message ID | 20230315130917.3633491-1-a.fatoum@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,1/2] net: dsa: realtek: fix out-of-bounds access | expand |
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
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 >
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 >
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 <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 >
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>
> 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>
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: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 >
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 --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;
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(-)