Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] bpf: Use-After-Free when BPF program detaches from an unbound panel - [High] bpf: Out-of-bounds read due to missing NUL-termination on user strings - [High] bpf: Out-of-bounds array access via negative enum indices in kfuncs - [Medium] drm/panel: Missing hotplug event prevents display pipeline activation - [Medium] bpf: Uninitialized kernel memory leaked over DSI bus via nullable pointers - [Low] bpf: Tracepoints log incorrect status data instead of return value --
commit 1f60161986e7e142490428d5f24fb3b831dddb58 Author: Maxime Ripard <[email protected]> Subject: drm/panel: Add generic MIPI-DSI panel driver with BPF init sequences This patch introduces a generic MIPI-DSI panel driver that uses BPF programs loaded from userspace for panel-specific behavior. The driver registers a disconnected drm_bridge, exposing sleepable kfuncs to control regulators, GPIOs, and DSI data transfer. > diff --git a/drivers/gpu/drm/panel/bpf/panel-bpf-mipi-dsi-ops.c > b/drivers/gpu/drm/panel/bpf/panel-bpf-mipi-dsi-ops.c > --- /dev/null > +++ b/drivers/gpu/drm/panel/bpf/panel-bpf-mipi-dsi-ops.c > @@ -32,25 +32,25 @@ > static int panel_bpf_mipi_dsi_bpf_init_member(const struct btf_type *t, > const struct btf_member *member, > void *kdata, const void *udata) > { > const struct drm_panel_dsi_bpf_ops *uops = > (const struct drm_panel_dsi_bpf_ops *)udata; > struct drm_panel_dsi_bpf_ops *kops = > (struct drm_panel_dsi_bpf_ops *)kdata; > u32 moff; > > moff = __btf_member_bit_offset(t, member) / 8; > > switch (moff) { > case offsetof(struct drm_panel_dsi_bpf_ops, panel_id): > memcpy(kops->panel_id, uops->panel_id, > sizeof(kops->panel_id)); > return 1; > case offsetof(struct drm_panel_dsi_bpf_ops, compatible): > memcpy(kops->compatible, uops->compatible, > sizeof(kops->compatible)); > return 1; [Severity: High] Does this memcpy() ensure the string is NUL-terminated? If userspace provides a string that is fully packed with non-NUL characters up to the length of the array, the kernel string will remain unterminated. [ ... ] > @@ -78,12 +78,12 @@ > static int panel_bpf_mipi_dsi_bpf_check_config(struct panel_bpf_mipi_dsi > *panel, > struct drm_panel_dsi_bpf_ops > *ops) > { > struct mipi_dsi_device *dsi = panel->dsi; > struct device *dev = &dsi->dev; > > if (!of_device_is_compatible(dev->of_node, ops->compatible)) { [Severity: High] Can this result in an out-of-bounds read? If ops->compatible was copied from userspace without a NUL terminator in panel_bpf_mipi_dsi_bpf_init_member(), the OF core functions like of_device_is_compatible() will read past the buffer boundary when attempting string comparison. > dev_err(dev, > "BPF compatible \"%s\" doesn't match panel\n", > ops->compatible); > return -EINVAL; > } [ ... ] > @@ -134,25 +134,25 @@ > static int panel_bpf_mipi_dsi_bpf_reg(void *kdata, struct bpf_link *link) > { > struct drm_panel_dsi_bpf_ops *ops = kdata; > struct panel_bpf_mipi_dsi *panel; > int ret; > > trace_panel_bpf_mipi_dsi_reg(ops->panel_id); > > guard(mutex)(&panel_bpf_mipi_dsi_list_lock); > > panel = panel_bpf_mipi_dsi_find_panel_unlocked(ops->panel_id); > if (!panel) > return -ENODEV; > > guard(mutex)(&panel->bpf_lock); > > if (panel->bpf_ops) > return -EBUSY; > > ret = panel_bpf_mipi_dsi_bpf_check_config(panel, ops); > if (ret) > return ret; > > ops->bridge = &panel->bridge; > panel->bpf_ops = ops; > > return 0; > } [Severity: Medium] When a BPF program successfully attaches here, it changes the bridge's detect callback behavior from returning connector_status_disconnected to connector_status_connected. Should this function also broadcast a hotplug notification to userspace so the display pipeline can be brought up without waiting for the next KMS polling cycle? [ ... ] > @@ -168,21 +168,21 @@ > static void panel_bpf_mipi_dsi_bpf_unreg(void *kdata, struct bpf_link *link) > { > struct drm_panel_dsi_bpf_ops *ops = kdata; > struct panel_bpf_mipi_dsi *panel; > > trace_panel_bpf_mipi_dsi_unreg(ops->panel_id); > > if (!ops->bridge) > return; > > panel = drm_bridge_to_bpf_panel(ops->bridge); > > scoped_guard(mutex, &panel->bpf_lock) { [Severity: High] Can this sequence trigger a use-after-free when unregistering? If a MIPI DSI device unbinds, devres triggers panel_bpf_mipi_dsi_list_cleanup() which removes the panel from the global list but doesn't detach the BPF program or clear ops->bridge before freeing the panel structure. If userspace subsequently unregisters the BPF struct_ops link, panel_bpf_mipi_dsi_bpf_unreg() appears to use the dangling ops->bridge pointer to resolve the freed panel structure, and then locks its bpf_lock. > diff --git a/drivers/gpu/drm/panel/bpf/panel-bpf-mipi-dsi-kfuncs.c > b/drivers/gpu/drm/panel/bpf/panel-bpf-mipi-dsi-kfuncs.c > --- /dev/null > +++ b/drivers/gpu/drm/panel/bpf/panel-bpf-mipi-dsi-kfuncs.c > @@ -36,15 +36,15 @@ > __bpf_kfunc int panel_bpf_mipi_dsi_regulator_enable_and_wait(struct > panel_bpf_mipi_dsi_ctx *ctx, > enum > panel_bpf_mipi_dsi_supply supply, > u32 settle_ms) > { > struct panel_bpf_mipi_dsi *panel = bpf_ctx_to_bpf_panel(ctx); > int ret; > > if (supply >= PANEL_BPF_MIPI_DSI_SUPPLY_COUNT) > return -EINVAL; [Severity: High] Does this bounds check adequately protect against out-of-bounds array accesses? The BPF verifier validates kfunc enum parameters as signed scalar values, which could be negative. A negative index here would pass the upper-bound check and allow an out-of-bounds read into panel->supplies. This same pattern exists for the GPIO enum checks elsewhere in this file. > > trace_panel_bpf_mipi_dsi_regulator_enable_and_wait(panel->panel_id, > > panel->supplies[supply].supply, > settle_ms); [ ... ] > @@ -177,19 +177,19 @@ > __bpf_kfunc int panel_bpf_mipi_dsi_dcs_write_and_wait(struct > panel_bpf_mipi_dsi_ctx *ctx, > u8 cmd, const u8 > *data__nullable, > u32 data__nullable__sz, > u32 settle_ms) > { > struct panel_bpf_mipi_dsi *panel = bpf_ctx_to_bpf_panel(ctx); > int ret; > > trace_panel_bpf_mipi_dsi_dcs_write_and_wait(panel->panel_id, cmd, > data__nullable, > data__nullable__sz, > settle_ms); [Severity: Low] Is this tracepoint logging the correct information? The trace event definition expects the bus transaction's return value for the fifth argument (ret), but this code passes the sleep duration settle_ms into the ret parameter. > > ret = mipi_dsi_dcs_write(panel->dsi, cmd, data__nullable, > data__nullable__sz); [Severity: Medium] Could this result in uninitialized kernel memory being leaked over the DSI bus? When data__nullable is NULL, the BPF verifier doesn't enforce that data__nullable__sz is zero. mipi_dsi_dcs_write() will allocate a buffer of size data__nullable__sz, skip the memcpy because the source is NULL, and then transmit the uninitialized kmalloc buffer to the display. Should there be a check for data__nullable == NULL && data__nullable__sz > 0? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
