[PATCH] common: avb_verify: prevent opening incorrect session

[PATCH] common: avb_verify: prevent opening incorrect session

[Ivan Khoronzhuk]

The arg->session is not valid if arg->ret != NULL, so can't be
assigned. Leave retry for just "ret" error to save same behaviour.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@gmail.com>
---
 common/avb_verify.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/common/avb_verify.c b/common/avb_verify.c
index 0520a71455..05d5a97896 100644
--- a/common/avb_verify.c
+++ b/common/avb_verify.c
@@ -619,10 +619,14 @@ static int get_open_session(struct AvbOpsData *ops_data)
 		memset(&arg, 0, sizeof(arg));
 		tee_optee_ta_uuid_to_octets(arg.uuid, &uuid);
 		rc = tee_open_session(tee, &arg, 0, NULL);
-		if (!rc) {
-			ops_data->tee = tee;
-			ops_data->session = arg.session;
-		}
+		if (rc)
+			continue;
+
+		if (arg.ret)
+			return AVB_IO_RESULT_ERROR_IO;
+
+		ops_data->tee = tee;
+		ops_data->session = arg.session;
 	}
 
 	return 0;
-- 
2.34.1

Re: [PATCH] common: avb_verify: prevent opening incorrect session

[Jens Wiklander]

On Sun, Jan 22, 2023 at 3:41 AM Ivan Khoronzhuk
<ivan.khoronzhuk@gmail.com> wrote:
>
> The arg->session is not valid if arg->ret != NULL, so can't be
> assigned. Leave retry for just "ret" error to save same behaviour.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@gmail.com>
> ---
>  common/avb_verify.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/common/avb_verify.c b/common/avb_verify.c
> index 0520a71455..05d5a97896 100644
> --- a/common/avb_verify.c
> +++ b/common/avb_verify.c
> @@ -619,10 +619,14 @@ static int get_open_session(struct AvbOpsData *ops_data)
>                 memset(&arg, 0, sizeof(arg));
>                 tee_optee_ta_uuid_to_octets(arg.uuid, &uuid);
>                 rc = tee_open_session(tee, &arg, 0, NULL);
> -               if (!rc) {
> -                       ops_data->tee = tee;
> -                       ops_data->session = arg.session;
> -               }
> +               if (rc)
> +                       continue;
> +
> +               if (arg.ret)
> +                       return AVB_IO_RESULT_ERROR_IO;

This should probably be an errno define instead since that's what is used above.

Cheers,
Jens

> +
> +               ops_data->tee = tee;
> +               ops_data->session = arg.session;
>         }
>
>         return 0;
> --
> 2.34.1
>

[PATCH v2] common: avb_verify: prevent opening incorrect session

[Ivan Khoronzhuk]

The arg->session is not valid if arg->ret != NULL, so can't be
assigned. Leave retry for just "ret" error to save same behaviour.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@gmail.com>
---
 common/avb_verify.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/common/avb_verify.c b/common/avb_verify.c
index 0520a71455..97451592f5 100644
--- a/common/avb_verify.c
+++ b/common/avb_verify.c
@@ -619,10 +619,14 @@ static int get_open_session(struct AvbOpsData *ops_data)
 		memset(&arg, 0, sizeof(arg));
 		tee_optee_ta_uuid_to_octets(arg.uuid, &uuid);
 		rc = tee_open_session(tee, &arg, 0, NULL);
-		if (!rc) {
-			ops_data->tee = tee;
-			ops_data->session = arg.session;
-		}
+		if (rc)
+			continue;
+
+		if (arg.ret)
+			return -EIO;
+
+		ops_data->tee = tee;
+		ops_data->session = arg.session;
 	}
 
 	return 0;
-- 
2.34.1

Re: [PATCH v2] common: avb_verify: prevent opening incorrect session

[Jens Wiklander]

On Mon, Jan 23, 2023 at 04:51:29PM +0200, Ivan Khoronzhuk wrote:
> The arg->session is not valid if arg->ret != NULL, so can't be
> assigned. Leave retry for just "ret" error to save same behaviour.
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@gmail.com>
> ---
>  common/avb_verify.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/common/avb_verify.c b/common/avb_verify.c
> index 0520a71455..97451592f5 100644
> --- a/common/avb_verify.c
> +++ b/common/avb_verify.c
> @@ -619,10 +619,14 @@ static int get_open_session(struct AvbOpsData *ops_data)
>  		memset(&arg, 0, sizeof(arg));
>  		tee_optee_ta_uuid_to_octets(arg.uuid, &uuid);
>  		rc = tee_open_session(tee, &arg, 0, NULL);
> -		if (!rc) {
> -			ops_data->tee = tee;
> -			ops_data->session = arg.session;
> -		}
> +		if (rc)
> +			continue;
> +
> +		if (arg.ret)
> +			return -EIO;
> +
> +		ops_data->tee = tee;
> +		ops_data->session = arg.session;
>  	}
>  
>  	return 0;

It looks like this function is still slightly broken. The function
should, if I understand it correctly, return usable tee and session
pointers on success, else return an error code. The unconditional return
0 at the end doesn't seem right.

Thanks,
Jens

Re: [PATCH v2] common: avb_verify: prevent opening incorrect session

[Ivan Khoronzhuk]

On Mon, Jan 23, 2023 at 04:34:33PM +0100, Jens Wiklander wrote:
>On Mon, Jan 23, 2023 at 04:51:29PM +0200, Ivan Khoronzhuk wrote:
>> The arg->session is not valid if arg->ret != NULL, so can't be
>> assigned. Leave retry for just "ret" error to save same behaviour.
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@gmail.com>
>> ---
>>  common/avb_verify.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/common/avb_verify.c b/common/avb_verify.c
>> index 0520a71455..97451592f5 100644
>> --- a/common/avb_verify.c
>> +++ b/common/avb_verify.c
>> @@ -619,10 +619,14 @@ static int get_open_session(struct AvbOpsData *ops_data)
>>  		memset(&arg, 0, sizeof(arg));
>>  		tee_optee_ta_uuid_to_octets(arg.uuid, &uuid);
>>  		rc = tee_open_session(tee, &arg, 0, NULL);
>> -		if (!rc) {
>> -			ops_data->tee = tee;
>> -			ops_data->session = arg.session;
>> -		}
>> +		if (rc)
>> +			continue;
>> +
>> +		if (arg.ret)
>> +			return -EIO;
>> +
>> +		ops_data->tee = tee;
>> +		ops_data->session = arg.session;
>>  	}
>>
>>  	return 0;
>
>It looks like this function is still slightly broken. The function
>should, if I understand it correctly, return usable tee and session
>pointers on success, else return an error code. The unconditional return
>0 at the end doesn't seem right.
>
>Thanks,
>Jens

It doesn't return, it loops infinitely...
Yes, it looks so, but it's how it works. I don't see a reason why the
function must loop trying to open the session that potentially never will be
opened. But this is how it's implemented and I didn't wont to change this
behaviour that can have some "sacral" roots, only add a fix I bother, I've
mentioned it in the comment. Better would be drop this loop ofc and add the
following:

+		if (ret || arg.ret)
+			return -EIO;

I can do this in v3 if you don't mind.

-- 
Regards,
Ivan Khoronzhuk

Re: [PATCH v2] common: avb_verify: prevent opening incorrect session

[Jens Wiklander]

On Mon, Jan 23, 2023 at 5:09 PM Ivan Khoronzhuk
<ivan.khoronzhuk@gmail.com> wrote:
>
> On Mon, Jan 23, 2023 at 04:34:33PM +0100, Jens Wiklander wrote:
> >On Mon, Jan 23, 2023 at 04:51:29PM +0200, Ivan Khoronzhuk wrote:
> >> The arg->session is not valid if arg->ret != NULL, so can't be
> >> assigned. Leave retry for just "ret" error to save same behaviour.
> >>
> >> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@gmail.com>
> >> ---
> >>  common/avb_verify.c | 12 ++++++++----
> >>  1 file changed, 8 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/common/avb_verify.c b/common/avb_verify.c
> >> index 0520a71455..97451592f5 100644
> >> --- a/common/avb_verify.c
> >> +++ b/common/avb_verify.c
> >> @@ -619,10 +619,14 @@ static int get_open_session(struct AvbOpsData *ops_data)
> >>              memset(&arg, 0, sizeof(arg));
> >>              tee_optee_ta_uuid_to_octets(arg.uuid, &uuid);
> >>              rc = tee_open_session(tee, &arg, 0, NULL);
> >> -            if (!rc) {
> >> -                    ops_data->tee = tee;
> >> -                    ops_data->session = arg.session;
> >> -            }
> >> +            if (rc)
> >> +                    continue;
> >> +
> >> +            if (arg.ret)
> >> +                    return -EIO;
> >> +
> >> +            ops_data->tee = tee;
> >> +            ops_data->session = arg.session;
> >>      }
> >>
> >>      return 0;
> >
> >It looks like this function is still slightly broken. The function
> >should, if I understand it correctly, return usable tee and session
> >pointers on success, else return an error code. The unconditional return
> >0 at the end doesn't seem right.
> >
> >Thanks,
> >Jens
>
> It doesn't return, it loops infinitely...
> Yes, it looks so, but it's how it works. I don't see a reason why the
> function must loop trying to open the session that potentially never will be
> opened. But this is how it's implemented and I didn't wont to change this
> behaviour that can have some "sacral" roots, only add a fix I bother, I've

No worries, no need to be bug compatible here. :-)

> mentioned it in the comment. Better would be drop this loop ofc and add the
> following:
>
> +               if (ret || arg.ret)
> +                       return -EIO;

A loop is needed to find the TEE device. Right now I guess there's
only one and it will be found in the first round.

>
> I can do this in v3 if you don't mind.

Yes, please fix the function to return an error if a session can't be found.

Thanks,
Jens

[PATCH v3] common: avb_verify: prevent opening incorrect session

[Ivan Khoronzhuk]

The arg->session is not valid if arg->ret != NULL, so can't be
assigned, correct this. Also remove "while" loop as there is no
reason for looping till correct session is opened.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
---
 common/avb_verify.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/common/avb_verify.c b/common/avb_verify.c
index 0520a71455..c3cccd986d 100644
--- a/common/avb_verify.c
+++ b/common/avb_verify.c
@@ -605,26 +605,26 @@ static AvbIOResult validate_vbmeta_public_key(AvbOps *ops,
 #ifdef CONFIG_OPTEE_TA_AVB
 static int get_open_session(struct AvbOpsData *ops_data)
 {
-	struct udevice *tee = NULL;
-
-	while (!ops_data->tee) {
-		const struct tee_optee_ta_uuid uuid = TA_AVB_UUID;
-		struct tee_open_session_arg arg;
-		int rc;
-
-		tee = tee_find_device(tee, NULL, NULL, NULL);
-		if (!tee)
-			return -ENODEV;
-
-		memset(&arg, 0, sizeof(arg));
-		tee_optee_ta_uuid_to_octets(arg.uuid, &uuid);
-		rc = tee_open_session(tee, &arg, 0, NULL);
-		if (!rc) {
-			ops_data->tee = tee;
-			ops_data->session = arg.session;
-		}
-	}
+	const struct tee_optee_ta_uuid uuid = TA_AVB_UUID;
+	struct tee_open_session_arg arg;
+	struct udevice *tee;
+	int rc;
+
+	if (ops_data->tee)
+		return 0;
+
+	tee = tee_find_device(NULL, NULL, NULL, NULL);
+	if (!tee)
+		return -ENODEV;
+
+	memset(&arg, 0, sizeof(arg));
+	tee_optee_ta_uuid_to_octets(arg.uuid, &uuid);
+	rc = tee_open_session(tee, &arg, 0, NULL);
+	if (rc || arg.ret)
+		return -EIO;
 
+	ops_data->tee = tee;
+	ops_data->session = arg.session;
 	return 0;
 }
 
-- 
2.34.1

Re: [PATCH v3] common: avb_verify: prevent opening incorrect session

[Ivan Khoronzhuk]

Ignore this version please, will send v4.
Skipped previous comment unintentionally.

-- 
Regards,
Ivan Khoronzhuk

[PATCH v4] common: avb_verify: prevent opening incorrect session

[Ivan Khoronzhuk]

The arg->session is not valid if arg->ret != NULL, so can't be
assigned, correct this.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
---
 common/avb_verify.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/common/avb_verify.c b/common/avb_verify.c
index 0520a71455..48ba8db51e 100644
--- a/common/avb_verify.c
+++ b/common/avb_verify.c
@@ -619,10 +619,11 @@ static int get_open_session(struct AvbOpsData *ops_data)
 		memset(&arg, 0, sizeof(arg));
 		tee_optee_ta_uuid_to_octets(arg.uuid, &uuid);
 		rc = tee_open_session(tee, &arg, 0, NULL);
-		if (!rc) {
-			ops_data->tee = tee;
-			ops_data->session = arg.session;
-		}
+		if (rc || arg.ret)
+			continue;
+
+		ops_data->tee = tee;
+		ops_data->session = arg.session;
 	}
 
 	return 0;
-- 
2.34.1

Re: [PATCH v4] common: avb_verify: prevent opening incorrect session

[Ivan Khoronzhuk]

Any comments to this patch?

-- 
Regards,
Ivan Khoronzhuk

Re: [PATCH v4] common: avb_verify: prevent opening incorrect session

[Jens Wiklander]

On Fri, Jan 27, 2023 at 9:02 PM Ivan Khoronzhuk
<ivan.khoronzhuk@gmail.com> wrote:
>
> The arg->session is not valid if arg->ret != NULL, so can't be
> assigned, correct this.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
> ---
>  common/avb_verify.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Thanks,
Jens

>
> diff --git a/common/avb_verify.c b/common/avb_verify.c
> index 0520a71455..48ba8db51e 100644
> --- a/common/avb_verify.c
> +++ b/common/avb_verify.c
> @@ -619,10 +619,11 @@ static int get_open_session(struct AvbOpsData *ops_data)
>                 memset(&arg, 0, sizeof(arg));
>                 tee_optee_ta_uuid_to_octets(arg.uuid, &uuid);
>                 rc = tee_open_session(tee, &arg, 0, NULL);
> -               if (!rc) {
> -                       ops_data->tee = tee;
> -                       ops_data->session = arg.session;
> -               }
> +               if (rc || arg.ret)
> +                       continue;
> +
> +               ops_data->tee = tee;
> +               ops_data->session = arg.session;
>         }
>
>         return 0;
> --
> 2.34.1
>

Re: [PATCH v4] common: avb_verify: prevent opening incorrect session

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 341 bytes --]

On Fri, Jan 27, 2023 at 10:02:14PM +0200, Ivan Khoronzhuk wrote:

> The arg->session is not valid if arg->ret != NULL, so can't be
> assigned, correct this.
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@globallogic.com>
> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]