[PATCH] drivers/net/wan/hdlc_ppp: fix potential null pointer in ppp_cp_event logging

[PATCH] drivers/net/wan/hdlc_ppp: fix potential null pointer in ppp_cp_event logging

[Kriish Sharma]

Fixes warnings observed during compilation with -Wformat-overflow:

drivers/net/wan/hdlc_ppp.c: In function ‘ppp_cp_event’:
drivers/net/wan/hdlc_ppp.c:353:17: warning: ‘%s’ directive argument is null [-Wformat-overflow=]
  353 |                 netdev_info(dev, "%s down\n", proto_name(pid));
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/wan/hdlc_ppp.c:342:17: warning: ‘%s’ directive argument is null [-Wformat-overflow=]
  342 |                 netdev_info(dev, "%s up\n", proto_name(pid));
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Introduce local variable `pname` and fallback to "unknown" if proto_name(pid)
returns NULL.

Fixes: 262858079afd ("Add linux-next specific files for 20250926")
Signed-off-by: Kriish Sharma <kriish.sharma2006@gmail.com>
---
 drivers/net/wan/hdlc_ppp.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wan/hdlc_ppp.c b/drivers/net/wan/hdlc_ppp.c
index 7496a2e9a282..f3b3fa8d46fd 100644
--- a/drivers/net/wan/hdlc_ppp.c
+++ b/drivers/net/wan/hdlc_ppp.c
@@ -339,7 +339,9 @@ static void ppp_cp_event(struct net_device *dev, u16 pid, u16 event, u8 code,
 		ppp_tx_cp(dev, pid, CP_CODE_REJ, ++ppp->seq, len, data);
 
 	if (old_state != OPENED && proto->state == OPENED) {
-		netdev_info(dev, "%s up\n", proto_name(pid));
+		const char *pname = proto_name(pid);
+
+		netdev_info(dev, "%s up\n", pname ? pname : "unknown");
 		if (pid == PID_LCP) {
 			netif_dormant_off(dev);
 			ppp_cp_event(dev, PID_IPCP, START, 0, 0, 0, NULL);
@@ -350,7 +352,9 @@ static void ppp_cp_event(struct net_device *dev, u16 pid, u16 event, u8 code,
 		}
 	}
 	if (old_state == OPENED && proto->state != OPENED) {
-		netdev_info(dev, "%s down\n", proto_name(pid));
+		const char *pname = proto_name(pid);
+
+		netdev_info(dev, "%s down\n", pname ? pname : "unknown");
 		if (pid == PID_LCP) {
 			netif_dormant_on(dev);
 			ppp_cp_event(dev, PID_IPCP, STOP, 0, 0, 0, NULL);
-- 
2.34.1

Re: [PATCH] drivers/net/wan/hdlc_ppp: fix potential null pointer in ppp_cp_event logging

[Dimitri Daskalakis]

On 10/2/25 11:05 AM, Kriish Sharma wrote:

> Fixes warnings observed during compilation with -Wformat-overflow:
>
> drivers/net/wan/hdlc_ppp.c: In function ‘ppp_cp_event’:
> drivers/net/wan/hdlc_ppp.c:353:17: warning: ‘%s’ directive argument is null [-Wformat-overflow=]
>   353 |                 netdev_info(dev, "%s down\n", proto_name(pid));
>       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/net/wan/hdlc_ppp.c:342:17: warning: ‘%s’ directive argument is null [-Wformat-overflow=]
>   342 |                 netdev_info(dev, "%s up\n", proto_name(pid));
>       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Introduce local variable `pname` and fallback to "unknown" if proto_name(pid)
> returns NULL.
>
> Fixes: 262858079afd ("Add linux-next specific files for 20250926")
> Signed-off-by: Kriish Sharma <kriish.sharma2006@gmail.com>
> ---
>  drivers/net/wan/hdlc_ppp.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wan/hdlc_ppp.c b/drivers/net/wan/hdlc_ppp.c
> index 7496a2e9a282..f3b3fa8d46fd 100644
> --- a/drivers/net/wan/hdlc_ppp.c
> +++ b/drivers/net/wan/hdlc_ppp.c
> @@ -339,7 +339,9 @@ static void ppp_cp_event(struct net_device *dev, u16 pid, u16 event, u8 code,
>  		ppp_tx_cp(dev, pid, CP_CODE_REJ, ++ppp->seq, len, data);
>  
>  	if (old_state != OPENED && proto->state == OPENED) {
> -		netdev_info(dev, "%s up\n", proto_name(pid));
> +		const char *pname = proto_name(pid);
> +
> +		netdev_info(dev, "%s up\n", pname ? pname : "unknown");
>  		if (pid == PID_LCP) {
>  			netif_dormant_off(dev);
>  			ppp_cp_event(dev, PID_IPCP, START, 0, 0, 0, NULL);
> @@ -350,7 +352,9 @@ static void ppp_cp_event(struct net_device *dev, u16 pid, u16 event, u8 code,
>  		}
>  	}
>  	if (old_state == OPENED && proto->state != OPENED) {
> -		netdev_info(dev, "%s down\n", proto_name(pid));
> +		const char *pname = proto_name(pid);
> +
> +		netdev_info(dev, "%s down\n", pname ? pname : "unknown");
>  		if (pid == PID_LCP) {
>  			netif_dormant_on(dev);
>  			ppp_cp_event(dev, PID_IPCP, STOP, 0, 0, 0, NULL);
Would it be better to return "unknown" in proto_name()'s default case?

Re: [PATCH] drivers/net/wan/hdlc_ppp: fix potential null pointer in ppp_cp_event logging

[Kriish Sharma]

Thanks for the suggestion. For this patch, I opted to handle the
fallback locally in ppp_cp_event to keep the change minimal and low
risk.

On Thu, Oct 2, 2025 at 11:46 PM Dimitri Daskalakis
<dimitri.daskalakis1@gmail.com> wrote:
>
> On 10/2/25 11:05 AM, Kriish Sharma wrote:
>
> > Fixes warnings observed during compilation with -Wformat-overflow:
> >
> > drivers/net/wan/hdlc_ppp.c: In function ‘ppp_cp_event’:
> > drivers/net/wan/hdlc_ppp.c:353:17: warning: ‘%s’ directive argument is null [-Wformat-overflow=]
> >   353 |                 netdev_info(dev, "%s down\n", proto_name(pid));
> >       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > drivers/net/wan/hdlc_ppp.c:342:17: warning: ‘%s’ directive argument is null [-Wformat-overflow=]
> >   342 |                 netdev_info(dev, "%s up\n", proto_name(pid));
> >       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Introduce local variable `pname` and fallback to "unknown" if proto_name(pid)
> > returns NULL.
> >
> > Fixes: 262858079afd ("Add linux-next specific files for 20250926")
> > Signed-off-by: Kriish Sharma <kriish.sharma2006@gmail.com>
> > ---
> >  drivers/net/wan/hdlc_ppp.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/wan/hdlc_ppp.c b/drivers/net/wan/hdlc_ppp.c
> > index 7496a2e9a282..f3b3fa8d46fd 100644
> > --- a/drivers/net/wan/hdlc_ppp.c
> > +++ b/drivers/net/wan/hdlc_ppp.c
> > @@ -339,7 +339,9 @@ static void ppp_cp_event(struct net_device *dev, u16 pid, u16 event, u8 code,
> >               ppp_tx_cp(dev, pid, CP_CODE_REJ, ++ppp->seq, len, data);
> >
> >       if (old_state != OPENED && proto->state == OPENED) {
> > -             netdev_info(dev, "%s up\n", proto_name(pid));
> > +             const char *pname = proto_name(pid);
> > +
> > +             netdev_info(dev, "%s up\n", pname ? pname : "unknown");
> >               if (pid == PID_LCP) {
> >                       netif_dormant_off(dev);
> >                       ppp_cp_event(dev, PID_IPCP, START, 0, 0, 0, NULL);
> > @@ -350,7 +352,9 @@ static void ppp_cp_event(struct net_device *dev, u16 pid, u16 event, u8 code,
> >               }
> >       }
> >       if (old_state == OPENED && proto->state != OPENED) {
> > -             netdev_info(dev, "%s down\n", proto_name(pid));
> > +             const char *pname = proto_name(pid);
> > +
> > +             netdev_info(dev, "%s down\n", pname ? pname : "unknown");
> >               if (pid == PID_LCP) {
> >                       netif_dormant_on(dev);
> >                       ppp_cp_event(dev, PID_IPCP, STOP, 0, 0, 0, NULL);
> Would it be better to return "unknown" in proto_name()'s default case?

Re: [PATCH] drivers/net/wan/hdlc_ppp: fix potential null pointer in ppp_cp_event logging

[Krzysztof Hałasa]

Hi Kriish,

Kriish Sharma <kriish.sharma2006@gmail.com> writes:

> Fixes warnings observed during compilation with -Wformat-overflow:
>
> drivers/net/wan/hdlc_ppp.c: In function ‘ppp_cp_event’:
> drivers/net/wan/hdlc_ppp.c:353:17: warning: ‘%s’ directive argument is null [-Wformat-overflow=]
>   353 |                 netdev_info(dev, "%s down\n", proto_name(pid));
>       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/net/wan/hdlc_ppp.c:342:17: warning: ‘%s’ directive argument is null [-Wformat-overflow=]
>   342 |                 netdev_info(dev, "%s up\n", proto_name(pid));
>       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

It appears proto_name(pid) never returns NULL there. Despite actually
saying "return NULL", that's right :-)

Perhaps you should change it to return "LCP" by default instead, and
not only on PID_LCP? It should silence the compiler.

This ppp_cp_event() is called in a few places:
- ppp_cp_parse_cr()
- ppp_rx()
- ppp_timer() (with a known protocol, though)
- and others, with PID_LCP.

Now, before printing proto_name(pid), ppp_cp_event() does
proto = get_proto(pid), and dereferences it :-)

The pid seems to always come from ppp_rx(). Fortunately it's checked
at start, and it case of an unknown proto it goes straight to rx_error.
-- 
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa

Re: [PATCH] drivers/net/wan/hdlc_ppp: fix potential null pointer in ppp_cp_event logging

[Kriish Sharma]

Hi,

Thanks for the clarification.
I can update proto_name() to return "LCP" by default instead of NULL,
which should silence the compiler without changing behavior.
I can send another patch for this if you'd like.

Best regards,
Kriish

On Fri, Oct 3, 2025 at 12:04 PM Krzysztof Hałasa <khalasa@piap.pl> wrote:
>
> Hi Kriish,
>
> Kriish Sharma <kriish.sharma2006@gmail.com> writes:
>
> > Fixes warnings observed during compilation with -Wformat-overflow:
> >
> > drivers/net/wan/hdlc_ppp.c: In function ‘ppp_cp_event’:
> > drivers/net/wan/hdlc_ppp.c:353:17: warning: ‘%s’ directive argument is null [-Wformat-overflow=]
> >   353 |                 netdev_info(dev, "%s down\n", proto_name(pid));
> >       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > drivers/net/wan/hdlc_ppp.c:342:17: warning: ‘%s’ directive argument is null [-Wformat-overflow=]
> >   342 |                 netdev_info(dev, "%s up\n", proto_name(pid));
> >       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> It appears proto_name(pid) never returns NULL there. Despite actually
> saying "return NULL", that's right :-)
>
> Perhaps you should change it to return "LCP" by default instead, and
> not only on PID_LCP? It should silence the compiler.
>
> This ppp_cp_event() is called in a few places:
> - ppp_cp_parse_cr()
> - ppp_rx()
> - ppp_timer() (with a known protocol, though)
> - and others, with PID_LCP.
>
> Now, before printing proto_name(pid), ppp_cp_event() does
> proto = get_proto(pid), and dereferences it :-)
>
> The pid seems to always come from ppp_rx(). Fortunately it's checked
> at start, and it case of an unknown proto it goes straight to rx_error.
> --
> Krzysztof "Chris" Hałasa
>
> Sieć Badawcza Łukasiewicz
> Przemysłowy Instytut Automatyki i Pomiarów PIAP
> Al. Jerozolimskie 202, 02-486 Warszawa

Re: [PATCH] drivers/net/wan/hdlc_ppp: fix potential null pointer in ppp_cp_event logging

[Simon Horman]

On Thu, Oct 02, 2025 at 06:05:41PM +0000, Kriish Sharma wrote:
> Fixes warnings observed during compilation with -Wformat-overflow:
> 
> drivers/net/wan/hdlc_ppp.c: In function ‘ppp_cp_event’:
> drivers/net/wan/hdlc_ppp.c:353:17: warning: ‘%s’ directive argument is null [-Wformat-overflow=]
>   353 |                 netdev_info(dev, "%s down\n", proto_name(pid));
>       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/net/wan/hdlc_ppp.c:342:17: warning: ‘%s’ directive argument is null [-Wformat-overflow=]
>   342 |                 netdev_info(dev, "%s up\n", proto_name(pid));
>       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Introduce local variable `pname` and fallback to "unknown" if proto_name(pid)
> returns NULL.
> 
> Fixes: 262858079afd ("Add linux-next specific files for 20250926")
> Signed-off-by: Kriish Sharma <kriish.sharma2006@gmail.com>

Hi Kriish,

As it looks like there will be another revision of this patch,
I have a few minor points on process for your consideration.

As a fix for Networking code present in the net tree this should probably
be targeted at the net tree. That means it should apply cleanly to that
tree (I assume it does). And the target tree should be denoted in the
subject.  Like this:

Subject: [PATCh net] ...

This is as opposed to non-fix patches which, generally, are targeted
at the net-nex tree.

Specifying the target tree helps land patches in the right place
for CI. And helps the maintainers too.

Also, git history isn't consistent here, but I would suggest
that a more succinct prefix is appropriate for this patch.
Perhaps 'hdlc_ppp:'

I.e.: Subject: [PATCH net] hdlc_ppp: ...

For more in process for networking patches please see:
https://docs.kernel.org/process/maintainer-netdev.html

Thanks!

...

Re: [PATCH] drivers/net/wan/hdlc_ppp: fix potential null pointer in ppp_cp_event logging

[Kriish Sharma]

Hi Simon,

Thanks for the review and guidance.
I’ll prepare a v2 targeting the net tree, updating the patch subject
and incorporating the suggested changes.

On Fri, Oct 3, 2025 at 2:03 PM Simon Horman <horms@kernel.org> wrote:
>
> On Thu, Oct 02, 2025 at 06:05:41PM +0000, Kriish Sharma wrote:
> > Fixes warnings observed during compilation with -Wformat-overflow:
> >
> > drivers/net/wan/hdlc_ppp.c: In function ‘ppp_cp_event’:
> > drivers/net/wan/hdlc_ppp.c:353:17: warning: ‘%s’ directive argument is null [-Wformat-overflow=]
> >   353 |                 netdev_info(dev, "%s down\n", proto_name(pid));
> >       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > drivers/net/wan/hdlc_ppp.c:342:17: warning: ‘%s’ directive argument is null [-Wformat-overflow=]
> >   342 |                 netdev_info(dev, "%s up\n", proto_name(pid));
> >       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Introduce local variable `pname` and fallback to "unknown" if proto_name(pid)
> > returns NULL.
> >
> > Fixes: 262858079afd ("Add linux-next specific files for 20250926")
> > Signed-off-by: Kriish Sharma <kriish.sharma2006@gmail.com>
>
> Hi Kriish,
>
> As it looks like there will be another revision of this patch,
> I have a few minor points on process for your consideration.
>
> As a fix for Networking code present in the net tree this should probably
> be targeted at the net tree. That means it should apply cleanly to that
> tree (I assume it does). And the target tree should be denoted in the
> subject.  Like this:
>
> Subject: [PATCh net] ...
>
> This is as opposed to non-fix patches which, generally, are targeted
> at the net-nex tree.
>
> Specifying the target tree helps land patches in the right place
> for CI. And helps the maintainers too.
>
> Also, git history isn't consistent here, but I would suggest
> that a more succinct prefix is appropriate for this patch.
> Perhaps 'hdlc_ppp:'
>
> I.e.: Subject: [PATCH net] hdlc_ppp: ...
>
> For more in process for networking patches please see:
> https://docs.kernel.org/process/maintainer-netdev.html
>
> Thanks!
>
> ...

Re: [PATCH] drivers/net/wan/hdlc_ppp: fix potential null pointer in ppp_cp_event logging

[Simon Horman]

On Fri, Oct 03, 2025 at 02:32:02PM +0530, Kriish Sharma wrote:
> Hi Simon,
> 
> Thanks for the review and guidance.
> I’ll prepare a v2 targeting the net tree, updating the patch subject
> and incorporating the suggested changes.

Thanks!

Re: [PATCH] drivers/net/wan/hdlc_ppp: fix potential null pointer in ppp_cp_event logging

[Paolo Abeni]

On 10/3/25 8:43 AM, Kriish Sharma wrote:
> Thanks for the clarification.
> I can update proto_name() to return "LCP" by default instead of NULL,
> which should silence the compiler without changing behavior.
> I can send another patch for this if you'd like.

If v2 is not ready yet, I think it would be better returning "unknown"
instead of "LCP" when the protocol id is actually unknown.

In the current code base, such case is unexpected/impossible, but the
compiler force us to handle it anyway. I think we should avoid hiding
the unexpected event.

Assuming all the code paths calling proto_name() ensure the pid is a
valid one, you should possibly add a WARN_ONCE() on the default case.

Thanks,

Paolo

Re: [PATCH] drivers/net/wan/hdlc_ppp: fix potential null pointer in ppp_cp_event logging

[Krzysztof Hałasa]

Paolo,

Paolo Abeni <pabeni@redhat.com> writes:

> If v2 is not ready yet, I think it would be better returning "unknown"
> instead of "LCP" when the protocol id is actually unknown.
>
> In the current code base, such case is unexpected/impossible, but the
> compiler force us to handle it anyway. I think we should avoid hiding
> the unexpected event.
>
> Assuming all the code paths calling proto_name() ensure the pid is a
> valid one, you should possibly add a WARN_ONCE() on the default case.

Look, this is really simple code. Do we need additional bloat
everywhere?

The compiler doesn't force us to anything. We define that, as far as
get_proto() is concerned, PID_IPCP is "IPCP", PID_IPV6CP is "IPV6CP",
and all other values mean "LCP". Then we construct the switch statement
accordingly. Well, it seems I failed it slightly originally, most
probably due to copy & paste from get_proto(). Now Kriish has noticed it
and agreed to make it perfect.

Do you really think we should now change semantics of this 20 years old
code (most probably never working incorrectly), adding some "unknown"
(yet impossible) case, and WARNing about a condition which is excluded
at the start of the whole RX parser?

Well, maybe gcc would identify and remove these new unneeded operations.
Maybe. I think we don't need more bloat at the source level either,
though.
-- 
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa

Re: [PATCH] drivers/net/wan/hdlc_ppp: fix potential null pointer in ppp_cp_event logging

[Paolo Abeni]

On 10/7/25 11:28 AM, Krzysztof Hałasa wrote:
> Paolo Abeni <pabeni@redhat.com> writes:
>> If v2 is not ready yet, I think it would be better returning "unknown"
>> instead of "LCP" when the protocol id is actually unknown.
>>
>> In the current code base, such case is unexpected/impossible, but the
>> compiler force us to handle it anyway. I think we should avoid hiding
>> the unexpected event.
>>
>> Assuming all the code paths calling proto_name() ensure the pid is a
>> valid one, you should possibly add a WARN_ONCE() on the default case.
> 
> Look, this is really simple code. Do we need additional bloat
> everywhere?
> 
> The compiler doesn't force us to anything. We define that, as far as
> get_proto() is concerned, PID_IPCP is "IPCP", PID_IPV6CP is "IPV6CP",
> and all other values mean "LCP". Then we construct the switch statement
> accordingly. Well, it seems I failed it slightly originally, most
> probably due to copy & paste from get_proto(). Now Kriish has noticed it
> and agreed to make it perfect.
> 
> Do you really think we should now change semantics of this 20 years old
> code (most probably never working incorrectly), adding some "unknown"
> (yet impossible) case, and WARNing about a condition which is excluded
> at the start of the whole RX parser?

Note that the suggested change is not going to change any semantic, just
make it clear for future changes that such case is not really expected.

And that in turn is my point. If someone else is going to touch this
code in the (not so near) future, such person will not have to read all
the possible code paths leading to proto_name() to understand the
assumption in the current code base.

Cheers,

Paolo

Re: [PATCH] drivers/net/wan/hdlc_ppp: fix potential null pointer in ppp_cp_event logging

[Krzysztof Hałasa]

Paolo Abeni <pabeni@redhat.com> writes:

> Note that the suggested change is not going to change any semantic, just
> make it clear for future changes that such case is not really
> expected.

But there is no "other case", any numeric argument this function is
called with is expected. This is not a hdlc-ppp-wide function. Such
a function was simply not needed. That the function returned NULL in an
impossible case is just, I guess, my coding deficiency. Not my worst,
though :-)

Would you rather like #define proto_name(x) ((x) == PID_LCP ? "LCP" : (x)
== PID_IPCP ? "IPCP" : "IPV6CP")? Or maybe you would like the "unknown"
case there as well?

This function is for only those 3 protocols, all of them being "control
protocols": IP control protocol, IPv6 control protocol, and link control
protocol. Think of it as of

enum control_protocols {LCP, IP, IPV6};

proto_name(enum control_protocols)
...

You must not call this function in any other context, e.g. it's not OK
to call it with PID_IP nor PID_IPV6 (which are otherwise perfectly valid
PIDs in this very file).

If the function's name is misleading, perhaps it could be extended to
control_proto_name(). Not that I find such changes entertaining, but it
would be technically correct after all.

HTH,
-- 
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa