| Message ID | 16c06528d13b2c0081229a45cacd4b1b9cdff738.1695921657.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI device authentication | expand |
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); >
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.
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>
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
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 --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);
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(-)