Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] Uninitialized kernel stack memory (union padding and trailing array elements) sent to firmware CCB (Command Control Buffer). -- --- Patch [1]: [PATCH] drm/imagination: clamp freelist reconstruction requests --- commit ba7e433ffb1bb57e684515b2def3023d1d0aff07 Author: Pengpeng Hou <[email protected]> drm/imagination: clamp freelist reconstruction requests This commit bounds the freelist reconstruction count from firmware to the maximum array capacity to prevent out-of-bounds accesses. It processes only the bounded subset and reports this bounded count back to firmware, emitting a one-time warning if the limit is exceeded. > diff --git a/drivers/gpu/drm/imagination/pvr_free_list.c > b/drivers/gpu/drm/imagination/pvr_free_list.c > index e85cac83834c6..23dbc227b081a 100644 > --- a/drivers/gpu/drm/imagination/pvr_free_list.c > +++ b/drivers/gpu/drm/imagination/pvr_free_list.c > @@ -612,13 +612,20 @@ pvr_free_list_process_reconstruct_req(struct pvr_device > *pvr_dev, > }; > struct rogue_fwif_freelists_reconstruction_data *resp = > &resp_cmd.cmd_data.free_lists_reconstruction_data; [Severity: High] This isn't a bug introduced by this patch, but does this initialization leave union padding uninitialized? The resp_cmd struct uses a designated initializer for cmd_type. This zero-initializes only the first member of its cmd_data union (cmd_kick_data), leaving the remainder of the union uninitialized. > + u32 count = min_t(u32, req->freelist_count, > + ARRAY_SIZE(req->freelist_ids)); > > - for (u32 i = 0; i < req->freelist_count; i++) > + if (count != req->freelist_count) > + drm_warn_once(from_pvr_device(pvr_dev), > + "Firmware requested reconstruction of %u > freelists, limiting to %u\n", > + req->freelist_count, count); > + > + for (u32 i = 0; i < count; i++) > pvr_free_list_reconstruct(pvr_dev, req->freelist_ids[i]); > > - resp->freelist_count = req->freelist_count; > + resp->freelist_count = count; > memcpy(resp->freelist_ids, req->freelist_ids, > - req->freelist_count * sizeof(resp->freelist_ids[0])); > + count * sizeof(resp->freelist_ids[0])); If count is small (e.g., 1), the memcpy only fills count elements of the array. The trailing elements of resp->freelist_ids will fall into the uninitialized union padding. > > WARN_ON(pvr_kccb_send_cmd(pvr_dev, &resp_cmd, NULL)); > } When pvr_kccb_send_cmd() copies the full sizeof(struct rogue_fwif_kccb_cmd) to the KCCB ring buffer, could it leak this uninitialized kernel stack memory to the GPU firmware? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
