* [PATCH] common: avb_verify: prevent opening incorrect session
@ 2023-01-22 2:41 Ivan Khoronzhuk
2023-01-23 9:55 ` Jens Wiklander
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Ivan Khoronzhuk @ 2023-01-22 2:41 UTC (permalink / raw)
To: igor.opaniuk, u-boot; +Cc: Jens Wiklander, 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] common: avb_verify: prevent opening incorrect session
2023-01-22 2:41 [PATCH] common: avb_verify: prevent opening incorrect session Ivan Khoronzhuk
@ 2023-01-23 9:55 ` Jens Wiklander
2023-01-23 14:51 ` [PATCH v2] " Ivan Khoronzhuk
` (2 subsequent siblings)
3 siblings, 0 replies; 12+ messages in thread
From: Jens Wiklander @ 2023-01-23 9:55 UTC (permalink / raw)
To: Ivan Khoronzhuk; +Cc: igor.opaniuk, u-boot
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] common: avb_verify: prevent opening incorrect session
2023-01-22 2:41 [PATCH] common: avb_verify: prevent opening incorrect session Ivan Khoronzhuk
2023-01-23 9:55 ` Jens Wiklander
@ 2023-01-23 14:51 ` Ivan Khoronzhuk
2023-01-23 15:34 ` Jens Wiklander
2023-01-27 18:42 ` [PATCH v3] " Ivan Khoronzhuk
2023-01-27 20:02 ` [PATCH v4] " Ivan Khoronzhuk
3 siblings, 1 reply; 12+ messages in thread
From: Ivan Khoronzhuk @ 2023-01-23 14:51 UTC (permalink / raw)
To: igor.opaniuk, u-boot; +Cc: Jens Wiklander, 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] common: avb_verify: prevent opening incorrect session
2023-01-23 14:51 ` [PATCH v2] " Ivan Khoronzhuk
@ 2023-01-23 15:34 ` Jens Wiklander
2023-01-23 16:09 ` Ivan Khoronzhuk
0 siblings, 1 reply; 12+ messages in thread
From: Jens Wiklander @ 2023-01-23 15:34 UTC (permalink / raw)
To: Ivan Khoronzhuk; +Cc: igor.opaniuk, u-boot
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] common: avb_verify: prevent opening incorrect session
2023-01-23 15:34 ` Jens Wiklander
@ 2023-01-23 16:09 ` Ivan Khoronzhuk
2023-01-24 10:15 ` Jens Wiklander
0 siblings, 1 reply; 12+ messages in thread
From: Ivan Khoronzhuk @ 2023-01-23 16:09 UTC (permalink / raw)
To: Jens Wiklander; +Cc: igor.opaniuk, u-boot
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] common: avb_verify: prevent opening incorrect session
2023-01-23 16:09 ` Ivan Khoronzhuk
@ 2023-01-24 10:15 ` Jens Wiklander
0 siblings, 0 replies; 12+ messages in thread
From: Jens Wiklander @ 2023-01-24 10:15 UTC (permalink / raw)
To: Jens Wiklander, igor.opaniuk, u-boot
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3] common: avb_verify: prevent opening incorrect session
2023-01-22 2:41 [PATCH] common: avb_verify: prevent opening incorrect session Ivan Khoronzhuk
2023-01-23 9:55 ` Jens Wiklander
2023-01-23 14:51 ` [PATCH v2] " Ivan Khoronzhuk
@ 2023-01-27 18:42 ` Ivan Khoronzhuk
2023-01-27 19:45 ` Ivan Khoronzhuk
2023-01-27 20:02 ` [PATCH v4] " Ivan Khoronzhuk
3 siblings, 1 reply; 12+ messages in thread
From: Ivan Khoronzhuk @ 2023-01-27 18:42 UTC (permalink / raw)
To: igor.opaniuk, u-boot; +Cc: Jens Wiklander, 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3] common: avb_verify: prevent opening incorrect session
2023-01-27 18:42 ` [PATCH v3] " Ivan Khoronzhuk
@ 2023-01-27 19:45 ` Ivan Khoronzhuk
0 siblings, 0 replies; 12+ messages in thread
From: Ivan Khoronzhuk @ 2023-01-27 19:45 UTC (permalink / raw)
To: igor.opaniuk, u-boot; +Cc: Jens Wiklander, Ivan Khoronzhuk
Ignore this version please, will send v4.
Skipped previous comment unintentionally.
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v4] common: avb_verify: prevent opening incorrect session
2023-01-22 2:41 [PATCH] common: avb_verify: prevent opening incorrect session Ivan Khoronzhuk
` (2 preceding siblings ...)
2023-01-27 18:42 ` [PATCH v3] " Ivan Khoronzhuk
@ 2023-01-27 20:02 ` Ivan Khoronzhuk
2023-02-02 20:00 ` Ivan Khoronzhuk
` (2 more replies)
3 siblings, 3 replies; 12+ messages in thread
From: Ivan Khoronzhuk @ 2023-01-27 20:02 UTC (permalink / raw)
To: igor.opaniuk, u-boot; +Cc: Jens Wiklander, 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v4] common: avb_verify: prevent opening incorrect session
2023-01-27 20:02 ` [PATCH v4] " Ivan Khoronzhuk
@ 2023-02-02 20:00 ` Ivan Khoronzhuk
2023-02-03 10:59 ` Jens Wiklander
2023-02-17 19:19 ` Tom Rini
2 siblings, 0 replies; 12+ messages in thread
From: Ivan Khoronzhuk @ 2023-02-02 20:00 UTC (permalink / raw)
To: igor.opaniuk, u-boot; +Cc: Jens Wiklander, Ivan Khoronzhuk
Any comments to this patch?
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4] common: avb_verify: prevent opening incorrect session
2023-01-27 20:02 ` [PATCH v4] " Ivan Khoronzhuk
2023-02-02 20:00 ` Ivan Khoronzhuk
@ 2023-02-03 10:59 ` Jens Wiklander
2023-02-17 19:19 ` Tom Rini
2 siblings, 0 replies; 12+ messages in thread
From: Jens Wiklander @ 2023-02-03 10:59 UTC (permalink / raw)
To: Ivan Khoronzhuk; +Cc: igor.opaniuk, u-boot, Ivan Khoronzhuk
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4] common: avb_verify: prevent opening incorrect session
2023-01-27 20:02 ` [PATCH v4] " Ivan Khoronzhuk
2023-02-02 20:00 ` Ivan Khoronzhuk
2023-02-03 10:59 ` Jens Wiklander
@ 2023-02-17 19:19 ` Tom Rini
2 siblings, 0 replies; 12+ messages in thread
From: Tom Rini @ 2023-02-17 19:19 UTC (permalink / raw)
To: Ivan Khoronzhuk; +Cc: igor.opaniuk, u-boot, Jens Wiklander, Ivan Khoronzhuk
[-- 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 --]
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-02-17 19:19 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-22 2:41 [PATCH] common: avb_verify: prevent opening incorrect session Ivan Khoronzhuk
2023-01-23 9:55 ` Jens Wiklander
2023-01-23 14:51 ` [PATCH v2] " Ivan Khoronzhuk
2023-01-23 15:34 ` Jens Wiklander
2023-01-23 16:09 ` Ivan Khoronzhuk
2023-01-24 10:15 ` Jens Wiklander
2023-01-27 18:42 ` [PATCH v3] " Ivan Khoronzhuk
2023-01-27 19:45 ` Ivan Khoronzhuk
2023-01-27 20:02 ` [PATCH v4] " Ivan Khoronzhuk
2023-02-02 20:00 ` Ivan Khoronzhuk
2023-02-03 10:59 ` Jens Wiklander
2023-02-17 19:19 ` Tom Rini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.