[PATCH] 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 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;
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
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;
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
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.
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
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; }
Ignore this version please, will send v4. Skipped previous comment unintentionally.
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;
Any comments to this patch?
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
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!
participants (3)
-
Ivan Khoronzhuk -
Jens Wiklander -
Tom Rini