diff mbox series

[v3,1/2] tee: optee: Fix supplicant based device enumeration

Message ID 20231030155901.80673-2-sumit.garg@linaro.org
State New
Headers
Series tee: optee: Fixes for supplicant dependent enumeration |

Commit Message

Sumit Garg Oct. 30, 2023, 3:59 p.m. UTC 
  Currently supplicant dependent optee device enumeration only registers
devices whenever tee-supplicant is invoked for the first time. But it
forgets to remove devices when tee-supplicant daemon stops running and
closes its context gracefully. This leads to following error for fTPM
driver during reboot/shutdown:

[   73.466791] tpm tpm0: ftpm_tee_tpm_op_send: SUBMIT_COMMAND invoke error: 0xffff3024

Fix this by separating supplicant dependent devices so that the
user-space service can detach supplicant devices before closing the
supplicant.

Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
Link: https://github.com/OP-TEE/optee_os/issues/6094
Fixes: 5f178bb71e3a ("optee: enable support for multi-stage bus enumeration")
Tested-by: Jan Kiszka <jan.kiszka@siemens.com>
Tested-by: Masahisa Kojima <masahisa.kojima@linaro.org>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 drivers/tee/optee/device.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

Comments

Jerome Forissier Oct. 31, 2023, 11:04 a.m. UTC | #1 
On 10/30/23 16:59, Sumit Garg wrote:
> Currently supplicant dependent optee device enumeration only registers
> devices whenever tee-supplicant is invoked for the first time. But it
> forgets to remove devices when tee-supplicant daemon stops running and
> closes its context gracefully. This leads to following error for fTPM
> driver during reboot/shutdown:
> 
> [   73.466791] tpm tpm0: ftpm_tee_tpm_op_send: SUBMIT_COMMAND invoke error: 0xffff3024
> 
> Fix this by separating supplicant dependent devices so that the
> user-space service can detach supplicant devices before closing the
> supplicant.
> 
> Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
> Link: https://github.com/OP-TEE/optee_os/issues/6094
> Fixes: 5f178bb71e3a ("optee: enable support for multi-stage bus enumeration")
> Tested-by: Jan Kiszka <jan.kiszka@siemens.com>
> Tested-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> ---
>  drivers/tee/optee/device.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
> index 64f0e047c23d..78fc0a15c463 100644
> --- a/drivers/tee/optee/device.c
> +++ b/drivers/tee/optee/device.c
> @@ -60,9 +60,10 @@ static void optee_release_device(struct device *dev)
>  	kfree(optee_device);
>  }
>  
> -static int optee_register_device(const uuid_t *device_uuid)
> +static int optee_register_device(const uuid_t *device_uuid, u32 func)
>  {
>  	struct tee_client_device *optee_device = NULL;
> +	const char *dev_name_fmt = NULL;
>  	int rc;
>  
>  	optee_device = kzalloc(sizeof(*optee_device), GFP_KERNEL);
> @@ -71,7 +72,13 @@ static int optee_register_device(const uuid_t *device_uuid)
>  
>  	optee_device->dev.bus = &tee_bus_type;
>  	optee_device->dev.release = optee_release_device;
> -	if (dev_set_name(&optee_device->dev, "optee-ta-%pUb", device_uuid)) {
> +
> +	if (func == PTA_CMD_GET_DEVICES_SUPP)
> +		dev_name_fmt = "optee-ta-supp-%pUb";
> +	else
> +		dev_name_fmt = "optee-ta-%pUb";

That's an ABI change, isn't it?

> +
> +	if (dev_set_name(&optee_device->dev, dev_name_fmt, device_uuid)) {
>  		kfree(optee_device);
>  		return -ENOMEM;
>  	}
> @@ -142,7 +149,7 @@ static int __optee_enumerate_devices(u32 func)
>  	num_devices = shm_size / sizeof(uuid_t);
>  
>  	for (idx = 0; idx < num_devices; idx++) {
> -		rc = optee_register_device(&device_uuid[idx]);
> +		rc = optee_register_device(&device_uuid[idx], func);
>  		if (rc)
>  			goto out_shm;
>  	}
Jan Kiszka Oct. 31, 2023, 11:44 a.m. UTC | #2 
On 31.10.23 12:04, Jerome Forissier wrote:
> 
> 
> On 10/30/23 16:59, Sumit Garg wrote:
>> Currently supplicant dependent optee device enumeration only registers
>> devices whenever tee-supplicant is invoked for the first time. But it
>> forgets to remove devices when tee-supplicant daemon stops running and
>> closes its context gracefully. This leads to following error for fTPM
>> driver during reboot/shutdown:
>>
>> [   73.466791] tpm tpm0: ftpm_tee_tpm_op_send: SUBMIT_COMMAND invoke error: 0xffff3024
>>
>> Fix this by separating supplicant dependent devices so that the
>> user-space service can detach supplicant devices before closing the
>> supplicant.
>>
>> Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
>> Link: https://github.com/OP-TEE/optee_os/issues/6094
>> Fixes: 5f178bb71e3a ("optee: enable support for multi-stage bus enumeration")
>> Tested-by: Jan Kiszka <jan.kiszka@siemens.com>
>> Tested-by: Masahisa Kojima <masahisa.kojima@linaro.org>
>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>> ---
>>  drivers/tee/optee/device.c | 13 ++++++++++---
>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
>> index 64f0e047c23d..78fc0a15c463 100644
>> --- a/drivers/tee/optee/device.c
>> +++ b/drivers/tee/optee/device.c
>> @@ -60,9 +60,10 @@ static void optee_release_device(struct device *dev)
>>  	kfree(optee_device);
>>  }
>>  
>> -static int optee_register_device(const uuid_t *device_uuid)
>> +static int optee_register_device(const uuid_t *device_uuid, u32 func)
>>  {
>>  	struct tee_client_device *optee_device = NULL;
>> +	const char *dev_name_fmt = NULL;
>>  	int rc;
>>  
>>  	optee_device = kzalloc(sizeof(*optee_device), GFP_KERNEL);
>> @@ -71,7 +72,13 @@ static int optee_register_device(const uuid_t *device_uuid)
>>  
>>  	optee_device->dev.bus = &tee_bus_type;
>>  	optee_device->dev.release = optee_release_device;
>> -	if (dev_set_name(&optee_device->dev, "optee-ta-%pUb", device_uuid)) {
>> +
>> +	if (func == PTA_CMD_GET_DEVICES_SUPP)
>> +		dev_name_fmt = "optee-ta-supp-%pUb";
>> +	else
>> +		dev_name_fmt = "optee-ta-%pUb";
> 
> That's an ABI change, isn't it?

Oh, here did this come from! Yes, I recently had to adjust some systemd
service due to carrying this patch but looking for the change only in 
upstream:

https://github.com/ilbers/isar/commit/83644ddf694e51f11793e6107e4aaf68dc0043a5

Jan

> 
>> +
>> +	if (dev_set_name(&optee_device->dev, dev_name_fmt, device_uuid)) {
>>  		kfree(optee_device);
>>  		return -ENOMEM;
>>  	}
>> @@ -142,7 +149,7 @@ static int __optee_enumerate_devices(u32 func)
>>  	num_devices = shm_size / sizeof(uuid_t);
>>  
>>  	for (idx = 0; idx < num_devices; idx++) {
>> -		rc = optee_register_device(&device_uuid[idx]);
>> +		rc = optee_register_device(&device_uuid[idx], func);
>>  		if (rc)
>>  			goto out_shm;
>>  	}
>
Sumit Garg Nov. 2, 2023, 7:37 a.m. UTC | #3 
On Tue, 31 Oct 2023 at 17:14, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> On 31.10.23 12:04, Jerome Forissier wrote:
> >
> >
> > On 10/30/23 16:59, Sumit Garg wrote:
> >> Currently supplicant dependent optee device enumeration only registers
> >> devices whenever tee-supplicant is invoked for the first time. But it
> >> forgets to remove devices when tee-supplicant daemon stops running and
> >> closes its context gracefully. This leads to following error for fTPM
> >> driver during reboot/shutdown:
> >>
> >> [   73.466791] tpm tpm0: ftpm_tee_tpm_op_send: SUBMIT_COMMAND invoke error: 0xffff3024
> >>
> >> Fix this by separating supplicant dependent devices so that the
> >> user-space service can detach supplicant devices before closing the
> >> supplicant.
> >>
> >> Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> Link: https://github.com/OP-TEE/optee_os/issues/6094
> >> Fixes: 5f178bb71e3a ("optee: enable support for multi-stage bus enumeration")
> >> Tested-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> Tested-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> >> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> >> ---
> >>  drivers/tee/optee/device.c | 13 ++++++++++---
> >>  1 file changed, 10 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
> >> index 64f0e047c23d..78fc0a15c463 100644
> >> --- a/drivers/tee/optee/device.c
> >> +++ b/drivers/tee/optee/device.c
> >> @@ -60,9 +60,10 @@ static void optee_release_device(struct device *dev)
> >>      kfree(optee_device);
> >>  }
> >>
> >> -static int optee_register_device(const uuid_t *device_uuid)
> >> +static int optee_register_device(const uuid_t *device_uuid, u32 func)
> >>  {
> >>      struct tee_client_device *optee_device = NULL;
> >> +    const char *dev_name_fmt = NULL;
> >>      int rc;
> >>
> >>      optee_device = kzalloc(sizeof(*optee_device), GFP_KERNEL);
> >> @@ -71,7 +72,13 @@ static int optee_register_device(const uuid_t *device_uuid)
> >>
> >>      optee_device->dev.bus = &tee_bus_type;
> >>      optee_device->dev.release = optee_release_device;
> >> -    if (dev_set_name(&optee_device->dev, "optee-ta-%pUb", device_uuid)) {
> >> +
> >> +    if (func == PTA_CMD_GET_DEVICES_SUPP)
> >> +            dev_name_fmt = "optee-ta-supp-%pUb";
> >> +    else
> >> +            dev_name_fmt = "optee-ta-%pUb";
> >
> > That's an ABI change, isn't it?
>

Indeed it is an ABI break although we would like this to be backported
but don't want to break existing users. So I brainstormed on it and
came up with an alternative fix via device attribute in v4. Please
have a look.

> Oh, here did this come from! Yes, I recently had to adjust some systemd
> service due to carrying this patch but looking for the change only in
> upstream:
>
> https://github.com/ilbers/isar/commit/83644ddf694e51f11793e6107e4aaf68dc0043a5
>

You don't need to unbind all of the optee devices. v4 would help you
to maintain backwards compatibility, can you retest it?

-Sumit

> Jan
>
> >
> >> +
> >> +    if (dev_set_name(&optee_device->dev, dev_name_fmt, device_uuid)) {
> >>              kfree(optee_device);
> >>              return -ENOMEM;
> >>      }
> >> @@ -142,7 +149,7 @@ static int __optee_enumerate_devices(u32 func)
> >>      num_devices = shm_size / sizeof(uuid_t);
> >>
> >>      for (idx = 0; idx < num_devices; idx++) {
> >> -            rc = optee_register_device(&device_uuid[idx]);
> >> +            rc = optee_register_device(&device_uuid[idx], func);
> >>              if (rc)
> >>                      goto out_shm;
> >>      }
> >
>
> --
> Siemens AG, Technology
> Linux Expert Center
>
Jan Kiszka Nov. 2, 2023, 7:58 a.m. UTC | #4 
On 02.11.23 08:37, Sumit Garg wrote:
> On Tue, 31 Oct 2023 at 17:14, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>
>> On 31.10.23 12:04, Jerome Forissier wrote:
>>>
>>>
>>> On 10/30/23 16:59, Sumit Garg wrote:
>>>> Currently supplicant dependent optee device enumeration only registers
>>>> devices whenever tee-supplicant is invoked for the first time. But it
>>>> forgets to remove devices when tee-supplicant daemon stops running and
>>>> closes its context gracefully. This leads to following error for fTPM
>>>> driver during reboot/shutdown:
>>>>
>>>> [   73.466791] tpm tpm0: ftpm_tee_tpm_op_send: SUBMIT_COMMAND invoke error: 0xffff3024
>>>>
>>>> Fix this by separating supplicant dependent devices so that the
>>>> user-space service can detach supplicant devices before closing the
>>>> supplicant.
>>>>
>>>> Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> Link: https://github.com/OP-TEE/optee_os/issues/6094
>>>> Fixes: 5f178bb71e3a ("optee: enable support for multi-stage bus enumeration")
>>>> Tested-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> Tested-by: Masahisa Kojima <masahisa.kojima@linaro.org>
>>>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>>>> ---
>>>>  drivers/tee/optee/device.c | 13 ++++++++++---
>>>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
>>>> index 64f0e047c23d..78fc0a15c463 100644
>>>> --- a/drivers/tee/optee/device.c
>>>> +++ b/drivers/tee/optee/device.c
>>>> @@ -60,9 +60,10 @@ static void optee_release_device(struct device *dev)
>>>>      kfree(optee_device);
>>>>  }
>>>>
>>>> -static int optee_register_device(const uuid_t *device_uuid)
>>>> +static int optee_register_device(const uuid_t *device_uuid, u32 func)
>>>>  {
>>>>      struct tee_client_device *optee_device = NULL;
>>>> +    const char *dev_name_fmt = NULL;
>>>>      int rc;
>>>>
>>>>      optee_device = kzalloc(sizeof(*optee_device), GFP_KERNEL);
>>>> @@ -71,7 +72,13 @@ static int optee_register_device(const uuid_t *device_uuid)
>>>>
>>>>      optee_device->dev.bus = &tee_bus_type;
>>>>      optee_device->dev.release = optee_release_device;
>>>> -    if (dev_set_name(&optee_device->dev, "optee-ta-%pUb", device_uuid)) {
>>>> +
>>>> +    if (func == PTA_CMD_GET_DEVICES_SUPP)
>>>> +            dev_name_fmt = "optee-ta-supp-%pUb";
>>>> +    else
>>>> +            dev_name_fmt = "optee-ta-%pUb";
>>>
>>> That's an ABI change, isn't it?
>>
> 
> Indeed it is an ABI break although we would like this to be backported
> but don't want to break existing users. So I brainstormed on it and
> came up with an alternative fix via device attribute in v4. Please
> have a look.
> 
>> Oh, here did this come from! Yes, I recently had to adjust some systemd
>> service due to carrying this patch but looking for the change only in
>> upstream:
>>
>> https://github.com/ilbers/isar/commit/83644ddf694e51f11793e6107e4aaf68dc0043a5
>>
> 
> You don't need to unbind all of the optee devices. v4 would help you
> to maintain backwards compatibility, can you retest it?

How do I know from tee-supplicant perspective which devices I need to
unbind? There could be one in the future that will also use storage and
will therefore also fail once the supplicant is gone.

Jan
Sumit Garg Nov. 2, 2023, 8:02 a.m. UTC | #5 
On Thu, 2 Nov 2023 at 13:28, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> On 02.11.23 08:37, Sumit Garg wrote:
> > On Tue, 31 Oct 2023 at 17:14, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>
> >> On 31.10.23 12:04, Jerome Forissier wrote:
> >>>
> >>>
> >>> On 10/30/23 16:59, Sumit Garg wrote:
> >>>> Currently supplicant dependent optee device enumeration only registers
> >>>> devices whenever tee-supplicant is invoked for the first time. But it
> >>>> forgets to remove devices when tee-supplicant daemon stops running and
> >>>> closes its context gracefully. This leads to following error for fTPM
> >>>> driver during reboot/shutdown:
> >>>>
> >>>> [   73.466791] tpm tpm0: ftpm_tee_tpm_op_send: SUBMIT_COMMAND invoke error: 0xffff3024
> >>>>
> >>>> Fix this by separating supplicant dependent devices so that the
> >>>> user-space service can detach supplicant devices before closing the
> >>>> supplicant.
> >>>>
> >>>> Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>> Link: https://github.com/OP-TEE/optee_os/issues/6094
> >>>> Fixes: 5f178bb71e3a ("optee: enable support for multi-stage bus enumeration")
> >>>> Tested-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>> Tested-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> >>>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> >>>> ---
> >>>>  drivers/tee/optee/device.c | 13 ++++++++++---
> >>>>  1 file changed, 10 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
> >>>> index 64f0e047c23d..78fc0a15c463 100644
> >>>> --- a/drivers/tee/optee/device.c
> >>>> +++ b/drivers/tee/optee/device.c
> >>>> @@ -60,9 +60,10 @@ static void optee_release_device(struct device *dev)
> >>>>      kfree(optee_device);
> >>>>  }
> >>>>
> >>>> -static int optee_register_device(const uuid_t *device_uuid)
> >>>> +static int optee_register_device(const uuid_t *device_uuid, u32 func)
> >>>>  {
> >>>>      struct tee_client_device *optee_device = NULL;
> >>>> +    const char *dev_name_fmt = NULL;
> >>>>      int rc;
> >>>>
> >>>>      optee_device = kzalloc(sizeof(*optee_device), GFP_KERNEL);
> >>>> @@ -71,7 +72,13 @@ static int optee_register_device(const uuid_t *device_uuid)
> >>>>
> >>>>      optee_device->dev.bus = &tee_bus_type;
> >>>>      optee_device->dev.release = optee_release_device;
> >>>> -    if (dev_set_name(&optee_device->dev, "optee-ta-%pUb", device_uuid)) {
> >>>> +
> >>>> +    if (func == PTA_CMD_GET_DEVICES_SUPP)
> >>>> +            dev_name_fmt = "optee-ta-supp-%pUb";
> >>>> +    else
> >>>> +            dev_name_fmt = "optee-ta-%pUb";
> >>>
> >>> That's an ABI change, isn't it?
> >>
> >
> > Indeed it is an ABI break although we would like this to be backported
> > but don't want to break existing users. So I brainstormed on it and
> > came up with an alternative fix via device attribute in v4. Please
> > have a look.
> >
> >> Oh, here did this come from! Yes, I recently had to adjust some systemd
> >> service due to carrying this patch but looking for the change only in
> >> upstream:
> >>
> >> https://github.com/ilbers/isar/commit/83644ddf694e51f11793e6107e4aaf68dc0043a5
> >>
> >
> > You don't need to unbind all of the optee devices. v4 would help you
> > to maintain backwards compatibility, can you retest it?
>
> How do I know from tee-supplicant perspective which devices I need to
> unbind? There could be one in the future that will also use storage and
> will therefore also fail once the supplicant is gone.
>

With v4, the devices where the below attribute is present need to
unbind before closing tee-supplicant.

/sys/bus/tee/devices/optee-ta-<uuid>/need_supplicant

-Sumit

> Jan
>
> --
> Siemens AG, Technology
> Linux Expert Center
>
Jan Kiszka Nov. 2, 2023, 8:05 a.m. UTC | #6 
On 02.11.23 09:02, Sumit Garg wrote:
> On Thu, 2 Nov 2023 at 13:28, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>
>> On 02.11.23 08:37, Sumit Garg wrote:
>>> On Tue, 31 Oct 2023 at 17:14, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>
>>>> On 31.10.23 12:04, Jerome Forissier wrote:
>>>>>
>>>>>
>>>>> On 10/30/23 16:59, Sumit Garg wrote:
>>>>>> Currently supplicant dependent optee device enumeration only registers
>>>>>> devices whenever tee-supplicant is invoked for the first time. But it
>>>>>> forgets to remove devices when tee-supplicant daemon stops running and
>>>>>> closes its context gracefully. This leads to following error for fTPM
>>>>>> driver during reboot/shutdown:
>>>>>>
>>>>>> [   73.466791] tpm tpm0: ftpm_tee_tpm_op_send: SUBMIT_COMMAND invoke error: 0xffff3024
>>>>>>
>>>>>> Fix this by separating supplicant dependent devices so that the
>>>>>> user-space service can detach supplicant devices before closing the
>>>>>> supplicant.
>>>>>>
>>>>>> Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>> Link: https://github.com/OP-TEE/optee_os/issues/6094
>>>>>> Fixes: 5f178bb71e3a ("optee: enable support for multi-stage bus enumeration")
>>>>>> Tested-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>> Tested-by: Masahisa Kojima <masahisa.kojima@linaro.org>
>>>>>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>>>>>> ---
>>>>>>  drivers/tee/optee/device.c | 13 ++++++++++---
>>>>>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
>>>>>> index 64f0e047c23d..78fc0a15c463 100644
>>>>>> --- a/drivers/tee/optee/device.c
>>>>>> +++ b/drivers/tee/optee/device.c
>>>>>> @@ -60,9 +60,10 @@ static void optee_release_device(struct device *dev)
>>>>>>      kfree(optee_device);
>>>>>>  }
>>>>>>
>>>>>> -static int optee_register_device(const uuid_t *device_uuid)
>>>>>> +static int optee_register_device(const uuid_t *device_uuid, u32 func)
>>>>>>  {
>>>>>>      struct tee_client_device *optee_device = NULL;
>>>>>> +    const char *dev_name_fmt = NULL;
>>>>>>      int rc;
>>>>>>
>>>>>>      optee_device = kzalloc(sizeof(*optee_device), GFP_KERNEL);
>>>>>> @@ -71,7 +72,13 @@ static int optee_register_device(const uuid_t *device_uuid)
>>>>>>
>>>>>>      optee_device->dev.bus = &tee_bus_type;
>>>>>>      optee_device->dev.release = optee_release_device;
>>>>>> -    if (dev_set_name(&optee_device->dev, "optee-ta-%pUb", device_uuid)) {
>>>>>> +
>>>>>> +    if (func == PTA_CMD_GET_DEVICES_SUPP)
>>>>>> +            dev_name_fmt = "optee-ta-supp-%pUb";
>>>>>> +    else
>>>>>> +            dev_name_fmt = "optee-ta-%pUb";
>>>>>
>>>>> That's an ABI change, isn't it?
>>>>
>>>
>>> Indeed it is an ABI break although we would like this to be backported
>>> but don't want to break existing users. So I brainstormed on it and
>>> came up with an alternative fix via device attribute in v4. Please
>>> have a look.
>>>
>>>> Oh, here did this come from! Yes, I recently had to adjust some systemd
>>>> service due to carrying this patch but looking for the change only in
>>>> upstream:
>>>>
>>>> https://github.com/ilbers/isar/commit/83644ddf694e51f11793e6107e4aaf68dc0043a5
>>>>
>>>
>>> You don't need to unbind all of the optee devices. v4 would help you
>>> to maintain backwards compatibility, can you retest it?
>>
>> How do I know from tee-supplicant perspective which devices I need to
>> unbind? There could be one in the future that will also use storage and
>> will therefore also fail once the supplicant is gone.
>>
> 
> With v4, the devices where the below attribute is present need to
> unbind before closing tee-supplicant.
> 
> /sys/bus/tee/devices/optee-ta-<uuid>/need_supplicant

OK - but that will only help in future kernels, nothing we have today.
Thus, the shutdown script cannot assume to alone kill those devices
unless it find a certain upcoming kernel release.

Jan
Sumit Garg Nov. 2, 2023, 8:09 a.m. UTC | #7 
On Thu, 2 Nov 2023 at 13:35, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> On 02.11.23 09:02, Sumit Garg wrote:
> > On Thu, 2 Nov 2023 at 13:28, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>
> >> On 02.11.23 08:37, Sumit Garg wrote:
> >>> On Tue, 31 Oct 2023 at 17:14, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >>>>
> >>>> On 31.10.23 12:04, Jerome Forissier wrote:
> >>>>>
> >>>>>
> >>>>> On 10/30/23 16:59, Sumit Garg wrote:
> >>>>>> Currently supplicant dependent optee device enumeration only registers
> >>>>>> devices whenever tee-supplicant is invoked for the first time. But it
> >>>>>> forgets to remove devices when tee-supplicant daemon stops running and
> >>>>>> closes its context gracefully. This leads to following error for fTPM
> >>>>>> driver during reboot/shutdown:
> >>>>>>
> >>>>>> [   73.466791] tpm tpm0: ftpm_tee_tpm_op_send: SUBMIT_COMMAND invoke error: 0xffff3024
> >>>>>>
> >>>>>> Fix this by separating supplicant dependent devices so that the
> >>>>>> user-space service can detach supplicant devices before closing the
> >>>>>> supplicant.
> >>>>>>
> >>>>>> Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>> Link: https://github.com/OP-TEE/optee_os/issues/6094
> >>>>>> Fixes: 5f178bb71e3a ("optee: enable support for multi-stage bus enumeration")
> >>>>>> Tested-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>> Tested-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> >>>>>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> >>>>>> ---
> >>>>>>  drivers/tee/optee/device.c | 13 ++++++++++---
> >>>>>>  1 file changed, 10 insertions(+), 3 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
> >>>>>> index 64f0e047c23d..78fc0a15c463 100644
> >>>>>> --- a/drivers/tee/optee/device.c
> >>>>>> +++ b/drivers/tee/optee/device.c
> >>>>>> @@ -60,9 +60,10 @@ static void optee_release_device(struct device *dev)
> >>>>>>      kfree(optee_device);
> >>>>>>  }
> >>>>>>
> >>>>>> -static int optee_register_device(const uuid_t *device_uuid)
> >>>>>> +static int optee_register_device(const uuid_t *device_uuid, u32 func)
> >>>>>>  {
> >>>>>>      struct tee_client_device *optee_device = NULL;
> >>>>>> +    const char *dev_name_fmt = NULL;
> >>>>>>      int rc;
> >>>>>>
> >>>>>>      optee_device = kzalloc(sizeof(*optee_device), GFP_KERNEL);
> >>>>>> @@ -71,7 +72,13 @@ static int optee_register_device(const uuid_t *device_uuid)
> >>>>>>
> >>>>>>      optee_device->dev.bus = &tee_bus_type;
> >>>>>>      optee_device->dev.release = optee_release_device;
> >>>>>> -    if (dev_set_name(&optee_device->dev, "optee-ta-%pUb", device_uuid)) {
> >>>>>> +
> >>>>>> +    if (func == PTA_CMD_GET_DEVICES_SUPP)
> >>>>>> +            dev_name_fmt = "optee-ta-supp-%pUb";
> >>>>>> +    else
> >>>>>> +            dev_name_fmt = "optee-ta-%pUb";
> >>>>>
> >>>>> That's an ABI change, isn't it?
> >>>>
> >>>
> >>> Indeed it is an ABI break although we would like this to be backported
> >>> but don't want to break existing users. So I brainstormed on it and
> >>> came up with an alternative fix via device attribute in v4. Please
> >>> have a look.
> >>>
> >>>> Oh, here did this come from! Yes, I recently had to adjust some systemd
> >>>> service due to carrying this patch but looking for the change only in
> >>>> upstream:
> >>>>
> >>>> https://github.com/ilbers/isar/commit/83644ddf694e51f11793e6107e4aaf68dc0043a5
> >>>>
> >>>
> >>> You don't need to unbind all of the optee devices. v4 would help you
> >>> to maintain backwards compatibility, can you retest it?
> >>
> >> How do I know from tee-supplicant perspective which devices I need to
> >> unbind? There could be one in the future that will also use storage and
> >> will therefore also fail once the supplicant is gone.
> >>
> >
> > With v4, the devices where the below attribute is present need to
> > unbind before closing tee-supplicant.
> >
> > /sys/bus/tee/devices/optee-ta-<uuid>/need_supplicant
>
> OK - but that will only help in future kernels, nothing we have today.
> Thus, the shutdown script cannot assume to alone kill those devices
> unless it find a certain upcoming kernel release.
>

This v4 fix will be backported to stable kernels. So you can update
your scripts once it lands into your stable tree.

-Sumit

> Jan
>
> --
> Siemens AG, Technology
> Linux Expert Center
>
diff mbox series

Patch

diff --git a/drivers/tee/optee/device.c b/drivers/tee/optee/device.c
index 64f0e047c23d..78fc0a15c463 100644
--- a/drivers/tee/optee/device.c
+++ b/drivers/tee/optee/device.c
@@ -60,9 +60,10 @@  static void optee_release_device(struct device *dev)
 	kfree(optee_device);
 }
 
-static int optee_register_device(const uuid_t *device_uuid)
+static int optee_register_device(const uuid_t *device_uuid, u32 func)
 {
 	struct tee_client_device *optee_device = NULL;
+	const char *dev_name_fmt = NULL;
 	int rc;
 
 	optee_device = kzalloc(sizeof(*optee_device), GFP_KERNEL);
@@ -71,7 +72,13 @@  static int optee_register_device(const uuid_t *device_uuid)
 
 	optee_device->dev.bus = &tee_bus_type;
 	optee_device->dev.release = optee_release_device;
-	if (dev_set_name(&optee_device->dev, "optee-ta-%pUb", device_uuid)) {
+
+	if (func == PTA_CMD_GET_DEVICES_SUPP)
+		dev_name_fmt = "optee-ta-supp-%pUb";
+	else
+		dev_name_fmt = "optee-ta-%pUb";
+
+	if (dev_set_name(&optee_device->dev, dev_name_fmt, device_uuid)) {
 		kfree(optee_device);
 		return -ENOMEM;
 	}
@@ -142,7 +149,7 @@  static int __optee_enumerate_devices(u32 func)
 	num_devices = shm_size / sizeof(uuid_t);
 
 	for (idx = 0; idx < num_devices; idx++) {
-		rc = optee_register_device(&device_uuid[idx]);
+		rc = optee_register_device(&device_uuid[idx], func);
 		if (rc)
 			goto out_shm;
 	}