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-gongrui...@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.
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