* [PATCH RFC 0/3] security: allow a LSM to specify NO-OP return code
@ 2023-08-03 17:12 Paolo Abeni
2023-08-03 17:12 ` [PATCH RFC 1/3] security: introduce and use call_int_hook_ignore_default() Paolo Abeni
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Paolo Abeni @ 2023-08-03 17:12 UTC (permalink / raw)
To: linux-security-module
Cc: Paul Moore, James Morris, Serge E. Hallyn, Ondrej Mosnacek,
KP Singh
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.
[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(-)
--
2.41.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH RFC 1/3] security: introduce and use call_int_hook_ignore_default()
2023-08-03 17:12 [PATCH RFC 0/3] security: allow a LSM to specify NO-OP return code Paolo Abeni
@ 2023-08-03 17:12 ` Paolo Abeni
2023-08-03 17:12 ` [PATCH RFC 2/3] security: two more call_int_hook_ignore_default use-cases Paolo Abeni
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Paolo Abeni @ 2023-08-03 17:12 UTC (permalink / raw)
To: linux-security-module
Cc: Paul Moore, James Morris, Serge E. Hallyn, Ondrej Mosnacek,
KP Singh
The following hooks currently don't allow the LSM module to tell the
core to ignore its return code:
sb_set_mnt_opts
inode_init_security
inode_getsecctx
socket_getpeersec_stream
socket_getpeersec_dgram
because the return value for the security function when no LSMs are
loaded is non zero, and LSMs use the current LSM_RET_DEFAULT() (zero)
to represent "success".
Introduce a new variant of the call_int_hook() macros the explicitly
ignores the return code from the LSM when equal to LSM_RET_DEFAULT,
use it for the above hooks, and change the default to 1.
All the exiting LSM except BPF don't use such return value, so no
functional change is expected.
After this change, LSM returning the LSM_RET_DEFAULT value will become
a no-op for the mentioned hooks.
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
include/linux/lsm_hook_defs.h | 10 ++++-----
security/security.c | 42 ++++++++++++++++++++++++++---------
2 files changed, 36 insertions(+), 16 deletions(-)
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 4f2621e87634..c9032e20d0b3 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -73,7 +73,7 @@ LSM_HOOK(int, 0, sb_mount, const char *dev_name, const struct path *path,
LSM_HOOK(int, 0, sb_umount, struct vfsmount *mnt, int flags)
LSM_HOOK(int, 0, sb_pivotroot, const struct path *old_path,
const struct path *new_path)
-LSM_HOOK(int, 0, sb_set_mnt_opts, struct super_block *sb, void *mnt_opts,
+LSM_HOOK(int, 1, sb_set_mnt_opts, struct super_block *sb, void *mnt_opts,
unsigned long kern_flags, unsigned long *set_kern_flags)
LSM_HOOK(int, 0, sb_clone_mnt_opts, const struct super_block *oldsb,
struct super_block *newsb, unsigned long kern_flags,
@@ -111,7 +111,7 @@ LSM_HOOK(int, 0, path_notify, const struct path *path, u64 mask,
unsigned int obj_type)
LSM_HOOK(int, 0, inode_alloc_security, struct inode *inode)
LSM_HOOK(void, LSM_RET_VOID, inode_free_security, struct inode *inode)
-LSM_HOOK(int, 0, inode_init_security, struct inode *inode,
+LSM_HOOK(int, 1, inode_init_security, struct inode *inode,
struct inode *dir, const struct qstr *qstr, const char **name,
void **value, size_t *len)
LSM_HOOK(int, 0, inode_init_security_anon, struct inode *inode,
@@ -272,7 +272,7 @@ LSM_HOOK(void, LSM_RET_VOID, release_secctx, char *secdata, u32 seclen)
LSM_HOOK(void, LSM_RET_VOID, inode_invalidate_secctx, struct inode *inode)
LSM_HOOK(int, 0, inode_notifysecctx, struct inode *inode, void *ctx, u32 ctxlen)
LSM_HOOK(int, 0, inode_setsecctx, struct dentry *dentry, void *ctx, u32 ctxlen)
-LSM_HOOK(int, 0, inode_getsecctx, struct inode *inode, void **ctx,
+LSM_HOOK(int, 1, inode_getsecctx, struct inode *inode, void **ctx,
u32 *ctxlen)
#if defined(CONFIG_SECURITY) && defined(CONFIG_WATCH_QUEUE)
@@ -308,9 +308,9 @@ LSM_HOOK(int, 0, socket_getsockopt, struct socket *sock, int level, int optname)
LSM_HOOK(int, 0, socket_setsockopt, struct socket *sock, int level, int optname)
LSM_HOOK(int, 0, socket_shutdown, struct socket *sock, int how)
LSM_HOOK(int, 0, socket_sock_rcv_skb, struct sock *sk, struct sk_buff *skb)
-LSM_HOOK(int, 0, socket_getpeersec_stream, struct socket *sock,
+LSM_HOOK(int, 1, socket_getpeersec_stream, struct socket *sock,
sockptr_t optval, sockptr_t optlen, unsigned int len)
-LSM_HOOK(int, 0, socket_getpeersec_dgram, struct socket *sock,
+LSM_HOOK(int, 1, socket_getpeersec_dgram, struct socket *sock,
struct sk_buff *skb, u32 *secid)
LSM_HOOK(int, 0, sk_alloc_security, struct sock *sk, int family, gfp_t priority)
LSM_HOOK(void, LSM_RET_VOID, sk_free_security, struct sock *sk)
diff --git a/security/security.c b/security/security.c
index 2dfc7b9f6ed9..b9a7b15e269e 100644
--- a/security/security.c
+++ b/security/security.c
@@ -784,6 +784,23 @@ static int lsm_superblock_alloc(struct super_block *sb)
RC; \
})
+#define call_int_hook_ignore_default(FUNC, IRC, ...) ({ \
+ int TRC, RC = IRC; \
+ do { \
+ struct security_hook_list *P; \
+ \
+ hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
+ TRC = P->hook.FUNC(__VA_ARGS__); \
+ if (TRC != LSM_RET_DEFAULT(FUNC)) \
+ continue; \
+ RC = TRC; \
+ if (RC != 0) \
+ break; \
+ } \
+ } while (0); \
+ RC; \
+})
+
/* Security operations */
/**
@@ -1405,9 +1422,9 @@ int security_sb_set_mnt_opts(struct super_block *sb,
unsigned long kern_flags,
unsigned long *set_kern_flags)
{
- return call_int_hook(sb_set_mnt_opts,
- mnt_opts ? -EOPNOTSUPP : 0, sb,
- mnt_opts, kern_flags, set_kern_flags);
+ return call_int_hook_ignore_default(sb_set_mnt_opts,
+ mnt_opts ? -EOPNOTSUPP : 0, sb,
+ mnt_opts, kern_flags, set_kern_flags);
}
EXPORT_SYMBOL(security_sb_set_mnt_opts);
@@ -1612,11 +1629,13 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
return 0;
if (!initxattrs)
- return call_int_hook(inode_init_security, -EOPNOTSUPP, inode,
- dir, qstr, NULL, NULL, NULL);
+ return call_int_hook_ignore_default(inode_init_security,
+ -EOPNOTSUPP, inode, dir,
+ qstr, NULL, NULL, NULL);
memset(new_xattrs, 0, sizeof(new_xattrs));
lsm_xattr = new_xattrs;
- ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir, qstr,
+ ret = call_int_hook_ignore_default(inode_init_security, -EOPNOTSUPP,
+ inode, dir, qstr,
&lsm_xattr->name,
&lsm_xattr->value,
&lsm_xattr->value_len);
@@ -3973,7 +3992,8 @@ EXPORT_SYMBOL(security_inode_setsecctx);
*/
int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
{
- return call_int_hook(inode_getsecctx, -EOPNOTSUPP, inode, ctx, ctxlen);
+ return call_int_hook_ignore_default(inode_getsecctx, -EOPNOTSUPP, inode,
+ ctx, ctxlen);
}
EXPORT_SYMBOL(security_inode_getsecctx);
@@ -4330,8 +4350,8 @@ EXPORT_SYMBOL(security_sock_rcv_skb);
int security_socket_getpeersec_stream(struct socket *sock, sockptr_t optval,
sockptr_t optlen, unsigned int len)
{
- return call_int_hook(socket_getpeersec_stream, -ENOPROTOOPT, sock,
- optval, optlen, len);
+ return call_int_hook_ignore_default(socket_getpeersec_stream, -ENOPROTOOPT,
+ sock, optval, optlen, len);
}
/**
@@ -4351,8 +4371,8 @@ int security_socket_getpeersec_stream(struct socket *sock, sockptr_t optval,
int security_socket_getpeersec_dgram(struct socket *sock,
struct sk_buff *skb, u32 *secid)
{
- return call_int_hook(socket_getpeersec_dgram, -ENOPROTOOPT, sock,
- skb, secid);
+ return call_int_hook_ignore_default(socket_getpeersec_dgram, -ENOPROTOOPT,
+ sock, skb, secid);
}
EXPORT_SYMBOL(security_socket_getpeersec_dgram);
--
2.41.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH RFC 2/3] security: two more call_int_hook_ignore_default use-cases
2023-08-03 17:12 [PATCH RFC 0/3] security: allow a LSM to specify NO-OP return code Paolo Abeni
2023-08-03 17:12 ` [PATCH RFC 1/3] security: introduce and use call_int_hook_ignore_default() Paolo Abeni
@ 2023-08-03 17:12 ` Paolo Abeni
2023-08-03 17:12 ` [PATCH RFC 3/3] security: " Paolo Abeni
2023-08-07 18:57 ` [PATCH RFC 0/3] security: allow a LSM to specify NO-OP return code Casey Schaufler
3 siblings, 0 replies; 6+ messages in thread
From: Paolo Abeni @ 2023-08-03 17:12 UTC (permalink / raw)
To: linux-security-module
Cc: Paul Moore, James Morris, Serge E. Hallyn, Ondrej Mosnacek,
KP Singh
Quite similar to the previous commit, the hooks:
inode_setxattr
inode_removexattr
don't allow the LSM to tell the core to ignore it's return code.
The main difference it that the above mentioned hooks explicitly check
for a non zero return value from the hook to perform the default action.
Changing the LSM_RET_DEFAULT to 1 and using call_int_hook_ignore_default
allows LSM returning the LSM_RET_DEFAULT value will become no-op for the
mentioned hooks.
All the exiting LSM except BPF never use 1 as return value, so no
functional change is expected.
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
include/linux/lsm_hook_defs.h | 4 ++--
security/security.c | 7 ++++---
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index c9032e20d0b3..49f1f9bed958 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -136,14 +136,14 @@ LSM_HOOK(int, 0, inode_follow_link, struct dentry *dentry, struct inode *inode,
LSM_HOOK(int, 0, inode_permission, struct inode *inode, int mask)
LSM_HOOK(int, 0, inode_setattr, struct dentry *dentry, struct iattr *attr)
LSM_HOOK(int, 0, inode_getattr, const struct path *path)
-LSM_HOOK(int, 0, inode_setxattr, struct mnt_idmap *idmap,
+LSM_HOOK(int, 1, inode_setxattr, struct mnt_idmap *idmap,
struct dentry *dentry, const char *name, const void *value,
size_t size, int flags)
LSM_HOOK(void, LSM_RET_VOID, inode_post_setxattr, struct dentry *dentry,
const char *name, const void *value, size_t size, int flags)
LSM_HOOK(int, 0, inode_getxattr, struct dentry *dentry, const char *name)
LSM_HOOK(int, 0, inode_listxattr, struct dentry *dentry)
-LSM_HOOK(int, 0, inode_removexattr, struct mnt_idmap *idmap,
+LSM_HOOK(int, 1, inode_removexattr, struct mnt_idmap *idmap,
struct dentry *dentry, const char *name)
LSM_HOOK(int, 0, inode_set_acl, struct mnt_idmap *idmap,
struct dentry *dentry, const char *acl_name, struct posix_acl *kacl)
diff --git a/security/security.c b/security/security.c
index b9a7b15e269e..0528cbef0624 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2158,8 +2158,8 @@ int security_inode_setxattr(struct mnt_idmap *idmap,
* SELinux and Smack integrate the cap call,
* so assume that all LSMs supplying this call do so.
*/
- ret = call_int_hook(inode_setxattr, 1, idmap, dentry, name, value,
- size, flags);
+ ret = call_int_hook_ignore_default(inode_setxattr, 1, idmap, dentry, name,
+ value, size, flags);
if (ret == 1)
ret = cap_inode_setxattr(dentry, name, value, size, flags);
@@ -2321,7 +2321,8 @@ int security_inode_removexattr(struct mnt_idmap *idmap,
* SELinux and Smack integrate the cap call,
* so assume that all LSMs supplying this call do so.
*/
- ret = call_int_hook(inode_removexattr, 1, idmap, dentry, name);
+ ret = call_int_hook_ignore_default(inode_removexattr, 1, idmap, dentry,
+ name);
if (ret == 1)
ret = cap_inode_removexattr(idmap, dentry, name);
if (ret)
--
2.41.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH RFC 3/3] security: more call_int_hook_ignore_default use-cases
2023-08-03 17:12 [PATCH RFC 0/3] security: allow a LSM to specify NO-OP return code Paolo Abeni
2023-08-03 17:12 ` [PATCH RFC 1/3] security: introduce and use call_int_hook_ignore_default() Paolo Abeni
2023-08-03 17:12 ` [PATCH RFC 2/3] security: two more call_int_hook_ignore_default use-cases Paolo Abeni
@ 2023-08-03 17:12 ` Paolo Abeni
2023-08-07 18:57 ` [PATCH RFC 0/3] security: allow a LSM to specify NO-OP return code Casey Schaufler
3 siblings, 0 replies; 6+ messages in thread
From: Paolo Abeni @ 2023-08-03 17:12 UTC (permalink / raw)
To: linux-security-module
Cc: Paul Moore, James Morris, Serge E. Hallyn, Ondrej Mosnacek,
KP Singh
The vm_enough_memory and xfrm_state_pol_flow_match behavior is somewhat
similar, as both possibly need to reconciliate multiple LSM return value
to a take a single decision. Currently xfrm_state_pol_flow_match has a
simple implementation leveraging the fact that only a LSM is supposed to
implement such hook.
This patch extend xfrm_state_pol_flow_match() to possibly cope with
multiple hooks alike what vm_enough_memory is currently doing, switch
both hooks to use the call_int_hook_ignore_default helper and change
the default hook return value to 1.
Overall the above should not lead to any functional change.
After this change, LSM returning the LSM_RET_DEFAULT value will become
a no-op for the mentioned hooks.
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
include/linux/lsm_hook_defs.h | 2 +-
security/security.c | 34 ++++++++--------------------------
2 files changed, 9 insertions(+), 27 deletions(-)
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 49f1f9bed958..e41ae0f90825 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -48,7 +48,7 @@ LSM_HOOK(int, 0, quota_on, struct dentry *dentry)
LSM_HOOK(int, 0, syslog, int type)
LSM_HOOK(int, 0, settime, const struct timespec64 *ts,
const struct timezone *tz)
-LSM_HOOK(int, 0, vm_enough_memory, struct mm_struct *mm, long pages)
+LSM_HOOK(int, 1, vm_enough_memory, struct mm_struct *mm, long pages)
LSM_HOOK(int, 0, bprm_creds_for_exec, struct linux_binprm *bprm)
LSM_HOOK(int, 0, bprm_creds_from_file, struct linux_binprm *bprm, struct file *file)
LSM_HOOK(int, 0, bprm_check_security, struct linux_binprm *bprm)
diff --git a/security/security.c b/security/security.c
index 0528cbef0624..b7b2d359c230 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1036,9 +1036,7 @@ int security_settime64(const struct timespec64 *ts, const struct timezone *tz)
*/
int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
{
- struct security_hook_list *hp;
- int cap_sys_admin = 1;
- int rc;
+ int cap_sys_admin;
/*
* The module will respond with a positive value if
@@ -1047,13 +1045,8 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
* agree that it should be set it will. If any module
* thinks it should not be set it won't.
*/
- hlist_for_each_entry(hp, &security_hook_heads.vm_enough_memory, list) {
- rc = hp->hook.vm_enough_memory(mm, pages);
- if (rc <= 0) {
- cap_sys_admin = 0;
- break;
- }
- }
+ cap_sys_admin = call_int_hook_ignore_default(vm_enough_memory, 1, mm,
+ pages);
return __vm_enough_memory(mm, pages, cap_sys_admin);
}
@@ -4922,24 +4915,13 @@ int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
struct xfrm_policy *xp,
const struct flowi_common *flic)
{
- struct security_hook_list *hp;
- int rc = LSM_RET_DEFAULT(xfrm_state_pol_flow_match);
-
/*
- * Since this function is expected to return 0 or 1, the judgment
- * becomes difficult if multiple LSMs supply this call. Fortunately,
- * we can use the first LSM's judgment because currently only SELinux
- * supplies this call.
- *
- * For speed optimization, we explicitly break the loop rather than
- * using the macro
+ * The module will respond with a 1 value if
+ * it thinks there is a match. If all of the modules
+ * agree we consider the match succesfull.
*/
- hlist_for_each_entry(hp, &security_hook_heads.xfrm_state_pol_flow_match,
- list) {
- rc = hp->hook.xfrm_state_pol_flow_match(x, xp, flic);
- break;
- }
- return rc;
+ return call_int_hook_ignore_default(xfrm_state_pol_flow_match,
+ 1, x, xp, flic);
}
/**
--
2.41.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH RFC 0/3] security: allow a LSM to specify NO-OP return code
2023-08-03 17:12 [PATCH RFC 0/3] security: allow a LSM to specify NO-OP return code Paolo Abeni
` (2 preceding siblings ...)
2023-08-03 17:12 ` [PATCH RFC 3/3] security: " Paolo Abeni
@ 2023-08-07 18:57 ` Casey Schaufler
2023-08-23 15:06 ` Paolo Abeni
3 siblings, 1 reply; 6+ messages in thread
From: Casey Schaufler @ 2023-08-07 18:57 UTC (permalink / raw)
To: Paolo Abeni, linux-security-module
Cc: Paul Moore, James Morris, Serge E. Hallyn, Ondrej Mosnacek,
KP Singh, 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(-)
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC 0/3] security: allow a LSM to specify NO-OP return code
2023-08-07 18:57 ` [PATCH RFC 0/3] security: allow a LSM to specify NO-OP return code Casey Schaufler
@ 2023-08-23 15:06 ` Paolo Abeni
0 siblings, 0 replies; 6+ messages in thread
From: Paolo Abeni @ 2023-08-23 15:06 UTC (permalink / raw)
To: Casey Schaufler, linux-security-module
Cc: Paul Moore, James Morris, Serge E. Hallyn, Ondrej Mosnacek,
KP Singh
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-08-23 15:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-03 17:12 [PATCH RFC 0/3] security: allow a LSM to specify NO-OP return code Paolo Abeni
2023-08-03 17:12 ` [PATCH RFC 1/3] security: introduce and use call_int_hook_ignore_default() Paolo Abeni
2023-08-03 17:12 ` [PATCH RFC 2/3] security: two more call_int_hook_ignore_default use-cases Paolo Abeni
2023-08-03 17:12 ` [PATCH RFC 3/3] security: " Paolo Abeni
2023-08-07 18:57 ` [PATCH RFC 0/3] security: allow a LSM to specify NO-OP return code Casey Schaufler
2023-08-23 15:06 ` Paolo Abeni
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).