Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
Pre-existing issues: - [High] Out-of-order core unbinding and subsequent rebinding corrupts the `rdev->cores` array, leading to heap out-of-bounds writes, memory corruption, and active cores being overwritten or ignored. - [High] Repeated unbinding and rebinding of all cores permanently leaks the global `struct rocket_device` and its associated devm-managed resources. - [High] Concurrent probe and remove operations cause data races on the unprotected global `rdev` pointer and `num_cores` counter. - [High] Core unbinding destroys the DRM scheduler while existing open file descriptors still hold references, leading to a Use-After-Free upon job submission. - [High] `rocket_open` unconditionally accesses `cores[0]` for IOMMU domain creation, leading to a crash if core 0 is unbound. - [High] Global static `rdev` retains an `ERR_PTR` if `rocket_device_init` fails, causing a crash on subsequent probes. - [High] Missing NULL check for `kmalloc_objs` in `rocket_job_open` causes a NULL pointer dereference on allocation failure. - [High] The DRM device is prematurely exposed to userspace before its backing cores are initialized, leading to a NULL pointer dereference on open. -- commit d884827ebadaac3af5198fed539a2afc027a3c79 Author: Igor Paunovic <[email protected]> accel/rocket: search every core slot when a core is removed rocket_remove() decrements rdev->num_cores for each core it removes, while find_core_for_dev() searches slots 0 to num_cores - 1. Record how many slots were allocated and search all of them so every core is found on removal. > num_cores never reaches zero, rocket_device_fini() never runs, and the > file-scoped rdev keeps pointing at a device that is going away. Binding > the cores again starts from that stale count, because rocket_probe() > takes rdev->num_cores as the slot to fill. [Severity: High] This is a pre-existing issue, but is the file-scoped rdev pointer and its num_cores counter protected against concurrent access? If multiple core devices are probed or unbound concurrently via sysfs, could threads race on if (rdev == NULL) in rocket_probe(), potentially double- initializing the device? Could they also race on rdev->num_cores++ or rdev->num_cores--, corrupting the core array indexing? > Record how many slots were allocated and search all of them. Every core > is then found on removal, num_cores reaches zero, the device is torn down > and a later bind starts from a clean rdev. [Severity: High] This is a pre-existing issue, but does unbinding all cores permanently leak the global struct rocket_device and its associated devm-managed resources? When all cores are removed, rocket_device_fini() is called, but it only unregisters the DRM device. The allocations made with devm_drm_dev_alloc() and devm_kcalloc() are tied to the global drm_dev platform device, which is never destroyed during unbind. Since rdev is set to NULL, would a later bind allocate a completely new device and permanently leak the previous allocations? [Severity: High] This is also a pre-existing issue, but does tearing down the device and its cores destroy the DRM scheduler while existing open file descriptors still hold references to it? When a core is removed, rocket_core_fini() calls rocket_job_fini(), which executes drm_sched_fini(). If a user still holds an open DRM file descriptor and submits a job via ioctl, would drm_sched_entity_push_job() operate on the already destroyed scheduler and trigger a Use-After-Free? > This does not make unbinding a single core out of several work. probe > still takes num_cores as the slot to fill, so rebinding one core while > its siblings stay bound would write over a slot that is already in use, > and rocket_open() still reaches for cores[0] whether or not anything is > there. Both of those want more thought than a fix should carry. [Severity: High] This is a pre-existing issue, but does leaving this unfixed allow memory corruption? Because rocket_remove() doesn't clear the dev pointer or compact the array, wouldn't subsequent out-of-order unbinds match stale pointers since find_core_for_dev() now searches up to max_cores? Also, because rocket_job_open() and sched_to_core() iterate strictly up to num_cores, would they miss active cores that have been shifted past num_cores - 1 and incorrectly read finalized schedulers from the torn down slots? [Severity: High] Regarding rocket_open(), this is a pre-existing issue, but does leaving it reaching for cores[0] allow an unprivileged user to trigger a kernel panic? If a root user unbinds core 0, would a subsequent open of the DRM device node by any user crash when it unconditionally calls rocket_iommu_domain_create(rdev->cores[0].dev)? > diff --git a/drivers/accel/rocket/rocket_device.c > b/drivers/accel/rocket/rocket_device.c > index 46e6ee1e72c5f..efd004194c1af 100644 > --- a/drivers/accel/rocket/rocket_device.c > +++ b/drivers/accel/rocket/rocket_device.c > @@ -31,6 +31,8 @@ struct rocket_device *rocket_device_init(struct > platform_device *pdev, > if (of_device_is_available(core_node)) > num_cores++; > > + rdev->max_cores = num_cores; > + > rdev->cores = devm_kcalloc(dev, num_cores, sizeof(*rdev->cores), > GFP_KERNEL); > if (!rdev->cores) > return ERR_PTR(-ENOMEM); [Severity: High] This is a pre-existing issue, but if rocket_device_init() fails here (or elsewhere) and returns an ERR_PTR, doesn't the global rdev pointer in rocket_probe() retain that ERR_PTR? Since rocket_probe() only checks if (rdev == NULL) and does not reset it on failure, would a subsequent bind attempt blindly dereference the ERR_PTR when it accesses rdev->num_cores? [Severity: High] This is another pre-existing issue, but is the DRM device prematurely exposed to userspace during this initialization? rocket_device_init() registers the DRM device via drm_dev_register(), making it immediately visible to userspace. However, the first core's device pointer (rdev->cores[core].dev) is only assigned in rocket_probe() after rocket_device_init() returns. If an unprivileged user opens the device during this window, would rocket_open() encounter a NULL pointer dereference when it accesses cores[0].dev? [Severity: High] Finally, this is a pre-existing issue, but does rocket_job_open() correctly handle memory allocation failures when creating the scheds array for these cores? It allocates the array using kmalloc_objs(*scheds, rdev->num_cores) but fails to check if the returned pointer is NULL before executing scheds[core] = &rdev->cores[core].sched;. Would this lead to a NULL pointer dereference on allocation failure? > diff --git a/drivers/accel/rocket/rocket_drv.c > b/drivers/accel/rocket/rocket_drv.c > index 8bbbce594883e..2bcfe4ab3c68f 100644 > --- a/drivers/accel/rocket/rocket_drv.c > +++ b/drivers/accel/rocket/rocket_drv.c > @@ -223,7 +223,7 @@ static int find_core_for_dev(struct device *dev) > { > struct rocket_device *rdev = dev_get_drvdata(dev); > > - for (unsigned int core = 0; core < rdev->num_cores; core++) { > + for (unsigned int core = 0; core < rdev->max_cores; core++) { > if (dev == rdev->cores[core].dev) > return core; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
