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

[GONG Ruiqi <gongruiqi1@huawei.com>]

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@huawei.com/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.

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

>                                                                                 
> - 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