linux-kernel - Re: [PATCH] KEYS: prevent NULL pointer dereference in find_asymmetric

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 19 Mar 2024 01:39:00 +0200
From: "Jarkko Sakkinen" <jarkko@...nel.org>
To: "Roman Smirnov" <r.smirnov@....ru>, "David Howells"
 <dhowells@...hat.com>, "Herbert Xu" <herbert@...dor.apana.org.au>, "David
 S. Miller" <davem@...emloft.net>, "Andrew Zaborowski"
 <andrew.zaborowski@...el.com>
Cc: <keyrings@...r.kernel.org>, <linux-crypto@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, <lvc-project@...uxtesting.org>, "Sergey
 Shtylyov" <s.shtylyov@....ru>
Subject: Re: [PATCH] KEYS: prevent NULL pointer dereference in
 find_asymmetric_key()

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@....ru>

Should be reported-by.

> Signed-off-by: Roman Smirnov <r.smirnov@....ru>
> Reviewed-by: Sergey Shtylyov <s.shtylyov@....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