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(-)
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
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 <[email protected]>
> ---
> 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,
Hi
On Wed, 6 Mar 2024 at 13:32, Rand Deeb <[email protected]> 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 <[email protected]>
> ---
> 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
On Wed, Mar 6, 2024 at 10:51 PM Jonas Gorski <[email protected]> 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.
On Thu, 7 Mar 2024 16:41:42 +0300
Rand Deeb <[email protected]> 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/
On Thu, Mar 7, 2024 at 9:24 PM Michael Büsch <[email protected]> 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
On Fri, 8 Mar 2024 00:19:28 +0300
Rand Deeb <[email protected]> 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/
On Thu, 7 Mar 2024 at 21:39, Michael Büsch <[email protected]> wrote:
>
> 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.
>
> > 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.
On Fri, Mar 8, 2024 at 12:39 AM Michael Büsch <[email protected]> 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
On Fri, 8 Mar 2024 02:29:27 +0300
Rand Deeb <[email protected]> wrote:
> On Fri, Mar 8, 2024 at 12:39 AM Michael Büsch <[email protected]> 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/
On Thu, 7 Mar 2024 22:02:29 +0000
James Dutton <[email protected]> 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/
On Fri, Mar 8, 2024 at 8:11 AM Michael Büsch <[email protected]> wrote:
>
> On Fri, 8 Mar 2024 02:29:27 +0300
> Rand Deeb <[email protected]> wrote:
>
> > On Fri, Mar 8, 2024 at 12:39 AM Michael Büsch <[email protected]> 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.
On Fri, Mar 8, 2024 at 4:05 AM James Dutton <[email protected]> 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
Rand Deeb <[email protected]> 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 <[email protected]>
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/[email protected]/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches