Re: [PATCH AUTOSEL 6.18-5.10] drm/amdgpu: Gate debugfs MMIO access on kernel lockdown

[sashiko-bot@kernel.org]

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 <asad.kamal@amd.com>

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 <asad.kamal@amd.com>
>     Reviewed-by: Lijo Lazar <lijo.lazar@amd.com>
>     Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>     Signed-off-by: Sasha Levin <sashal@kernel.org>
> 
> 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/20261019135441.1420968-351-sashal@kernel.org?part=1