Re: [PATCH v2] tee: optee: Fix supplicant based device enumeration

On Fri, 28 Jul 2023 at 22:48, Sumit Garg sumit.garg@linaro.org 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. While at it use the global system workqueue for OP-TEE bus scanning work rather than our own custom one.

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") Signed-off-by: Sumit Garg sumit.garg@linaro.org

---

Changes in v2:

Apologies for taking it too long push this v2. Actually I did brainstorm how to best fix this tee-supplicant dependent device probing. Its hard to predict the lifetime of user-space daemon from kernel space. So following is the least intrusive approach:

• Use device names to seperate out tee-supplicant dependent devices via this patch.

• Since user-space service is aware about tee-supplicant lifespan, so allow the user-space service to unbind tee-supplicant dependent devices before killing the supplicant. Following command has to be added to the tee-supplicant service file.

  $ for dev in /sys/bus/tee/devices/*; do if [[ "$dev" == *"optee-ta-supp-"* ]]; \ then echo $(basename "$dev") > $dev/driver/unbind; fi done

drivers/tee/optee/core.c | 13 ++----------- drivers/tee/optee/device.c | 13 ++++++++++--- drivers/tee/optee/optee_private.h | 2 -- 3 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c index d01ca47f7bde..8ee3c71bd989 100644 --- a/drivers/tee/optee/core.c +++ b/drivers/tee/optee/core.c @@ -15,7 +15,6 @@ #include <linux/string.h> #include <linux/tee_drv.h> #include <linux/types.h> -#include <linux/workqueue.h> #include "optee_private.h"

int optee_pool_op_alloc_helper(struct tee_shm_pool *pool, struct tee_shm *shm, @@ -110,12 +109,7 @@ int optee_open(struct tee_context *ctx, bool cap_memref_null)

            if (!optee->scan_bus_done) {
                    INIT_WORK(&optee->scan_bus_work, optee_bus_scan);

•                   optee->scan_bus_wq = create_workqueue("optee_bus_scan");
  

•                   if (!optee->scan_bus_wq) {
  

•                           kfree(ctxdata);
  

•                           return -ECHILD;
  

•                   }
  

•                   queue_work(optee->scan_bus_wq, &optee->scan_bus_work);
  

•                   schedule_work(&optee->scan_bus_work);
                    optee->scan_bus_done = true;
            }
    }
  

@@ -159,10 +153,7 @@ void optee_release_supp(struct tee_context *ctx) struct optee *optee = tee_get_drvdata(ctx->teedev);

    optee_release_helper(ctx, optee_close_session_helper);

•   if (optee->scan_bus_wq) {
  

•           destroy_workqueue(optee->scan_bus_wq);
  

•           optee->scan_bus_wq = NULL;
  

•   }
  

•   optee_supp_release(&optee->supp);
  

}

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;
    }
  

diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h index 6dcecb83c893..af4aa266c3fb 100644 --- a/drivers/tee/optee/optee_private.h +++ b/drivers/tee/optee/optee_private.h @@ -193,7 +193,6 @@ struct optee_ops {

• @pool: shared memory pool
• @rpc_param_count: If > 0 number of RPC parameters to make room for
• @scan_bus_done flag if device registation was already done.

• • @scan_bus_wq workqueue to scan optee bus and register optee drivers
  • @scan_bus_work workq to scan optee bus and register optee drivers
  */

struct optee { @@ -212,7 +211,6 @@ struct optee { struct tee_shm_pool *pool; unsigned int rpc_param_count; bool scan_bus_done;

•   struct workqueue_struct *scan_bus_wq;
    struct work_struct scan_bus_work;
  

};

It works for me, I tested with a 6.4.0-based kernel on the Developerbox. Both fTPM and tee-based EFI variable service are shutdown successfully.

Tested-by: Masahisa Kojima masahisa.kojima@linaro.org

Thanks, Masahisa Kojima

-- 2.34.1