Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/panthor: missing validation of protected memory for PROTM suspend buffers - [Low] drm: dead macro GROUP_CREATE_FLAGS referencing undefined identifier --
commit a26512a2f35a2a18c407c42a57bf8210da106e39 Author: Boris Brezillon <[email protected]> drm/panthor: Protm support for groups This commit extends struct drm_panthor_group_create to allow a user-provided protm suspend buffer. It imports the user-supplied suspend buffer and allows the PROT_REGION instruction for groups that provide it. It also stores the suspend buffer sizes provided by the FW. > diff --git a/drivers/gpu/drm/panthor/panthor_sched.c > b/drivers/gpu/drm/panthor/panthor_sched.c > index 13c12a24e2f45..bfdabd7ee7beb 100644 > --- a/drivers/gpu/drm/panthor/panthor_sched.c > +++ b/drivers/gpu/drm/panthor/panthor_sched.c [ ... ] > @@ -4074,6 +4079,7 @@ static void add_group_kbo_sizes(struct panthor_device > *ptdev, > } > > #define MAX_GROUPS_PER_POOL 128 > +#define GROUP_CREATE_FLAGS DRM_PANTHOR_GROUP_CREATE_PROTECTED [Severity: Low] Is this macro needed? The identifier DRM_PANTHOR_GROUP_CREATE_PROTECTED is not defined anywhere in the codebase, and GROUP_CREATE_FLAGS is never used. While it has no runtime effect, it is a compilation hazard if the macro is ever evaluated and constitutes dead code. > > int panthor_group_create(struct drm_file *file, > const struct drm_panthor_group_create *group_args, [ ... ] > @@ -4136,14 +4138,51 @@ int panthor_group_create(struct drm_file *file, > goto err_put_group; > } > > - suspend_size = csg_iface->control->suspend_size; > - group->suspend_buf = panthor_fw_alloc_suspend_buf_mem(ptdev, > suspend_size); > + group->suspend_buf = panthor_fw_alloc_suspend_buf_mem( > + ptdev, panthor_fw_suspend_size(ptdev)); > if (IS_ERR(group->suspend_buf)) { > ret = PTR_ERR(group->suspend_buf); > group->suspend_buf = NULL; > goto err_put_group; > } > > + if (group_args->protected_suspend_bo_handle) { > + struct drm_gem_object *obj; > + > + /* Read a correct snapshot of the state */ > + if (!(smp_load_acquire(&ptdev->protm.info.state) & > + DRM_PANTHOR_PROTM_INITIALIZED)) { > + ret = -EINVAL; > + goto err_put_group; > + } > + > + obj = drm_gem_object_lookup( > + file, group_args->protected_suspend_bo_handle); > + if (!obj) { > + ret = -ENOENT; > + goto err_put_group; > + } > + > + if (obj->size < > + ptdev->protm.info.group_protected_suspend_buf_size) { > + drm_gem_object_put(obj); > + ret = -EINVAL; > + goto err_put_group; > + } > + > + group->protm_suspend_buf = panthor_kernel_bo_create_from_gem( > + ptdev, panthor_fw_vm(ptdev), to_panthor_bo(obj), > + DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC, > + PANTHOR_VM_KERNEL_AUTO_VA, > + panthor_fw_protm_suspend_size(ptdev)); [Severity: High] Does this code map the user-provided BO into the firmware VM without validating that it actually corresponds to a protected DMA-BUF heap? The uAPI comments state this BO must be allocated from a protected DMA-BUF heap, but there doesn't appear to be any verification of its security properties or whether it's actually backed by protected memory. If the Mali firmware writes protected state into this suspend buffer during context switches, could a malicious userspace application supply a standard shmem-backed BO to read the protected state from normal RAM? Even if the GPU hardware traps non-secure accesses to secure physical addresses, should the kernel verify the user-provided memory handle is actually secure before using it for firmware structures? (Note that a similar missing validation seems to exist for the firmware protected sections initialization in panthor_fw_protm_init(), though that path is protected by CAP_SYS_MODULE). > + drm_gem_object_put(obj); > + if (IS_ERR(group->protm_suspend_buf)) { > + ret = PTR_ERR(group->protm_suspend_buf); > + group->protm_suspend_buf = NULL; > + goto err_put_group; > + } > + } > + > group->syncobjs = panthor_kernel_bo_create(ptdev, group->vm, > group_args->queues.count * > sizeof(struct > panthor_syncobj_64b), -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=11
