Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Character device is registered and exposed to user-space before critical context fields (spinlocks, krefs, lists, idr) are initialized, enabling severe race conditions and NULL pointer dereferences on concurrent open(). - [High] Use-After-Free on `fastrpc_rpmsg_probe` error paths after device registration. -- commit 449b11a15887fd75279efb078924d324d2fb8fb1 Author: Jianping Li <[email protected]> Subject: misc: fastrpc: Reduce console noise from expected DSP info, reserved memory and session messages The commit downgrades expected error messages from the DSP and reserved DMA memory to dev_dbg, and rate-limits the session availability error to reduce console flooding on certain platforms. Link: https://lore.kernel.org/all/[email protected]/ > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index 90fd669636ec1..f40356c950519 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c [ ... ] > @@ -2563,7 +2563,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device > *rpdev) > } > > if (of_reserved_mem_device_init_by_idx(rdev, rdev->of_node, 0)) > - dev_info(rdev, "no reserved DMA memory for FASTRPC\n"); > + dev_dbg(rdev, "no reserved DMA memory for FASTRPC\n"); > > vmcount = of_property_read_variable_u32_array(rdev->of_node, > "qcom,vmids", &vmids[0], 0, FASTRPC_MAX_VMIDS); [Severity: High] This isn't a bug introduced by this patch, but could the initialization in fastrpc_rpmsg_probe() lead to a race condition with concurrent opens? Looking at fastrpc_rpmsg_probe(), we register the device before initializing important context fields: err = fastrpc_device_register(rdev, data, secure_dsp, domain); if (err) goto err_free_data; ... kref_init(&data->refcount); atomic_set(&data->ctx_seq, 0); ... INIT_LIST_HEAD(&data->users); spin_lock_init(&data->lock); If a user-space process opens the character device immediately after fastrpc_device_register() completes, fastrpc_device_open() will be called. Since data->refcount and data->users are uninitialized, won't this cause a null pointer dereference when it calls fastrpc_channel_ctx_get() or attempts to lock data->lock and add to the zeroed data->users list? [Severity: High] This is also a pre-existing issue, but is there a risk of a use-after-free in the error handling paths of fastrpc_rpmsg_probe()? If an error occurs after the device is registered (for example, if a subsequent fastrpc_device_register() fails or if of_platform_populate() fails), the code branches to the teardown paths: err = fastrpc_device_register(rdev, data, false, domain); if (err) goto err_deregister_fdev; ... err_deregister_fdev: if (data->fdevice) misc_deregister(&data->fdevice->miscdev); if (data->secure_fdevice) misc_deregister(&data->secure_fdevice->miscdev); err_free_data: kfree(data); return err; Since misc_deregister() does not revoke active file descriptors, what happens if a concurrent open() has already acquired a reference to the device before we call misc_deregister()? Will the unconditional kfree(data) result in a use-after-free later when the user closes the file and fastrpc_device_release() accesses the freed data? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
