[PATCH v2] lib/mbedtls: Do not use ERR_PTR with mbedTLS error values
Raymond Mao
raymondmaoca at gmail.com
Wed Jul 8 19:06:39 CEST 2026
Hi Vsevolod,
On Wed, Jul 8, 2026 at 11:10 AM Vsevolod Kozlov <zaba at mm.st> wrote:
>
> 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?
>
The reason for ERR_PTR in mbedtls port is to provide the same ABI for
EFI loader, compared with the legacy crypto libraries.
I would suggest to:
void *ERR_PTR_MBEDTLS(int ret) {
if (ret < -MAX_ERRNO) {
error("mbedtls internal error: %x\n", ret);
ret = -EIO;
}
return ERR_PTR(ret);
}
Regards,
Raymond
> 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