Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [Critical] Unprotected access to `vc4->bin_bo` during command list validation leads to NULL Pointer Dereference or Use-After-Free, which the patch escalates to a severe memory write via `memset`. -- --- Patch [2]: [PATCH RESEND v2 2/2] drm/vc4: Zero the tile state data array before each BIN job --- commit e8713542a8743898c85cf163587b5ea2ab0ad42b Author: Maíra Canal <[email protected]> drm/vc4: Zero the tile state data array before each BIN job The binner BO is a single 16MB buffer split into 512KB slots that are handed out to jobs at submission time and recycled as jobs complete, without ever being cleared. Each slot holds the job's Tile State Data Array (TSDA) at its start, followed by the tile allocation pool. Link: https://github.com/raspberrypi/linux/issues/3221 [1] Link: https://github.com/raspberrypi/linux/issues/5780 [2] > diff --git a/drivers/gpu/drm/vc4/vc4_validate.c > b/drivers/gpu/drm/vc4/vc4_validate.c > index 7f2fadfde7a8e..d2a65c968b1f9 100644 > --- a/drivers/gpu/drm/vc4/vc4_validate.c > +++ b/drivers/gpu/drm/vc4/vc4_validate.c > @@ -385,6 +385,23 @@ validate_tile_binning_config(VALIDATE_ARGS) > return -EINVAL; > } > > + /* The tile state data array is 48 bytes per tile, and we put it at > + * the start of a BO containing both it and the tile alloc. > + */ > + tile_state_size = 48 * tile_count; > + > + /* Since the tile alloc array will follow us, align. */ > + tile_state_size = roundup(tile_state_size, 4096); > + > + /* Reject configurations whose tile state would leave no room for > + * the tile alloc pool that follows it in the slot. > + */ > + if (tile_state_size >= vc4->bin_alloc_size) { > + DRM_DEBUG("Tile binning config of %dx%d too large\n", > + exec->bin_tiles_x, exec->bin_tiles_y); > + return -EINVAL; > + } > + > bin_slot = vc4_v3d_get_bin_slot(vc4); [ ... ] > @@ -400,13 +417,13 @@ validate_tile_binning_config(VALIDATE_ARGS) > exec->bin_slots |= BIT(bin_slot); > bin_addr = vc4->bin_bo->base.dma_addr + bin_slot * vc4->bin_alloc_size; > > - /* The tile state data array is 48 bytes per tile, and we put it at > - * the start of a BO containing both it and the tile alloc. > - */ > - tile_state_size = 48 * tile_count; > + exec->tile_alloc_offset = bin_addr + tile_state_size; > > - /* Since the tile alloc array will follow us, align. */ > - exec->tile_alloc_offset = bin_addr + roundup(tile_state_size, 4096); > + /* The TSDA area must be zeroed out before use, otherwise the PTB might > + * consume a stale tile state. > + */ > + memset(vc4->bin_bo->base.vaddr + bin_slot * vc4->bin_alloc_size, 0, > + tile_state_size); [Severity: Critical] This is a pre-existing issue, but this patch escalates it to a potential kernel memory corruption via a NULL pointer dereference or use-after-free write. During command list validation, vc4->bin_bo is accessed here before it is guaranteed to be allocated or referenced. The actual reference to the BO is obtained later in vc4_get_bcl(): drivers/gpu/drm/vc4/vc4_gem.c:vc4_get_bcl() { ... ret = vc4_validate_bin_cl(dev, ...); ... if (exec->found_tile_binning_mode_config_packet) { ret = vc4_v3d_bin_bo_get(vc4, &exec->bin_bo_used); ... } If a user imports PRIME BOs without creating a local BO first, vc4->bin_bo can be NULL. Furthermore, if a previous BO was freed, bin_bo_release() sets vc4->bin_bo to NULL but leaves vc4->bin_alloc_size as a stale non-zero value: drivers/gpu/drm/vc4/vc4_v3d.c:bin_bo_release() { ... drm_gem_object_put(&vc4->bin_bo->base.base); vc4->bin_bo = NULL; } Because vc4->bin_alloc_size is not reset, the newly added size check (tile_state_size >= vc4->bin_alloc_size) can pass, allowing the memset to dereference a NULL or freed vc4->bin_bo and write up to ~512KB into unallocated memory. Could we ensure vc4->bin_bo is valid and properly referenced before attempting to clear the memory here, or defer the initialization until after vc4_v3d_bin_bo_get() has been called? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
