On 7/2/25 10:07 PM, GONG Ruiqi wrote:

Hi Mimi,

On 7/3/2025 9:38 AM, Mimi Zohar wrote:

quoted

[CC: Nayna Jain]

On Sat, 2025-06-28 at 14:32 +0800, GONG Ruiqi wrote:

quoted

...

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 (local)

quoted

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

quoted

                                                                                 
- 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