| Message ID | 20240306123028.164155-1-rand.sec96@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 789c17185fb0f39560496c2beab9b57ce1d0cbe7 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v3] ssb: Fix potential NULL pointer dereference in ssb_device_uevent | expand |
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,
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
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.
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.
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
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.
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.
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
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.
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.
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.
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.
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
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.
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()
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,
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(-)