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

[Paolo Abeni <pabeni@redhat.com>]

Hello,

On Mon, 2023-08-07 at 11:57 -0700, Casey Schaufler wrote:
> 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.

First things first, thank you for your feedback and I'm sorry for the
very late reply: I have been off for the past few weeks.

I'm unsure how to interpret the above: is that an explicit nack to this
approach, it that almost an ack modulo some needed cleanup or something
in between ?!? ;)

Regarding the new macro introduced in patch 1/3, I think of it more as
a generalization then a special case. In fact it could replace all the
existing:

	call_int_hook(/* */, 0, ...)

call sites with no functional changes expected (modulo bugs).

I avoided that change to keep the series small, but it could clean-up
the code in the longer run and help isolating which code really needs a
special case.

But I guess there is a certain degree of personal style preferences
with this kind changes.

Any additional feedback more then welcome!

Cheers,

Paolo