Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
New issues: - [Critical] Heap buffer overflow in `qcom_pas_tee_get_rsc_table` when copying the input resource table. - [High] Out-of-bounds read and info leak in `qcom_pas_tee_get_rsc_table` when duplicating the returned resource table. - [High] The first TEE invocation fails to pass the input resource table data to the TA, resulting in an invalid parameter error. - [High] Unsynchronized mutation of the global operations structure and unconditional unregistration during remove. - [Medium] The `output_rt_size` pointer is left uninitialized if the TEE returns a size of 0. - [Medium] The driver incorrectly treats the expected `TEEC_ERROR_SHORT_BUFFER` response as a fatal error. Pre-existing issues: - [High] Missing protection for `ops_ptr` reads causes a TOCTOU NULL pointer dereference. -- commit f2659330f38d92cd91647244dc269a73194079c0 Author: Sumit Garg <[email protected]> firmware: qcom: Add a PAS TEE service This patch adds support for a Peripheral Authentication Service (PAS) driver based on the TEE bus, using OP-TEE as the backend implementation. > diff --git a/drivers/firmware/qcom/qcom_pas_tee.c > b/drivers/firmware/qcom/qcom_pas_tee.c > new file mode 100644 > index 0000000000000..a2ba3af05a502 > --- /dev/null > +++ b/drivers/firmware/qcom/qcom_pas_tee.c [ ... ] > +static void *qcom_pas_tee_get_rsc_table(struct device *dev, > + struct qcom_pas_context *ctx, > + void *input_rt, size_t input_rt_size, > + size_t *output_rt_size) > +{ > + struct qcom_pas_tee_private *data = dev_get_drvdata(dev); > + struct tee_ioctl_invoke_arg inv_arg = { > + .func = TA_QCOM_PAS_GET_RESOURCE_TABLE, > + .session = data->session_id, > + .num_params = TEE_NUM_PARAMS > + }; > + struct tee_param param[4] = { > + [0] = { > + .attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT, > + .u.value.a = ctx->pas_id, > + }, > + [1] = { > + .attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT, > + .u.memref.size = input_rt_size, > + } > + }; [Severity: High] Does this first tee_client_invoke_func() invocation pass an invalid memory reference to the TA? In qcom_pas_tee_get_rsc_table(), param[1] is initialized with u.memref.size = input_rt_size, but u.memref.shm is left uninitialized (NULL). If the firmware has an embedded input resource table (input_rt_size > 0), will the TEE subsystem reject this call with an invalid parameter error before the TA is even invoked? > + void *rt_buf = NULL; > + int ret; > + > + ret = tee_client_invoke_func(data->ctx, &inv_arg, param); > + if (ret < 0 || inv_arg.ret != 0) { [Severity: Medium] Will this error check abort the loading process if the TA follows the standard GlobalPlatform TEE Client API specification? When queried for a buffer size, a standard TA is expected to return TEEC_ERROR_SHORT_BUFFER (a non-zero error code) since the initial buffer is NULL or too small. Since this check treats any non-zero inv_arg.ret as a failure, does this prevent the code from proceeding to the second pass to allocate the correctly sized buffer? > + dev_err(dev, "PAS get RT failed, pas_id: %d, ret: %d, err: > 0x%x\n", > + ctx->pas_id, ret, inv_arg.ret); > + return ret ? ERR_PTR(ret) : ERR_PTR(-EINVAL); > + } > + > + if (param[1].u.memref.size) { [Severity: Medium] Is it possible for output_rt_size to remain uninitialized here? If the TA legitimately returns param[1].u.memref.size as 0 during the first invocation (indicating no resources), this block is skipped and the function returns NULL. Since output_rt_size is never assigned a value, callers like those in qcom_q6v5_pas.c might read garbage stack data for the table size. > + struct tee_shm *rt_shm __free(shm_free) = > + tee_shm_alloc_kernel_buf(data->ctx, > + param[1].u.memref.size); > + void *rt_shm_va; > + > + if (IS_ERR_OR_NULL(rt_shm)) { > + dev_err(dev, "rt_shm allocation failed\n"); > + rt_shm = NULL; > + return ERR_PTR(-ENOMEM); > + } > + > + rt_shm_va = tee_shm_get_va(rt_shm, 0); > + if (IS_ERR(rt_shm_va)) { > + dev_err(dev, "rt_shm get VA failed\n"); > + return ERR_CAST(rt_shm_va); > + } > + memcpy(rt_shm_va, input_rt, input_rt_size); [Severity: Critical] Can this memcpy overflow the allocated rt_shm buffer? The rt_shm buffer is allocated using the size requested by the TA from the first invocation (param[1].u.memref.size). However, this memcpy unconditionally copies input_rt_size bytes. If the TA returns a size smaller than input_rt_size, wouldn't this write past the bounds of the allocated kernel heap buffer? > + > + param[1].u.memref.shm = rt_shm; > + ret = tee_client_invoke_func(data->ctx, &inv_arg, param); > + if (ret < 0 || inv_arg.ret != 0) { > + dev_err(dev, "PAS get RT failed, pas_id: %d, ret: %d, > err: 0x%x\n", > + ctx->pas_id, ret, inv_arg.ret); > + return ret ? ERR_PTR(ret) : ERR_PTR(-EINVAL); > + } > + > + if (param[1].u.memref.size) { > + *output_rt_size = param[1].u.memref.size; > + rt_buf = kmemdup(rt_shm_va, *output_rt_size, > GFP_KERNEL); [Severity: High] Could this kmemdup read out of bounds of the initially allocated rt_shm buffer? During the second tee_client_invoke_func() call, the TA can update param[1].u.memref.size to a value larger than the initially allocated rt_shm capacity. If output_rt_size is updated to this larger value, kmemdup will read past the end of the rt_shm_va buffer, potentially leaking adjacent sensitive kernel heap data to the caller. > + if (!rt_buf) > + return ERR_PTR(-ENOMEM); > + } > + } > + > + return rt_buf; > +} [ ... ] > +static int qcom_pas_tee_probe(struct tee_client_device *pas_dev) > +{ [ ... ] > + data->session_id = sess_arg.session; > + dev_set_drvdata(dev, data); > + qcom_pas_ops_tee.dev = dev; > + qcom_pas_ops_register(&qcom_pas_ops_tee); [Severity: High] Is it safe to mutate the static qcom_pas_ops_tee.dev singleton without synchronization? If qcom_pas_ops_register() is called when another backend (like SCM) is already registered, it fails silently but the probe still succeeds. Could this lead to conflicting states if multiple backends probe concurrently? [Severity: High] This isn't a bug introduced by this patch, but is there a TOCTOU race condition when reading ops_ptr in qcom_pas.c? In functions like qcom_pas_init_image(), ops_ptr is checked for NULL and subsequently dereferenced: if (!ops_ptr) return -ENODEV; return ops_ptr->init_image(ops_ptr->dev, pas_id, metadata, size, ctx); Since ops_ptr is not protected by RCU, a lock, or READ_ONCE(), a concurrent call to qcom_pas_ops_unregister() (e.g., via module unload) can set ops_ptr to NULL between the check and the dereference, causing a NULL pointer dereference. > + > + return ret; > +} > + > +static void qcom_pas_tee_remove(struct tee_client_device *pas_dev) > +{ > + struct device *dev = &pas_dev->dev; > + struct qcom_pas_tee_private *data = dev_get_drvdata(dev); > + > + qcom_pas_ops_unregister(); [Severity: High] Does this unconditional unregister break the SCM backend? If the TEE module is unloaded, qcom_pas_tee_remove() forcefully clears the global ops_ptr. If the SCM backend was the active registered backend instead of TEE, wouldn't this destroy the active SCM registration and break PAS functionality globally? > + tee_client_close_session(data->ctx, data->session_id); > + tee_client_close_context(data->ctx); > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
