Re: [PATCH] init/main.c: Initialize early LSMs after arch code

[Guenter Roeck <linux@roeck-us.net>]

On 8/12/24 15:02, KP Singh wrote:
> On Mon, Aug 12, 2024 at 11:33 PM Paul Moore <paul@paul-moore.com> wrote:
>>
>> On Mon, Aug 12, 2024 at 5:14 PM KP Singh <kpsingh@kernel.org> wrote:
>>> On Mon, Aug 12, 2024 at 9:33 PM Paul Moore <paul@paul-moore.com> wrote:
>>>> On Mon, Aug 12, 2024 at 1:12 PM KP Singh <kpsingh@kernel.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@kpsingh:~/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)                                        \
         (struct lsm_static_call) {                                      \
                 .key = &STATIC_CALL_KEY(LSM_STATIC_CALL(NAME, NUM)),    \

Guenter