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

[Sergey Shtylyov <s.shtylyov@omp.ru>]

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