[PATCH v3] ssb: Fix potential NULL pointer dereference in ssb_device_uevent

[PATCH v3] ssb: Fix potential NULL pointer dereference in ssb_device_uevent

[Rand Deeb]

The ssb_device_uevent function first attempts to convert the 'dev' pointer
to 'struct ssb_device *'. However, it mistakenly dereferences 'dev' before
performing the NULL check, potentially leading to a NULL pointer
dereference if 'dev' is NULL.

To fix this issue, this patch moves the NULL check before dereferencing the
'dev' pointer, ensuring that the pointer is valid before attempting to use
it.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Rand Deeb <rand.sec96@gmail.com>
---
 drivers/ssb/main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/ssb/main.c b/drivers/ssb/main.c
index b9934b9c2d70..070a99a4180c 100644
--- a/drivers/ssb/main.c
+++ b/drivers/ssb/main.c
@@ -341,11 +341,13 @@ static int ssb_bus_match(struct device *dev, struct device_driver *drv)
 
 static int ssb_device_uevent(const struct device *dev, struct kobj_uevent_env *env)
 {
-	const struct ssb_device *ssb_dev = dev_to_ssb_dev(dev);
+	const struct ssb_device *ssb_dev;
 
 	if (!dev)
 		return -ENODEV;
 
+	ssb_dev = dev_to_ssb_dev(dev);
+
 	return add_uevent_var(env,
 			     "MODALIAS=ssb:v%04Xid%04Xrev%02X",
 			     ssb_dev->id.vendor, ssb_dev->id.coreid,
-- 
2.34.1

Re: [PATCH v3] ssb: Fix potential NULL pointer dereference in ssb_device_uevent

[Jeff Johnson]

On 3/6/2024 4:30 AM, Rand Deeb wrote:
> The ssb_device_uevent function first attempts to convert the 'dev' pointer
> to 'struct ssb_device *'. However, it mistakenly dereferences 'dev' before
> performing the NULL check, potentially leading to a NULL pointer
> dereference if 'dev' is NULL.
> 
> To fix this issue, this patch moves the NULL check before dereferencing the

see
<https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes>

"Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to
do frotz", as if you are giving orders to the codebase to change its
behaviour."

so please use imperative mood: s/this patch moves/move/

> 'dev' pointer, ensuring that the pointer is valid before attempting to use
> it.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Rand Deeb <rand.sec96@gmail.com>
> ---
>  drivers/ssb/main.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ssb/main.c b/drivers/ssb/main.c
> index b9934b9c2d70..070a99a4180c 100644
> --- a/drivers/ssb/main.c
> +++ b/drivers/ssb/main.c
> @@ -341,11 +341,13 @@ static int ssb_bus_match(struct device *dev, struct device_driver *drv)
>  
>  static int ssb_device_uevent(const struct device *dev, struct kobj_uevent_env *env)
>  {
> -	const struct ssb_device *ssb_dev = dev_to_ssb_dev(dev);
> +	const struct ssb_device *ssb_dev;
>  
>  	if (!dev)
>  		return -ENODEV;
>  
> +	ssb_dev = dev_to_ssb_dev(dev);
> +
>  	return add_uevent_var(env,
>  			     "MODALIAS=ssb:v%04Xid%04Xrev%02X",
>  			     ssb_dev->id.vendor, ssb_dev->id.coreid,

Re: [PATCH v3] ssb: Fix potential NULL pointer dereference in ssb_device_uevent

[Jonas Gorski]

Hi

On Wed, 6 Mar 2024 at 13:32, Rand Deeb <rand.sec96@gmail.com> wrote:
>
> The ssb_device_uevent function first attempts to convert the 'dev' pointer
> to 'struct ssb_device *'. However, it mistakenly dereferences 'dev' before
> performing the NULL check, potentially leading to a NULL pointer
> dereference if 'dev' is NULL.
>
> To fix this issue, this patch moves the NULL check before dereferencing the
> 'dev' pointer, ensuring that the pointer is valid before attempting to use
> it.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Signed-off-by: Rand Deeb <rand.sec96@gmail.com>
> ---
>  drivers/ssb/main.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/ssb/main.c b/drivers/ssb/main.c
> index b9934b9c2d70..070a99a4180c 100644
> --- a/drivers/ssb/main.c
> +++ b/drivers/ssb/main.c
> @@ -341,11 +341,13 @@ static int ssb_bus_match(struct device *dev, struct device_driver *drv)
>
>  static int ssb_device_uevent(const struct device *dev, struct kobj_uevent_env *env)
>  {
> -       const struct ssb_device *ssb_dev = dev_to_ssb_dev(dev);
> +       const struct ssb_device *ssb_dev;
>
>         if (!dev)
>                 return -ENODEV;
>
> +       ssb_dev = dev_to_ssb_dev(dev);
> +

The NULL check is what needs to be fixed/removed, not the code
surrounding it. This function will be called from dev_uevent() [1]
where dev cannot be NULL. So a NULL dereference cannot happen.

Most other implementors of bus_type::uevent have no NULL check. To be
precise, there is only one other implementor with a NULL check,
rio_uevent(), and none of the other ones have one. See e.g.
bcma_device_uevent(), memstick_uevent(), mips_cdmm_uevent(), or
fsl_mc_bus_uevent().

[1] https://elixir.bootlin.com/linux/v6.7.8/source/drivers/base/core.c#L2590

Best Regards,
Jonas

Re: [PATCH v3] ssb: Fix potential NULL pointer dereference in ssb_device_uevent

[Rand Deeb]

On Wed, Mar 6, 2024 at 10:51 PM Jonas Gorski <jonas.gorski@gmail.com> wrote:
>
> Hi
>
> The NULL check is what needs to be fixed/removed, not the code
> surrounding it. This function will be called from dev_uevent() [1]
> where dev cannot be NULL. So a NULL dereference cannot happen.
>
> Most other implementors of bus_type::uevent have no NULL check. To be
> precise, there is only one other implementor with a NULL check,
> rio_uevent(), and none of the other ones have one. See e.g.
> bcma_device_uevent(), memstick_uevent(), mips_cdmm_uevent(), or
> fsl_mc_bus_uevent().


Hi Jonas,

Thank you for the feedback. To be precise there are actually 8 other 
implementors (and potentially more) with a NULL check not just one 
(parisc_uevent, serio_uevent, ipack_uevent, pci_uevent, pcmcia_bus_uevent, 
rio_uevent, zorro_uevent, and soundbus_ueven).

After a second review, I totally concur with your observations. I quickly 
judged, I believed there might be an alternative way to call the function 
because it's not the only one with a null check, and actually the patch 
version 1 got accknowleded, that's why i'm confused. 

Given that null is improbable in this context due to the calls being made 
through uevent, we should eliminate the redundant condition. In light of 
this, would you recommend sending a new version (v4) of the patch with the 
correct title and info, or do you think it would be more appropriate to 
submit an entirely fresh patch? i'll also send patches to all of the 
implementors.

Best regards.

Re: [PATCH v3] ssb: Fix potential NULL pointer dereference in ssb_device_uevent

[Michael Büsch]

[-- Attachment #1: Type: text/plain, Size: 962 bytes --]

On Thu,  7 Mar 2024 16:41:42 +0300
Rand Deeb <rand.sec96@gmail.com> wrote:

> Given that null is improbable in this context due to the calls being made 
> through uevent, we should eliminate the redundant condition. In light of 
> this, would you recommend sending a new version (v4)

No.

We should _not_ eliminate NULL checks in this code.
Ever.
There is only one reason to eliminate NULL checks:
In extremely performance critical code, if it improves performance
significantly and it is clearly documented that the pointer can never be NULL.

This is not that case here. This is not critical code.

Having NULL checks is defensive programming.
Removing NULL checks is a hazard.
Not having these checks is a big part of why security sucks in today's software.

V3 shall be applied, because it fixes a clear bug. Whether this bug can actually
be triggered or not in today's kernel doesn't matter.

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3] ssb: Fix potential NULL pointer dereference in ssb_device_uevent

[Rand Deeb]

On Thu, Mar 7, 2024 at 9:24 PM Michael Büsch <m@bues.ch> wrote:

> There is only one reason to eliminate NULL checks:
> In extremely performance critical code, if it improves performance
> significantly and it is clearly documented that the pointer can never be NULL.
>
> This is not that case here. This is not critical code.

Hi Michael, thank you for your collaboration and feedback.
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) ?
Please take a look at https://git.kernel.org/netdev/net-next/c/92fc97ae9cfd
same situation but it has been applied ! why ?


> Having NULL checks is defensive programming.
> Removing NULL checks is a hazard.
> Not having these checks is a big part of why security sucks in today's software.

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.
Defensive programming is typically applied when there's a potential risk, 
but in our scenario, it's impossible for 'dev' to be NULL. If we adopt this
approach as a form of defensive programming, we'd find ourselves adding 
similar conditions to numerous functions and parameters. Moreover, this 
would unnecessarily complicate the codebase, especially during reviews. For
instance, encountering such a condition might lead one to assume that 'dev'
could indeed be NULL before reaching this function. That's my viewpoint.

> V3 shall be applied, because it fixes a clear bug. Whether this bug can actually
> be triggered or not in today's kernel doesn't matter.

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

--
Best Regards
Rand Deeb

Re: [PATCH v3] ssb: Fix potential NULL pointer dereference in ssb_device_uevent

[Michael Büsch]

[-- Attachment #1: Type: text/plain, Size: 1625 bytes --]

On Fri,  8 Mar 2024 00:19:28 +0300
Rand Deeb <rand.sec96@gmail.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/

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3] ssb: Fix potential NULL pointer dereference in ssb_device_uevent

[James Dutton]

On Thu, 7 Mar 2024 at 21:39, Michael Büsch <m@bues.ch> wrote:
>
> On Fri,  8 Mar 2024 00:19:28 +0300
> Rand Deeb <rand.sec96@gmail.com> wrote:
>
> > 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.
>

Hi,

This is an interesting discussion. Just to add my 2 cents.
If one does a NULL check after it has been previously dereferenced,
the compiler will totally remove the NULL check anyway, so although
the NULL check was in the source code, it will be absent from the
compiled code.
Re-arranging the NULL check to be before the dereference is fixing
that, but not necessarily in the way you expect.

Re: [PATCH v3] ssb: Fix potential NULL pointer dereference in ssb_device_uevent

[Michael Büsch]

[-- Attachment #1: Type: text/plain, Size: 448 bytes --]

On Thu, 7 Mar 2024 22:02:29 +0000
James Dutton <james.dutton@gmail.com> wrote:

> If one does a NULL check after it has been previously dereferenced,
> the compiler will totally remove the NULL check anyway,

I think the kernel uses -fno-delete-null-pointer-checks.

But anyway, this doesn't invalidate the point of having a NULL check.
The intent of the code was very clear.
So don't remove it.

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3] ssb: Fix potential NULL pointer dereference in ssb_device_uevent

[Rand Deeb]

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

Re: [PATCH v3] ssb: Fix potential NULL pointer dereference in ssb_device_uevent

[James Dutton]

On Thu, 7 Mar 2024 at 23:29, Rand Deeb <rand.sec96@gmail.com> wrote:
>
>
> 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

Note: I am not so sure that this is 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;
> }
> ...
>

The reason it is probably not a good design is what comes later.
Another developer comes along and says I see a nice SUM(a, b);
function that I would like to re-use in my new function I am adding.
But that new developer introduces a bug whereby they have implemented
their CHECK(a) wrongly which results in SUM(a, b) now being a security
exploit point because of some very subtle bug in CHECK(a) that no one
noticed initially.
After a while, we might have ten functions that all re-use SUM(a, b)
at which point it becomes too time consuming for someone to check all
ten functions don't have bugs in their CHECK(a) steps.
It is always easier for the safety checks to be done as close as
possible to the potential exploit point (e.g. NULL pointer
dereference) so that it catches all future cases of re-use of the
function.
For example, there exist today zero day exploits in the Linux wireless
code that is due to the absence of these checks being done at the
exploit point.
The biggest problem with all this, is if I sutily (without wishing to
give away that it is to fix a zero day exploit) submitted a patch to
do an extra check in SUM(a, b) that I know prevents a zero day
exploit, my patch would be rejected.

Re: [PATCH v3] ssb: Fix potential NULL pointer dereference in ssb_device_uevent

[Rand Deeb]

On Fri, Mar 8, 2024 at 4:05 AM James Dutton <james.dutton@gmail.com> wrote:
> The reason it is probably not a good design is what comes later.
> Another developer comes along and says I see a nice SUM(a, b);
> function that I would like to re-use in my new function I am adding.
> But that new developer introduces a bug whereby they have implemented
> their CHECK(a) wrongly which results in SUM(a, b) now being a security
> exploit point because of some very subtle bug in CHECK(a) that no one
> noticed initially.
> After a while, we might have ten functions that all re-use SUM(a, b)
> at which point it becomes too time consuming for someone to check all
> ten functions don't have bugs in their CHECK(a) steps.
> It is always easier for the safety checks to be done as close as
> possible to the potential exploit point (e.g. NULL pointer
> dereference) so that it catches all future cases of re-use of the
> function.
> For example, there exist today zero day exploits in the Linux wireless
> code that is due to the absence of these checks being done at the
> exploit point.
> The biggest problem with all this, is if I sutily (without wishing to
> give away that it is to fix a zero day exploit) submitted a patch to
> do an extra check in SUM(a, b) that I know prevents a zero day
> exploit, my patch would be rejected.

Hi James,

Thank you very much for your detailed and interesting interaction. In fact,
while I was writing the example, I expected such a comment :)

The example is just an explanation, do not take it literally, but it is
definitely better than the second method. Now, if the developer makes a
mistake using the function, this is possible, but this is not a convincing
reason to add redundant code everywhere. In addition mistakes could occur
in all scenarios.

I agree with you in general, but I will answer you in simple statements:

For this reason, modification permission should not be granted to anyone,
and for this reason there are reviewers and documentation. In the end,
Somehow, software engineering concepts are applied because this is one of
the most important projects and not a calculator project as homework for
the university.

Best Regards

Re: [PATCH v3] ssb: Fix potential NULL pointer dereference in ssb_device_uevent

[Michael Büsch]

[-- Attachment #1: Type: text/plain, Size: 2421 bytes --]

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

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

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/

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3] ssb: Fix potential NULL pointer dereference in ssb_device_uevent

[Rand Deeb]

On Fri, Mar 8, 2024 at 8:11 AM Michael Büsch <m@bues.ch> wrote:
>
> On Fri,  8 Mar 2024 02:29:27 +0300
> Rand Deeb <rand.sec96@gmail.com> wrote:
>
> > 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.
>
> 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.)

I respect your opinion, but it seems we are stuck in a while(1) loop
without a break. Again, I don't agree with this. Adding a redundant null
check goes against code clarity instead of enhancing it. Just because the
condition checks for null does not justify its presence if it's redundant.
I could insert this check every two lines.
You advocate for this approach based on the potential occurrence in the
future. However, this is one of the reasons why there are reviewers like
yourself are responsible and authorized to approve or reject patches and
verify their integrity before acceptance.

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

Our discussion isn't about what the compiler will do; I know that. The
discussion revolves around the code itself. Alright, let's add a null
check for the 'env' parameter as well. Perhaps we could even automate this
process and add null checks for each function in the file.

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

Yes there is !
The check is literally redundant. Whether we keep it or remove it, the
outcome remains the same. In this case maybe it's not a big deal, but
adopting this approach could lead to an accumulation of redundant
statements throughout the codebase

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

add completely unnecessary confusion?

> I will not ack a patch that reduces code quality.
> Removing NULL checks almost always reduces the quality of the code.
Even at this point there is a misunderstanding. The whole discussion is not
about the ack because you've already given it to the first version.
The discussion is because i'm interested in knowing different points of
views.
I think this discussion took more than its time.
This represents your personal point of view, with which I disagree.
Thank you again, and you can do whatever you want, continue with the first
or third version of the patch.

Re: [PATCH v3] ssb: Fix potential NULL pointer dereference in ssb_device_uevent

[Michael Büsch]

[-- Attachment #1: Type: text/plain, Size: 493 bytes --]

On Fri, 8 Mar 2024 14:36:56 +0300
Rand Deeb <rand.sec96@gmail.com> wrote:
> Adding a redundant null
> check goes against code clarity instead of enhancing it.

You keep on moving goal posts.
The check is already there. Therefore, this is about removal of this NULL check.
Which is not acceptable.

> I respect your opinion, but it seems we are stuck in a while(1) loop
> without a break.

Don't worry. I'll ignore this thread from now on.

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [v3] ssb: Fix potential NULL pointer dereference in ssb_device_uevent()

[Kalle Valo]

Rand Deeb <rand.sec96@gmail.com> wrote:

> The ssb_device_uevent() function first attempts to convert the 'dev' pointer
> to 'struct ssb_device *'. However, it mistakenly dereferences 'dev' before
> performing the NULL check, potentially leading to a NULL pointer
> dereference if 'dev' is NULL.
> 
> To fix this issue, move the NULL check before dereferencing the 'dev' pointer,
> ensuring that the pointer is valid before attempting to use it.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Rand Deeb <rand.sec96@gmail.com>

Patch applied to wireless-next.git, thanks.

789c17185fb0 ssb: Fix potential NULL pointer dereference in ssb_device_uevent()

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20240306123028.164155-1-rand.sec96@gmail.com/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches