| Message ID | 20240315103320.18754-1-r.smirnov@omp.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | KEYS: prevent NULL pointer dereference in find_asymmetric_key() | expand |
On Fri Mar 15, 2024 at 12:33 PM EET, Roman Smirnov wrote: > With the current code, in case all NULLs are passed in id_{0,1,2}, "current code" is not unambigious reference of any part of the kernel tree. Please just write down the function name instead. > the kernel will first print out a WARNING and then have an oops > because id_2 gets dereferenced anyway. Would be more exact": s/print out a WARNING/emit WARN/ > Note that WARN_ON() is also considered harmful by Greg Kroah- > Hartman since it causes the Android kernels to panic as they > get booted with the panic_on_warn option. Despite full respect to Greg, and agreeing what he had said about the topic (which you are lacking lore link meaning that in all cases the current description is incomplete), the only thing that should be documented should be that since WARN_ON() can emit panic when panic_on_warn is set in the *kernel command-line* (not "option") this condition should be relaxed. > > Found by Linux Verification Center (linuxtesting.org) with Svace. I'm not sure if this should be part of the commit message. > > Fixes: 7d30198ee24f ("keys: X.509 public key issuer lookup without AKID") > Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru> Should be reported-by. > Signed-off-by: Roman Smirnov <r.smirnov@omp.ru> > Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> > --- > crypto/asymmetric_keys/asymmetric_type.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c > index a5da8ccd353e..f5cbd6ff14e2 100644 > --- a/crypto/asymmetric_keys/asymmetric_type.c > +++ b/crypto/asymmetric_keys/asymmetric_type.c > @@ -60,17 +60,17 @@ struct key *find_asymmetric_key(struct key *keyring, > char *req, *p; > int len; > > - WARN_ON(!id_0 && !id_1 && !id_2); > - Weird, I recall discussing about this issue in the past. Unfortunately could not find the thread from lore. Anyway I agree with the code change. > if (id_0) { > lookup = id_0->data; > len = id_0->len; > } else if (id_1) { > lookup = id_1->data; > len = id_1->len; > - } else { > + } else if (id_2) { > lookup = id_2->data; > len = id_2->len; > + } else { > + return ERR_PTR(-EINVAL); > } > > /* Construct an identifier "id:<keyid>". */ BR, Jarkko
On Tue, 19 Mar 2024 01:39:00 +0200 Jarkko Sakkinen wrote: > On Fri Mar 15, 2024 at 12:33 PM EET, Roman Smirnov wrote: > > With the current code, in case all NULLs are passed in id_{0,1,2}, > > "current code" is not unambigious reference of any part of the kernel > tree. Please just write down the function name instead. > > > the kernel will first print out a WARNING and then have an oops > > because id_2 gets dereferenced anyway. > > Would be more exact": > > s/print out a WARNING/emit WARN/ Okay, I'll prepare a second version of the patch. > > Note that WARN_ON() is also considered harmful by Greg Kroah- > > Hartman since it causes the Android kernels to panic as they > > get booted with the panic_on_warn option. > > Despite full respect to Greg, and agreeing what he had said about > the topic (which you are lacking lore link meaning that in all > cases the current description is incomplete), the only thing that > should be documented should be that since WARN_ON() can emit > panic when panic_on_warn is set in the *kernel command-line* > (not "option") this condition should be relaxed. Here's a link to the discussion: https://lore.kernel.org/all/2024011213-situated-augmented-64a4@gregkh/ From the context, I thought WARN_ON() would be better removed. > > > > Found by Linux Verification Center (linuxtesting.org) with Svace. > > I'm not sure if this should be part of the commit message. I have already submitted patches with this line, some have been accepted. It is important for the Linux Verification Center to mark patches as closing issues found with Svace. > > > > Fixes: 7d30198ee24f ("keys: X.509 public key issuer lookup without AKID") > > Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru> > > Should be reported-by. The suggested-by tag belongs to Sergey because he suggested the fix, subject/description of the patch. The tag reported-by belongs to Svace tool. Thank you for the reply.
On Tue Mar 19, 2024 at 4:44 PM EET, Roman Smirnov wrote: > On Tue, 19 Mar 2024 01:39:00 +0200 Jarkko Sakkinen wrote: > > On Fri Mar 15, 2024 at 12:33 PM EET, Roman Smirnov wrote: > > > With the current code, in case all NULLs are passed in id_{0,1,2}, > > > > "current code" is not unambigious reference of any part of the kernel > > tree. Please just write down the function name instead. > > > > > the kernel will first print out a WARNING and then have an oops > > > because id_2 gets dereferenced anyway. > > > > Would be more exact": > > > > s/print out a WARNING/emit WARN/ > > Okay, I'll prepare a second version of the patch. > > > > Note that WARN_ON() is also considered harmful by Greg Kroah- > > > Hartman since it causes the Android kernels to panic as they > > > get booted with the panic_on_warn option. > > > > Despite full respect to Greg, and agreeing what he had said about > > the topic (which you are lacking lore link meaning that in all > > cases the current description is incomplete), the only thing that > > should be documented should be that since WARN_ON() can emit > > panic when panic_on_warn is set in the *kernel command-line* > > (not "option") this condition should be relaxed. > > Here's a link to the discussion: > https://lore.kernel.org/all/2024011213-situated-augmented-64a4@gregkh/ > From the context, I thought WARN_ON() would be better removed. Not sure what you are trying to claim here that goes against what I just said. > > > > > > > Found by Linux Verification Center (linuxtesting.org) with Svace. > > > > I'm not sure if this should be part of the commit message. > > I have already submitted patches with this line, some have been > accepted. It is important for the Linux Verification Center to mark > patches as closing issues found with Svace. > > > > > > > Fixes: 7d30198ee24f ("keys: X.509 public key issuer lookup without AKID") > > > Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru> > > > > Should be reported-by. > > The suggested-by tag belongs to Sergey because he suggested the fix, > subject/description of the patch. The tag reported-by belongs to > Svace tool. 1. I did not see any reported-by tags in this which is requirement. 2. Who did find the issue using that tool? I don't put reported-by to GDB even if I use that find the bug. > > Thank you for the reply. BR, Jarkko
On Tue, 19 Mar 2024 22:14:22 +0200 Jarkko Sakkinen wrote: > On Tue Mar 19, 2024 at 4:44 PM EET, Roman Smirnov wrote: > > On Tue, 19 Mar 2024 01:39:00 +0200 Jarkko Sakkinen wrote: > > > On Fri Mar 15, 2024 at 12:33 PM EET, Roman Smirnov wrote: [...] > > > > > > > > Found by Linux Verification Center (linuxtesting.org) with Svace. > > > > > > I'm not sure if this should be part of the commit message. > > > > I have already submitted patches with this line, some have been > > accepted. It is important for the Linux Verification Center to mark > > patches as closing issues found with Svace. > > > > > > > > > > Fixes: 7d30198ee24f ("keys: X.509 public key issuer lookup without AKID") > > > > Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru> > > > > > > Should be reported-by. > > > > The suggested-by tag belongs to Sergey because he suggested the fix, > > subject/description of the patch. The tag reported-by belongs to > > Svace tool. > > 1. I did not see any reported-by tags in this which is requirement. > 2. Who did find the issue using that tool? I don't put reported-by to > GDB even if I use that find the bug. Svace is an automated bug finding tool. This error was found during source code analysis by the program, so the tag reported-by does not belong to any person. I don't know what to do in such a situation, but write something like: Reported-by: Svace would be weird. I think that the line "Found by Linux ... with Svace" could be a substitute for the tag.
On Wed Mar 20, 2024 at 10:21 AM EET, Roman Smirnov wrote: > On Tue, 19 Mar 2024 22:14:22 +0200 Jarkko Sakkinen wrote: > > On Tue Mar 19, 2024 at 4:44 PM EET, Roman Smirnov wrote: > > > On Tue, 19 Mar 2024 01:39:00 +0200 Jarkko Sakkinen wrote: > > > > On Fri Mar 15, 2024 at 12:33 PM EET, Roman Smirnov wrote: > [...] > > > > > > > > > > Found by Linux Verification Center (linuxtesting.org) with Svace. > > > > > > > > I'm not sure if this should be part of the commit message. > > > > > > I have already submitted patches with this line, some have been > > > accepted. It is important for the Linux Verification Center to mark > > > patches as closing issues found with Svace. > > > > > > > > > > > > > Fixes: 7d30198ee24f ("keys: X.509 public key issuer lookup without AKID") > > > > > Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru> > > > > > > > > Should be reported-by. > > > > > > The suggested-by tag belongs to Sergey because he suggested the fix, > > > subject/description of the patch. The tag reported-by belongs to > > > Svace tool. > > > > 1. I did not see any reported-by tags in this which is requirement. > > 2. Who did find the issue using that tool? I don't put reported-by to > > GDB even if I use that find the bug. > > Svace is an automated bug finding tool. This error was found during > source code analysis by the program, so the tag reported-by does not > belong to any person. I don't know what to do in such a situation, > but write something like: > > Reported-by: Svace > > would be weird. I think that the line "Found by Linux ... with Svace" > could be a substitute for the tag. I'd prefer a person here that used the tool as it is not korg hosted automated tool. BR, Jarkko
Hello! Sorry for (so long!) delay -- we're trying to finalize the status of our yet unmerged patches... On 3/21/24 7:12 PM, Jarkko Sakkinen wrote: [...] >>>>>> Found by Linux Verification Center (linuxtesting.org) with Svace. >>>>> >>>>> I'm not sure if this should be part of the commit message. >>>> >>>> I have already submitted patches with this line, some have been >>>> accepted. It is important for the Linux Verification Center to mark >>>> patches as closing issues found with Svace. >>>> >>>>>> >>>>>> Fixes: 7d30198ee24f ("keys: X.509 public key issuer lookup without AKID") >>>>>> Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru> >>>>> >>>>> Should be reported-by. >>>> >>>> The suggested-by tag belongs to Sergey because he suggested the fix, >>>> subject/description of the patch. The tag reported-by belongs to >>>> Svace tool. >>> >>> 1. I did not see any reported-by tags in this which is requirement. >>> 2. Who did find the issue using that tool? I don't put reported-by to >>> GDB even if I use that find the bug. >> >> Svace is an automated bug finding tool. This error was found during >> source code analysis by the program, so the tag reported-by does not >> belong to any person. I don't know what to do in such a situation, >> but write something like: >> >> Reported-by: Svace >> >> would be weird. I think that the line "Found by Linux ... with Svace" >> could be a substitute for the tag. > > I'd prefer a person here that used the tool as it is not korg hosted > automated tool. It's a long ago established practice with the Linux Verification Center (http://linuxtesting.org): you can find 700+ merged patches with a similar line (mentioning the LVC's website) and without the Reported-by tag: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=grep&q=linuxtesting.org > BR, Jarkko MBR, Sergey
On 3/19/24 2:39 AM, Jarkko Sakkinen wrote: [...] >> With the current code, in case all NULLs are passed in id_{0,1,2}, > > "current code" is not unambigious reference of any part of the kernel > tree. Please just write down the function name instead. > >> the kernel will first print out a WARNING and then have an oops >> because id_2 gets dereferenced anyway. > > Would be more exact": > > s/print out a WARNING/emit WARN/ Well, technically calling WARN_ON() it prints out WARNING: ... -- hence the wording... :-) >> Note that WARN_ON() is also considered harmful by Greg Kroah- >> Hartman since it causes the Android kernels to panic as they >> get booted with the panic_on_warn option. As it turns out, not all Android kernels really do this thging (at least the Samsung's ones do, according to Greg)... > Despite full respect to Greg, and agreeing what he had said about > the topic (which you are lacking lore link meaning that in all > cases the current description is incomplete), the only thing that > should be documented should be that since WARN_ON() can emit > panic when panic_on_warn is set in the *kernel command-line* > (not "option") this condition should be relaxed. Linus' opinion seems to be that the people using panic_on_warn get what they deserve -- see: https://lore.kernel.org/all/CAHk-=wgF7K2gSSpy=m_=K3Nov4zaceUX9puQf1TjkTJLA2XC_g@mail.gmail.com/ >> Found by Linux Verification Center (linuxtesting.org) with Svace. > > I'm not sure if this should be part of the commit message. > >> >> Fixes: 7d30198ee24f ("keys: X.509 public key issuer lookup without AKID") >> Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru> > > Should be reported-by. > >> Signed-off-by: Roman Smirnov <r.smirnov@omp.ru> >> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> >> --- >> crypto/asymmetric_keys/asymmetric_type.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c >> index a5da8ccd353e..f5cbd6ff14e2 100644 >> --- a/crypto/asymmetric_keys/asymmetric_type.c >> +++ b/crypto/asymmetric_keys/asymmetric_type.c >> @@ -60,17 +60,17 @@ struct key *find_asymmetric_key(struct key *keyring, >> char *req, *p; >> int len; >> >> - WARN_ON(!id_0 && !id_1 && !id_2); >> - > > Weird, I recall discussing about this issue in the past. Unfortunately > could not find the thread from lore. There was also that (denied) patch: https://lore.kernel.org/all/20240414170850.148122-1-elder@linaro.org/ > Anyway I agree with the code change. > >> if (id_0) { >> lookup = id_0->data; >> len = id_0->len; >> } else if (id_1) { >> lookup = id_1->data; >> len = id_1->len; >> - } else { >> + } else if (id_2) { >> lookup = id_2->data; >> len = id_2->len; >> + } else { We can perhaps place the WARN_ON(1) call here instead of where it is now... >> + return ERR_PTR(-EINVAL); >> } >> >> /* Construct an identifier "id:<keyid>". */ > BR, Jarkko MBR, Sergey
On Mon Sep 9, 2024 at 9:25 PM EEST, Sergey Shtylyov wrote: > Hello! > > Sorry for (so long!) delay -- we're trying to finalize the status > of our yet unmerged patches... > > On 3/21/24 7:12 PM, Jarkko Sakkinen wrote: > [...] > > >>>>>> Found by Linux Verification Center (linuxtesting.org) with Svace. > >>>>> > >>>>> I'm not sure if this should be part of the commit message. > >>>> > >>>> I have already submitted patches with this line, some have been > >>>> accepted. It is important for the Linux Verification Center to mark > >>>> patches as closing issues found with Svace. > >>>> > >>>>>> > >>>>>> Fixes: 7d30198ee24f ("keys: X.509 public key issuer lookup without AKID") > >>>>>> Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru> > >>>>> > >>>>> Should be reported-by. > >>>> > >>>> The suggested-by tag belongs to Sergey because he suggested the fix, > >>>> subject/description of the patch. The tag reported-by belongs to > >>>> Svace tool. > >>> > >>> 1. I did not see any reported-by tags in this which is requirement. > >>> 2. Who did find the issue using that tool? I don't put reported-by to > >>> GDB even if I use that find the bug. > >> > >> Svace is an automated bug finding tool. This error was found during > >> source code analysis by the program, so the tag reported-by does not q >> belong to any person. I don't know what to do in such a situation, > >> but write something like: > >> > >> Reported-by: Svace > >> > >> would be weird. I think that the line "Found by Linux ... with Svace" > >> could be a substitute for the tag. > > > > I'd prefer a person here that used the tool as it is not korg hosted > > automated tool. > > It's a long ago established practice with the Linux Verification Center (http://linuxtesting.org): you can find 700+ merged patches with a similar > line (mentioning the LVC's website) and without the Reported-by tag: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=grep&q=linuxtesting.org I see examples alike of "Found by Linux Verification Center (linuxtesting.org) with Syzkaller." It neither has an email address, meaning that reported-by tag even has incorrect format. NAK, because "Svace" means nothing to me as it is in the tag. BR, Jarkko
diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c index a5da8ccd353e..f5cbd6ff14e2 100644 --- a/crypto/asymmetric_keys/asymmetric_type.c +++ b/crypto/asymmetric_keys/asymmetric_type.c @@ -60,17 +60,17 @@ struct key *find_asymmetric_key(struct key *keyring, char *req, *p; int len; - WARN_ON(!id_0 && !id_1 && !id_2); - if (id_0) { lookup = id_0->data; len = id_0->len; } else if (id_1) { lookup = id_1->data; len = id_1->len; - } else { + } else if (id_2) { lookup = id_2->data; len = id_2->len; + } else { + return ERR_PTR(-EINVAL); } /* Construct an identifier "id:<keyid>". */
With the current code, in case all NULLs are passed in id_{0,1,2}, the kernel will first print out a WARNING and then have an oops because id_2 gets dereferenced anyway. Note that WARN_ON() is also considered harmful by Greg Kroah- Hartman since it causes the Android kernels to panic as they get booted with the panic_on_warn option. Found by Linux Verification Center (linuxtesting.org) with Svace. Fixes: 7d30198ee24f ("keys: X.509 public key issuer lookup without AKID") Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru> Signed-off-by: Roman Smirnov <r.smirnov@omp.ru> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> --- crypto/asymmetric_keys/asymmetric_type.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)