Re: [PATCH 04/18] X.509: Move certificate length retrieval into new helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The exported helper `x509_get_certificate_length()` uses a flawed logical condition (`&&` instead of `||`) that fails to properly validate ASN.1 headers, leading to parser differentials, incorrect length calculations, and functional failures.
--

commit 0f604cd0b6dd899bd029f10862a9b7010589e559
Author: Lukas Wunner <lukas@wunner.de>

X.509: Move certificate length retrieval into new helper

This commit extracts the certificate length parsing logic from
x509_load_certificate_list() into a newly exported helper,
x509_get_certificate_length(), to be reused by the upcoming in-kernel
SPDM library.

> diff --git a/crypto/asymmetric_keys/x509_loader.c b/crypto/asymmetric_keys/x509_loader.c
> index a417413269989..25ff027fad1d3 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>
>  
> +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)
> +		return -EINVAL;

Does this logical condition properly enforce the expected ASN.1 header?

If an untrusted device supplies a valid certificate smaller than 256 bytes
using a 1-byte length encoding (for example p[0] = 0x30, p[1] = 0x81), this
check evaluates to false because p[0] is equal to 0x30. This allows the
short certificate to bypass the check.

> +
> +	plen = (p[2] << 8) | p[3];
> +	plen += 4;

If a short certificate bypasses the check above, does this calculate an
incorrect length?

For a 1-byte length encoding, p[2] would be the actual length and p[3]
would be the first byte of the payload, resulting in a potentially very
large calculated length.

If this bogus length exceeds the buffer size, it would return an error and
cause functional failures. If the length is within the buffer size, could
it advance the offset incorrectly and cause parser desynchronization by
skipping over subsequent certificates or payload data?

> +	if (plen > buflen)
> +		return -EINVAL;
> +
> +	return plen;
> +}
> +EXPORT_SYMBOL_GPL(x509_get_certificate_length);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508031710.514574-1-alistair.francis@wdc.com?part=4