ssb: Fix potential NULL pointer dereference in ssb_device_uevent

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 <[email protected]>

---
 drivers/ssb/main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/ssb/main.c +3 -1

index b9934b9c2d70..070a99a4180c 100644
@@ -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,

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 <[email protected]>

---
 drivers/ssb/main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/ssb/main.c +0 -0

index b9934b9c2d70..070a99a4180c 100644
@@ -341,11 +341,13 @@ static int ssb_bus_match(struct device *dev, struct device_driver *drv)
 

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

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)

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.

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) ?

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,

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.

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

Quoted 1 line · On Fri, 8 Mar 2024 00:19:28 +0300 Rand Deeb <[email protected]> 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.

Quoted 3 lines

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.

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

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

Quoted 2 lines

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.

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

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.

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.

Quoted 2 lines · Michael Büsch

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.

Quoted 4 lines

Quoted 2 lines

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;
}

...

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.

Quoted 2 lines · Michael Büsch

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.

Quoted 2 lines

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.

Quoted 3 lines

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.

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.

Quoted 11 lines · On Fri, 8 Mar 2024 02:29:27 +0300 Rand Deeb <[email protected]> wrote:

Quoted 2 lines · Michael Büsch

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

Quoted 6 lines

Quoted 2 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 3 lines

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.

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

Adding a redundant null
check goes against code clarity instead of enhancing it.

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

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 <[email protected]>