On Tue, Jul 9, 2024 at 8:36 AM KP Singh [off-list ref] wrote:

quoted

quoted

--- a/security/security.c
+++ b/security/security.c

...

quoted

quoted

-#define lsm_for_each_hook(scall, NAME)                                       \
-     for (scall = static_calls_table.NAME;                           \
-          scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++)  \
-             if (static_key_enabled(&scall->active->key))
+/*
+ * Can be used in the context passed to lsm_for_each_hook to get the lsmid of the
+ * current hook
+ */
+#define current_lsmid() _hook_lsmid

See my comments below about security_getselfattr(), I think we can drop
the current_lsmid() macro.  If we really must keep it, we need to rename
it to something else as it clashes too much with the other current_XXX()
macros/functions which are useful outside of our wacky macros.

call_hook_with_lsmid is a pattern used by quite a few hooks, happy to
update the name.

What do you think about __security_hook_lsm_id().

I guess we can't get rid of it due to the crazy macro stuff with loop
unrolling, BEFORE/AFTER blocks, etc.  Ooof.  If you were looking for
another example of why I don't really like these patches, this would
be a good candidate.

More below ...

quoted

I know I was the one who asked to implement the static_calls for *all*
of the LSM functions - thank you for doing that - but I think we can
all agree that some of the resulting code is pretty awful.  I'm probably
missing something important, but would an apporach similar to the pseudo
code below work?

This does not work.

The special macro you are defining does not have the static_call
invocation and if you add that bit it's basically the __CALL_HOOK
macro or __CALL_STATIC_INT, __CALL_STATIC_VOID macro inlined
everywhere, I tried implementing it but it gets very dirty.

Thanks for testing it out.  Perhaps trying to move all of these hooks
to use the static_call approach was a mistake.  I realize you're doing
your best adapting the static_call API to support multiple LSMs, but
it just doesn't look like a good fit to me for the "unconventional"
hooks here in this patch.

quoted

I think we may need to admit defeat with security_getselfattr() and
leave it as-is, the above is just too ugly to live.  I'd suggest
adding a comment explaining that it wasn't converted due to complexity
and the resulting awfulness.

I think your position on fixing everything is actually a valid one for
security, which is why I did not contest it.

The security_getselfattr is called very close to the syscall boundary
and the closer to the boundary the call is, the greater control the
attacker has over arguments and the easier it is to mount the attack.
This is why LSM indirect calls are a lucrative target because they
happen fairly early in the transition from user to kernel.
security_getselfattr is literally just in a SYSCALL_DEFINE

I recognize that your comments are in reference to that last flaw
rooted in the hardware that used indirect calls at an attack vector,
but wasn't that resolved through other means?  I never saw the PoC or
had time to follow up on whatever mitigation was ultimately merged (if
any).  However, my understanding is that the move to static_calls is
not strictly necessary to patch over that particular hardware flaw, it
is just a we-really-want-this for either a performance or a
non-specific security reason; pick your favorite  of the two based on
your audience.

Regardless, since none of the previous suggestions/options proved to
be workable, I'm going to suggest we just kill this patch too and move
forward with the others.  I had hoped we could get the changes in this
patch cleaned up, but it doesn't look like that is going to be the
case, or at least not within a week or two, so let's drop it and we
can always reconsider this in the future if a cleaner implementation
is presented.

-- 
paul-moore.com