* [PATCH master] optee: don't warn about missing OP-TEE header @ 2024-02-28 18:03 Ahmad Fatoum 2024-02-29 9:10 ` Marco Felsch 2024-03-01 9:21 ` Sascha Hauer 0 siblings, 2 replies; 8+ messages in thread From: Ahmad Fatoum @ 2024-02-28 18:03 UTC (permalink / raw) To: barebox; +Cc: mfe, Ahmad Fatoum OP-TEE header is checked once in PBL, saved into scratch area after verification and then checked again in barebox proper. The check in PBL fails silently, but the check in barebox proper that should always follow, because the header isn't written to the scratch area is printed with error log level. Printing an error in either case is wrong though as using a raw OP-TEE binary without header is a valid use case and the OP-TEE header may also be missing when barebox is chainloaded from a running barebox. Therefore reduce the message to debug log level. Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> --- common/optee.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/optee.c b/common/optee.c index 34667f1f51e0..a8a43554e757 100644 --- a/common/optee.c +++ b/common/optee.c @@ -14,8 +14,8 @@ int optee_verify_header(const struct optee_header *hdr) return -EINVAL; if (hdr->magic != OPTEE_MAGIC) { - pr_err("Invalid header magic 0x%08x, expected 0x%08x\n", - hdr->magic, OPTEE_MAGIC); + pr_debug("Invalid header magic 0x%08x, expected 0x%08x\n", + hdr->magic, OPTEE_MAGIC); return -EINVAL; } -- 2.39.2 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH master] optee: don't warn about missing OP-TEE header 2024-02-28 18:03 [PATCH master] optee: don't warn about missing OP-TEE header Ahmad Fatoum @ 2024-02-29 9:10 ` Marco Felsch 2024-02-29 9:17 ` Ahmad Fatoum 2024-03-01 9:21 ` Sascha Hauer 1 sibling, 1 reply; 8+ messages in thread From: Marco Felsch @ 2024-02-29 9:10 UTC (permalink / raw) To: Ahmad Fatoum; +Cc: barebox Hi Ahmad, On 24-02-28, Ahmad Fatoum wrote: > OP-TEE header is checked once in PBL, saved into scratch area after > verification and then checked again in barebox proper. > > The check in PBL fails silently, but the check in barebox proper that > should always follow, because the header isn't written to the scratch > area is printed with error log level. > > Printing an error in either case is wrong though as using a raw OP-TEE > binary without header is a valid use case and the OP-TEE header may > also be missing when barebox is chainloaded from a running barebox. > > Therefore reduce the message to debug log level. > > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> > --- > common/optee.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/common/optee.c b/common/optee.c > index 34667f1f51e0..a8a43554e757 100644 > --- a/common/optee.c > +++ b/common/optee.c > @@ -14,8 +14,8 @@ int optee_verify_header(const struct optee_header *hdr) > return -EINVAL; Shouldn't be the fix: if (IS_ERR_OR_NULL(hdr)) return -EINVAL; to fail silently. Regards, Marco > > if (hdr->magic != OPTEE_MAGIC) { > - pr_err("Invalid header magic 0x%08x, expected 0x%08x\n", > - hdr->magic, OPTEE_MAGIC); > + pr_debug("Invalid header magic 0x%08x, expected 0x%08x\n", > + hdr->magic, OPTEE_MAGIC); > return -EINVAL; > } > > -- > 2.39.2 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH master] optee: don't warn about missing OP-TEE header 2024-02-29 9:10 ` Marco Felsch @ 2024-02-29 9:17 ` Ahmad Fatoum 2024-02-29 9:33 ` Marco Felsch 0 siblings, 1 reply; 8+ messages in thread From: Ahmad Fatoum @ 2024-02-29 9:17 UTC (permalink / raw) To: Marco Felsch; +Cc: barebox On 29.02.24 10:10, Marco Felsch wrote: > Hi Ahmad, > > On 24-02-28, Ahmad Fatoum wrote: >> OP-TEE header is checked once in PBL, saved into scratch area after >> verification and then checked again in barebox proper. >> >> The check in PBL fails silently, but the check in barebox proper that >> should always follow, because the header isn't written to the scratch >> area is printed with error log level. >> >> Printing an error in either case is wrong though as using a raw OP-TEE >> binary without header is a valid use case and the OP-TEE header may >> also be missing when barebox is chainloaded from a running barebox. >> >> Therefore reduce the message to debug log level. >> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> >> --- >> common/optee.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/common/optee.c b/common/optee.c >> index 34667f1f51e0..a8a43554e757 100644 >> --- a/common/optee.c >> +++ b/common/optee.c >> @@ -14,8 +14,8 @@ int optee_verify_header(const struct optee_header *hdr) >> return -EINVAL; > > Shouldn't be the fix: > > if (IS_ERR_OR_NULL(hdr)) > return -EINVAL; > > to fail silently. hdr is a valid pointer for me, but it doesn't point at a header, which causes me to get an error message. Thanks, Ahmad > > Regards, > Marco > >> >> if (hdr->magic != OPTEE_MAGIC) { >> - pr_err("Invalid header magic 0x%08x, expected 0x%08x\n", >> - hdr->magic, OPTEE_MAGIC); >> + pr_debug("Invalid header magic 0x%08x, expected 0x%08x\n", >> + hdr->magic, OPTEE_MAGIC); >> return -EINVAL; >> } >> >> -- >> 2.39.2 >> >> > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH master] optee: don't warn about missing OP-TEE header 2024-02-29 9:17 ` Ahmad Fatoum @ 2024-02-29 9:33 ` Marco Felsch 2024-02-29 9:46 ` Ahmad Fatoum 0 siblings, 1 reply; 8+ messages in thread From: Marco Felsch @ 2024-02-29 9:33 UTC (permalink / raw) To: Ahmad Fatoum; +Cc: barebox On 24-02-29, Ahmad Fatoum wrote: > On 29.02.24 10:10, Marco Felsch wrote: > > Hi Ahmad, > > > > On 24-02-28, Ahmad Fatoum wrote: > >> OP-TEE header is checked once in PBL, saved into scratch area after > >> verification and then checked again in barebox proper. > >> > >> The check in PBL fails silently, but the check in barebox proper that > >> should always follow, because the header isn't written to the scratch > >> area is printed with error log level. > >> > >> Printing an error in either case is wrong though as using a raw OP-TEE > >> binary without header is a valid use case and the OP-TEE header may > >> also be missing when barebox is chainloaded from a running barebox. > >> > >> Therefore reduce the message to debug log level. > >> > >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> > >> --- > >> common/optee.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/common/optee.c b/common/optee.c > >> index 34667f1f51e0..a8a43554e757 100644 > >> --- a/common/optee.c > >> +++ b/common/optee.c > >> @@ -14,8 +14,8 @@ int optee_verify_header(const struct optee_header *hdr) > >> return -EINVAL; > > > > Shouldn't be the fix: > > > > if (IS_ERR_OR_NULL(hdr)) > > return -EINVAL; > > > > to fail silently. > > hdr is a valid pointer for me, but it doesn't point at a header, which causes > me to get an error message. Yes, I have noticed the code path for non-pbl part as well now :/ I was thinking about: if (hdr->magic == 0) return -EINVAL; but this is not far awways from you change. Therefore: Reviewed-by: Marco Felsch <m.felsch@pengutronix.de> PS: Could you please fix the silent check as well by using IS_ERR_OR_NULL()? Regards, Marco > > Thanks, > Ahmad > > > > > Regards, > > Marco > > > >> > >> if (hdr->magic != OPTEE_MAGIC) { > >> - pr_err("Invalid header magic 0x%08x, expected 0x%08x\n", > >> - hdr->magic, OPTEE_MAGIC); > >> + pr_debug("Invalid header magic 0x%08x, expected 0x%08x\n", > >> + hdr->magic, OPTEE_MAGIC); > >> return -EINVAL; > >> } > >> > >> -- > >> 2.39.2 > >> > >> > > > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH master] optee: don't warn about missing OP-TEE header 2024-02-29 9:33 ` Marco Felsch @ 2024-02-29 9:46 ` Ahmad Fatoum 2024-02-29 11:13 ` Marco Felsch 0 siblings, 1 reply; 8+ messages in thread From: Ahmad Fatoum @ 2024-02-29 9:46 UTC (permalink / raw) To: Marco Felsch; +Cc: barebox On 29.02.24 10:33, Marco Felsch wrote: > On 24-02-29, Ahmad Fatoum wrote: >> hdr is a valid pointer for me, but it doesn't point at a header, which causes >> me to get an error message. > > Yes, I have noticed the code path for non-pbl part as well now :/ > > I was thinking about: > > if (hdr->magic == 0) > return -EINVAL; > > but this is not far awways from you change. Therefore: This would assume that DRAM is zero-initialized after POR, which isn't given. > > Reviewed-by: Marco Felsch <m.felsch@pengutronix.de> > > PS: Could you please fix the silent check as well by using IS_ERR_OR_NULL()? I will send a separate patch for that, so this can be picked up as-is. Cheers, Ahmad > > Regards, > Marco > >> >> Thanks, >> Ahmad >> >>> >>> Regards, >>> Marco >>> >>>> >>>> if (hdr->magic != OPTEE_MAGIC) { >>>> - pr_err("Invalid header magic 0x%08x, expected 0x%08x\n", >>>> - hdr->magic, OPTEE_MAGIC); >>>> + pr_debug("Invalid header magic 0x%08x, expected 0x%08x\n", >>>> + hdr->magic, OPTEE_MAGIC); >>>> return -EINVAL; >>>> } >>>> >>>> -- >>>> 2.39.2 >>>> >>>> >>> >> >> -- >> Pengutronix e.K. | | >> Steuerwalder Str. 21 | http://www.pengutronix.de/ | >> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | >> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | >> >> > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH master] optee: don't warn about missing OP-TEE header 2024-02-29 9:46 ` Ahmad Fatoum @ 2024-02-29 11:13 ` Marco Felsch 2024-02-29 11:46 ` Ahmad Fatoum 0 siblings, 1 reply; 8+ messages in thread From: Marco Felsch @ 2024-02-29 11:13 UTC (permalink / raw) To: Ahmad Fatoum; +Cc: barebox On 24-02-29, Ahmad Fatoum wrote: > On 29.02.24 10:33, Marco Felsch wrote: > > On 24-02-29, Ahmad Fatoum wrote: > >> hdr is a valid pointer for me, but it doesn't point at a header, which causes > >> me to get an error message. > > > > Yes, I have noticed the code path for non-pbl part as well now :/ > > > > I was thinking about: > > > > if (hdr->magic == 0) > > return -EINVAL; > > > > but this is not far awways from you change. Therefore: > > This would assume that DRAM is zero-initialized after POR, which isn't given. For i.MX8M and i.MX9 it should be the case since we call imx8m*_init_scratch_space which in turn does set the scratch space to zero. Anyway as said, I'm fine with lowering it to pr_debug() :) Regards, Marco > > Reviewed-by: Marco Felsch <m.felsch@pengutronix.de> > > > > PS: Could you please fix the silent check as well by using IS_ERR_OR_NULL()? > > I will send a separate patch for that, so this can be picked up as-is. > > Cheers, > Ahmad > > > > > Regards, > > Marco > > > >> > >> Thanks, > >> Ahmad > >> > >>> > >>> Regards, > >>> Marco > >>> > >>>> > >>>> if (hdr->magic != OPTEE_MAGIC) { > >>>> - pr_err("Invalid header magic 0x%08x, expected 0x%08x\n", > >>>> - hdr->magic, OPTEE_MAGIC); > >>>> + pr_debug("Invalid header magic 0x%08x, expected 0x%08x\n", > >>>> + hdr->magic, OPTEE_MAGIC); > >>>> return -EINVAL; > >>>> } > >>>> > >>>> -- > >>>> 2.39.2 > >>>> > >>>> > >>> > >> > >> -- > >> Pengutronix e.K. | | > >> Steuerwalder Str. 21 | http://www.pengutronix.de/ | > >> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > >> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | > >> > >> > > > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH master] optee: don't warn about missing OP-TEE header 2024-02-29 11:13 ` Marco Felsch @ 2024-02-29 11:46 ` Ahmad Fatoum 0 siblings, 0 replies; 8+ messages in thread From: Ahmad Fatoum @ 2024-02-29 11:46 UTC (permalink / raw) To: Marco Felsch; +Cc: barebox On 29.02.24 12:13, Marco Felsch wrote: > On 24-02-29, Ahmad Fatoum wrote: >> On 29.02.24 10:33, Marco Felsch wrote: >>> On 24-02-29, Ahmad Fatoum wrote: >>>> hdr is a valid pointer for me, but it doesn't point at a header, which causes >>>> me to get an error message. >>> >>> Yes, I have noticed the code path for non-pbl part as well now :/ >>> >>> I was thinking about: >>> >>> if (hdr->magic == 0) >>> return -EINVAL; >>> >>> but this is not far awways from you change. Therefore: >> >> This would assume that DRAM is zero-initialized after POR, which isn't given. > > For i.MX8M and i.MX9 it should be the case since we call > imx8m*_init_scratch_space which in turn does set the scratch space to > zero. Anyway as said, I'm fine with lowering it to pr_debug() :) Ah, I see. First time I ran into this, I was chainloading a new barebox from an old barebox, so it read gibberish. Cheers, Ahmad > > Regards, > Marco > >>> Reviewed-by: Marco Felsch <m.felsch@pengutronix.de> >>> >>> PS: Could you please fix the silent check as well by using IS_ERR_OR_NULL()? >> >> I will send a separate patch for that, so this can be picked up as-is. >> >> Cheers, >> Ahmad >> >>> >>> Regards, >>> Marco >>> >>>> >>>> Thanks, >>>> Ahmad >>>> >>>>> >>>>> Regards, >>>>> Marco >>>>> >>>>>> >>>>>> if (hdr->magic != OPTEE_MAGIC) { >>>>>> - pr_err("Invalid header magic 0x%08x, expected 0x%08x\n", >>>>>> - hdr->magic, OPTEE_MAGIC); >>>>>> + pr_debug("Invalid header magic 0x%08x, expected 0x%08x\n", >>>>>> + hdr->magic, OPTEE_MAGIC); >>>>>> return -EINVAL; >>>>>> } >>>>>> >>>>>> -- >>>>>> 2.39.2 >>>>>> >>>>>> >>>>> >>>> >>>> -- >>>> Pengutronix e.K. | | >>>> Steuerwalder Str. 21 | http://www.pengutronix.de/ | >>>> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | >>>> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | >>>> >>>> >>> >> >> -- >> Pengutronix e.K. | | >> Steuerwalder Str. 21 | http://www.pengutronix.de/ | >> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | >> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | >> >> > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH master] optee: don't warn about missing OP-TEE header 2024-02-28 18:03 [PATCH master] optee: don't warn about missing OP-TEE header Ahmad Fatoum 2024-02-29 9:10 ` Marco Felsch @ 2024-03-01 9:21 ` Sascha Hauer 1 sibling, 0 replies; 8+ messages in thread From: Sascha Hauer @ 2024-03-01 9:21 UTC (permalink / raw) To: barebox, Ahmad Fatoum; +Cc: mfe On Wed, 28 Feb 2024 19:03:20 +0100, Ahmad Fatoum wrote: > OP-TEE header is checked once in PBL, saved into scratch area after > verification and then checked again in barebox proper. > > The check in PBL fails silently, but the check in barebox proper that > should always follow, because the header isn't written to the scratch > area is printed with error log level. > > [...] Applied, thanks! [1/1] optee: don't warn about missing OP-TEE header https://git.pengutronix.de/cgit/barebox/commit/?id=d69c84346b30 (link may not be stable) Best regards, -- Sascha Hauer <s.hauer@pengutronix.de> ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-03-01 9:21 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-02-28 18:03 [PATCH master] optee: don't warn about missing OP-TEE header Ahmad Fatoum 2024-02-29 9:10 ` Marco Felsch 2024-02-29 9:17 ` Ahmad Fatoum 2024-02-29 9:33 ` Marco Felsch 2024-02-29 9:46 ` Ahmad Fatoum 2024-02-29 11:13 ` Marco Felsch 2024-02-29 11:46 ` Ahmad Fatoum 2024-03-01 9:21 ` Sascha Hauer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox