Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] Infinite probe deferral when the optional `fsl,prg` device tree property is absent. - [High] dc_fu_get_dprc() incorrectly assumes successful DPRC probe and stores a dangling pointer if dc_dprc_probe() previously failed. - [High] Use-After-Free (UAF) vulnerability due to missing reference counting and device links during PRG/DPRC lookups. - [Low] Misuse of dev_err_probe() from a component bind callback sets the deferral reason on the wrong device. --
commit 6264221c06949730f7283e2f3f3d4083c0f6d10e Author: Liu Ying <[email protected]> Subject: drm/imx: dc-fu: Get DPR channel This commit adds a helper dc_fu_get_dprc() to get the DPR channel for a FetchUnit. It also calls this helper from the component bind functions for FetchLayer and FetchWarp to initialize their DPR channels. > diff --git a/drivers/gpu/drm/imx/dc/dc-fu.c b/drivers/gpu/drm/imx/dc/dc-fu.c > index 2e197b3344bd1..8339f44c00919 100644 > --- a/drivers/gpu/drm/imx/dc/dc-fu.c > +++ b/drivers/gpu/drm/imx/dc/dc-fu.c [ ... ] > @@ -256,3 +257,20 @@ void dc_fu_common_hw_init(struct dc_fu *fu) > dc_fu_set_pixel_blend_mode(fu, i); > } > } > + > +int dc_fu_get_dprc(struct dc_fu *fu) > +{ > + struct dc_prg *prg; > + > + prg = dc_prg_lookup_by_phandle(fu->dev, "fsl,prg", 0); [Severity: High] Does this code enter an infinite probe deferral loop on hardware platforms that do not include a PRG? The device tree bindings define the fsl,prg property as optional. However, if the property is entirely missing, of_parse_phandle() returns NULL, and this unconditionally returns -EPROBE_DEFER. Should this code distinguish between a missing property (which should be tolerated) and a missing/unprobed device? [Severity: High] Is it safe to use this prg pointer without holding a reference count or device link? dc_prg_lookup_by_phandle() locates the prg structure under a lock but returns it without taking a reference count (e.g., get_device()). The unprotected pointer is used outside the lock here. If the PRG device is concurrently removed (e.g., via DT overlay removal), the prg structure could be freed, causing a use-after-free when we call dc_prg_get_dprc(prg) later. > + if (!prg) > + return dev_err_probe(fu->dev, -EPROBE_DEFER, > + "failed to lookup PRG\n"); [Severity: Low] Is dev_err_probe() being called on the correct device here? This is called from the component bind callbacks dc_fl_bind() and dc_fw_bind(), passing the component device fu->dev. Since the component has already successfully probed and is in the bind phase, it is not on the deferred probe list. Does this attach the deferred probe reason to the wrong device, bypassing the devices_deferred debugfs file and causing a minor memory leak since driver_deferred_probe_del() will never free the string? > + > + fu->dprc = dc_prg_get_dprc(prg); [Severity: High] Can this store a dangling pointer in fu->dprc if the DPRC previously failed to probe? In dc_dprc_probe(), dprc is allocated with devm_kzalloc() and stored in prg->dprc. If dc_dprc_probe() subsequently fails (e.g., devm_request_irq() fails or defers), the dprc structure is automatically freed by devres cleanup, but prg->dprc is not reset to NULL. When this code retrieves prg->dprc, the pointer will be non-NULL despite pointing to freed memory. It will bypass the -EPROBE_DEFER check and store the dangling pointer, which could lead to memory corruption when accessed by the display pipeline. [Severity: High] Does storing this pointer for long-term usage require a device link? Because the prg/dprc pointer was acquired without a reference count or device link, the loosely coupled PRG/DPRC devices can be safely unbound while the Fetch Unit remains active, leading to a use-after-free when the Fetch Unit accesses fu->dprc. > + if (!fu->dprc) > + return dev_err_probe(fu->dev, -EPROBE_DEFER, > + "failed to get DPR channel\n"); > + > + return 0; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
