When a fastrpc client process dies abnormally without closing its file descriptor, fastrpc_device_release() is never called and the compute-cb session slot is never freed. The abnormal client death on the DSP side triggers a glink channel teardown which propagates up through the rpmsg bus and calls fastrpc_rpmsg_remove(). fastrpc_rpmsg_remove() nulls cctx->rpdev before calling misc_deregister(). If a concurrent open() enters fastrpc_device_open() in this window and fastrpc_session_alloc() returns NULL (sessions exhausted by the leaked slot), the error path dereferences cctx->rpdev->dev causing a NULL pointer dereference: Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000 pc : fastrpc_device_open+0x1b8/0x258 [fastrpc] Call trace: fastrpc_device_open+0x1b8/0x258 [fastrpc] (P) misc_open+0xd8/0x1a0 Fix by moving misc_deregister() before rpdev = NULL. misc_open() and misc_deregister() both take misc_mtx, so misc_deregister() will either block until fastrpc_device_open() completes or prevent new opens from entering it entirely. Fixes: 7c11df42d0c7 ("misc: fastrpc: Fix device_open when no session is available") Cc: stable@vger.kernel.org Reviewed-by: Ekansh Gupta Signed-off-by: Vinayak Katoch --- drivers/misc/fastrpc.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c index 5dd1990d3e2a..f280bff2edf0 100644 --- a/drivers/misc/fastrpc.c +++ b/drivers/misc/fastrpc.c @@ -2697,6 +2697,12 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev) struct fastrpc_user *user; unsigned long flags; + if (cctx->fdevice) + misc_deregister(&cctx->fdevice->miscdev); + + if (cctx->secure_fdevice) + misc_deregister(&cctx->secure_fdevice->miscdev); + /* No invocations past this point */ spin_lock_irqsave(&cctx->lock, flags); cctx->rpdev = NULL; @@ -2704,12 +2710,6 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev) fastrpc_notify_users(user); spin_unlock_irqrestore(&cctx->lock, flags); - if (cctx->fdevice) - misc_deregister(&cctx->fdevice->miscdev); - - if (cctx->secure_fdevice) - misc_deregister(&cctx->secure_fdevice->miscdev); - list_for_each_entry_safe(buf, b, &cctx->invoke_interrupted_mmaps, node) list_del(&buf->node); -- 2.34.1 fastrpc_device_register() calls misc_register() which makes the device node visible to userspace. However, kref_init(), spin_lock_init(), INIT_LIST_HEAD(), dev_set_drvdata() and fastrpc_cb_devices_create() were all called after the switch/misc_register block, leaving a window where userspace can open the device and call fastrpc_device_open() on a partially initialised channel context. On a concurrent open(), fastrpc_channel_ctx_get() calls kref_get() on a kzalloc-zeroed, never kref_init()'d refcount of 0, triggering: refcount_t: addition on 0; use-after-free. Call trace: refcount_warn_saturate+0x120/0x148 (P) fastrpc_device_open+0x204/0x258 [fastrpc] misc_open+0xd8/0x1a0 Additionally, fastrpc_cb_devices_create() not having run means sesscount is still 0, so fastrpc_session_alloc() returns NULL and fastrpc_device_open() hits the error path which calls fastrpc_channel_ctx_put() on the already-bogus refcount. This triggers the free callback, kfree()-ing the channel context. A subsequent IRQ-path fastrpc_rpmsg_callback() then dereferences the freed spinlock: Unable to handle kernel paging request at virtual address ffffcd5841510ca0 Internal error: Oops: 0000000096000047 [#1] SMP Call trace: queued_spin_lock_slowpath+0x414/0x5e0 (P) _raw_spin_lock_irqsave+0x74/0x90 fastrpc_rpmsg_callback+0x4c/0xf0 [fastrpc] qcom_glink_native_rx+0x6c4/0xf10 qcom_glink_smem_intr+0x1c/0x38 [qcom_glink_smem] Kernel panic - not syncing: Oops: Fatal exception in interrupt Fix by moving all channel context initialisation (kref_init, spin_lock_init, INIT_LIST_HEAD, idr_init, dev_set_drvdata) and fastrpc_cb_devices_create() before fastrpc_device_register(), ensuring the struct is fully ready before the device node is visible to userspace. Add err_depopulate label in the error path since fastrpc_cb_devices_create() now runs before misc_register(); a fastrpc_device_register() failure must call fastrpc_cb_devices_destroy() to clean up the compute-cb children. On CDSP/GDSP the secure node is registered before the non-secure one, so an application may already have opened the first node and hold a channel context reference when the second registration fails. Freeing the context with kfree() there would leave that user with a dangling pointer. Handle it like fastrpc_rpmsg_remove(): after the nodes are deregistered, clear rpdev, wake pending invocations with -EPIPE and drop only the probe reference with fastrpc_channel_ctx_put(), so the last holder frees the context. Move fastrpc_notify_users() above probe so it can be used there. Fixes: 3abe3ab3cdab ("misc: fastrpc: add secure domain support") Cc: stable@vger.kernel.org Signed-off-by: Vinayak Katoch --- drivers/misc/fastrpc.c | 83 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 50 insertions(+), 33 deletions(-) diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c index f280bff2edf0..fc09cd27af4b 100644 --- a/drivers/misc/fastrpc.c +++ b/drivers/misc/fastrpc.c @@ -2548,6 +2548,18 @@ static int fastrpc_init_reserved_mem(struct fastrpc_channel_ctx *cctx, cctx->vmperms, cctx->vmcount); } +static void fastrpc_notify_users(struct fastrpc_user *user) +{ + struct fastrpc_invoke_ctx *ctx; + + spin_lock(&user->lock); + list_for_each_entry(ctx, &user->pending, node) { + ctx->retval = -EPIPE; + complete(&ctx->work); + } + spin_unlock(&user->lock); +} + static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) { struct device *rdev = &rpdev->dev; @@ -2557,6 +2569,8 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) bool secure_dsp; unsigned int vmids[FASTRPC_MAX_VMIDS]; const struct fastrpc_soc_data *soc_data; + struct fastrpc_user *user; + unsigned long flags; soc_data = device_get_match_data(rdev); @@ -2605,6 +2619,23 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) data->poll_mode_supported = soc_data->poll_mode_supported || of_machine_get_match(fastrpc_poll_supported_machines); + kref_init(&data->refcount); + atomic_set(&data->ctx_seq, 0); + + rdev->dma_mask = &data->dma_mask; + dma_set_mask_and_coherent(rdev, DMA_BIT_MASK(32)); + INIT_LIST_HEAD(&data->users); + INIT_LIST_HEAD(&data->invoke_interrupted_mmaps); + spin_lock_init(&data->lock); + idr_init(&data->ctx_idr); + data->domain_id = domain_id; + data->rpdev = rpdev; + dev_set_drvdata(&rpdev->dev, data); + + err = fastrpc_cb_devices_create(rpdev); + if (err) + goto err_free_data; + switch (domain_id) { case ADSP_DOMAIN_ID: case MDSP_DOMAIN_ID: @@ -2613,7 +2644,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) data->unsigned_support = false; err = fastrpc_device_register(rdev, data, secure_dsp, domain); if (err) - goto err_free_data; + goto err_depopulate; break; case CDSP_DOMAIN_ID: case GDSP_DOMAIN_ID: @@ -2621,7 +2652,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) /* Create both device nodes so that we can allow both Signed and Unsigned PD */ err = fastrpc_device_register(rdev, data, true, domain); if (err) - goto err_free_data; + goto err_depopulate; err = fastrpc_device_register(rdev, data, false, domain); if (err) @@ -2629,30 +2660,12 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) break; default: err = -EINVAL; - goto err_free_data; + goto err_depopulate; } - kref_init(&data->refcount); - atomic_set(&data->ctx_seq, 0); - - rdev->dma_mask = &data->dma_mask; - dma_set_mask_and_coherent(rdev, DMA_BIT_MASK(32)); - INIT_LIST_HEAD(&data->users); - INIT_LIST_HEAD(&data->invoke_interrupted_mmaps); - spin_lock_init(&data->lock); - idr_init(&data->ctx_idr); - data->domain_id = domain_id; - data->rpdev = rpdev; - dev_set_drvdata(&rpdev->dev, data); - - err = fastrpc_cb_devices_create(rpdev); - if (err) - goto err_deregister_fdev; - if (data->domain_id == ADSP_DOMAIN_ID && data->sesscount > 0) { struct fastrpc_session_ctx *last_sess; struct fastrpc_session_ctx *dup_sess; - unsigned long flags; spin_lock_irqsave(&data->lock, flags); last_sess = &data->session[data->sesscount - 1]; @@ -2673,23 +2686,27 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) if (data->secure_fdevice) misc_deregister(&data->secure_fdevice->miscdev); +err_depopulate: + /* + * A registered device node may already have been opened, so a user + * can hold a cctx reference. Drop only our reference instead of + * freeing the context directly. + */ + spin_lock_irqsave(&data->lock, flags); + data->rpdev = NULL; + list_for_each_entry(user, &data->users, user) + fastrpc_notify_users(user); + spin_unlock_irqrestore(&data->lock, flags); + + fastrpc_cb_devices_destroy(rpdev); + fastrpc_channel_ctx_put(data); + return err; + err_free_data: kfree(data); return err; } -static void fastrpc_notify_users(struct fastrpc_user *user) -{ - struct fastrpc_invoke_ctx *ctx; - - spin_lock(&user->lock); - list_for_each_entry(ctx, &user->pending, node) { - ctx->retval = -EPIPE; - complete(&ctx->work); - } - spin_unlock(&user->lock); -} - static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev) { struct fastrpc_channel_ctx *cctx = dev_get_drvdata(&rpdev->dev); -- 2.34.1