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: Fri, 8 Mar 2024 06:09:43 +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, james.dutton@...il.com
Subject: Re: [PATCH v3] ssb: Fix potential NULL pointer dereference in
 ssb_device_uevent

On Fri,  8 Mar 2024 02:29:27 +0300
Rand Deeb <rand.sec96@...il.com> wrote:

> On Fri, Mar 8, 2024 at 12:39 AM Michael Büsch <m@...s.ch> wrote:
> 
> > The point is that leaving them in is defensive programming against future changes
> > or against possible misunderstandings of the situation.  
> 
> Dear Michael, I understand your point. It's essential to consider defensive
> programming principles to anticipate and mitigate potential issues in the 
> future. However, it's also crucial to strike a balance and not overburden 
> every function with excessive checks. It's about adopting a mindset of 
> anticipating potential problems while also maintaining code clarity and 
> efficiency.

Removing NULL checks is the opposite of maintainability and code clarity.
Efficiency doesn't matter here. (And besides that, NULL checks do not always mean less efficiency.)

> > A NULL pointer dereference is Undefined Behavior.
> > It can't get much worse in C.  
> 
> Again, If we adopt this approach, we'll find ourselves adding a null check 
> to every function we write, assuming that such changes may occur in the 
> future.

This would be a good thing.
Let the compiler remove redundant checks or let them stay there in the resulting
program, if the compiler can't fiure it out.
Checks are a good thing.

> > 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.  
> 
> My suggestion was to remove a (REDUNDANT) null pointer check, and not a 
> null pointer check, there is a big difference.

No. There is no difference.

> However, if the reviewer encounters this check, they 
> might mistakenly assume that 'dev' could indeed be NULL before the function
> call.

So? Nothing would happen.

> Conversely, if they read that 'dev' cannot be NULL, it could lead to 
> confusion, and perhaps they want the actual null check. Removing redundant 
> checks could mitigate confusion and minimize the risk of overlooking the 
> actual null check for example.

I fundamentally disagree.
Removing a NULL check _adds_ confusion.
NULL is "the billion mistake" of computing.
Please don't ever make it worse.
Thanks.

I will not ack a patch that reduces code quality.
Removing NULL checks almost always reduces the quality of the code.

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

Content of type "application/pgp-signature" skipped