netdev - Re: [PATCH] drivers/net/wan/hdlc_ppp: fix potential null pointer in ppp_cp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <71dda358-c1f7-46ab-a241-dffc3c1c065d@redhat.com>
Date: Tue, 7 Oct 2025 12:46:02 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Krzysztof Hałasa <khalasa@...p.pl>
Cc: Kriish Sharma <kriish.sharma2006@...il.com>, khc@...waw.pl,
 andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drivers/net/wan/hdlc_ppp: fix potential null pointer in
 ppp_cp_event logging

On 10/7/25 11:28 AM, Krzysztof Hałasa wrote:
> Paolo Abeni <pabeni@...hat.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