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

[Mimi Zohar]

On Thu, 2025-07-03 at 10:07 +0800, GONG Ruiqi wrote:
> Hi Mimi,
> 
> On 7/3/2025 9:38 AM, Mimi Zohar wrote:
> > [CC: Nayna Jain]
> > 
> > On Sat, 2025-06-28 at 14:32 +0800, GONG Ruiqi wrote:
> > > ...
> > 
> > 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?    
> >                                                                                 
> 
> Sorry for not stating that clearly in this patch. I think the cover
> letter of V3 I just sent few minutes ago can answer your question, and I
> quote:
> 
> "We encountered a boot failure issue in an in-house testing, where the
> kernel refused to load its modules since it couldn't verify their
> signature. The root cause turned out to be the early return of
> load_uefi_certs(), where arch_ima_get_secureboot() returned false
> unconditionally due to CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT=n, even
> though the secure boot was enabled.
> 
> This patch set attempts to remove this implicit dependency by shifting
> the functionality of efi secure boot enquiry from IMA to the integrity
> subsystem, so that both certificate loading and IMA can make use of it
> independently."
> 
> Here's the link of V3, and please take a look:
> https://lore.kernel.org/all/20250703014353.3366268-1-gongruiqi1@xxxxxxxxxx/T/#mef6d5ea47a4ee19745c5292ab8948eba9e16628d
> 
> > 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.  
> 
> You're right. I didn't realize it's arch-specific in the first place,
> and moving and renaming arch_ima_get_secureboot() turned out to be a
> real mess ...
> 
> So the V3 keeps the prototype of arch_ima_get_secureboot(), and only
> moves out its body, which I think can also better represent the
> intention of the patch.

It's definitely much better.  To summarize, arch_ima_get_secureboot() becomes a
wrapper for integrity_get_efi_secureboot().  Before loading the MOK/MOKx keys,
load_uefi_certs() calls integrity_get_efi_secureboot() directly.

With load_uefi_certs() calling integrity_get_efi_secureboot() directly, please
check to see whether an integrity_get_efi_secureboot() stub function needs to be
defined.

Mimi

> 
> As of the name of the new file, as V3 has been sent earlier and still
> uses secureboot.c, I can't change it there. I can do it in V4.
> 
> -Ruiqi