linux-kernel - Re: [PATCH v3] ssb: Fix potential NULL pointer dereference in ssb_device

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]

Date: Thu, 7 Mar 2024 22:38:49 +0100
From: Michael Büsch <m@...s.ch>
To: Rand Deeb <rand.sec96@...il.com>
Cc: deeb.rand@...fident.ru, jonas.gorski@...il.com, khoroshilov@...ras.ru,
 kvalo@...nel.org, linux-kernel@...r.kernel.org,
 linux-wireless@...r.kernel.org, lvc-project@...uxtesting.org,
 voskresenski.stanislav@...fident.ru
Subject: Re: [PATCH v3] ssb: Fix potential NULL pointer dereference in
 ssb_device_uevent

On Fri,  8 Mar 2024 00:19:28 +0300
Rand Deeb <rand.sec96@...il.com> wrote:

> Yes, I agree, this is not critical code, but what's the point of leaving 
> redundant conditions even if they won't make a significant performance 
> difference, regardless of the policy (In other words, as a friendly 
> discussion) ?

The point is that leaving them in is defensive programming against future changes
or against possible misunderstandings of the situation.

Removing this check would improve nothing.

> I understand and respect your point of view as software engineer but it's a
> matter of design problems which is not our case here.

No, it very well is.

> Defensive programming is typically applied when there's a potential risk, 

A NULL pointer dereference is Undefined Behavior.
It can't get much worse in C.

> If we adopt this
> approach as a form of defensive programming, we'd find ourselves adding 
> similar conditions to numerous functions and parameters.

Not at all.
Your suggestion was about REMOVING a null pointer check.
Not about adding one.
I NAK-ed the REMOVAL of a null pointer check. Not the addition.

> Moreover, this 
> would unnecessarily complicate the codebase, especially during reviews.

Absolutely wrong.
Not having a NULL check complicates reviews.
Reviewers will have to prove that pointers cannot be NULL, if there is no check.

> so would you recommend fix the commit message as Jeff Johnson recommended ?
> or just keep it as it is ?

I don't care about the commit message.
I comment on the change itself.

-- 
Michael Büsch
https://bues.ch/

Content of type "application/pgp-signature" skipped