Re: [PATCH v2] common: avb_verify: prevent opening incorrect session
On Mon, Jan 23, 2023 at 5:09 PM Ivan Khoronzhuk
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
> >> ---
> >> 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(, 0, sizeof(arg));
> >> tee_optee_ta_uuid_to_octets(arg.uuid, );
> >> rc = tee_open_session(tee, , 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
Re: [PATCH v2] common: avb_verify: prevent opening incorrect session
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
---
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(, 0, sizeof(arg));
tee_optee_ta_uuid_to_octets(arg.uuid, );
rc = tee_open_session(tee, , 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
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
> ---
> 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(, 0, sizeof(arg));
> tee_optee_ta_uuid_to_octets(arg.uuid, );
> rc = tee_open_session(tee, , 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
[PATCH v2] common: avb_verify: prevent opening incorrect session
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
---
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(, 0, sizeof(arg));
tee_optee_ta_uuid_to_octets(arg.uuid, );
rc = tee_open_session(tee, , 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
