[PATCH v2] lib/mbedtls: Do not use ERR_PTR with mbedTLS error values
Vsevolod Kozlov
zaba at mm.st
Wed Jul 8 17:10:09 CEST 2026
Hi Raymond,
On Tue, Jul 07, 2026 at 05:23:08PM -0400, Raymond Mao wrote:
> There might be some misunderstandings. I meant that there were two options:
Thank you again for the review. I am also not in love with this
approach, but at least it technically matches the commit message now
:)
> 1. Like what you mentioned, update ERR_PTR/IS_ERR.
I have reservations about ERR_PTR/IS_PTR in the context of U-Boot in
general, because they flag the upper 4KiB of the 32-bit address space
as errors, which on a 32-bit system may well be valid RAM? I don't
like the idea of expanding that range even further only to accommodate
MbedTLS.
> 2. Have a shim wrapper on top of ERR_PTR/IS_ERR for lib/mbedtls use only.
> But having wrappers on those mbedtls_ functions is not a good idea,
> especially explicitly with a prefix wrapped_.
Do you mean something along the lines of
void *ERR_PTR_MBEDTLS(int ret) {
if (ret < -MAX_ERRNO)
ret = -EINVAL;
return ERR_PTR(ret);
}
to fold all out-of-range negative error codes into a valid negative
errno? This would stop the crashes, but it would also make it
impossible in general to tell where an error came from, because
mbedTLS error codes range from -1 to -32767, so there is potential for
ambiguity between U-Boot's own negative-errno and mbedTLS's error
codes. I don't see a nice way to handle this without losing
potentially useful debugging information.
I think a better way to simply stop the crashes without improving
diagonics would be to change pkcs7_parse_message and x509_cert_parse
to return NULL on error and change the callers to use IS_ERR_OR_NULL.
Some of them already use it for x509_cert_parse (but not
consistently), and none of them actually make use of the error code
information anyway. What do you think?
Thank you and best wishes
> Regards,
> Raymond
>
> > lib/mbedtls/pkcs7_parser.c | 48 ++++++++++++++++++++++++++--------
> > lib/mbedtls/x509_cert_parser.c | 10 +++++--
> > 2 files changed, 45 insertions(+), 13 deletions(-)
> >
> > diff --git a/lib/mbedtls/pkcs7_parser.c b/lib/mbedtls/pkcs7_parser.c
> > index bf8ee17b5b8..6d255250e62 100644
> > --- a/lib/mbedtls/pkcs7_parser.c
> > +++ b/lib/mbedtls/pkcs7_parser.c
> > @@ -29,6 +29,20 @@ static void pkcs7_free_sinfo_mbedtls_ctx(struct pkcs7_sinfo_mbedtls_ctx *ctx)
> > }
> > }
> >
> > +/* Convert mbedTLS error codes to negative errno to avoid creating
> > + * out-of-range ERR_PTR values */
> > +static int wrapped_asn1_get_tag(unsigned char **p,
> > + const unsigned char *end,
> > + size_t *len, int tag)
> > +{
> > + int ret = mbedtls_asn1_get_tag(p, end, len, tag);
> > + if (ret) {
> > + debug("mbedtls_asn1_get_tag() failed: %x\n", ret);
> > + return -EINVAL;
> > + }
> > + return 0;
> > +}
> > +
> > /*
> > * Parse Authenticate Attributes
> > * TODO: Shall we consider to integrate decoding of authenticate attribute into
> > @@ -105,30 +119,30 @@ static int authattrs_parse(struct pkcs7_message *msg, void *aa, size_t aa_len,
> > unsigned char *inner_p;
> > size_t seq_len = 0;
> >
> > - ret = mbedtls_asn1_get_tag(&p, end, &seq_len,
> > + ret = wrapped_asn1_get_tag(&p, end, &seq_len,
> > MBEDTLS_ASN1_CONTEXT_SPECIFIC |
> > MBEDTLS_ASN1_CONSTRUCTED);
> > if (ret)
> > return ret;
> >
> > - while (!mbedtls_asn1_get_tag(&p, end, &seq_len,
> > + while (!wrapped_asn1_get_tag(&p, end, &seq_len,
> > MBEDTLS_ASN1_CONSTRUCTED |
> > MBEDTLS_ASN1_SEQUENCE)) {
> > inner_p = p;
> > - ret = mbedtls_asn1_get_tag(&inner_p, p + seq_len, &len,
> > + ret = wrapped_asn1_get_tag(&inner_p, p + seq_len, &len,
> > MBEDTLS_ASN1_OID);
> > if (ret)
> > return ret;
> >
> > if (!MBEDTLS_OID_CMP_RAW(MBEDTLS_OID_PKCS9_CONTENTTYPE, inner_p, len)) {
> > inner_p += len;
> > - ret = mbedtls_asn1_get_tag(&inner_p, p + seq_len, &len,
> > + ret = wrapped_asn1_get_tag(&inner_p, p + seq_len, &len,
> > MBEDTLS_ASN1_CONSTRUCTED |
> > MBEDTLS_ASN1_SET);
> > if (ret)
> > return ret;
> >
> > - ret = mbedtls_asn1_get_tag(&inner_p, p + seq_len, &len,
> > + ret = wrapped_asn1_get_tag(&inner_p, p + seq_len, &len,
> > MBEDTLS_ASN1_OID);
> > if (ret)
> > return ret;
> > @@ -151,13 +165,13 @@ static int authattrs_parse(struct pkcs7_message *msg, void *aa, size_t aa_len,
> > } else if (!MBEDTLS_OID_CMP_RAW(MBEDTLS_OID_PKCS9_MESSAGEDIGEST, inner_p,
> > len)) {
> > inner_p += len;
> > - ret = mbedtls_asn1_get_tag(&inner_p, p + seq_len, &len,
> > + ret = wrapped_asn1_get_tag(&inner_p, p + seq_len, &len,
> > MBEDTLS_ASN1_CONSTRUCTED |
> > MBEDTLS_ASN1_SET);
> > if (ret)
> > return ret;
> >
> > - ret = mbedtls_asn1_get_tag(&inner_p, p + seq_len, &len,
> > + ret = wrapped_asn1_get_tag(&inner_p, p + seq_len, &len,
> > MBEDTLS_ASN1_OCTET_STRING);
> > if (ret)
> > return ret;
> > @@ -172,15 +186,18 @@ static int authattrs_parse(struct pkcs7_message *msg, void *aa, size_t aa_len,
> > mbedtls_x509_time st;
> >
> > inner_p += len;
> > - ret = mbedtls_asn1_get_tag(&inner_p, p + seq_len, &len,
> > + ret = wrapped_asn1_get_tag(&inner_p, p + seq_len, &len,
> > MBEDTLS_ASN1_CONSTRUCTED |
> > MBEDTLS_ASN1_SET);
> > if (ret)
> > return ret;
> >
> > ret = mbedtls_x509_get_time(&inner_p, p + seq_len, &st);
> > - if (ret)
> > + if (ret) {
> > + debug("mbedtls_x509_get_time() failed: %x\n", ret);
> > + ret = -EINVAL;
> > return ret;
> > + }
> > sinfo->signing_time = x509_get_timestamp(&st);
> >
> > if (__test_and_set_bit(sinfo_has_signing_time, &sinfo->aa_set))
> > @@ -287,8 +304,11 @@ static int x509_populate_sinfo(struct pkcs7_message *msg,
> > * info. Assume that we are using RSA.
> > */
> > ret = mbedtls_oid_get_md_alg(&mb_sinfo->alg_identifier, &md_alg);
> > - if (ret)
> > + if (ret) {
> > + debug("mbedtls_oid_get_md_alg() failed: %x\n", ret);
> > + ret = -EINVAL;
> > goto out_err_sinfo;
> > + }
> > s->pkey_algo = "rsa";
> >
> > /* Translate the hash algorithm */
> > @@ -451,8 +471,14 @@ struct pkcs7_message *pkcs7_parse_message(const void *data, size_t datalen)
> > mbedtls_pkcs7_init(&pkcs7_ctx);
> > ret = mbedtls_pkcs7_parse_der(&pkcs7_ctx, data, datalen);
> > /* Check if it is a PKCS#7 message with signed data */
> > - if (ret != MBEDTLS_PKCS7_SIGNED_DATA)
> > + if (ret != MBEDTLS_PKCS7_SIGNED_DATA) {
> > + debug("mbedtls_pkcs7_parse_der() failed: %x\n", ret);
> > +
> > + /* MbedTLS error codes lie beyond MAX_ERRNO and are not
> > + * suitable for ERR_PTR. */
> > + ret = -EINVAL;
> > goto parse_fail;
> > + }
> >
> > /* Assume that we are using PKCS#7, not CMS ver 3 */
> > msg->version = 1; /* 1 for [PKCS#7 or CMS ver 1] */
> > diff --git a/lib/mbedtls/x509_cert_parser.c b/lib/mbedtls/x509_cert_parser.c
> > index e163e16b9bc..181727e1eda 100644
> > --- a/lib/mbedtls/x509_cert_parser.c
> > +++ b/lib/mbedtls/x509_cert_parser.c
> > @@ -425,13 +425,19 @@ struct x509_certificate *x509_cert_parse(const void *data, size_t datalen)
> > {
> > mbedtls_x509_crt mbedtls_cert;
> > struct x509_certificate *cert = NULL;
> > - long ret;
> > + int ret;
> >
> > /* Parse DER encoded certificate */
> > mbedtls_x509_crt_init(&mbedtls_cert);
> > ret = mbedtls_x509_crt_parse_der(&mbedtls_cert, data, datalen);
> > - if (ret)
> > + if (ret) {
> > + debug("mbedtls_x509_crt_parse_der() failed: %x\n", ret);
> > +
> > + /* MbedTLS error codes lie beyond MAX_ERRNO and are not
> > + * suitable for ERR_PTR. */
> > + ret = -EINVAL;
> > goto clean_up_ctx;
> > + }
> >
> > /* Populate x509_certificate from mbedtls_x509_crt */
> > ret = x509_populate_cert(&mbedtls_cert, &cert);
> > --
> > 2.47.3
> >
More information about the U-Boot
mailing list