Re: [PATCH v2 3/4] KEYS: x509: remove never-set ->unsupported_key flag

From: Jarkko Sakkinen <jarkko@kernel.org>
To: Eric Biggers <ebiggers@kernel.org>
Cc: keyrings@vger.kernel.org, David Howells <dhowells@redhat.com>,
	linux-crypto@vger.kernel.org
Subject: Re: [PATCH v2 3/4] KEYS: x509: remove never-set ->unsupported_key flag
Date: Wed, 26 Jan 2022 16:13:53 +0200	[thread overview]
Message-ID: <YfFXIY29zlIfQTcH@iki.fi> (raw)
In-Reply-To: <20220119005436.119072-4-ebiggers@kernel.org>

On Tue, Jan 18, 2022 at 04:54:35PM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> The X.509 parser always sets cert->pub->pkey_algo on success, since
> x509_extract_key_data() is a mandatory action in the X.509 ASN.1
> grammar, and it returns an error if the algorithm is unknown.  Thus,
> remove the dead code which handled this field being NULL.  This results
> in the ->unsupported_key flag never being set, so remove that too.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  crypto/asymmetric_keys/pkcs7_verify.c    | 7 ++-----
>  crypto/asymmetric_keys/x509_parser.h     | 1 -
>  crypto/asymmetric_keys/x509_public_key.c | 9 ---------
>  3 files changed, 2 insertions(+), 15 deletions(-)
> 
> diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
> index 0b4d07aa88111..d37b187faf9ae 100644
> --- a/crypto/asymmetric_keys/pkcs7_verify.c
> +++ b/crypto/asymmetric_keys/pkcs7_verify.c
> @@ -226,9 +226,6 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
>  			return 0;
>  		}
>  
> -		if (x509->unsupported_key)
> -			goto unsupported_crypto_in_x509;
> -
>  		pr_debug("- issuer %s\n", x509->issuer);
>  		sig = x509->sig;
>  		if (sig->auth_ids[0])
> @@ -245,7 +242,7 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
>  			 * authority.
>  			 */
>  			if (x509->unsupported_sig)
> -				goto unsupported_crypto_in_x509;
> +				goto unsupported_sig_in_x509;
>  			x509->signer = x509;
>  			pr_debug("- self-signed\n");
>  			return 0;
> @@ -309,7 +306,7 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
>  		might_sleep();
>  	}
>  
> -unsupported_crypto_in_x509:
> +unsupported_sig_in_x509:
>  	/* Just prune the certificate chain at this point if we lack some
>  	 * crypto module to go further.  Note, however, we don't want to set
>  	 * sinfo->unsupported_crypto as the signed info block may still be
> diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h
> index c233f136fb354..da854c94f1115 100644
> --- a/crypto/asymmetric_keys/x509_parser.h
> +++ b/crypto/asymmetric_keys/x509_parser.h
> @@ -36,7 +36,6 @@ struct x509_certificate {
>  	bool		seen;			/* Infinite recursion prevention */
>  	bool		verified;
>  	bool		self_signed;		/* T if self-signed (check unsupported_sig too) */
> -	bool		unsupported_key;	/* T if key uses unsupported crypto */
>  	bool		unsupported_sig;	/* T if signature uses unsupported crypto */
>  	bool		blacklisted;
>  };
> diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
> index fe14cae115b51..b03d04d78eb9d 100644
> --- a/crypto/asymmetric_keys/x509_public_key.c
> +++ b/crypto/asymmetric_keys/x509_public_key.c
> @@ -33,9 +33,6 @@ int x509_get_sig_params(struct x509_certificate *cert)
>  	sig->data = cert->tbs;
>  	sig->data_size = cert->tbs_size;
>  
> -	if (!cert->pub->pkey_algo)
> -		cert->unsupported_key = true;
> -
>  	if (!sig->pkey_algo)
>  		cert->unsupported_sig = true;
>  
> @@ -173,12 +170,6 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
>  
>  	pr_devel("Cert Issuer: %s\n", cert->issuer);
>  	pr_devel("Cert Subject: %s\n", cert->subject);
> -
> -	if (cert->unsupported_key) {
> -		ret = -ENOPKG;
> -		goto error_free_cert;
> -	}
> -
>  	pr_devel("Cert Key Algo: %s\n", cert->pub->pkey_algo);
>  	pr_devel("Cert Valid period: %lld-%lld\n", cert->valid_from, cert->valid_to);
>  
> -- 
> 2.34.1
> 


Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko