Received-SPF: pass (google.com: domain of linux-wireless+bounces-4477-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: Rand Deeb <rand.sec96@gmail.com>
To: m@bues.ch
Cc: deeb.rand@confident.ru,
	jonas.gorski@gmail.com,
	khoroshilov@ispras.ru,
	kvalo@kernel.org,
	linux-kernel@vger.kernel.org,
	linux-wireless@vger.kernel.org,
	lvc-project@linuxtesting.org,
	rand.sec96@gmail.com,
	voskresenski.stanislav@confident.ru,
	james.dutton@gmail.com
Subject: Re: [PATCH v3] ssb: Fix potential NULL pointer dereference in ssb_device_uevent
Date: Fri,  8 Mar 2024 02:29:27 +0300
Message-Id: <20240307232927.171197-1-rand.sec96@gmail.com>
In-Reply-To: <20240307223849.13d5b58b@barney>
References: <20240307223849.13d5b58b@barney>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit


On Fri, Mar 8, 2024 at 12:39 AM Michael Büsch <m@bues.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.

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

I'm talking about your phrase "Not having these checks is a big part of why
security sucks in today's software."
I think it's a matter of design problem, when you don't have a good design 
of course you'll need to add so many checks everywhere.
Let me explain my point of view by example, 

// Good design
CHECK(x){
	if x != null && x is a number
		return true;
	else return false;
}
MULTIPLY(a, b){
	return a*b;
}
SUM(a, b){
	return a+b;
}
....
MAIN(){
	// input a, b
	CHECK(a);
	CHECK(b);
	// now do the operations
	SUM(a, b)
	MULTIPLY(a, b)
}

// Bad design
SUM(x, y){
	if x != null && x is a number
		return x+y;
}
MULTIPLY(x, y){
	if x != null && x is a number
		return x*y;
}
...


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


> 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.
Would you please check the link in the previous comment about a similar 
situation got accepted and applied.


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

With all due respect, I respectfully disagree with you on this point. In 
your prior comment, you stated, "it is clearly documented that the pointer 
can never be NULL" However, if the reviewer encounters this check, they 
might mistakenly assume that 'dev' could indeed be NULL before the function
call. 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.

--
Best Regards
Rand Deeb