Hi Raymond, On Mon, Jul 06, 2026 at 11:38:43AM -0400, Raymond Mao wrote: > The direction is good, but the fixes do not match the commit message > statement. > In the commit message, only the MbedTLS native error code will be > substituted, but the fixes below arbitrarily overridden all error > codes including the standard U-Boot errnos.
Thank you for the review. I have re-read the code better and I think it would be feasible to convert return values of mbedtls functions to negative errnos right at their respective call sites, because there are not too many of them. The only exception is the mbedtls_asn1_get_tag function that gets called quite many times in pkcs7_parser.c, but a wrapper function could be added for it. Do you think that is the right approach, or should perhaps these code paths be converted away from ERR_PTR/IS_ERR entirely and just return NULL on error, given that none of the callers do anything with the error codes anyway? Thank you and best wishes > Please see my below inline comments. > > > Signed-off-by: Vsevolod Kozlov <[email protected]> > > --- > > lib/mbedtls/pkcs7_parser.c | 5 +++++ > > lib/mbedtls/x509_cert_parser.c | 6 +++++- > > 2 files changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/lib/mbedtls/pkcs7_parser.c b/lib/mbedtls/pkcs7_parser.c > > index bf8ee17b5b8..2c85dcc9699 100644 > > --- a/lib/mbedtls/pkcs7_parser.c > > +++ b/lib/mbedtls/pkcs7_parser.c > > @@ -495,6 +495,11 @@ struct pkcs7_message *pkcs7_parse_message(const void > > *data, size_t datalen) > > parse_fail: > > mbedtls_pkcs7_free(&pkcs7_ctx); > > pkcs7_free_message(msg); > > + > > + /* MbedTLS error codes lie beyond MAX_ERRNO and are not suitable > > + * for ERR_PTR, but can be useful for debugging. */ > > + debug("mbedtls returned error: %x\n", ret); > > + ret = -EINVAL; > > This collapses all non-zero failures to -EINVAL, including valid > U-Boot errnos such as -ENOMEM. > > > out_no_msg: > > msg = ERR_PTR(ret); > > return msg; > > diff --git a/lib/mbedtls/x509_cert_parser.c b/lib/mbedtls/x509_cert_parser.c > > index e163e16b9bc..1c70a8fcdcb 100644 > > --- a/lib/mbedtls/x509_cert_parser.c > > +++ b/lib/mbedtls/x509_cert_parser.c > > @@ -425,7 +425,7 @@ 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); > > @@ -443,5 +443,9 @@ clean_up_ctx: > > if (!ret) > > return cert; > > > > + /* MbedTLS error codes lie beyond MAX_ERRNO and are not suitable > > + * for ERR_PTR, but can be useful for debugging. */ > > + debug("mbedtls returned error: %x\n", ret); > > + ret = -EINVAL; > > This collapses all non-zero failures to -EINVAL, including valid > U-Boot errnos such as -ENOMEM. > > Regards, > Raymond > > > return ERR_PTR(ret); > > } > > -- > > 2.47.3
