diff mbox series

[03/12] X.509: Move certificate length retrieval into new helper

Message ID 16c06528d13b2c0081229a45cacd4b1b9cdff738.1695921657.git.lukas@wunner.de (mailing list archive)
State New
Headers show
Series PCI device authentication | expand

Commit Message

Lukas Wunner Sept. 28, 2023, 5:32 p.m. UTC
The upcoming in-kernel SPDM library (Security Protocol and Data Model,
https://www.dmtf.org/dsp/DSP0274) needs to retrieve the length from
ASN.1 DER-encoded X.509 certificates.

Such code already exists in x509_load_certificate_list(), so move it
into a new helper for reuse by SPDM.

No functional change intended.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 crypto/asymmetric_keys/x509_loader.c | 38 +++++++++++++++++++---------
 include/keys/asymmetric-type.h       |  2 ++
 2 files changed, 28 insertions(+), 12 deletions(-)

Comments

Jonathan Cameron Oct. 2, 2023, 4:44 p.m. UTC | #1
On Thu, 28 Sep 2023 19:32:32 +0200
Lukas Wunner <lukas@wunner.de> wrote:

> The upcoming in-kernel SPDM library (Security Protocol and Data Model,
> https://www.dmtf.org/dsp/DSP0274) needs to retrieve the length from
> ASN.1 DER-encoded X.509 certificates.
> 
> Such code already exists in x509_load_certificate_list(), so move it
> into a new helper for reuse by SPDM.
> 
> No functional change intended.
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Good find :)  I vaguely remember carrying a hack for this so
good to do something more general + save on the duplication.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


> ---
>  crypto/asymmetric_keys/x509_loader.c | 38 +++++++++++++++++++---------
>  include/keys/asymmetric-type.h       |  2 ++
>  2 files changed, 28 insertions(+), 12 deletions(-)
> 
> diff --git a/crypto/asymmetric_keys/x509_loader.c b/crypto/asymmetric_keys/x509_loader.c
> index a41741326998..121460a0de46 100644
> --- a/crypto/asymmetric_keys/x509_loader.c
> +++ b/crypto/asymmetric_keys/x509_loader.c
> @@ -4,28 +4,42 @@
>  #include <linux/key.h>
>  #include <keys/asymmetric-type.h>
>  
> +int x509_get_certificate_length(const u8 *p, unsigned long buflen)
> +{
> +	int plen;
> +
> +	/* Each cert begins with an ASN.1 SEQUENCE tag and must be more
> +	 * than 256 bytes in size.
> +	 */
> +	if (buflen < 4)
> +		return -EINVAL;
> +
> +	if (p[0] != 0x30 &&
> +	    p[1] != 0x82)
> +		return -EINVAL;
> +
> +	plen = (p[2] << 8) | p[3];
> +	plen += 4;
> +	if (plen > buflen)
> +		return -EINVAL;
> +
> +	return plen;
> +}
> +EXPORT_SYMBOL_GPL(x509_get_certificate_length);
> +
>  int x509_load_certificate_list(const u8 cert_list[],
>  			       const unsigned long list_size,
>  			       const struct key *keyring)
>  {
>  	key_ref_t key;
>  	const u8 *p, *end;
> -	size_t plen;
> +	int plen;
>  
>  	p = cert_list;
>  	end = p + list_size;
>  	while (p < end) {
> -		/* Each cert begins with an ASN.1 SEQUENCE tag and must be more
> -		 * than 256 bytes in size.
> -		 */
> -		if (end - p < 4)
> -			goto dodgy_cert;
> -		if (p[0] != 0x30 &&
> -		    p[1] != 0x82)
> -			goto dodgy_cert;
> -		plen = (p[2] << 8) | p[3];
> -		plen += 4;
> -		if (plen > end - p)
> +		plen = x509_get_certificate_length(p, end - p);
> +		if (plen < 0)
>  			goto dodgy_cert;
>  
>  		key = key_create_or_update(make_key_ref(keyring, 1),
> diff --git a/include/keys/asymmetric-type.h b/include/keys/asymmetric-type.h
> index 69a13e1e5b2e..6705cfde25b9 100644
> --- a/include/keys/asymmetric-type.h
> +++ b/include/keys/asymmetric-type.h
> @@ -84,6 +84,8 @@ extern struct key *find_asymmetric_key(struct key *keyring,
>  				       const struct asymmetric_key_id *id_2,
>  				       bool partial);
>  
> +int x509_get_certificate_length(const u8 *p, unsigned long buflen);
> +
>  int x509_load_certificate_list(const u8 cert_list[], const unsigned long list_size,
>  			       const struct key *keyring);
>
Ilpo Järvinen Oct. 3, 2023, 8:31 a.m. UTC | #2
On Thu, 28 Sep 2023, Lukas Wunner wrote:

> The upcoming in-kernel SPDM library (Security Protocol and Data Model,
> https://www.dmtf.org/dsp/DSP0274) needs to retrieve the length from
> ASN.1 DER-encoded X.509 certificates.
> 
> Such code already exists in x509_load_certificate_list(), so move it
> into a new helper for reuse by SPDM.
> 
> No functional change intended.
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  crypto/asymmetric_keys/x509_loader.c | 38 +++++++++++++++++++---------
>  include/keys/asymmetric-type.h       |  2 ++
>  2 files changed, 28 insertions(+), 12 deletions(-)
> 
> diff --git a/crypto/asymmetric_keys/x509_loader.c b/crypto/asymmetric_keys/x509_loader.c
> index a41741326998..121460a0de46 100644
> --- a/crypto/asymmetric_keys/x509_loader.c
> +++ b/crypto/asymmetric_keys/x509_loader.c
> @@ -4,28 +4,42 @@
>  #include <linux/key.h>
>  #include <keys/asymmetric-type.h>
>  
> +int x509_get_certificate_length(const u8 *p, unsigned long buflen)

Make the return type ssize_t.

unsigned long -> size_t buflen (or perhaps ssize_t if you want to compare 
below to have the same signedness).

> +{
> +	int plen;

ssize_t

> +
> +	/* Each cert begins with an ASN.1 SEQUENCE tag and must be more
> +	 * than 256 bytes in size.
> +	 */
> +	if (buflen < 4)
> +		return -EINVAL;
> +
> +	if (p[0] != 0x30 &&
> +	    p[1] != 0x82)
> +		return -EINVAL;
> +
> +	plen = (p[2] << 8) | p[3];
> +	plen += 4;
> +	if (plen > buflen)
> +		return -EINVAL;
> +
> +	return plen;
> +}
> +EXPORT_SYMBOL_GPL(x509_get_certificate_length);
> +
>  int x509_load_certificate_list(const u8 cert_list[],
>  			       const unsigned long list_size,
>  			       const struct key *keyring)
>  {
>  	key_ref_t key;
>  	const u8 *p, *end;
> -	size_t plen;
> +	int plen;

ssize_t plen.
Dan Williams Oct. 6, 2023, 7:15 p.m. UTC | #3
Lukas Wunner wrote:
> The upcoming in-kernel SPDM library (Security Protocol and Data Model,
> https://www.dmtf.org/dsp/DSP0274) needs to retrieve the length from
> ASN.1 DER-encoded X.509 certificates.
> 
> Such code already exists in x509_load_certificate_list(), so move it
> into a new helper for reuse by SPDM.
> 
> No functional change intended.
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  crypto/asymmetric_keys/x509_loader.c | 38 +++++++++++++++++++---------
>  include/keys/asymmetric-type.h       |  2 ++
>  2 files changed, 28 insertions(+), 12 deletions(-)
> 
> diff --git a/crypto/asymmetric_keys/x509_loader.c b/crypto/asymmetric_keys/x509_loader.c
> index a41741326998..121460a0de46 100644
> --- a/crypto/asymmetric_keys/x509_loader.c
> +++ b/crypto/asymmetric_keys/x509_loader.c
> @@ -4,28 +4,42 @@
>  #include <linux/key.h>
>  #include <keys/asymmetric-type.h>
>  
> +int x509_get_certificate_length(const u8 *p, unsigned long buflen)
> +{
> +	int plen;
> +
> +	/* Each cert begins with an ASN.1 SEQUENCE tag and must be more
> +	 * than 256 bytes in size.
> +	 */
> +	if (buflen < 4)
> +		return -EINVAL;
> +
> +	if (p[0] != 0x30 &&
> +	    p[1] != 0x82)
> +		return -EINVAL;
> +
> +	plen = (p[2] << 8) | p[3];
> +	plen += 4;
> +	if (plen > buflen)
> +		return -EINVAL;
> +
> +	return plen;
> +}
> +EXPORT_SYMBOL_GPL(x509_get_certificate_length);

Given CONFIG_PCI is a bool, is the export needed? Maybe save this export
until the modular consumer arrives, or identify the modular consumer in the
changelog?

Other than that:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Lukas Wunner March 4, 2024, 6:57 a.m. UTC | #4
On Fri, Oct 06, 2023 at 12:15:13PM -0700, Dan Williams wrote:
> Lukas Wunner wrote:
> > The upcoming in-kernel SPDM library (Security Protocol and Data Model,
> > https://www.dmtf.org/dsp/DSP0274) needs to retrieve the length from
> > ASN.1 DER-encoded X.509 certificates.
> > 
> > Such code already exists in x509_load_certificate_list(), so move it
> > into a new helper for reuse by SPDM.
[...]
> > +EXPORT_SYMBOL_GPL(x509_get_certificate_length);
> 
> Given CONFIG_PCI is a bool, is the export needed? Maybe save this export
> until the modular consumer arrives, or identify the modular consumer in the
> changelog?

The x509_get_certificate_length() helper introduced by this patch
isn't needed directly by the PCI core, but by the SPDM library.

The SPDM library is tristate and is selected by CONFIG_PCI_CMA,
which is indeed bool.

However SCSI and ATA (both tristate) have explicitly expressed an
interest to use the SPDM library.

If I drop the export, I'd have to declare the SPDM library bool.

I'm leaning towards keeping the SPDM library tristate (and keep the
export) to accommodate SCSI, ATA and possibly others.

Please let me know if you disagree.

Thanks,

Lukas
Dan Williams March 4, 2024, 7:19 p.m. UTC | #5
Lukas Wunner wrote:
> On Fri, Oct 06, 2023 at 12:15:13PM -0700, Dan Williams wrote:
> > Lukas Wunner wrote:
> > > The upcoming in-kernel SPDM library (Security Protocol and Data Model,
> > > https://www.dmtf.org/dsp/DSP0274) needs to retrieve the length from
> > > ASN.1 DER-encoded X.509 certificates.
> > > 
> > > Such code already exists in x509_load_certificate_list(), so move it
> > > into a new helper for reuse by SPDM.
> [...]
> > > +EXPORT_SYMBOL_GPL(x509_get_certificate_length);
> > 
> > Given CONFIG_PCI is a bool, is the export needed? Maybe save this export
> > until the modular consumer arrives, or identify the modular consumer in the
> > changelog?
> 
> The x509_get_certificate_length() helper introduced by this patch
> isn't needed directly by the PCI core, but by the SPDM library.
> 
> The SPDM library is tristate and is selected by CONFIG_PCI_CMA,
> which is indeed bool.
> 
> However SCSI and ATA (both tristate) have explicitly expressed an
> interest to use the SPDM library.
> 
> If I drop the export, I'd have to declare the SPDM library bool.
> 
> I'm leaning towards keeping the SPDM library tristate (and keep the
> export) to accommodate SCSI, ATA and possibly others.
> 
> Please let me know if you disagree.

Oh, missed that the SPDM library is the first modular consumer. Looks
good to me.
diff mbox series

Patch

diff --git a/crypto/asymmetric_keys/x509_loader.c b/crypto/asymmetric_keys/x509_loader.c
index a41741326998..121460a0de46 100644
--- a/crypto/asymmetric_keys/x509_loader.c
+++ b/crypto/asymmetric_keys/x509_loader.c
@@ -4,28 +4,42 @@ 
 #include <linux/key.h>
 #include <keys/asymmetric-type.h>
 
+int x509_get_certificate_length(const u8 *p, unsigned long buflen)
+{
+	int plen;
+
+	/* Each cert begins with an ASN.1 SEQUENCE tag and must be more
+	 * than 256 bytes in size.
+	 */
+	if (buflen < 4)
+		return -EINVAL;
+
+	if (p[0] != 0x30 &&
+	    p[1] != 0x82)
+		return -EINVAL;
+
+	plen = (p[2] << 8) | p[3];
+	plen += 4;
+	if (plen > buflen)
+		return -EINVAL;
+
+	return plen;
+}
+EXPORT_SYMBOL_GPL(x509_get_certificate_length);
+
 int x509_load_certificate_list(const u8 cert_list[],
 			       const unsigned long list_size,
 			       const struct key *keyring)
 {
 	key_ref_t key;
 	const u8 *p, *end;
-	size_t plen;
+	int plen;
 
 	p = cert_list;
 	end = p + list_size;
 	while (p < end) {
-		/* Each cert begins with an ASN.1 SEQUENCE tag and must be more
-		 * than 256 bytes in size.
-		 */
-		if (end - p < 4)
-			goto dodgy_cert;
-		if (p[0] != 0x30 &&
-		    p[1] != 0x82)
-			goto dodgy_cert;
-		plen = (p[2] << 8) | p[3];
-		plen += 4;
-		if (plen > end - p)
+		plen = x509_get_certificate_length(p, end - p);
+		if (plen < 0)
 			goto dodgy_cert;
 
 		key = key_create_or_update(make_key_ref(keyring, 1),
diff --git a/include/keys/asymmetric-type.h b/include/keys/asymmetric-type.h
index 69a13e1e5b2e..6705cfde25b9 100644
--- a/include/keys/asymmetric-type.h
+++ b/include/keys/asymmetric-type.h
@@ -84,6 +84,8 @@  extern struct key *find_asymmetric_key(struct key *keyring,
 				       const struct asymmetric_key_id *id_2,
 				       bool partial);
 
+int x509_get_certificate_length(const u8 *p, unsigned long buflen);
+
 int x509_load_certificate_list(const u8 cert_list[], const unsigned long list_size,
 			       const struct key *keyring);