Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Critical] drm: kernel crash on FW reload due to mapping protected PROT section - [High] drm: data race on protm_section->mem causes ERR_PTR dereference - [Medium] drm: mixing goto-based cleanup and guard() in panthor_fw_protm_init - [Medium] drm/panthor: missing VA reservation for PROT section allows auto-VA clash - [Low] drm: data race in panthor_ioctl_dev_query copying PROTM_INFO --
commit 6a74417abb2feb49f589b33202b41b8e8a067ce4 Author: Boris Brezillon <[email protected]> Subject: drm/panthor: Support FW init of protm This commit adds support for initializing the Mali GPU firmware's protected mode (protm) memory section. It introduces user queries for protm state and a new ioctl for user-initiated protected mode initialization. > diff --git a/drivers/gpu/drm/panthor/panthor_drv.c > b/drivers/gpu/drm/panthor/panthor_drv.c > index 51a2e5358cd51..cc1c050ea3806 100644 > --- a/drivers/gpu/drm/panthor/panthor_drv.c > +++ b/drivers/gpu/drm/panthor/panthor_drv.c [ ... ] > @@ -991,6 +997,9 @@ static int panthor_ioctl_dev_query(struct drm_device > *ddev, void *data, struct d > case DRM_PANTHOR_DEV_QUERY_MMU_INFO: > return PANTHOR_UOBJ_SET(args->pointer, args->size, > ptdev->mmu_info); > > + case DRM_PANTHOR_DEV_QUERY_PROTM_INFO: > + return PANTHOR_UOBJ_SET(args->pointer, args->size, > ptdev->protm.info); [Severity: Low] Is it safe to directly pass ptdev->protm.info to PANTHOR_UOBJ_SET() here? Since panthor_fw_protm_init() updates the state field concurrently using smp_store_release(), this byte/word copy in copy_to_user() might race and copy torn state to userspace. Should this create a local copy using smp_load_acquire() before passing it to copy_to_user(), similar to how timestamp_info is handled? > + > default: > return -EINVAL; > } > diff --git a/drivers/gpu/drm/panthor/panthor_fw.c > b/drivers/gpu/drm/panthor/panthor_fw.c > index fc07796fc9ef7..0af6addfd55a5 100644 > --- a/drivers/gpu/drm/panthor/panthor_fw.c > +++ b/drivers/gpu/drm/panthor/panthor_fw.c [ ... ] > @@ -462,6 +470,9 @@ static void panthor_fw_init_section_mem(struct > panthor_device *ptdev, > !(section->flags & CSF_FW_BINARY_IFACE_ENTRY_ZERO)) > return; > > + if (!section->mem) > + return; [Severity: High] Does this check properly handle the error pointer state? If panthor_fw_protm_init() is currently executing concurrently, section->mem might briefly contain an ERR_PTR if panthor_kernel_bo_create_from_gem() fails. Since the reset worker reads section->mem without holding the protm_init_lock, could it observe the intermediate ERR_PTR, bypass this check, and crash when panthor_kernel_bo_vmap() dereferences it? > + > ret = panthor_kernel_bo_vmap(section->mem); > if (drm_WARN_ON(&ptdev->base, ret)) > return; > > memcpy(section->mem->kmap, section->data.buf, section->data.size); [Severity: Critical] Will this cause a secure fault (SError) or kernel panic during a GPU reset? If PROTM_INIT was successfully called by userspace, section->mem now holds protected memory. During a slow reset, panthor_reload_fw_sections() iterates over all firmware sections and unconditionally calls panthor_fw_init_section_mem(). If it processes the PROT section, it will attempt to vmap and memset/memcpy the protected memory from the CPU non-secure world. Should this function or the reload loop explicitly skip the CSF_FW_BINARY_IFACE_ENTRY_PROT section to avoid a secure fault? [ ... ] > @@ -678,7 +683,22 @@ static int panthor_fw_load_section_entry(struct > panthor_device *ptdev, > section->size = section_size; > section->va = hdr.va.start; > > - if (section_size) { > + 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; > + } > + > + ptdev->protm.info.fw_protected_sections_size = > + ALIGN(section->size, vm_pgsz); > + ptdev->fw->protm_section = section; > + } > + > + /* 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)) { [Severity: Medium] Does skipping the memory allocation here also unintentionally skip reserving the virtual address in the firmware VM? If the firmware binary places the protected mode (PROT) section within the kernel_auto_va range, and we skip panthor_kernel_bo_create() (which allocates the VA), could an unprivileged user allocating queues or suspend buffers steal this unreserved VA? This would later cause panthor_fw_protm_init() to fail with -ENOSPC when it attempts to reserve the exact VA. Should the VA still be reserved early, even if memory allocation is deferred? > u32 vm_map_flags = section_vm_map_flags(section); > > section->mem = panthor_kernel_bo_create( [ ... ] > @@ -1585,6 +1610,70 @@ int panthor_fw_protm_exit(struct panthor_device > *ptdev, u32 timeout_ms) > return panthor_fw_protm_exit_wait(ptdev, timeout_ms); > } > > +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: Medium] This isn't a bug, but this function mixes scope-based cleanup (guard(mutex)) with traditional goto cleanup labels (like goto out_gem_put). According to the cleanup helpers documentation, goto and scope-based cleanup should not be mixed in the same function. Could all resources in this function be converted to use scope-based cleanup to avoid ownership confusion? > + > + /* Thread safe access so other readers do not need protm_init_lock */ > + if (smp_load_acquire(&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_create_from_gem( > + 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; [Severity: High] Is it safe to assign directly to the globally visible protm_section->mem before checking for errors? If this allocation fails, protm_section->mem briefly holds an ERR_PTR until the error check clears it. A concurrent GPU reset worker, which reads section->mem without holding protm_init_lock, could observe this intermediate state and crash when trying to vmap it. Should the result be stored in a local variable and only assigned to protm_section->mem upon success? > + goto out_gem_put; > + } > + > + /* Make sure all above is completed before we update state */ > + smp_store_release(&ptdev->protm.info.state, > + DRM_PANTHOR_PROTM_INITIALIZED); > + > +out_gem_put: > + drm_gem_object_put(obj); > + > +out_dev_exit: > + drm_dev_exit(cookie); > + return ret; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=10
