[PATCH master] optee: don't warn about missing OP-TEE header

[PATCH master] optee: don't warn about missing OP-TEE header

[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

Re: [PATCH master] optee: don't warn about missing OP-TEE header

[Marco Felsch]

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
> 
> 

Re: [PATCH master] optee: don't warn about missing OP-TEE header

[Ahmad Fatoum]

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 |

Re: [PATCH master] optee: don't warn about missing OP-TEE header

[Marco Felsch]

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 |
> 
> 

Re: [PATCH master] optee: don't warn about missing OP-TEE header

[Ahmad Fatoum]

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 |

Re: [PATCH master] optee: don't warn about missing OP-TEE header

[Marco Felsch]

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 |
> 
> 

Re: [PATCH master] optee: don't warn about missing OP-TEE header

[Ahmad Fatoum]

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 |

Re: [PATCH master] optee: don't warn about missing OP-TEE header

[Sascha Hauer]

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>