Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Improper handling of `krealloc_array()` failure in `cla_dev_domain_get()` overwrites the array pointer with NULL, causing a memory leak and subsequent NULL pointer dereference. - [High] Missing `platform_driver_unregister()` on the `cla_domains_finalise()` error path leaves the driver registered, leading to a Use-After-Free. - [Medium] Integer overflow when calculating `cla_nr_domains` causes lookup tables to remain unpopulated, silently breaking device initialization. --
--- Patch [3]: [RFC PATCH v1 3/8] misc/arm-cla: Probe firmware-described devices --- commit f4618493e3d162e6d466cff0e6d759171e5655f5 Author: Jean-Philippe Brucker <[email protected]> misc/arm-cla: Probe firmware-described devices Discover CLA devices from firmware and instantiate the driver with platform_driver_probe(). Map the MMIO frame, select the kernel-accessible privilege-level frame and associate each CLA with the CPU named by firmware. > diff --git a/drivers/misc/arm-cla/cla-init.c b/drivers/misc/arm-cla/cla-init.c > --- a/drivers/misc/arm-cla/cla-init.c > +++ b/drivers/misc/arm-cla/cla-init.c [ ... ] > ret = platform_driver_probe(&cla_driver, cla_probe); > if (ret) { > if (ret != -ENODEV) > pr_err("arm-cla: probe failed with %d\n", ret); > /* Some domains may have been created during probe */ > goto err_domains_free; > } > > ret = cla_domains_finalise(); > if (ret) { > pr_err("arm-cla: failed to finalise domains: %d", ret); > goto err_domains_free; [Severity: High] If cla_domains_finalise() fails, this jumps to err_domains_free, which skips platform_driver_unregister(). Will this leave dangling pointers to the freed cla_driver struct on the platform bus? > } > > /* > * Each CPU initializes their own CLA. CPUHP uses a pair of smp_mb() > * when calling the startup callback, ensuring that cla_dev_setup() > * reads fully initialized cla_lut_cpu and cla_dev structures. > */ > ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "arm-cla", > cla_dev_setup, cla_dev_teardown); > if (ret < 0) { > pr_err("arm-cla: failed to setup cpuhp: %d", ret); > goto err_driver_unregister; > } > cla_cpuhp_state = ret; > > return 0; > > err_driver_unregister: > platform_driver_unregister(&cla_driver); > err_domains_free: > cla_domains_free(); > return ret; > } [ ... ] > diff --git a/drivers/misc/arm-cla/cla-topology.c > b/drivers/misc/arm-cla/cla-topology.c > --- /dev/null > +++ b/drivers/misc/arm-cla/cla-topology.c [ ... ] > +struct cla_domain *cla_dev_domain_get(struct cla_dev *dev) > +{ > + int ret; > + unsigned int domain_id; > + struct cla_domain *domain; > + > + /* Domain ID is provided by firmware */ > + ret = of_property_read_u32(dev->dev->of_node, "domain", &domain_id); > + if (WARN_ON(ret)) > + return ERR_PTR(-EINVAL); > + > + domain = xa_load(&cla_domains, domain_id); > + if (domain) { > + domain->nr_devs++; > + domain->devs = krealloc_array(domain->devs, domain->nr_devs, > + sizeof(*domain->devs), > GFP_KERNEL); [Severity: High] Does this code leak the original domain->devs array if krealloc_array() fails and returns NULL? Also, since domain->nr_devs was unconditionally incremented, will a subsequent call to cla_domains_finalise() dereference the NULL pointer at domain->devs[j]? > + if (!domain->devs) > + return ERR_PTR(-ENOMEM); > + domain->devs[domain->nr_devs - 1] = dev; > + return domain; > + } > + > + domain = cla_domain_alloc(dev, domain_id); > + if (IS_ERR(domain)) > + return domain; > + > + cla_nr_domains = max(domain_id + 1, cla_nr_domains); [Severity: Medium] Can this calculation overflow to 0 if firmware provides a domain_id of UINT_MAX? If it overflows, will cla_domains_finalise() skip its loop and leave the device uninitialized in the lookup tables? > + > + return domain; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
