[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CACYkzJ4eZWh2R_ZoeiNLLKFARWJOWo7Hkdp015fHEnmYLJaHGQ@mail.gmail.com>
Date: Tue, 13 Aug 2024 17:56:20 +0200
From: KP Singh <kpsingh@...nel.org>
To: Guenter Roeck <linux@...ck-us.net>
Cc: Paul Moore <paul@...l-moore.com>, Nathan Chancellor <nathan@...nel.org>, linux-kernel@...r.kernel.org,
linux-security-module@...r.kernel.org, bp@...en8.de, sfr@...b.auug.org.au,
peterz@...radead.org, ink@...assic.park.msu.ru, richard.henderson@...aro.org
Subject: Re: [PATCH] init/main.c: Initialize early LSMs after arch code
On Tue, Aug 13, 2024 at 6:08 AM Guenter Roeck <linux@...ck-us.net> wrote:
>
> On 8/12/24 15:02, KP Singh wrote:
> > On Mon, Aug 12, 2024 at 11:33 PM Paul Moore <paul@...l-moore.com> wrote:
> >>
> >> On Mon, Aug 12, 2024 at 5:14 PM KP Singh <kpsingh@...nel.org> wrote:
> >>> On Mon, Aug 12, 2024 at 9:33 PM Paul Moore <paul@...l-moore.com> wrote:
> >>>> On Mon, Aug 12, 2024 at 1:12 PM KP Singh <kpsingh@...nel.org> wrote:
> >>>>>
> >>>>> 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@...ingh:~/projects/linux$ git diff
> > diff --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 description
> >
>
> Your 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
>
Powered by blists - more mailing lists