Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] drm/amdgpu: init-only lockdown check in amdgpu_debugfs_regs_init() bypassed --
commit d191beaf95739f7c453353f0fddff9c76024a56e Author: Asad Kamal <[email protected]> drm/amdgpu: Gate debugfs MMIO access on kernel lockdown This commit skips creating hardware register debugfs nodes in amdgpu_debugfs_regs_init() when kernel lockdown (LOCKDOWN_PCI_ACCESS) is active during initialization. > drm/amdgpu: Gate debugfs MMIO access on kernel lockdown > > [ Upstream commit 17ac73b24006700f50972d37e297dec1f523c14a ] > > amdgpu_regs, amdgpu_regs2, and related debugfs nodes allow > arbitrary MMIO read/write via RREG32/WREG32 without checking > security_locked_down(). On kernel_lockdown=integrity systems > this bypasses the same restrictions as /dev/mem and PCI config > space sysfs. > > Check LOCKDOWN_PCI_ACCESS (matching pci-sysfs) at the entry of every > debugfs handler that performs direct register access. > > v2: Use consistent check as per previous check to use > LOCKDOWN_DEBUGFS(Lijo) > > v3: Do not create any entry from amdgpu_debugfs_regs_init() if > LOCKDOWN_PCI_ACCESS is active and log once. (Lijo) > > Signed-off-by: Asad Kamal <[email protected]> > Reviewed-by: Lijo Lazar <[email protected]> > Signed-off-by: Alex Deucher <[email protected]> > Signed-off-by: Sasha Levin <[email protected]> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > index a70651050acff..5dac8f0f34143 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c [ ... ] > @@ -1646,6 +1647,12 @@ int amdgpu_debugfs_regs_init(struct amdgpu_device > *adev) > struct dentry *ent, *root = minor->debugfs_root; > unsigned int i; > > + if (security_locked_down(LOCKDOWN_PCI_ACCESS)) { > + drm_info(adev_to_drm(adev), > + "amdgpu: HW debugfs nodes disabled (kernel > lockdown)\n"); > + return 0; > + } > + > for (i = 0; i < ARRAY_SIZE(debugfs_regs); i++) { > ent = debugfs_create_file(debugfs_regs_names[i], > S_IFREG | 0400, root, [Severity: Medium] Can this lockdown check be bypassed when kernel lockdown is enabled at runtime after the driver has already probed? If amdgpu initializes before lockdown is enabled (for example, when built-in or loaded from initramfs) and userspace later enables integrity or confidentiality lockdown by writing to /sys/kernel/security/lockdown via lockdown_write() -> lock_kernel_down(), amdgpu_debugfs_regs_init() has already created the debugfs_regs files with mode 0400: security/lockdown/lockdown.c:lockdown_write() { ... if (label && !strcmp(state, label)) err = lock_kernel_down("securityfs", level); ... } Because amdgpu_regs, amdgpu_regs_didt, amdgpu_regs_pcie, amdgpu_regs_smc, amdgpu_wave, and amdgpu_gpr have mode 0400 and their file_operations structs do not define .unlocked_ioctl, .compat_ioctl, or .mmap, opening them with O_RDONLY is permitted by debugfs_locked_down() without consulting security_locked_down(LOCKDOWN_DEBUGFS): fs/debugfs/file.c:debugfs_locked_down() { if ((inode->i_mode & 07777 & ~0444) == 0 && !(filp->f_mode & FMODE_WRITE) && (!real_fops || (!real_fops->unlocked_ioctl && !real_fops->compat_ioctl && !real_fops->mmap))) return 0; if (security_locked_down(LOCKDOWN_DEBUGFS)) return -EPERM; return 0; } Subsequent read() calls then reach amdgpu_debugfs_process_reg_op() and its sibling handlers with no security_locked_down(LOCKDOWN_PCI_ACCESS) check, allowing arbitrary RREG32() reads as well as MMIO writes to bank and index selection registers such as GRBM_GFX_INDEX via amdgpu_gfx_select_se_sh() and SRBM_GFX_CNTL via amdgpu_gfx_select_me_pipe_q(). In addition, any file descriptor opened O_RDWR before lockdown was activated retains unrestricted MMIO write and ioctl access (WREG32() / amdgpu_mm_wreg_mmio_rlc()) after lockdown is enabled. Also, the commit message body still states: "Check LOCKDOWN_PCI_ACCESS (matching pci-sysfs) at the entry of every debugfs handler that performs direct register access." Should security_locked_down(LOCKDOWN_PCI_ACCESS) also be checked in the debugfs file operations handlers (or should the commit message be updated to match the v3 implementation)? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
