[v2] KEYS: prevent NULL pointer dereference in find_asymmetric_key()

[PATCH v2] KEYS: prevent NULL pointer dereference in find_asymmetric_key()

Posted by Roman Smirnov 1 year, 5 months ago

In find_asymmetric_key(), if all NULLs are passed in id_{0,1,2} parameters
the kernel will first emit WARN and then have an oops because id_2 gets
dereferenced anyway.

Found by Linux Verification Center (linuxtesting.org) with Svace static
analysis tool.

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 | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c
index a5da8ccd353e..43af5fa510c0 100644
--- a/crypto/asymmetric_keys/asymmetric_type.c
+++ b/crypto/asymmetric_keys/asymmetric_type.c
@@ -60,17 +60,18 @@ 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 {
+		WARN_ON(1);
+		return ERR_PTR(-EINVAL);
 	}
 
 	/* Construct an identifier "id:<keyid>". */
-- 
2.34.1

Re: [PATCH v2] KEYS: prevent NULL pointer dereference in find_asymmetric_key()

Posted by Jarkko Sakkinen 1 year, 5 months ago

On Tue Sep 10, 2024 at 2:18 PM EEST, Roman Smirnov wrote:
> In find_asymmetric_key(), if all NULLs are passed in id_{0,1,2} parameters
> the kernel will first emit WARN and then have an oops because id_2 gets
> dereferenced anyway.
>
> Found by Linux Verification Center (linuxtesting.org) with Svace static
> analysis tool.

Weird, I recall that I've either sent a patch to address the same site
OR have commented a patch with similar reasoning. Well, it does not
matter, I think it this makes sense to me.

You could further add to the motivation that given the panic_on_warn
kernel command-line parameter, it is for the best limit the scope and
use of the WARN-macro.

>
> Fixes: 7d30198ee24f ("keys: X.509 public key issuer lookup without AKID")

I would still call this an improvement. It overuses warn but I don't
think this a bug. 

> 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 | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c
> index a5da8ccd353e..43af5fa510c0 100644
> --- a/crypto/asymmetric_keys/asymmetric_type.c
> +++ b/crypto/asymmetric_keys/asymmetric_type.c
> @@ -60,17 +60,18 @@ 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 {
> +		WARN_ON(1);

This is totally fine. It is an improvement to the current situation.

> +		return ERR_PTR(-EINVAL);
>  	}
>  
>  	/* Construct an identifier "id:<keyid>". */

Can be applied as an improvement and with the added bits about
panic_on_warn to the commit message.

BR, Jarkko

Re: [PATCH v2] KEYS: prevent NULL pointer dereference in find_asymmetric_key()

Posted by Sergey Shtylyov 1 year, 5 months ago

On 9/10/24 4:38 PM, Jarkko Sakkinen wrote:
[...]

>> In find_asymmetric_key(), if all NULLs are passed in id_{0,1,2} parameters
>> the kernel will first emit WARN and then have an oops because id_2 gets
>> dereferenced anyway.
>>
>> Found by Linux Verification Center (linuxtesting.org) with Svace static
>> analysis tool.
> 
> Weird, I recall that I've either sent a patch to address the same site
> OR have commented a patch with similar reasoning. Well, it does not
> matter, I think it this makes sense to me.
> 
> You could further add to the motivation that given the panic_on_warn
> kernel command-line parameter, it is for the best limit the scope and
> use of the WARN-macro.

   I don't understand what you mean -- this version of the patch keeps
the WARN_ON() call, it just moves that call, so that the duplicate id_{0,1,2}
checks are avoided...

>> Fixes: 7d30198ee24f ("keys: X.509 public key issuer lookup without AKID")
> 
> I would still call this an improvement. It overuses warn but I don't
> think this a bug. 

   I think warning about passing all NULL ptrs but then causing a NULL ptr
deref anyway wasn't really intended -- seems like a bug to me...
 
>> 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>
>> ---

   I forgot to tell Roman to place the changelog here when doing an internal
review. Anyway, here is some from me:

Changed in v2:
- kept the WARN_ON() call, just moved it to avoid extra prr checks, updated
  the patch description accordingly;
- reworded the patch description according to feedback.

[...]

>> diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c
>> index a5da8ccd353e..43af5fa510c0 100644
>> --- a/crypto/asymmetric_keys/asymmetric_type.c
>> +++ b/crypto/asymmetric_keys/asymmetric_type.c
>> @@ -60,17 +60,18 @@ 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 {
>> +		WARN_ON(1);
> 
> This is totally fine. It is an improvement to the current situation.

   That update also fixes a kernel oops...

>> +		return ERR_PTR(-EINVAL);
>>  	}
>>  
>>  	/* Construct an identifier "id:<keyid>". */
> 
> Can be applied as an improvement and with the added bits about
> panic_on_warn to the commit message.

   We no longer care about panic_on_warn...

> BR, Jarkko

MBR, Sergey

Re: [PATCH v2] KEYS: prevent NULL pointer dereference in find_asymmetric_key()

Posted by Jarkko Sakkinen 1 year, 4 months ago

On Tue Sep 10, 2024 at 8:38 PM EEST, Sergey Shtylyov wrote:
> On 9/10/24 4:38 PM, Jarkko Sakkinen wrote:
> [...]
>
> >> In find_asymmetric_key(), if all NULLs are passed in id_{0,1,2} parameters
> >> the kernel will first emit WARN and then have an oops because id_2 gets
> >> dereferenced anyway.
> >>
> >> Found by Linux Verification Center (linuxtesting.org) with Svace static
> >> analysis tool.
> > 
> > Weird, I recall that I've either sent a patch to address the same site
> > OR have commented a patch with similar reasoning. Well, it does not
> > matter, I think it this makes sense to me.
> > 
> > You could further add to the motivation that given the panic_on_warn
> > kernel command-line parameter, it is for the best limit the scope and
> > use of the WARN-macro.
>
>    I don't understand what you mean -- this version of the patch keeps
> the WARN_ON() call, it just moves that call, so that the duplicate id_{0,1,2}
> checks are avoided...

I overlooked the code change (my bad sorry). Here's a better version of
the first paragraph:

"find_asymmetric_keys() has nullity checks of id_0 and id_1 but ignores
validation for id_2. Check nullity also for id_2."

Yep, and it changes no situation with WARN_ON() macro for better or
worse. It would logically separate issue to discuss and address so
as far as I'm concerned, with this clarification I think the change
makes sense to me.

BR, Jarkko

Re: [PATCH v2] KEYS: prevent NULL pointer dereference in find_asymmetric_key()

Posted by Sergey Shtylyov 1 year, 4 months ago

On 9/11/24 4:18 PM, Jarkko Sakkinen wrote:
[...]

>>>> In find_asymmetric_key(), if all NULLs are passed in id_{0,1,2} parameters
>>>> the kernel will first emit WARN and then have an oops because id_2 gets
>>>> dereferenced anyway.
>>>>
>>>> Found by Linux Verification Center (linuxtesting.org) with Svace static
>>>> analysis tool.
>>>
>>> Weird, I recall that I've either sent a patch to address the same site
>>> OR have commented a patch with similar reasoning. Well, it does not
>>> matter, I think it this makes sense to me.
>>>
>>> You could further add to the motivation that given the panic_on_warn
>>> kernel command-line parameter, it is for the best limit the scope and
>>> use of the WARN-macro.
>>
>>    I don't understand what you mean -- this version of the patch keeps
>> the WARN_ON() call, it just moves that call, so that the duplicate id_{0,1,2}
>> checks are avoided...
> 
> I overlooked the code change (my bad sorry). Here's a better version of
> the first paragraph:
> 
> "find_asymmetric_keys() has nullity checks of id_0 and id_1 but ignores
> validation for id_2. Check nullity also for id_2."

   Hm, what about WARN_ON(!id_0 && !id_1 && !id_2) -- it used to check all
the pointers, right? I think our variant was closer to reality... :-)

[...]

> BR, Jarkko

MBR, Sergey

Re: [PATCH v2] KEYS: prevent NULL pointer dereference in find_asymmetric_key()

Posted by Jarkko Sakkinen 1 year, 4 months ago

On Thu Sep 12, 2024 at 4:51 PM EEST, Sergey Shtylyov wrote:
> On 9/11/24 4:18 PM, Jarkko Sakkinen wrote:
> [...]
>
> >>>> In find_asymmetric_key(), if all NULLs are passed in id_{0,1,2} parameters
> >>>> the kernel will first emit WARN and then have an oops because id_2 gets
> >>>> dereferenced anyway.
> >>>>
> >>>> Found by Linux Verification Center (linuxtesting.org) with Svace static
> >>>> analysis tool.
> >>>
> >>> Weird, I recall that I've either sent a patch to address the same site
> >>> OR have commented a patch with similar reasoning. Well, it does not
> >>> matter, I think it this makes sense to me.
> >>>
> >>> You could further add to the motivation that given the panic_on_warn
> >>> kernel command-line parameter, it is for the best limit the scope and
> >>> use of the WARN-macro.
> >>
> >>    I don't understand what you mean -- this version of the patch keeps
> >> the WARN_ON() call, it just moves that call, so that the duplicate id_{0,1,2}
> >> checks are avoided...
> > 
> > I overlooked the code change (my bad sorry). Here's a better version of
> > the first paragraph:
> > 
> > "find_asymmetric_keys() has nullity checks of id_0 and id_1 but ignores
> > validation for id_2. Check nullity also for id_2."
>
>    Hm, what about WARN_ON(!id_0 && !id_1 && !id_2) -- it used to check all
> the pointers, right? I think our variant was closer to reality... :-)

Right (lazy validation, first null ignores rest)

BR, Jarkko

Re: [PATCH v2] KEYS: prevent NULL pointer dereference in find_asymmetric_key()

Posted by Sergey Shtylyov 1 year, 4 months ago

On 9/12/24 5:27 PM, Jarkko Sakkinen wrote:
[...]

>>>>>> In find_asymmetric_key(), if all NULLs are passed in id_{0,1,2} parameters
>>>>>> the kernel will first emit WARN and then have an oops because id_2 gets
>>>>>> dereferenced anyway.
>>>>>>
>>>>>> Found by Linux Verification Center (linuxtesting.org) with Svace static
>>>>>> analysis tool.
>>>>>
>>>>> Weird, I recall that I've either sent a patch to address the same site
>>>>> OR have commented a patch with similar reasoning. Well, it does not
>>>>> matter, I think it this makes sense to me.
>>>>>
>>>>> You could further add to the motivation that given the panic_on_warn
>>>>> kernel command-line parameter, it is for the best limit the scope and
>>>>> use of the WARN-macro.
>>>>
>>>>    I don't understand what you mean -- this version of the patch keeps
>>>> the WARN_ON() call, it just moves that call, so that the duplicate id_{0,1,2}
>>>> checks are avoided...
>>>
>>> I overlooked the code change (my bad sorry). Here's a better version of
>>> the first paragraph:
>>>
>>> "find_asymmetric_keys() has nullity checks of id_0 and id_1 but ignores
>>> validation for id_2. Check nullity also for id_2."
>>
>>    Hm, what about WARN_ON(!id_0 && !id_1 && !id_2) -- it used to check all
>> the pointers, right? I think our variant was closer to reality... :-)
> 
> Right (lazy validation, first null ignores rest)

   No, contrariwise: since we use && and !, first non-NULL would ignore the rest.

> BR, Jarkko

MBR, Sergey

Re: [PATCH v2] KEYS: prevent NULL pointer dereference in find_asymmetric_key()

Posted by Jarkko Sakkinen 1 year, 4 months ago

On Thu Sep 12, 2024 at 8:36 PM EEST, Sergey Shtylyov wrote:
> On 9/12/24 5:27 PM, Jarkko Sakkinen wrote:
> [...]
>
> >>>>>> In find_asymmetric_key(), if all NULLs are passed in id_{0,1,2} parameters
> >>>>>> the kernel will first emit WARN and then have an oops because id_2 gets
> >>>>>> dereferenced anyway.
> >>>>>>
> >>>>>> Found by Linux Verification Center (linuxtesting.org) with Svace static
> >>>>>> analysis tool.
> >>>>>
> >>>>> Weird, I recall that I've either sent a patch to address the same site
> >>>>> OR have commented a patch with similar reasoning. Well, it does not
> >>>>> matter, I think it this makes sense to me.
> >>>>>
> >>>>> You could further add to the motivation that given the panic_on_warn
> >>>>> kernel command-line parameter, it is for the best limit the scope and
> >>>>> use of the WARN-macro.
> >>>>
> >>>>    I don't understand what you mean -- this version of the patch keeps
> >>>> the WARN_ON() call, it just moves that call, so that the duplicate id_{0,1,2}
> >>>> checks are avoided...
> >>>
> >>> I overlooked the code change (my bad sorry). Here's a better version of
> >>> the first paragraph:
> >>>
> >>> "find_asymmetric_keys() has nullity checks of id_0 and id_1 but ignores
> >>> validation for id_2. Check nullity also for id_2."
> >>
> >>    Hm, what about WARN_ON(!id_0 && !id_1 && !id_2) -- it used to check all
> >> the pointers, right? I think our variant was closer to reality... :-)
> > 
> > Right (lazy validation, first null ignores rest)
>
>    No, contrariwise: since we use && and !, first non-NULL would ignore the rest.

Oops correct :-/

BR, Jarkko

Re: [PATCH v2] KEYS: prevent NULL pointer dereference in find_asymmetric_key()

Posted by Jarkko Sakkinen 1 year, 4 months ago

On Wed Sep 11, 2024 at 4:18 PM EEST, Jarkko Sakkinen wrote:
> On Tue Sep 10, 2024 at 8:38 PM EEST, Sergey Shtylyov wrote:
> > On 9/10/24 4:38 PM, Jarkko Sakkinen wrote:
> > [...]
> >
> > >> In find_asymmetric_key(), if all NULLs are passed in id_{0,1,2} parameters
> > >> the kernel will first emit WARN and then have an oops because id_2 gets
> > >> dereferenced anyway.
> > >>
> > >> Found by Linux Verification Center (linuxtesting.org) with Svace static
> > >> analysis tool.
> > > 
> > > Weird, I recall that I've either sent a patch to address the same site
> > > OR have commented a patch with similar reasoning. Well, it does not
> > > matter, I think it this makes sense to me.
> > > 
> > > You could further add to the motivation that given the panic_on_warn
> > > kernel command-line parameter, it is for the best limit the scope and
> > > use of the WARN-macro.
> >
> >    I don't understand what you mean -- this version of the patch keeps
> > the WARN_ON() call, it just moves that call, so that the duplicate id_{0,1,2}
> > checks are avoided...
>
> I overlooked the code change (my bad sorry). Here's a better version of
> the first paragraph:
>
> "find_asymmetric_keys() has nullity checks of id_0 and id_1 but ignores
> validation for id_2. Check nullity also for id_2."
>
> Yep, and it changes no situation with WARN_ON() macro for better or
> worse. It would logically separate issue to discuss and address so
> as far as I'm concerned, with this clarification I think the change
> makes sense to me.

Actually explicitly stating that call paths leading to WARN_ON()
invocation are intact by the commit (as a reminder for future).

BR, Jarkko

Re: [PATCH v2] KEYS: prevent NULL pointer dereference in find_asymmetric_key()

Posted by Sergey Shtylyov 1 year, 4 months ago

On 9/11/24 4:19 PM, Jarkko Sakkinen wrote:

[...]

>>>>> In find_asymmetric_key(), if all NULLs are passed in id_{0,1,2} parameters
>>>>> the kernel will first emit WARN and then have an oops because id_2 gets
>>>>> dereferenced anyway.
>>>>>
>>>>> Found by Linux Verification Center (linuxtesting.org) with Svace static
>>>>> analysis tool.
>>>>
>>>> Weird, I recall that I've either sent a patch to address the same site
>>>> OR have commented a patch with similar reasoning. Well, it does not
>>>> matter, I think it this makes sense to me.
>>>>
>>>> You could further add to the motivation that given the panic_on_warn
>>>> kernel command-line parameter, it is for the best limit the scope and
>>>> use of the WARN-macro.
>>>
>>>    I don't understand what you mean -- this version of the patch keeps
>>> the WARN_ON() call, it just moves that call, so that the duplicate id_{0,1,2}
>>> checks are avoided...
>>
>> I overlooked the code change (my bad sorry). Here's a better version of
>> the first paragraph:
>>
>> "find_asymmetric_keys() has nullity checks of id_0 and id_1 but ignores
>> validation for id_2. Check nullity also for id_2."
>>
>> Yep, and it changes no situation with WARN_ON() macro for better or
>> worse. It would logically separate issue to discuss and address so
>> as far as I'm concerned, with this clarification I think the change
>> makes sense to me.
> 
> Actually explicitly stating that call paths leading to WARN_ON()
> invocation are intact by the commit (as a reminder for future).

   OK...
   Do you still think the Fixes tag should be dropped (and thus the
Reported-by tag would become unnecessary?)?

> BR, Jarkko

MBR, Sergey

Re: [PATCH v2] KEYS: prevent NULL pointer dereference in find_asymmetric_key()

Posted by Jarkko Sakkinen 1 year, 4 months ago

On Wed Sep 11, 2024 at 5:45 PM EEST, Sergey Shtylyov wrote:
> On 9/11/24 4:19 PM, Jarkko Sakkinen wrote:
>
> [...]
>
> >>>>> In find_asymmetric_key(), if all NULLs are passed in id_{0,1,2} parameters
> >>>>> the kernel will first emit WARN and then have an oops because id_2 gets
> >>>>> dereferenced anyway.
> >>>>>
> >>>>> Found by Linux Verification Center (linuxtesting.org) with Svace static
> >>>>> analysis tool.
> >>>>
> >>>> Weird, I recall that I've either sent a patch to address the same site
> >>>> OR have commented a patch with similar reasoning. Well, it does not
> >>>> matter, I think it this makes sense to me.
> >>>>
> >>>> You could further add to the motivation that given the panic_on_warn
> >>>> kernel command-line parameter, it is for the best limit the scope and
> >>>> use of the WARN-macro.
> >>>
> >>>    I don't understand what you mean -- this version of the patch keeps
> >>> the WARN_ON() call, it just moves that call, so that the duplicate id_{0,1,2}
> >>> checks are avoided...
> >>
> >> I overlooked the code change (my bad sorry). Here's a better version of
> >> the first paragraph:
> >>
> >> "find_asymmetric_keys() has nullity checks of id_0 and id_1 but ignores
> >> validation for id_2. Check nullity also for id_2."
> >>
> >> Yep, and it changes no situation with WARN_ON() macro for better or
> >> worse. It would logically separate issue to discuss and address so
> >> as far as I'm concerned, with this clarification I think the change
> >> makes sense to me.
> > 
> > Actually explicitly stating that call paths leading to WARN_ON()
> > invocation are intact by the commit (as a reminder for future).
>
>    OK...
>    Do you still think the Fixes tag should be dropped (and thus the
> Reported-by tag would become unnecessary?)?

I think we can keep them.

BR, Jarkko

Re: [PATCH v2] KEYS: prevent NULL pointer dereference in find_asymmetric_key()

Posted by Jarkko Sakkinen 1 year, 4 months ago

On Wed Sep 11, 2024 at 5:45 PM EEST, Sergey Shtylyov wrote:
> On 9/11/24 4:19 PM, Jarkko Sakkinen wrote:
>
> [...]
>
> >>>>> In find_asymmetric_key(), if all NULLs are passed in id_{0,1,2} parameters
> >>>>> the kernel will first emit WARN and then have an oops because id_2 gets
> >>>>> dereferenced anyway.
> >>>>>
> >>>>> Found by Linux Verification Center (linuxtesting.org) with Svace static
> >>>>> analysis tool.
> >>>>
> >>>> Weird, I recall that I've either sent a patch to address the same site
> >>>> OR have commented a patch with similar reasoning. Well, it does not
> >>>> matter, I think it this makes sense to me.
> >>>>
> >>>> You could further add to the motivation that given the panic_on_warn
> >>>> kernel command-line parameter, it is for the best limit the scope and
> >>>> use of the WARN-macro.
> >>>
> >>>    I don't understand what you mean -- this version of the patch keeps
> >>> the WARN_ON() call, it just moves that call, so that the duplicate id_{0,1,2}
> >>> checks are avoided...
> >>
> >> I overlooked the code change (my bad sorry). Here's a better version of
> >> the first paragraph:
> >>
> >> "find_asymmetric_keys() has nullity checks of id_0 and id_1 but ignores
> >> validation for id_2. Check nullity also for id_2."
> >>
> >> Yep, and it changes no situation with WARN_ON() macro for better or
> >> worse. It would logically separate issue to discuss and address so
> >> as far as I'm concerned, with this clarification I think the change
> >> makes sense to me.
> > 
> > Actually explicitly stating that call paths leading to WARN_ON()
> > invocation are intact by the commit (as a reminder for future).
>
>    OK...
>    Do you still think the Fixes tag should be dropped (and thus the
> Reported-by tag would become unnecessary?)?

Good question but I think we should keep them (spent 15 minutes thinking
about this).

It's a glitch at least and would not do harm for stable series to have
it like that :-) So if you polish the message, send a new version I'll
pick it, and put to my next PR.

Thanks for the patience with this.

>
> > BR, Jarkko
>
> MBR, Sergey

BR, Jarkko