| Message ID | 20240112095724.154197-1-chentao@kylinos.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: phy: Fix possible NULL pointer dereference issues caused by phy_attached_info_irq | expand |
On Fri, Jan 12, 2024 at 05:57:24PM +0800, Kunwu Chan wrote: > kasprintf() returns a pointer to dynamically allocated memory > which can be NULL upon failure. Ensure the allocation was successful > by checking the pointer validity. > > Fixes: e27f178793de ("net: phy: Added IRQ print to phylink_bringup_phy()") > Signed-off-by: Kunwu Chan <chentao@kylinos.cn> > --- > drivers/net/phy/phy_device.c | 3 +++ > drivers/net/phy/phylink.c | 2 ++ > 2 files changed, 5 insertions(+) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 3611ea64875e..10fa99d957c0 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -1299,6 +1299,9 @@ void phy_attached_print(struct phy_device *phydev, const char *fmt, ...) > const char *unbound = phydev->drv ? "" : "[unbound] "; > char *irq_str = phy_attached_info_irq(phydev); > > + if (!irq_str) > + return; > + > if (!fmt) { > phydev_info(phydev, ATTACHED_FMT "\n", unbound, > phydev_name(phydev), irq_str); This part looks O.K. > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > index ed0b4ccaa6a6..db0a545c9468 100644 > --- a/drivers/net/phy/phylink.c > +++ b/drivers/net/phy/phylink.c > @@ -1884,6 +1884,8 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy, > phy->phy_link_change = phylink_phy_change; > > irq_str = phy_attached_info_irq(phy); > + if (!irq_str) > + return -ENOMEM; Here, i would just skip the print and continue with the reset of the function. The print is just useful information, its not a big problem if its not printed. However, if this function does not complete, the network interface is likely to be dead. Andrew
Thanks for your reply. On 2024/1/12 23:32, Andrew Lunn wrote: > On Fri, Jan 12, 2024 at 05:57:24PM +0800, Kunwu Chan wrote: >> kasprintf() returns a pointer to dynamically allocated memory >> which can be NULL upon failure. Ensure the allocation was successful >> by checking the pointer validity. >> >> Fixes: e27f178793de ("net: phy: Added IRQ print to phylink_bringup_phy()") >> Signed-off-by: Kunwu Chan <chentao@kylinos.cn> >> --- >> drivers/net/phy/phy_device.c | 3 +++ >> drivers/net/phy/phylink.c | 2 ++ >> 2 files changed, 5 insertions(+) >> >> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c >> index 3611ea64875e..10fa99d957c0 100644 >> --- a/drivers/net/phy/phy_device.c >> +++ b/drivers/net/phy/phy_device.c >> @@ -1299,6 +1299,9 @@ void phy_attached_print(struct phy_device *phydev, const char *fmt, ...) >> const char *unbound = phydev->drv ? "" : "[unbound] "; >> char *irq_str = phy_attached_info_irq(phydev); >> >> + if (!irq_str) >> + return; >> + >> if (!fmt) { >> phydev_info(phydev, ATTACHED_FMT "\n", unbound, >> phydev_name(phydev), irq_str); > > This part looks O.K. > >> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c >> index ed0b4ccaa6a6..db0a545c9468 100644 >> --- a/drivers/net/phy/phylink.c >> +++ b/drivers/net/phy/phylink.c >> @@ -1884,6 +1884,8 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy, >> phy->phy_link_change = phylink_phy_change; >> >> irq_str = phy_attached_info_irq(phy); >> + if (!irq_str) >> + return -ENOMEM; > > Here, i would just skip the print and continue with the reset of the > function. The print is just useful information, its not a big problem > if its not printed. However, if this function does not complete, the > network interface is likely to be dead. Thanks for the reminder. The second part doesn't look so perfect, can we just print an empty string when the irq_str is empty? --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -1886,7 +1886,7 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy, irq_str = phy_attached_info_irq(phy); phylink_info(pl, "PHY [%s] driver [%s] (irq=%s)\n", - dev_name(&phy->mdio.dev), phy->drv->name, irq_str); + dev_name(&phy->mdio.dev), phy->drv->name, irq_str ? irq_str : ""); kfree(irq_str); > > Andrew
> > Here, i would just skip the print and continue with the reset of the > > function. The print is just useful information, its not a big problem > > if its not printed. However, if this function does not complete, the > > network interface is likely to be dead. > Thanks for the reminder. > The second part doesn't look so perfect, can we just print an empty string > when the irq_str is empty? > > --- a/drivers/net/phy/phylink.c > +++ b/drivers/net/phy/phylink.c > @@ -1886,7 +1886,7 @@ static int phylink_bringup_phy(struct phylink *pl, > struct phy_device *phy, > irq_str = phy_attached_info_irq(phy); > phylink_info(pl, > "PHY [%s] driver [%s] (irq=%s)\n", > - dev_name(&phy->mdio.dev), phy->drv->name, irq_str); > + dev_name(&phy->mdio.dev), phy->drv->name, irq_str ? > irq_str : ""); > kfree(irq_str); That is O.K, or skip the whole phylink_info(). Andrew
On 2024/1/15 11:45, Andrew Lunn wrote: >>> Here, i would just skip the print and continue with the reset of the >>> function. The print is just useful information, its not a big problem >>> if its not printed. However, if this function does not complete, the >>> network interface is likely to be dead. >> Thanks for the reminder. >> The second part doesn't look so perfect, can we just print an empty string >> when the irq_str is empty? >> >> --- a/drivers/net/phy/phylink.c >> +++ b/drivers/net/phy/phylink.c >> @@ -1886,7 +1886,7 @@ static int phylink_bringup_phy(struct phylink *pl, >> struct phy_device *phy, >> irq_str = phy_attached_info_irq(phy); >> phylink_info(pl, >> "PHY [%s] driver [%s] (irq=%s)\n", >> - dev_name(&phy->mdio.dev), phy->drv->name, irq_str); >> + dev_name(&phy->mdio.dev), phy->drv->name, irq_str ? >> irq_str : ""); >> kfree(irq_str); > > That is O.K, or skip the whole phylink_info(). > > Andrew Thanks, I will update it in v2 patch. Personal view, print a msg is good for debug.
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 3611ea64875e..10fa99d957c0 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -1299,6 +1299,9 @@ void phy_attached_print(struct phy_device *phydev, const char *fmt, ...) const char *unbound = phydev->drv ? "" : "[unbound] "; char *irq_str = phy_attached_info_irq(phydev); + if (!irq_str) + return; + if (!fmt) { phydev_info(phydev, ATTACHED_FMT "\n", unbound, phydev_name(phydev), irq_str); diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index ed0b4ccaa6a6..db0a545c9468 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -1884,6 +1884,8 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy, phy->phy_link_change = phylink_phy_change; irq_str = phy_attached_info_irq(phy); + if (!irq_str) + return -ENOMEM; phylink_info(pl, "PHY [%s] driver [%s] (irq=%s)\n", dev_name(&phy->mdio.dev), phy->drv->name, irq_str);
kasprintf() returns a pointer to dynamically allocated memory which can be NULL upon failure. Ensure the allocation was successful by checking the pointer validity. Fixes: e27f178793de ("net: phy: Added IRQ print to phylink_bringup_phy()") Signed-off-by: Kunwu Chan <chentao@kylinos.cn> --- drivers/net/phy/phy_device.c | 3 +++ drivers/net/phy/phylink.c | 2 ++ 2 files changed, 5 insertions(+)