[PATCH v12 5/5] bpf: Only enable BPF LSM hooks when an LSM program is attached

[KP Singh]

On Tue, Jun 11, 2024 at 6:35 AM Paul Moore <paul at paul-moore.com> wrote:
>
> On May 15, 2024 KP Singh <kpsingh at kernel.org> wrote:
> >
> >

[...]

> > +/**
> > + * security_toggle_hook - Toggle the state of the LSM hook.
> > + * @hook_addr: The address of the hook to be toggled.
> > + * @state: Whether to enable for disable the hook.
> > + *
> > + * Returns 0 on success, -EINVAL if the address is not found.
> > + */
> > +int security_toggle_hook(void *hook_addr, bool state)
> > +{
> > +     struct lsm_static_call *scalls = ((void *)&static_calls_table);
>
> GCC (v14.1.1 if that matters) is complaining about casting randomized
> structs.  Looking quickly at the two structs, lsm_static_call and
> lsm_static_calls_table, I suspect the cast is harmless even if the
> randstruct case, but I would like to see some sort of fix for this so
> I don't get spammed by GCC every time I do a build.  On the other hand,
> if this cast really is a problem in the randstruct case we obviously
> need to fix that.
>

The cast is not a problem with rand struct, we are iterating through a
2 dimensional array and it does not matter in which order we iterate
the first dimension.

diff --git a/security/security.c b/security/security.c
index 2ee880b3a39a..4cc0e368d07f 100644
--- a/security/security.c
+++ b/security/security.c
@@ -899,23 +899,24 @@ int lsm_fill_user_ctx(struct lsm_ctx __user
*uctx, u32 *uctx_len,
  */
 int security_toggle_hook(void *hook_addr, bool state)
 {
-       struct lsm_static_call *scalls = ((void *)&static_calls_table);
+       struct lsm_static_call *scall;
+       void *scalls_table = ((void *)&static_calls_table);
        unsigned long num_entries =
                (sizeof(static_calls_table) / sizeof(struct lsm_static_call));
        int i;

        for (i = 0; i < num_entries; i++) {
-
-               if (!scalls[i].hl || !scalls[i].hl->runtime)
+               scall = scalls_table + (i * sizeof(struct lsm_static_call));
+               if (!scall->hl || !scall->hl->runtime)
                        continue;

-               if (scalls[i].hl->hook.lsm_func_addr != hook_addr)
+               if (scall->hl->hook.lsm_func_addr != hook_addr)
                        continue;

                if (state)
-                       static_branch_enable(scalls[i].active);
+                       static_branch_enable(scall->active);
                else
-                       static_branch_disable(scalls[i].active);
+                       static_branch_disable(scall->active);
                return 0;
        }
        return -EINVAL;

fixes the error. I will respin.




> Either way, resolve this and make sure you test with GCC/randstruct
> enabled.
>
> > +     unsigned long num_entries =
> > +             (sizeof(static_calls_table) / sizeof(struct lsm_static_call));
> > +     int i;
> > +
> > +     for (i = 0; i < num_entries; i++) {
> > +
> > +             if (!scalls[i].hl || !scalls[i].hl->runtime)
> > +                     continue;
> > +
> > +             if (scalls[i].hl->hook.lsm_func_addr != hook_addr)
> > +                     continue;
> > +
> > +             if (state)
> > +                     static_branch_enable(scalls[i].active);
> > +             else
> > +                     static_branch_disable(scalls[i].active);
> > +             return 0;
> > +     }
> > +     return -EINVAL;
> > +}
> > +
> >  /*
> >   * The default value of the LSM hook is defined in linux/lsm_hook_defs.h and
> >   * can be accessed with:
> > --
> > 2.45.0.rc1.225.g2a3ae87e7f-goog
>
> --
> paul-moore.com