From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from frasgout.his.huawei.com (frasgout.his.huawei.com [185.176.79.56]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id CA96C1DA3D for ; Thu, 18 Jul 2024 11:04:04 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.176.79.56 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1721300647; cv=none; b=TovJDXkACeKMuU35Og3CNfJP90r9B+buvjmGdW4HsJTY0BitFMexgF0LDdh0xFreWCJqWWbZOAzQKtGRRL6IvCP2pY9+fP/G7hnsPVUvZ9t8PF+Q6mbrzQ02PZ7uzCoVaBxsPcAM2iaiSAEl7Qr0oRB9xNU5lhouL/bLDuGKBoA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1721300647; c=relaxed/simple; bh=I/0ZzAVCOebuM9O7BzGpCAsAC3hNWzhHlKZRe6qlOJM=; h=Date:From:To:CC:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=o0Djl2LJBY1Q8cA6PclE+SzFDbJar/0h2lEJsfK4p0CxRucGun7ap0a6WqPcy29cJA+velR1t+KCdK5PE1Y+SJNe6lzsuxHWGSqzP4tCX1nfP59ud0i/EU49+y387VnelztYbUx3kzyuknMhtuLbtGeRofDaCnOrBM6pKWDyG2M= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=Huawei.com; spf=pass smtp.mailfrom=huawei.com; arc=none smtp.client-ip=185.176.79.56 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=Huawei.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=huawei.com Received: from mail.maildlp.com (unknown [172.18.186.216]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4WPqdr6yQwz6HJdS; Thu, 18 Jul 2024 19:02:40 +0800 (CST) Received: from lhrpeml500005.china.huawei.com (unknown [7.191.163.240]) by mail.maildlp.com (Postfix) with ESMTPS id 09F28140593; Thu, 18 Jul 2024 19:04:02 +0800 (CST) Received: from localhost (10.203.174.77) by lhrpeml500005.china.huawei.com (7.191.163.240) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.39; Thu, 18 Jul 2024 12:04:01 +0100 Date: Thu, 18 Jul 2024 12:04:00 +0100 From: Jonathan Cameron To: Lukas Wunner CC: Bjorn Helgaas , David Howells , Herbert Xu , "David S. Miller" , David Woodhouse , "James Bottomley" , , , , , , , David Box , Dan Williams , "Li, Ming" , Ilpo Jarvinen , Alistair Francis , Wilfred Mallawa , Damien Le Moal , "Alexey Kardashevskiy" , Dhaval Giani , Gobikrishna Dhanuskodi , Jason Gunthorpe , Peter Gonda , Jerome Glisse , Sean Christopherson , "Alexander Graf" , Samuel Ortiz , Eric Biggers Subject: Re: [PATCH v2 03/18] X.509: Move certificate length retrieval into new helper Message-ID: <20240718120400.00006a70@Huawei.com> In-Reply-To: References: Organization: Huawei Technologies Research and Development (UK) Ltd. X-Mailer: Claws Mail 4.1.0 (GTK 3.24.33; x86_64-w64-mingw32) Precedence: bulk X-Mailing-List: linux-coco@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit X-ClientProxiedBy: lhrpeml500002.china.huawei.com (7.191.160.78) To lhrpeml500005.china.huawei.com (7.191.163.240) On Sun, 30 Jun 2024 21:38:00 +0200 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 the helper so that SPDM can be tristate. (Some upcoming users of > the SPDM libray may be modular, such as SCSI and ATA.) > > No functional change intended. > > Signed-off-by: Lukas Wunner > Reviewed-by: Dan Williams > Reviewed-by: Jonathan Cameron Rereading some of these early patches to try and get my head back into what is going on here.. Passing comments inline, but given you are just moving the code rather than writing it for the first time I don't mind keeping it as things stand. > --- > 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..25ff027fad1d 100644 > --- a/crypto/asymmetric_keys/x509_loader.c > +++ b/crypto/asymmetric_keys/x509_loader.c > @@ -4,28 +4,42 @@ > #include > #include > > +ssize_t x509_get_certificate_length(const u8 *p, unsigned long buflen) > +{ > + ssize_t 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) Not sure readability would be hurt significantly by putting that on one line. > + return -EINVAL; > + > + plen = (p[2] << 8) | p[3]; get_unaligned_be16() perhaps > + plen += 4; It's kind of obvious, but maybe a comment no why +4 would be good. > + 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; > + ssize_t 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..e2af07fec3c6 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); > > +ssize_t 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); >