[PATCH] security: fix no-op hook logic in security_inode_{set, remove}xattr()

[Paul Moore]

On Mon, Jan 29, 2024 at 8:31 AM Ondrej Mosnacek <omosnace at redhat.com> wrote:
>
> These two hooks currently work like this:
> 1. If no LSM registers the hook, cap_inode_{set,remove}xattr() is
>    called.
> 2. If an LSM hook call returns 0, the loop continues to call further
>    LSMs (and only stops on an error return value).
> 3. The "default" return value is 0, so e.g. the default BPF LSM hook
>    just returns 0.
>
> This works if BPF LSM is enabled along with SELinux or SMACK (or not
> enabled at all), but if it's the only LSM implementing the hook, then
> the cap_inode_{set,remove}xattr() is erroneously skipped.
>
> Fix this by using 1 as the default return value and make the loop
> recognize it as a no-op return value (i.e. if an LSM returns this value
> it is treated as if it wasn't called at all). The final logic is similar
> to that of security_fs_context_parse_param().
>
> Fixes: 98e828a0650f ("security: Refactor declaration of LSM hooks")
> Signed-off-by: Ondrej Mosnacek <omosnace at redhat.com>
> ---
>  include/linux/lsm_hook_defs.h |  4 ++--
>  security/security.c           | 45 +++++++++++++++++++++++++----------
>  2 files changed, 35 insertions(+), 14 deletions(-)

Thanks for working on this Ondrej, I've got a couple of thoughts on
the approach taken here, but we definitely need to fix this.

My first thought is that we really should move the
cap_inode_setxattr() and cap_inode_removexattr() calls in security.c
over to using the LSM hook infrastructure just as we do with other
capability hooks in commoncap.c:

  LSM_HOOK_INIT(inode_setxattr, cap_inode_setxattr);
  LSM_HOOK_INIT(inode_removexattr, cap_inode_removexattr);

... of course we will need to adjust cap_inode_setxattr to take (and
ignore the idmap) parameter, but that is easy enough.  It looks like
cap_inode_removexattr() can be used as-is.  Modifications to the only
two LSMs, SELinux and Smack, which explicitly call out to these
capability hooks looks rather straightforward as well.  Doing this
should simplify the LSM hooks significantly, and lower the chance of a
future LSM mistakenly not doing the required capability calls.  There
should also be a slight performance bump for the few (one? two?)
people running both SELinux and Smack in a production environment.

My second thought is that we *really* need to add to the function
header block comment/description for both these hooks.  Of course the
details here will change depending on the bits above about the
capability hooks, but if we need any special handling like you're
proposing here we really should document it in the hook's header
block.

-- 
paul-moore.com