On Tue, Jul 9, 2024 at 8:36 AM KP Singh <kpsingh@xxxxxxxxxx> wrote: > > > --- a/security/security.c > > > +++ b/security/security.c ... > > > -#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 ... > > 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. > > 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
