Re: [PATCH] init/main.c: Initialize early LSMs after arch code
From: KP Singh <kpsingh@kernel.org>
Date: 2024-08-13 15:56:32
Also in:
lkml
On Tue, Aug 13, 2024 at 6:08 AM Guenter Roeck [off-list ref] wrote:
quoted hunk ↗ jump to hunk
On 8/12/24 15:02, KP Singh wrote:quoted
On Mon, Aug 12, 2024 at 11:33 PM Paul Moore [off-list ref] wrote:quoted
On Mon, Aug 12, 2024 at 5:14 PM KP Singh [off-list ref] wrote:quoted
On Mon, Aug 12, 2024 at 9:33 PM Paul Moore [off-list ref] wrote:quoted
On Mon, Aug 12, 2024 at 1:12 PM KP Singh [off-list ref] wrote:quoted
JFYI, I synced with Guenter and all arch seem to pass and alpha does not work due to a reason that I am unable to debug. I will try doing more debugging but I will need more alpha help here (Added the maintainers to this thread).Thanks for the update; I was hoping that we might have a resolution for the Alpha failure by now but it doesn't look like we're that lucky. Hopefully the Alpha devs will be able to help resolve this without too much trouble. Unfortunately, this does mean that I'm going to drop the static call patches from the lsm/dev branch so that we can continue merging other things. Of course this doesn't mean the static call patches can't come back in later during this dev cycle once everything is solved if there is still time, and worst case there is always the next dev cycle.Do we really want to drop them for alpha? I would rather disable CONFIG_SECURITY for alpha and if people really care for alpha we can enable it. Alpha folks, what do you think?Seriously? I realize Alpha is an older, lesser used arch, but it is still a supported arch and we are not going to cause a regression for the sake of a new feature. As I mentioned earlier, once the problem is resolved we can bring the patchset back into lsm/dev; if it gets resolved soon enough we can even do it during this dev cycle.Okay, more data for the alpha folks, when I moved trap_init() before early_security_init() everything seemed to work, I think we might need to call trap_init() from setup_arch and this would fix the issue. As to why? I don't know :) Would alpha folks be okay with this patch: kpsingh@kpsingh:~/projects/linux$ git diffdiff --git a/arch/alpha/kernel/setup.c b/arch/alpha/kernel/setup.c index bebdffafaee8..53909c1be4cf 100644 --- a/arch/alpha/kernel/setup.c +++ b/arch/alpha/kernel/setup.c@@ -657,6 +657,7 @@ setup_arch(char **cmdline_p) setup_smp(); #endif paging_init(); + trap_init(); }and provide me some reason as to why this works, it would be great for a patch descriptionYour code triggers a trap (do_entUna, unaligned access) which isn't handled unless trap_init() has been called before. Reason is that static_calls_table is not 8-byte aligned, causing the unaligned access in: static void __init lsm_static_call_init(struct security_hook_list *hl) { struct lsm_static_call *scall = hl->scalls; int i; for (i = 0; i < MAX_LSM_COUNT; i++) { /* Update the first static call that is not used yet */ if (!scall->hl) { <-- here __static_call_update(scall->key, scall->trampoline, hl->hook.lsm_func_addr); scall->hl = hl; static_branch_enable(scall->active); return; } scall++; } panic("%s - Ran out of static slots.\n", __func__); } A somewhat primitive alternate fix is:diff --git a/security/security.c b/security/security.c index aa059d0cfc29..dea9736b2014 100644 --- a/security/security.c +++ b/security/security.c@@ -156,7 +156,7 @@ static __initdata struct lsm_info *exclusive; * and a trampoline (STATIC_CALL_TRAMP) which are used to call * __static_call_update when updating the static call. */ -struct lsm_static_calls_table static_calls_table __ro_after_init = { +struct lsm_static_calls_table static_calls_table __ro_after_init __attribute__((aligned(8))) = { #define INIT_LSM_STATIC_CALL(NUM, NAME) \
I think it's worth making it aligned at 8 byte, a much simpler fix than the arch change. Paul, I will rebase my series with these patches, better descriptions and post them later today. - KP
(struct lsm_static_call) { \
.key = &STATIC_CALL_KEY(LSM_STATIC_CALL(NAME, NUM)), \
Guenter