Re: [PATCH v2] integrity: Extract secure boot enquiry function out of IMA

[Mimi Zohar <zohar@linux.ibm.com>]

[CC: Nayna Jain]

On Sat, 2025-06-28 at 14:32 +0800, GONG Ruiqi wrote:
> Commit 92ad19559ea9 ("integrity: Do not load MOK and MOKx when secure
> boot be disabled") utilizes arch_ima_get_secureboot() to perform a
> secure boot status check before loading the Machine Owner Key (MOK).
> However, only when CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT=y can this
> function be functional, while this config has nothing to do with secure
> boot or MOK loading.
> 
> Given that arch_ima_get_secureboot() is just a helper to retrieve info
> about secure boot via EFI and doesn't necessarily be a part of IMA,
> rename it to arch_integrity_get_secureboot(), decouple its functionality
> from IMA and extract it to be a integrity subsystem helper, so that both
> certificate loading and IMA can make use of it.
> 
> Compile-tested on powerpc, s390 and x86, with CONFIG_IMA_ARCH_POLICY=n
> and =y based on defconfig and allmodconfig.
> 
> Signed-off-by: GONG Ruiqi <gongruiqi1@huawei.com>

The original reason for querying the secure boot status of the system was in
order to differentiate IMA policies.  Subsequently, the secure boot check was
also added to safely allow loading of the certificates stored in MOK. So loading
IMA policies and the MOK certificates ARE dependent on the secure boot mode.
                                                                                
What is your real motivation for moving the secure boot checking out of IMA?    
                                                                                
FYI, there are a number of problems with the patch itself.  From a very high
level:  
                                                                                
- The EFI secure boot check is co-located with loading the architecture specific
policies.  By co-locating the secure boot check with loading the architecture
specific IMA policies, there aren't any ifdef's in C code.  Please refer to the
"conditional compilation" section in the kernel coding-style documentation on
avoiding ifdef's in C code.
                                                                                
- Each architecture has it's own method of detecting secure boot. Originally the
x86 code was in arch/x86, but to prevent code duplication it was moved to IMA. 
The new file should at least be named efi_secureboot.c.  
                                                                                
- The patch title should be about moving and renaming the secure boot check. 
The patch description should include a valid reason for the change.

Mimi & Nayna