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

[Ivan Khoronzhuk <ivan.khoronzhuk@gmail.com>]

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