Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] An unprivileged userspace process can trigger a full GPU reset by omitting the protected suspend buffer during group creation and then submitting commands that switch to protected mode. - [High] Slow reset path triggers a NULL pointer dereference on the uninitialized protected firmware section in `panthor_fw_init_section_mem`. - [Low] Data race on the `ptdev->protm.info.state` flag. - [Low] Mixing `guard()` and `goto`-based cleanup in `panthor_fw_protm_init` violates subsystem cleanup guidelines. --
commit b7ec40f4bb5c3ae17349c8d0c7a683e90abc16c4 Author: Boris Brezillon <[email protected]> Subject: drm/panthor: Expose protected rendering features This commit introduces extensions to the Panthor uAPI to support protected rendering. It adds a new IOCTL for providing protected firmware memory and extends group creation to allow userspace to provide a protected suspend buffer. > diff --git a/drivers/gpu/drm/panthor/panthor_fw.c > b/drivers/gpu/drm/panthor/panthor_fw.c > index 96770ce34da84..4b44baf82a037 100644 > --- a/drivers/gpu/drm/panthor/panthor_fw.c > +++ b/drivers/gpu/drm/panthor/panthor_fw.c [ ... ] > @@ -644,35 +677,36 @@ static int panthor_fw_load_section_entry(struct > panthor_device *ptdev, > section->name = name; > } > > - if (section_size) { > - u32 cache_mode = hdr.flags & > CSF_FW_BINARY_IFACE_ENTRY_CACHE_MODE_MASK; > - u32 vm_map_flags = 0; > - u64 va = hdr.va.start; > + section->size = section_size; > + section->va = hdr.va.start; > > - if (!(hdr.flags & CSF_FW_BINARY_IFACE_ENTRY_WR)) > - vm_map_flags |= DRM_PANTHOR_VM_BIND_OP_MAP_READONLY; > + if (hdr.flags & CSF_FW_BINARY_IFACE_ENTRY_PROT) { > + if (ptdev->fw->protm_section) { > + drm_err(&ptdev->base, > + "Only one protected section supported\n"); > + return -EINVAL; > + } > > - if (!(hdr.flags & CSF_FW_BINARY_IFACE_ENTRY_EX)) > - vm_map_flags |= DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC; > + ptdev->protm.info.fw_protected_sections_size = > + ALIGN(section->size, vm_pgsz); > + ptdev->fw->protm_section = section; > + } > > - /* TODO: CSF_FW_BINARY_IFACE_ENTRY_CACHE_MODE_*_COHERENT are > mapped to > - * non-cacheable for now. We might want to introduce a new > - * IOMMU_xxx flag (or abuse IOMMU_MMIO, which maps to device > - * memory and is currently not used by our driver) for > - * AS_MEMATTR_AARCH64_SHARED memory, so we can take benefit > - * of IO-coherent systems. > - */ > - if (cache_mode != CSF_FW_BINARY_IFACE_ENTRY_CACHE_MODE_CACHED) > - vm_map_flags |= DRM_PANTHOR_VM_BIND_OP_MAP_UNCACHED; > + /* Defer the section->mem creation if this is a protected entry. > + * This will be populated when DRM_IOCTL_PANTHOR_PROTM_INIT is called. > + */ > + if (section->size && !(hdr.flags & CSF_FW_BINARY_IFACE_ENTRY_PROT)) { > + u32 vm_map_flags = section_vm_map_flags(section); [Severity: High] Can this cause a kernel panic if a GPU hang occurs before the protected mode is initialized? Because section->mem is intentionally left as NULL here for protected sections, if an unprivileged user triggers a GPU hang before calling the PROTM_INIT ioctl, the slow reset path will be invoked. When panthor_reload_fw_sections() iterates over all sections, it calls panthor_fw_init_section_mem() which dereferences section->mem without checking if it is valid (e.g., bool was_mapped = !!section->mem->kmap). Would it make sense to skip uninitialized protected sections during reset? [ ... ] > @@ -1566,6 +1605,66 @@ int panthor_fw_protm_exit(struct panthor_device > *ptdev, u32 timeout_ms) > return ret; > } > > +int panthor_fw_protm_init(struct drm_file *file, > + struct drm_panthor_protm_init *args) > +{ > + struct panthor_file *pfile = file->driver_priv; > + struct panthor_device *ptdev = pfile->ptdev; > + struct panthor_fw_section *protm_section = ptdev->fw->protm_section; > + struct drm_gem_object *obj; > + u32 vm_map_flags; > + int cookie, ret = 0; > + > + if (!capable(CAP_SYS_MODULE)) > + return -EPERM; > + > + if (args->pad) > + return -EINVAL; > + > + if (!protm_section || !protm_section->size) > + return -EINVAL; > + > + guard(mutex)(&ptdev->fw->protm_init_lock); [Severity: Low] Is it safe to mix guard() and goto-based cleanup in the same function? The cleanup subsystem guidelines suggest avoiding the mixture of scope-based locks like guard(mutex) with goto labels (such as out_gem_put and out_dev_exit) in the same routine, as it can lead to confusing ownership semantics and future maintenance bugs. > + > + if (ptdev->protm.info.state & DRM_PANTHOR_PROTM_INITIALIZED) > + return 0; > + > + if (!drm_dev_enter(&ptdev->base, &cookie)) > + return -ENODEV; > + > + obj = drm_gem_object_lookup(file, > + args->fw_protected_sections_bo_handle); > + if (!obj) { > + ret = -ENOENT; > + goto out_dev_exit; > + } > + > + if (obj->size < ptdev->protm.info.fw_protected_sections_size) { > + ret = -EINVAL; > + goto out_gem_put; > + } > + > + vm_map_flags = section_vm_map_flags(protm_section); > + > + protm_section->mem = panthor_kernel_bo_import( > + ptdev, panthor_fw_vm(ptdev), to_panthor_bo(obj), vm_map_flags, > + protm_section->va, protm_section->size); > + if (IS_ERR(protm_section->mem)) { > + ret = PTR_ERR(protm_section->mem); > + protm_section->mem = NULL; > + goto out_gem_put; > + } > + > + ptdev->protm.info.state |= DRM_PANTHOR_PROTM_INITIALIZED; [Severity: Low] Could this non-atomic modification cause torn reads or compiler reordering issues? This updates ptdev->protm.info.state using a bitwise OR, but the same state flag is read concurrently in cs_slot_process_protm_pending_event_locked() without locks or READ_ONCE(). > + > +out_gem_put: > + drm_gem_object_put(obj); > + > +out_dev_exit: > + drm_dev_exit(cookie); > + return ret; > +} [ ... ] > diff --git a/drivers/gpu/drm/panthor/panthor_sched.c > b/drivers/gpu/drm/panthor/panthor_sched.c > index 1fe77e5c41995..d73fc2f6633a6 100644 > --- a/drivers/gpu/drm/panthor/panthor_sched.c > +++ b/drivers/gpu/drm/panthor/panthor_sched.c [ ... ] > @@ -1429,10 +1429,15 @@ cs_slot_process_protm_pending_event_locked(struct > panthor_device *ptdev, > if (!group) > return; > > - /* Do not allow user space work to switch into protected mode, as we > - * do not fully support this quite yet. > + /* Do not allow user space work to switch into protected mode if we > + * do not support protected mode on this device. > + * User space should query (and init) this support before attempting > + * to use such GPU instructions. > */ > - atomic_or(BIT(cs_id), &group->fatal_queues); > + if (!(ptdev->protm.info.state & DRM_PANTHOR_PROTM_INITIALIZED)) > + atomic_or(BIT(cs_id), &group->fatal_queues); > + else > + atomic_or(BIT(cs_id), &group->protm_pending_queues); > > sched_queue_delayed_work(sched, tick, 0); > } [Severity: High] Does this allow an unprivileged userspace process to trigger a full system-wide GPU reset? The logic here approves a transition to protected mode by only checking the global DRM_PANTHOR_PROTM_INITIALIZED flag. However, if a user creates a group with protected_suspend_bo_handle set to 0 (resulting in group->protm_suspend_buf being NULL), they can still submit a PROT_REGION command. Because the scheduler does not verify if the specific group provided a valid suspend buffer, the hardware interface gets programmed with a zero address for the suspend buffer in panthor_group_bind_locked(). When the firmware later attempts to suspend the group, it writes to address 0, causing an unrecoverable exception and Denial of Service. Should this also check if group->protm_suspend_buf is valid? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
