2024-01-12 09:57:50

by Kunwu Chan

Subject: [PATCH net] net: phy: Fix possible NULL pointer dereference issues caused by phy_attached_info_irq

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 <[email protected]>
---
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);
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);
--
2.39.2

2024-01-12 15:33:09

by Andrew Lunn

Subject: Re: [PATCH net] net: phy: Fix possible NULL pointer dereference issues caused by phy_attached_info_irq

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 <[email protected]>
> ---
> 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

2024-01-15 03:46:00

by Andrew Lunn

Subject: Re: [PATCH net] net: phy: Fix possible NULL pointer dereference issues caused by phy_attached_info_irq

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

2024-01-15 03:50:19

by Kunwu Chan

Subject: Re: [PATCH net] net: phy: Fix possible NULL pointer dereference issues caused by phy_attached_info_irq

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 <[email protected]>
>> ---
>> 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
--
Thanks,
Kunwu

2024-01-15 07:35:21

by Kunwu Chan

Subject: Re: [PATCH net] net: phy: Fix possible NULL pointer dereference issues caused by phy_attached_info_irq

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.
--
Thanks,
Kunwu