[PATCH RFC 0/3] security: allow a LSM to specify NO-OP return code

[Casey Schaufler]

On 8/3/2023 10:12 AM, Paolo Abeni wrote:
> This is another attempt to solve the current problem with eBPF LSM,
> already discussed at least in [1].
>
> The basic idea is to introduce the minimum amount of changes to let
> the core consider a no-op any LSM hooks returning the
> LSM_RET_DEFAULT [2].
>
> AFAICS that is already the case for most int hooks with LSM_RET_DEFAULT
> equal to 0 due to the current call_int_hook implementation. Even most
> int hook with non zero LSM_RET_DEFAULT are not problematic. Specifically
> the hooks [3]:
>
> fs_context_parse_param
> dentry_init_security
> inode_getsecurity
> inode_setsecurity
> inode_copy_up_xattr
> task_prctl
> security_secid_to_secctx 
>
> already have special handling for to basically ignore default return
> value from the LSMs, while:
>
> security_getprocattr
> security_setprocattr
>
> only operate on the specified LSM.
>
> The only hooks that need some love are:
>
> * hooks that have a 0 LSM_RET_DEFAULT, but with no LSM loaded returns a
>   non zero value to the security_<hook> caller:
> sb_set_mnt_opts
> inode_init_security
> inode_getsecctx
> socket_getpeersec_stream
> socket_getpeersec_dgram
>
> * hooks that have a 0 LSM_RET_DEFAULT, but internally security_<hook>
>   uses a non zero return value as a selector to perform a default
>   action:
> inode_setxattr
> inode_removexattr
>
> * hooks the somehow have to reconciliate multiple, non-zero, LSM return
>   values to take a single decision:
> vm_enough_memory
> xfrm_state_pol_flow_match
>
> This series introduces a new variant of the call_int_hook macro and
> changes the LSM_RET_DEFAULT for the mentioned hooks, to achieve the
> goal [2].
>
> The patches have been split according to the above grouping with the
> hope to simplify the reviews, but I guess could be squashed in a single
> one.
>
> A simple follow-up would be extend the new hook usage to the hooks [3]
> to reduce the code duplication.
>
> Sharing as an early RFC (with almost no testing) to try to understand if
> this path is a no go or instead is somewhat viable.

I am not an advocate of adding macros for these special cases.
The only reason the existing macros are used is that open coding
every hook with the exact same logic would have created an enormous
security.c file. Special cases shouldn't be hidden. The reason they
are special should be documented.

Should the stacking patch set ever come in there are going to be
more and more kinds of special cases. I don't see that adding code
macros for each of the peculiar behaviors is a good idea.

>
> [1] https://patchwork.kernel.org/project/netdevbpf/patch/20220609234601.2026362-1-kpsingh@kernel.org/
>
> Paolo Abeni (3):
>   security: introduce and use call_int_hook_ignore_default()
>   security: two more call_int_hook_ignore_default use-cases
>   security: more call_int_hook_ignore_default use-cases
>
>  include/linux/lsm_hook_defs.h | 16 +++----
>  security/security.c           | 83 ++++++++++++++++++-----------------
>  2 files changed, 51 insertions(+), 48 deletions(-)
>