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

[Nayna Jain <nayna@linux.ibm.com>]

On 7/2/25 10:07 PM, 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.
Thanks for sharing additional details.

 From x86 Kconfig:

/For config x86:

     imply IMA_SECURE_AND_OR_TRUSTED_BOOT    if EFI
/
And IMA_SECURE_AND_OR_TRUSTED_BOOT is dependent on IMA_ARCH_POLICY .

And from Linux Kernel Kbuild documentation( 
https://docs.kernel.org/kbuild/kconfig-language.html) :

/weak reverse dependencies: “imply” <symbol> [“if” <expr>]

This is similar to “select” as it enforces a lower limit on another 
symbol except that the “implied” symbol’s value may still be set to n 
from a direct dependency or with a visible prompt.

/Following the example from the documentation, if  it is EFI enabled and 
IMA_ARCH_POLICY is set to y then this config should be default enabled.

If it is EFI enabled and IMA_ARCH_POLICY is set to N, then the setting 
for IMA_SECURE_AND_OR_TRUSTED_BOOT should be prompted during the build. 
The default setting for prompt is N. So, the person doing the build 
should actually select Y to enable IMA_ARCH_POLICY.

Wondering what is the scenario for you? Unless you have IMA_ARCH_POLICY 
set to N, this config should have been ideally enabled. If you have 
explicitly set it to N, am curious any specific reason for that.

Thanks & Regards,

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