| Message ID | cover.1691082677.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | security: allow a LSM to specify NO-OP return code | expand |
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(-) >
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