Hi Vsevolod, On Mon, Jul 6, 2026 at 1:19 PM Vsevolod Kozlov <[email protected]> wrote: > > 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? >
Yes. I prefer to update ERR_PTR/IS_ERR. Or at least, to have a wrapper on top of ERR_PTR/IS_ERR for MbedTLS use only. But I think the former is better. Raymond > 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
