[RFC][PATCH 0/4] security: Ensure LSMs return expected values

[RFC][PATCH 0/4] security: Ensure LSMs return expected values

[Roberto Sassu]

From: Roberto Sassu <roberto.sassu@huawei.com>

LSMs should follow the conventions stated in include/linux/lsm_hooks.h for
return values, as these conventions are followed by callers of the LSM
infrastructure for error handling.

The ability of an LSM to return arbitrary values could cause big troubles.
For LSMs aiming at being upstreamed, this event is unlikely, as each LSM is
carefully reviewed and it won't be accepted if it does not meet the return
value conventions. However, the recent introduction of BPF LSM allows
security modules (not part of the kernel) to inject arbitrary values,
without BPF LSM verifying them.

The initial idea was to fix BPF LSM itself. However, due to technical
difficulties to determine the precise interval of return values from a
static code analysis of eBPF programs, the new approach was to put the
fix in the LSM infrastructure, so that all LSMs can benefit from this work
as well.

The biggest problem of allowing arbitrary return values is when an LSM
returns a positive value, instead of a negative value, as it could be
converted to a pointer. Since such pointer escapes the IS_ERR() check, its
use later in the code can cause unpredictable consequences (e.g. invalid
memory access).

Another problem is returning zero when an LSM is supposed to have done some
operations. For example, the inode_init_security hook expects that their
implementations return zero only if they set the fields of the new xattr to
be added to the new inode. Otherwise, other kernel subsystems might
encounter unexpected conditions leading to a crash (e.g.
evm_protected_xattr_common() getting NULL as argument). This problem is
addressed separately in another patch set.

Finally, there are LSM hooks which are supposed to return just 1 as
positive value, or non-negative values. Also in these cases, although it
seems less critical, it is safer to return to callers of the LSM
infrastructure more precisely what they expect.

Patches 1 and 2 ensure that the documentation of LSM return values is
complete and accurate. Then, patch 3 introduces four flags (LSM_RET_NEG,
LSM_RET_ZERO, LSM_RET_ONE, LSM_RET_GT_ONE), one for each interval of
interest (< 0, = 0, = 1, > 1), and sets the correct flags for each LSM
hook. Finally, patch 4 verifies for each return value from LSMs that it is
an expected one.

Roberto Sassu (4):
  lsm: Clarify documentation of vm_enough_memory hook
  lsm: Add missing return values doc in lsm_hooks.h and fix formatting
  lsm: Redefine LSM_HOOK() macro to add return value flags as argument
  security: Enforce limitations on return values from LSMs

 include/linux/bpf_lsm.h       |   2 +-
 include/linux/lsm_hook_defs.h | 779 ++++++++++++++++++++--------------
 include/linux/lsm_hooks.h     | 136 ++++--
 kernel/bpf/bpf_lsm.c          |   5 +-
 security/bpf/hooks.c          |   2 +-
 security/security.c           |  38 +-
 6 files changed, 589 insertions(+), 373 deletions(-)

-- 
2.25.1

[RFC][PATCH 1/4] lsm: Clarify documentation of vm_enough_memory hook

[Roberto Sassu]

From: Roberto Sassu <roberto.sassu@huawei.com>

include/linux/lsm_hooks.h reports the result of the LSM infrastructure to
the callers, not what LSMs should return to the LSM infrastructure.

Clarify that and add that returning 1 from the LSMs means calling
__vm_enough_memory() with cap_sys_admin set, 0 without.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: KP Singh <kpsingh@kernel.org>
---
 include/linux/lsm_hooks.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 4ec80b96c22e..f40b82ca91e7 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1411,7 +1411,9 @@
  *	Check permissions for allocating a new virtual mapping.
  *	@mm contains the mm struct it is being added to.
  *	@pages contains the number of pages.
- *	Return 0 if permission is granted.
+ *	Return 0 if permission is granted by LSMs to the caller. LSMs should
+ *	return 1 if __vm_enough_memory() should be called with
+ *	cap_sys_admin set, 0 if not.
  *
  * @ismaclabel:
  *	Check if the extended attribute specified by @name
-- 
2.25.1

Re: [RFC][PATCH 1/4] lsm: Clarify documentation of vm_enough_memory hook

[Paul Moore]

On Tue, Nov 15, 2022 at 12:57 PM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> From: Roberto Sassu <roberto.sassu@huawei.com>
>
> include/linux/lsm_hooks.h reports the result of the LSM infrastructure to
> the callers, not what LSMs should return to the LSM infrastructure.
>
> Clarify that and add that returning 1 from the LSMs means calling
> __vm_enough_memory() with cap_sys_admin set, 0 without.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Reviewed-by: KP Singh <kpsingh@kernel.org>
> ---
>  include/linux/lsm_hooks.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 4ec80b96c22e..f40b82ca91e7 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1411,7 +1411,9 @@
>   *     Check permissions for allocating a new virtual mapping.
>   *     @mm contains the mm struct it is being added to.
>   *     @pages contains the number of pages.
> - *     Return 0 if permission is granted.
> + *     Return 0 if permission is granted by LSMs to the caller. LSMs should
> + *     return 1 if __vm_enough_memory() should be called with
> + *     cap_sys_admin set, 0 if not.

I think this is a nice addition, but according to the code, any value
greater than zero will trigger the caller-should-have-CAP_SYS_ADMIN
behavior, not just 1.  I suggest updating the comment.

-- 
paul-moore.com

Re: [RFC][PATCH 1/4] lsm: Clarify documentation of vm_enough_memory hook

[Roberto Sassu]

On Tue, 2022-11-15 at 21:11 -0500, Paul Moore wrote:
> On Tue, Nov 15, 2022 at 12:57 PM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > include/linux/lsm_hooks.h reports the result of the LSM infrastructure to
> > the callers, not what LSMs should return to the LSM infrastructure.
> > 
> > Clarify that and add that returning 1 from the LSMs means calling
> > __vm_enough_memory() with cap_sys_admin set, 0 without.
> > 
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > Reviewed-by: KP Singh <kpsingh@kernel.org>
> > ---
> >  include/linux/lsm_hooks.h | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index 4ec80b96c22e..f40b82ca91e7 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -1411,7 +1411,9 @@
> >   *     Check permissions for allocating a new virtual mapping.
> >   *     @mm contains the mm struct it is being added to.
> >   *     @pages contains the number of pages.
> > - *     Return 0 if permission is granted.
> > + *     Return 0 if permission is granted by LSMs to the caller. LSMs should
> > + *     return 1 if __vm_enough_memory() should be called with
> > + *     cap_sys_admin set, 0 if not.
> 
> I think this is a nice addition, but according to the code, any value
> greater than zero will trigger the caller-should-have-CAP_SYS_ADMIN
> behavior, not just 1.  I suggest updating the comment.

Ok, yes. Thanks.

Roberto

Re: [RFC][PATCH 1/4] lsm: Clarify documentation of vm_enough_memory hook

[KP Singh]

On Wed, Nov 16, 2022 at 9:06 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> On Tue, 2022-11-15 at 21:11 -0500, Paul Moore wrote:
> > On Tue, Nov 15, 2022 at 12:57 PM Roberto Sassu
> > <roberto.sassu@huaweicloud.com> wrote:
> > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > >
> > > include/linux/lsm_hooks.h reports the result of the LSM infrastructure to
> > > the callers, not what LSMs should return to the LSM infrastructure.
> > >
> > > Clarify that and add that returning 1 from the LSMs means calling
> > > __vm_enough_memory() with cap_sys_admin set, 0 without.
> > >
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > Reviewed-by: KP Singh <kpsingh@kernel.org>
> > > ---
> > >  include/linux/lsm_hooks.h | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > > index 4ec80b96c22e..f40b82ca91e7 100644
> > > --- a/include/linux/lsm_hooks.h
> > > +++ b/include/linux/lsm_hooks.h
> > > @@ -1411,7 +1411,9 @@
> > >   *     Check permissions for allocating a new virtual mapping.
> > >   *     @mm contains the mm struct it is being added to.
> > >   *     @pages contains the number of pages.
> > > - *     Return 0 if permission is granted.
> > > + *     Return 0 if permission is granted by LSMs to the caller. LSMs should
> > > + *     return 1 if __vm_enough_memory() should be called with
> > > + *     cap_sys_admin set, 0 if not.
> >
> > I think this is a nice addition, but according to the code, any value
> > greater than zero will trigger the caller-should-have-CAP_SYS_ADMIN
> > behavior, not just 1.  I suggest updating the comment.
>
> Ok, yes. Thanks.

Also, this is an unrelated patch and you can probably send it
independently, especially
since the other changes will now land mostly via BPF.

>
> Roberto
>

Re: [RFC][PATCH 1/4] lsm: Clarify documentation of vm_enough_memory hook

[Paul Moore]

On Wed, Nov 16, 2022 at 2:18 PM KP Singh <kpsingh@kernel.org> wrote:
> On Wed, Nov 16, 2022 at 9:06 AM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> >
> > On Tue, 2022-11-15 at 21:11 -0500, Paul Moore wrote:
> > > On Tue, Nov 15, 2022 at 12:57 PM Roberto Sassu
> > > <roberto.sassu@huaweicloud.com> wrote:
> > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > >
> > > > include/linux/lsm_hooks.h reports the result of the LSM infrastructure to
> > > > the callers, not what LSMs should return to the LSM infrastructure.
> > > >
> > > > Clarify that and add that returning 1 from the LSMs means calling
> > > > __vm_enough_memory() with cap_sys_admin set, 0 without.
> > > >
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > Reviewed-by: KP Singh <kpsingh@kernel.org>
> > > > ---
> > > >  include/linux/lsm_hooks.h | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > > > index 4ec80b96c22e..f40b82ca91e7 100644
> > > > --- a/include/linux/lsm_hooks.h
> > > > +++ b/include/linux/lsm_hooks.h
> > > > @@ -1411,7 +1411,9 @@
> > > >   *     Check permissions for allocating a new virtual mapping.
> > > >   *     @mm contains the mm struct it is being added to.
> > > >   *     @pages contains the number of pages.
> > > > - *     Return 0 if permission is granted.
> > > > + *     Return 0 if permission is granted by LSMs to the caller. LSMs should
> > > > + *     return 1 if __vm_enough_memory() should be called with
> > > > + *     cap_sys_admin set, 0 if not.
> > >
> > > I think this is a nice addition, but according to the code, any value
> > > greater than zero will trigger the caller-should-have-CAP_SYS_ADMIN
> > > behavior, not just 1.  I suggest updating the comment.
> >
> > Ok, yes. Thanks.
>
> Also, this is an unrelated patch and you can probably send it
> independently, especially
> since the other changes will now land mostly via BPF.

Yes, the doc/comment changes really have nothing to do with the other
stuff we are discussing in this patchset.

-- 
paul-moore.com

[RFC][PATCH 2/4] lsm: Add missing return values doc in lsm_hooks.h and fix formatting

[Roberto Sassu]

From: Roberto Sassu <roberto.sassu@huawei.com>

Ensure that for non-void LSM hooks there is a description of the return
values. Also replace spaces with tab for indentation, remove empty lines
between the hook description and the list of parameters and add the period
at the end of the parameter description.

Finally, replace the description of the sb_parse_opts_str hook, which was
removed with commit 757cbe597fe8 ("LSM: new method: ->sb_add_mnt_opt()"),
with one for the new hook sb_add_mnt_opt.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 include/linux/lsm_hooks.h | 123 ++++++++++++++++++++++++++------------
 1 file changed, 86 insertions(+), 37 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index f40b82ca91e7..c0c570b7eabd 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -92,6 +92,7 @@
  *	is initialised to NULL by the caller.
  *	@fc indicates the new filesystem context.
  *	@src_fc indicates the original filesystem context.
+ *	Return 0 on success or a negative error code on failure.
  * @fs_context_parse_param:
  *	Userspace provided a parameter to configure a superblock.  The LSM may
  *	reject it with an error and may use it for itself, in which case it
@@ -99,6 +100,9 @@
  *	the filesystem.
  *	@fc indicates the filesystem context.
  *	@param The parameter
+ *	Return 0 to indicate that the parameter should be passed on to the
+ *	filesystem, 1 to indicate that the parameter should be discarded or an
+ *	error to indicate that the parameter should be rejected.
  *
  * Security hooks for filesystem operations.
  *
@@ -118,6 +122,7 @@
  * 	Free memory associated with @mnt_ops.
  * @sb_eat_lsm_opts:
  * 	Eat (scan @orig options) and save them in @mnt_opts.
+ *	Return 0 if permission is granted.
  * @sb_statfs:
  *	Check permission before obtaining filesystem statistics for the @mnt
  *	mountpoint.
@@ -158,9 +163,11 @@
  *	@data contains the filesystem-specific data.
  *	Return 0 if permission is granted.
  * @sb_kern_mount:
- * 	Mount this @sb if allowed by permissions.
+ *	Mount this @sb if allowed by permissions.
+ *	Return 0 if permission is granted.
  * @sb_show_options:
  * 	Show (print on @m) mount options for this @sb.
+ *	Return 0 if permission is granted.
  * @sb_umount:
  *	Check permission before the @mnt file system is unmounted.
  *	@mnt contains the mounted file system.
@@ -176,18 +183,22 @@
  *	Set the security relevant mount options used for a superblock
  *	@sb the superblock to set security mount options for
  *	@opts binary data structure containing all lsm mount data
+ *	Return 0 on success, error on failure.
  * @sb_clone_mnt_opts:
  *	Copy all security options from a given superblock to another
  *	@oldsb old superblock which contain information to clone
  *	@newsb new superblock which needs filled in
- * @sb_parse_opts_str:
- *	Parse a string of security data filling in the opts structure
- *	@options string containing all mount options known by the LSM
- *	@opts binary data structure usable by the LSM
+ *	Return 0 on success, error on failure.
+ * @add_mnt_opt:
+ *	Add a new mount option @option with value @val and length @len to the
+ *	existing mount options @mnt_opts.
+ *	Return 0 if the option was successfully added, a negative value
+ *	otherwise.
  * @move_mount:
  *	Check permission before a mount is moved.
  *	@from_path indicates the mount that is going to be moved.
  *	@to_path indicates the mountpoint that will be mounted upon.
+ *	Return 0 if permission is granted.
  * @dentry_init_security:
  *	Compute a context for a dentry as the inode is not yet available
  *	since NFSv4 has no label backed by an EA anyway.
@@ -199,6 +210,7 @@
  *		    a pointer to static string.
  *	@ctx pointer to place the pointer to the resulting context in.
  *	@ctxlen point to place the length of the resulting context.
+ *	Return 0 if permission is granted.
  * @dentry_create_files_as:
  *	Compute a context for a dentry as the inode is not yet available
  *	and set that context in passed in creds so that new files are
@@ -209,6 +221,7 @@
  *	@name name of the last path component used to create file
  *	@old creds which should be used for context calculation
  *	@new creds to modify
+ *	Return 0 if permission is granted.
  *
  *
  * Security hooks for inode operations.
@@ -380,6 +393,7 @@
  * @path_notify:
  *	Check permissions before setting a watch on events as defined by @mask,
  *	on an object at @path, whose type is defined by @obj_type.
+ *	Return 0 if permission is granted.
  * @inode_readlink:
  *	Check the permission to read the symbolic link.
  *	@dentry contains the dentry structure for the file link.
@@ -439,9 +453,9 @@
  *	Retrieve a copy of the extended attribute representation of the
  *	security label associated with @name for @inode via @buffer.  Note that
  *	@name is the remainder of the attribute name after the security prefix
- *	has been removed. @alloc is used to specify of the call should return a
- *	value via the buffer or just the value length Return size of buffer on
- *	success.
+ *	has been removed. @alloc is used to specify if the call should return a
+ *	value via the buffer or just the value length.
+ *	Return size of buffer on success.
  * @inode_setsecurity:
  *	Set the security label associated with @name for @inode from the
  *	extended attribute value @value.  @size indicates the size of the
@@ -491,20 +505,22 @@
  *	to abort the copy up. Note that the caller is responsible for reading
  *	and writing the xattrs as this hook is merely a filter.
  * @d_instantiate:
- * 	Fill in @inode security information for a @dentry if allowed.
+ *	Fill in @inode security information for a @dentry if allowed.
  * @getprocattr:
- * 	Read attribute @name for process @p and store it into @value if allowed.
+ *	Read attribute @name for process @p and store it into @value if allowed.
+ *	Return the length of @value on success, a negative value otherwise.
  * @setprocattr:
- * 	Write (set) attribute @name to @value, size @size if allowed.
+ *	Write (set) attribute @name to @value, size @size if allowed.
+ *	Return written bytes on success, a negative value otherwise.
  *
  * Security hooks for kernfs node operations
  *
  * @kernfs_init_security:
  *	Initialize the security context of a newly created kernfs node based
  *	on its own and its parent's attributes.
- *
  *	@kn_dir the parent kernfs node
  *	@kn the new child kernfs node
+ *	Return 0 if permission is granted.
  *
  * Security hooks for file operations
  *
@@ -602,6 +618,7 @@
  *	Save open-time permission checking state for later use upon
  *	file_permission, and recheck access if anything has changed
  *	since inode_permission.
+ *	Return 0 if permission is granted.
  *
  * Security hooks for task operations.
  *
@@ -619,6 +636,7 @@
  *	@gfp indicates the atomicity of any memory allocations.
  *	Only allocate sufficient memory and attach to @cred such that
  *	cred_transfer() will not get ENOMEM.
+ *	Return 0 on success, negative values on failure.
  * @cred_free:
  *	@cred points to the credentials.
  *	Deallocate and clear the cred->security field in a set of credentials.
@@ -627,6 +645,7 @@
  *	@old points to the original credentials.
  *	@gfp indicates the atomicity of any memory allocations.
  *	Prepare a new set of credentials by copying the data from the old set.
+ *	Return 0 on success, negative values on failure.
  * @cred_transfer:
  *	@new points to the new credentials.
  *	@old points to the original credentials.
@@ -873,6 +892,7 @@
  *	@type contains the requested communications type.
  *	@protocol contains the requested protocol.
  *	@kern set to 1 if a kernel socket.
+ *	Return 0 if permission is granted.
  * @socket_socketpair:
  *	Check permissions before creating a fresh pair of sockets.
  *	@socka contains the first socket structure.
@@ -956,6 +976,7 @@
  *	Must not sleep inside this hook because some callers hold spinlocks.
  *	@sk contains the sock (not socket) associated with the incoming sk_buff.
  *	@skb contains the incoming network data.
+ *	Return 0 if permission is granted.
  * @socket_getpeersec_stream:
  *	This hook allows the security module to provide peer socket security
  *	state for unix or connected tcp sockets to userspace via getsockopt
@@ -983,6 +1004,7 @@
  * @sk_alloc_security:
  *	Allocate and attach a security structure to the sk->sk_security field,
  *	which is used to copy security attributes between local stream sockets.
+ *	Return 0 on success, error on failure.
  * @sk_free_security:
  *	Deallocate security structure.
  * @sk_clone_security:
@@ -995,17 +1017,19 @@
  * @inet_conn_request:
  *	Sets the openreq's sid to socket's sid with MLS portion taken
  *	from peer sid.
+ *	Return 0 if permission is granted.
  * @inet_csk_clone:
  *	Sets the new child socket's sid to the openreq sid.
  * @inet_conn_established:
  *	Sets the connection's peersid to the secmark on skb.
  * @secmark_relabel_packet:
- *	check if the process should be allowed to relabel packets to
- *	the given secid
+ *	Check if the process should be allowed to relabel packets to
+ *	the given secid.
+ *	Return 0 if permission is granted.
  * @secmark_refcount_inc:
- *	tells the LSM to increment the number of secmark labeling rules loaded
+ *	Tells the LSM to increment the number of secmark labeling rules loaded.
  * @secmark_refcount_dec:
- *	tells the LSM to decrement the number of secmark labeling rules loaded
+ *	Tells the LSM to decrement the number of secmark labeling rules loaded.
  * @req_classify_flow:
  *	Sets the flow's sid to the openreq sid.
  * @tun_dev_alloc_security:
@@ -1019,18 +1043,22 @@
  *	@security pointer to the TUN device's security structure
  * @tun_dev_create:
  *	Check permissions prior to creating a new TUN device.
+ *	Return 0 if permission is granted.
  * @tun_dev_attach_queue:
  *	Check permissions prior to attaching to a TUN device queue.
  *	@security pointer to the TUN device's security structure.
+ *	Return 0 if permission is granted.
  * @tun_dev_attach:
  *	This hook can be used by the module to update any security state
  *	associated with the TUN device's sock structure.
  *	@sk contains the existing sock structure.
  *	@security pointer to the TUN device's security structure.
+ *	Return 0 if permission is granted.
  * @tun_dev_open:
  *	This hook can be used by the module to update any security state
  *	associated with the TUN device's security structure.
  *	@security pointer to the TUN devices's security structure.
+ *	Return 0 if permission is granted.
  *
  * Security hooks for SCTP
  *
@@ -1063,6 +1091,7 @@
  *	to the security module.
  *	@asoc pointer to sctp association structure.
  *	@skb pointer to skbuff of association packet.
+ *	Return 0 on success, error on failure.
  *
  * Security hooks for Infiniband
  *
@@ -1071,15 +1100,17 @@
  *	@subnet_prefix the subnet prefix of the port being used.
  *	@pkey the pkey to be accessed.
  *	@sec pointer to a security structure.
+ *	Return 0 if permission is granted.
  * @ib_endport_manage_subnet:
  *	Check permissions to send and receive SMPs on a end port.
  *	@dev_name the IB device name (i.e. mlx4_0).
  *	@port_num the port number.
  *	@sec pointer to a security structure.
+ *	Return 0 if permission is granted.
  * @ib_alloc_security:
  *	Allocate a security structure for Infiniband objects.
  *	@sec pointer to a security structure pointer.
- *	Returns 0 on success, non-zero on failure
+ *	Returns 0 on success, non-zero on failure.
  * @ib_free_security:
  *	Deallocate an Infiniband security structure.
  *	@sec contains the security structure to be freed.
@@ -1107,6 +1138,7 @@
  * @xfrm_policy_delete_security:
  *	@ctx contains the xfrm_sec_ctx.
  *	Authorize deletion of xp->security.
+ *	Return 0 if permission is granted.
  * @xfrm_state_alloc:
  *	@x contains the xfrm_state being added to the Security Association
  *	Database by the XFRM system.
@@ -1132,6 +1164,7 @@
  * @xfrm_state_delete_security:
  *	@x contains the xfrm_state.
  *	Authorize deletion of x->security.
+ *	Return 0 if permission is granted.
  * @xfrm_policy_lookup:
  *	@ctx contains the xfrm_sec_ctx for which the access control is being
  *	checked.
@@ -1432,10 +1465,12 @@
  *	@secdata contains the pointer that stores the converted security
  *	context.
  *	@seclen pointer which contains the length of the data
+ *	Return 0 on success, error on failure.
  * @secctx_to_secid:
  *	Convert security context to secid.
  *	@secid contains the pointer to the generated security ID.
  *	@secdata contains the security context.
+ *	Return 0 on success, error on failure.
  *
  * @release_secctx:
  *	Release the security context.
@@ -1489,6 +1524,7 @@
  *	@inode we wish to set the security context of.
  *	@ctx contains the string which we wish to set in the inode.
  *	@ctxlen contains the length of @ctx.
+ *	Return 0 on success, error on failure.
  *
  * @inode_setsecctx:
  *	Change the security context of an inode.  Updates the
@@ -1502,6 +1538,7 @@
  *	@dentry contains the inode we wish to set the security context of.
  *	@ctx contains the string which we wish to set in the inode.
  *	@ctxlen contains the length of @ctx.
+ *	Return 0 on success, error on failure.
  *
  * @inode_getsecctx:
  *	On success, returns 0 and fills out @ctx and @ctxlen with the security
@@ -1509,6 +1546,7 @@
  *	@inode we wish to get the security context of.
  *	@ctx is a pointer in which to place the allocated security context.
  *	@ctxlen points to the place to put the length of @ctx.
+ *	Return 0 on success, error on failure.
  *
  * Security hooks for the general notification queue:
  *
@@ -1518,11 +1556,13 @@
  *	@w_cred: The credentials of the whoever set the watch.
  *	@cred: The event-triggerer's credentials
  *	@n: The notification being posted
+ *	Return 0 if permission is granted.
  *
  * @watch_key:
  *	Check to see if a process is allowed to watch for event notifications
  *	from a key or keyring.
  *	@key: The key to watch.
+ *	Return 0 if permission is granted.
  *
  * Security hooks for using the eBPF maps and programs functionalities through
  * eBPF syscalls.
@@ -1531,65 +1571,74 @@
  *	Do a initial check for all bpf syscalls after the attribute is copied
  *	into the kernel. The actual security module can implement their own
  *	rules to check the specific cmd they need.
+ *	Return 0 if permission is granted.
  *
  * @bpf_map:
  *	Do a check when the kernel generate and return a file descriptor for
  *	eBPF maps.
- *
- *	@map: bpf map that we want to access
- *	@mask: the access flags
+ *	@map: bpf map that we want to access.
+ *	@mask: the access flags.
+ *	Return 0 if permission is granted.
  *
  * @bpf_prog:
  *	Do a check when the kernel generate and return a file descriptor for
  *	eBPF programs.
- *
  *	@prog: bpf prog that userspace want to use.
+ *	Return 0 if permission is granted.
  *
  * @bpf_map_alloc_security:
  *	Initialize the security field inside bpf map.
+ *	Return 0 on success, error on failure.
  *
  * @bpf_map_free_security:
  *	Clean up the security information stored inside bpf map.
  *
  * @bpf_prog_alloc_security:
  *	Initialize the security field inside bpf program.
+ *	Return 0 on success, error on failure.
  *
  * @bpf_prog_free_security:
  *	Clean up the security information stored inside bpf prog.
  *
  * @locked_down:
- *     Determine whether a kernel feature that potentially enables arbitrary
- *     code execution in kernel space should be permitted.
- *
- *     @what: kernel feature being accessed
+ *	Determine whether a kernel feature that potentially enables arbitrary
+ *	code execution in kernel space should be permitted.
+ *	@what: kernel feature being accessed.
+ *	Return 0 if permission is granted.
  *
  * Security hooks for perf events
  *
  * @perf_event_open:
- * 	Check whether the @type of perf_event_open syscall is allowed.
+ *	Check whether the @type of perf_event_open syscall is allowed.
+ *	Return 0 if permission is granted.
  * @perf_event_alloc:
- * 	Allocate and save perf_event security info.
+ *	Allocate and save perf_event security info.
+ *	Return 0 on success, error on failure.
  * @perf_event_free:
- * 	Release (free) perf_event security info.
+ *	Release (free) perf_event security info.
  * @perf_event_read:
- * 	Read perf_event security info if allowed.
+ *	Read perf_event security info if allowed.
+ *	Return 0 if permission is granted.
  * @perf_event_write:
- * 	Write perf_event security info if allowed.
+ *	Write perf_event security info if allowed.
+ *	Return 0 if permission is granted.
  *
  * Security hooks for io_uring
  *
  * @uring_override_creds:
- *      Check if the current task, executing an io_uring operation, is allowed
- *      to override it's credentials with @new.
- *
- *      @new: the new creds to use
+ *	Check if the current task, executing an io_uring operation, is allowed
+ *	to override it's credentials with @new.
+ *	@new: the new creds to use.
+ *	Return 0 if permission is granted.
  *
  * @uring_sqpoll:
- *      Check whether the current task is allowed to spawn a io_uring polling
- *      thread (IORING_SETUP_SQPOLL).
+ *	Check whether the current task is allowed to spawn a io_uring polling
+ *	thread (IORING_SETUP_SQPOLL).
+ *	Return 0 if permission is granted.
  *
  * @uring_cmd:
- *      Check whether the file_operations uring_cmd is allowed to run.
+ *	Check whether the file_operations uring_cmd is allowed to run.
+ *	Return 0 if permission is granted.
  *
  */
 union security_list_options {
-- 
2.25.1

Re: [RFC][PATCH 2/4] lsm: Add missing return values doc in lsm_hooks.h and fix formatting

[Paul Moore]

On Tue, Nov 15, 2022 at 12:57 PM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> From: Roberto Sassu <roberto.sassu@huawei.com>
>
> Ensure that for non-void LSM hooks there is a description of the return
> values. Also replace spaces with tab for indentation, remove empty lines
> between the hook description and the list of parameters and add the period
> at the end of the parameter description.
>
> Finally, replace the description of the sb_parse_opts_str hook, which was
> removed with commit 757cbe597fe8 ("LSM: new method: ->sb_add_mnt_opt()"),
> with one for the new hook sb_add_mnt_opt.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  include/linux/lsm_hooks.h | 123 ++++++++++++++++++++++++++------------
>  1 file changed, 86 insertions(+), 37 deletions(-)

...

> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index f40b82ca91e7..c0c570b7eabd 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -176,18 +183,22 @@
>   *     Set the security relevant mount options used for a superblock
>   *     @sb the superblock to set security mount options for
>   *     @opts binary data structure containing all lsm mount data
> + *     Return 0 on success, error on failure.
>   * @sb_clone_mnt_opts:
>   *     Copy all security options from a given superblock to another
>   *     @oldsb old superblock which contain information to clone
>   *     @newsb new superblock which needs filled in
> - * @sb_parse_opts_str:
> - *     Parse a string of security data filling in the opts structure
> - *     @options string containing all mount options known by the LSM
> - *     @opts binary data structure usable by the LSM
> + *     Return 0 on success, error on failure.
> + * @add_mnt_opt:
> + *     Add a new mount option @option with value @val and length @len to the
> + *     existing mount options @mnt_opts.
> + *     Return 0 if the option was successfully added, a negative value
> + *     otherwise.

I really appreciate the effort to improve the LSM hook comments/docs,
but the "sb_add_mnt_opt" hook was removed in 52f982f00b22
("security,selinux: remove security_add_mnt_opt()").

-- 
paul-moore.com

Re: [RFC][PATCH 2/4] lsm: Add missing return values doc in lsm_hooks.h and fix formatting

[Roberto Sassu]

On Tue, 2022-11-15 at 21:23 -0500, Paul Moore wrote:
> On Tue, Nov 15, 2022 at 12:57 PM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > Ensure that for non-void LSM hooks there is a description of the return
> > values. Also replace spaces with tab for indentation, remove empty lines
> > between the hook description and the list of parameters and add the period
> > at the end of the parameter description.
> > 
> > Finally, replace the description of the sb_parse_opts_str hook, which was
> > removed with commit 757cbe597fe8 ("LSM: new method: ->sb_add_mnt_opt()"),
> > with one for the new hook sb_add_mnt_opt.
> > 
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >  include/linux/lsm_hooks.h | 123 ++++++++++++++++++++++++++------------
> >  1 file changed, 86 insertions(+), 37 deletions(-)
> 
> ...
> 
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index f40b82ca91e7..c0c570b7eabd 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -176,18 +183,22 @@
> >   *     Set the security relevant mount options used for a superblock
> >   *     @sb the superblock to set security mount options for
> >   *     @opts binary data structure containing all lsm mount data
> > + *     Return 0 on success, error on failure.
> >   * @sb_clone_mnt_opts:
> >   *     Copy all security options from a given superblock to another
> >   *     @oldsb old superblock which contain information to clone
> >   *     @newsb new superblock which needs filled in
> > - * @sb_parse_opts_str:
> > - *     Parse a string of security data filling in the opts structure
> > - *     @options string containing all mount options known by the LSM
> > - *     @opts binary data structure usable by the LSM
> > + *     Return 0 on success, error on failure.
> > + * @add_mnt_opt:
> > + *     Add a new mount option @option with value @val and length @len to the
> > + *     existing mount options @mnt_opts.
> > + *     Return 0 if the option was successfully added, a negative value
> > + *     otherwise.
> 
> I really appreciate the effort to improve the LSM hook comments/docs,
> but the "sb_add_mnt_opt" hook was removed in 52f982f00b22
> ("security,selinux: remove security_add_mnt_opt()").

Right, sorry, didn't notice.

Thanks

Roberto

Re: [RFC][PATCH 2/4] lsm: Add missing return values doc in lsm_hooks.h and fix formatting

[Paul Moore]

On Wed, Nov 16, 2022 at 3:07 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> On Tue, 2022-11-15 at 21:23 -0500, Paul Moore wrote:
> > On Tue, Nov 15, 2022 at 12:57 PM Roberto Sassu
> > <roberto.sassu@huaweicloud.com> wrote:
> > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > >
> > > Ensure that for non-void LSM hooks there is a description of the return
> > > values. Also replace spaces with tab for indentation, remove empty lines
> > > between the hook description and the list of parameters and add the period
> > > at the end of the parameter description.
> > >
> > > Finally, replace the description of the sb_parse_opts_str hook, which was
> > > removed with commit 757cbe597fe8 ("LSM: new method: ->sb_add_mnt_opt()"),
> > > with one for the new hook sb_add_mnt_opt.
> > >
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > ---
> > >  include/linux/lsm_hooks.h | 123 ++++++++++++++++++++++++++------------
> > >  1 file changed, 86 insertions(+), 37 deletions(-)
> >
> > ...
> >
> > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > > index f40b82ca91e7..c0c570b7eabd 100644
> > > --- a/include/linux/lsm_hooks.h
> > > +++ b/include/linux/lsm_hooks.h
> > > @@ -176,18 +183,22 @@
> > >   *     Set the security relevant mount options used for a superblock
> > >   *     @sb the superblock to set security mount options for
> > >   *     @opts binary data structure containing all lsm mount data
> > > + *     Return 0 on success, error on failure.
> > >   * @sb_clone_mnt_opts:
> > >   *     Copy all security options from a given superblock to another
> > >   *     @oldsb old superblock which contain information to clone
> > >   *     @newsb new superblock which needs filled in
> > > - * @sb_parse_opts_str:
> > > - *     Parse a string of security data filling in the opts structure
> > > - *     @options string containing all mount options known by the LSM
> > > - *     @opts binary data structure usable by the LSM
> > > + *     Return 0 on success, error on failure.
> > > + * @add_mnt_opt:
> > > + *     Add a new mount option @option with value @val and length @len to the
> > > + *     existing mount options @mnt_opts.
> > > + *     Return 0 if the option was successfully added, a negative value
> > > + *     otherwise.
> >
> > I really appreciate the effort to improve the LSM hook comments/docs,
> > but the "sb_add_mnt_opt" hook was removed in 52f982f00b22
> > ("security,selinux: remove security_add_mnt_opt()").
>
> Right, sorry, didn't notice.

No problem.  I just wanted to make it clear that I really like this
patch, and if you can fix the above and double-check the others I'll
gladly merge this.  As a general rule I *love* doc improvements :)

-- 
paul-moore.com

[RFC][PATCH 3/4] lsm: Redefine LSM_HOOK() macro to add return value flags as argument

[Roberto Sassu]

From: Roberto Sassu <roberto.sassu@huawei.com>

Define four return value flags (LSM_RET_NEG, LSM_RET_ZERO, LSM_RET_ONE,
LSM_RET_GT_ONE), one for each interval of interest (< 0, = 0, = 1, > 1).

Redefine the LSM_HOOK() macro to add return value flags as argument, and
set the correct flags for each LSM hook.

Implementors of new LSM hooks should do the same as well.

Cc: stable@vger.kernel.org # 5.7.x
Fixes: 9d3fdea789c8 ("bpf: lsm: Provide attachment points for BPF LSM programs")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 include/linux/bpf_lsm.h       |   2 +-
 include/linux/lsm_hook_defs.h | 779 ++++++++++++++++++++--------------
 include/linux/lsm_hooks.h     |   9 +-
 kernel/bpf/bpf_lsm.c          |   5 +-
 security/bpf/hooks.c          |   2 +-
 security/security.c           |   4 +-
 6 files changed, 466 insertions(+), 335 deletions(-)

diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
index 4bcf76a9bb06..650ab044e705 100644
--- a/include/linux/bpf_lsm.h
+++ b/include/linux/bpf_lsm.h
@@ -13,7 +13,7 @@
 
 #ifdef CONFIG_BPF_LSM
 
-#define LSM_HOOK(RET, DEFAULT, NAME, ...) \
+#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \
 	RET bpf_lsm_##NAME(__VA_ARGS__);
 #include <linux/lsm_hook_defs.h>
 #undef LSM_HOOK
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index ec119da1d89b..ed8336a5a0d8 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -18,395 +18,520 @@
  * The macro LSM_HOOK is used to define the data structures required by
  * the LSM framework using the pattern:
  *
- *	LSM_HOOK(<return_type>, <default_value>, <hook_name>, args...)
+ *	LSM_HOOK(<return_type>, <default_value>, <return_flags>, <hook_name>, args...)
  *
  * struct security_hook_heads {
- *   #define LSM_HOOK(RET, DEFAULT, NAME, ...) struct hlist_head NAME;
+ *   #define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) struct hlist_head NAME;
  *   #include <linux/lsm_hook_defs.h>
  *   #undef LSM_HOOK
  * };
  */
-LSM_HOOK(int, 0, binder_set_context_mgr, const struct cred *mgr)
-LSM_HOOK(int, 0, binder_transaction, const struct cred *from,
-	 const struct cred *to)
-LSM_HOOK(int, 0, binder_transfer_binder, const struct cred *from,
-	 const struct cred *to)
-LSM_HOOK(int, 0, binder_transfer_file, const struct cred *from,
-	 const struct cred *to, struct file *file)
-LSM_HOOK(int, 0, ptrace_access_check, struct task_struct *child,
-	 unsigned int mode)
-LSM_HOOK(int, 0, ptrace_traceme, struct task_struct *parent)
-LSM_HOOK(int, 0, capget, struct task_struct *target, kernel_cap_t *effective,
-	 kernel_cap_t *inheritable, kernel_cap_t *permitted)
-LSM_HOOK(int, 0, capset, struct cred *new, const struct cred *old,
-	 const kernel_cap_t *effective, const kernel_cap_t *inheritable,
-	 const kernel_cap_t *permitted)
-LSM_HOOK(int, 0, capable, const struct cred *cred, struct user_namespace *ns,
-	 int cap, unsigned int opts)
-LSM_HOOK(int, 0, quotactl, int cmds, int type, int id, struct super_block *sb)
-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, 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)
-LSM_HOOK(void, LSM_RET_VOID, bprm_committing_creds, struct linux_binprm *bprm)
-LSM_HOOK(void, LSM_RET_VOID, bprm_committed_creds, struct linux_binprm *bprm)
-LSM_HOOK(int, 0, fs_context_dup, struct fs_context *fc,
-	 struct fs_context *src_sc)
-LSM_HOOK(int, -ENOPARAM, fs_context_parse_param, struct fs_context *fc,
+
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, binder_set_context_mgr,
+	 const struct cred *mgr)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, binder_transaction,
+	 const struct cred *from, const struct cred *to)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, binder_transfer_binder,
+	 const struct cred *from, const struct cred *to)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, binder_transfer_file,
+	 const struct cred *from, const struct cred *to, struct file *file)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, ptrace_access_check,
+	 struct task_struct *child, unsigned int mode)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, ptrace_traceme,
+	 struct task_struct *parent)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, capget, struct task_struct *target,
+	 kernel_cap_t *effective, kernel_cap_t *inheritable,
+	 kernel_cap_t *permitted)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, capset, struct cred *new,
+	 const struct cred *old, const kernel_cap_t *effective,
+	 const kernel_cap_t *inheritable, const kernel_cap_t *permitted)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, capable, const struct cred *cred,
+	 struct user_namespace *ns, int cap, unsigned int opts)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, quotactl, int cmds, int type,
+	 int id, struct super_block *sb)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, quota_on, struct dentry *dentry)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, syslog, int type)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, settime,
+	 const struct timespec64 *ts, const struct timezone *tz)
+LSM_HOOK(int, 0, LSM_RET_ZERO | LSM_RET_ONE, vm_enough_memory,
+	 struct mm_struct *mm, long pages)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, bprm_creds_for_exec,
+	 struct linux_binprm *bprm)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, bprm_creds_from_file,
+	 struct linux_binprm *bprm, struct file *file)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, bprm_check_security,
+	 struct linux_binprm *bprm)
+LSM_HOOK(void, LSM_RET_VOID, 0, bprm_committing_creds, struct linux_binprm *bprm)
+LSM_HOOK(void, LSM_RET_VOID, 0, bprm_committed_creds, struct linux_binprm *bprm)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, fs_context_dup,
+	 struct fs_context *fc, struct fs_context *src_sc)
+LSM_HOOK(int, -ENOPARAM, LSM_RET_NEG | LSM_RET_ZERO | LSM_RET_ONE,
+	 fs_context_parse_param, struct fs_context *fc,
 	 struct fs_parameter *param)
-LSM_HOOK(int, 0, sb_alloc_security, struct super_block *sb)
-LSM_HOOK(void, LSM_RET_VOID, sb_delete, struct super_block *sb)
-LSM_HOOK(void, LSM_RET_VOID, sb_free_security, struct super_block *sb)
-LSM_HOOK(void, LSM_RET_VOID, sb_free_mnt_opts, void *mnt_opts)
-LSM_HOOK(int, 0, sb_eat_lsm_opts, char *orig, void **mnt_opts)
-LSM_HOOK(int, 0, sb_mnt_opts_compat, struct super_block *sb, void *mnt_opts)
-LSM_HOOK(int, 0, sb_remount, struct super_block *sb, void *mnt_opts)
-LSM_HOOK(int, 0, sb_kern_mount, struct super_block *sb)
-LSM_HOOK(int, 0, sb_show_options, struct seq_file *m, struct super_block *sb)
-LSM_HOOK(int, 0, sb_statfs, struct dentry *dentry)
-LSM_HOOK(int, 0, sb_mount, const char *dev_name, const struct path *path,
-	 const char *type, unsigned long flags, void *data)
-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,
-	 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,
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sb_alloc_security,
+	 struct super_block *sb)
+LSM_HOOK(void, LSM_RET_VOID, 0, sb_delete, struct super_block *sb)
+LSM_HOOK(void, LSM_RET_VOID, 0, sb_free_security, struct super_block *sb)
+LSM_HOOK(void, LSM_RET_VOID, 0, sb_free_mnt_opts, void *mnt_opts)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sb_eat_lsm_opts, char *orig,
+	 void **mnt_opts)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sb_mnt_opts_compat,
+	 struct super_block *sb, void *mnt_opts)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sb_remount, struct super_block *sb,
+	 void *mnt_opts)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sb_kern_mount,
+	 struct super_block *sb)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sb_show_options,
+	 struct seq_file *m, struct super_block *sb)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sb_statfs, struct dentry *dentry)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sb_mount, const char *dev_name,
+	 const struct path *path, const char *type, unsigned long flags,
+	 void *data)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sb_umount, struct vfsmount *mnt,
+	 int flags)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sb_pivotroot,
+	 const struct path *old_path, const struct path *new_path)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sb_set_mnt_opts,
+	 struct super_block *sb, void *mnt_opts, unsigned long kern_flags,
 	 unsigned long *set_kern_flags)
-LSM_HOOK(int, 0, move_mount, const struct path *from_path,
-	 const struct path *to_path)
-LSM_HOOK(int, -EOPNOTSUPP, dentry_init_security, struct dentry *dentry,
-	 int mode, const struct qstr *name, const char **xattr_name,
-	 void **ctx, u32 *ctxlen)
-LSM_HOOK(int, 0, dentry_create_files_as, struct dentry *dentry, int mode,
-	 struct qstr *name, const struct cred *old, struct cred *new)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sb_clone_mnt_opts,
+	 const struct super_block *oldsb, struct super_block *newsb,
+	 unsigned long kern_flags, unsigned long *set_kern_flags)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, move_mount,
+	 const struct path *from_path, const struct path *to_path)
+LSM_HOOK(int, -EOPNOTSUPP, LSM_RET_NEG | LSM_RET_ZERO, dentry_init_security,
+	 struct dentry *dentry, int mode, const struct qstr *name,
+	 const char **xattr_name, void **ctx, u32 *ctxlen)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, dentry_create_files_as,
+	 struct dentry *dentry, int mode, struct qstr *name,
+	 const struct cred *old, struct cred *new)
 
 #ifdef CONFIG_SECURITY_PATH
-LSM_HOOK(int, 0, path_unlink, const struct path *dir, struct dentry *dentry)
-LSM_HOOK(int, 0, path_mkdir, const struct path *dir, struct dentry *dentry,
-	 umode_t mode)
-LSM_HOOK(int, 0, path_rmdir, const struct path *dir, struct dentry *dentry)
-LSM_HOOK(int, 0, path_mknod, const struct path *dir, struct dentry *dentry,
-	 umode_t mode, unsigned int dev)
-LSM_HOOK(int, 0, path_truncate, const struct path *path)
-LSM_HOOK(int, 0, path_symlink, const struct path *dir, struct dentry *dentry,
-	 const char *old_name)
-LSM_HOOK(int, 0, path_link, struct dentry *old_dentry,
-	 const struct path *new_dir, struct dentry *new_dentry)
-LSM_HOOK(int, 0, path_rename, const struct path *old_dir,
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, path_unlink,
+	 const struct path *dir, struct dentry *dentry)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, path_mkdir, const struct path *dir,
+	 struct dentry *dentry, umode_t mode)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, path_rmdir, const struct path *dir,
+	 struct dentry *dentry)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, path_mknod, const struct path *dir,
+	 struct dentry *dentry, umode_t mode, unsigned int dev)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, path_truncate,
+	 const struct path *path)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, path_symlink,
+	 const struct path *dir, struct dentry *dentry, const char *old_name)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, path_link,
 	 struct dentry *old_dentry, const struct path *new_dir,
-	 struct dentry *new_dentry, unsigned int flags)
-LSM_HOOK(int, 0, path_chmod, const struct path *path, umode_t mode)
-LSM_HOOK(int, 0, path_chown, const struct path *path, kuid_t uid, kgid_t gid)
-LSM_HOOK(int, 0, path_chroot, const struct path *path)
+	 struct dentry *new_dentry)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, path_rename,
+	 const struct path *old_dir, struct dentry *old_dentry,
+	 const struct path *new_dir, struct dentry *new_dentry,
+	 unsigned int flags)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, path_chmod,
+	 const struct path *path, umode_t mode)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, path_chown,
+	 const struct path *path, kuid_t uid, kgid_t gid)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, path_chroot,
+	 const struct path *path)
 #endif /* CONFIG_SECURITY_PATH */
 
 /* Needed for inode based security check */
-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,
-	 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,
-	 const struct qstr *name, const struct inode *context_inode)
-LSM_HOOK(int, 0, inode_create, struct inode *dir, struct dentry *dentry,
-	 umode_t mode)
-LSM_HOOK(int, 0, inode_link, struct dentry *old_dentry, struct inode *dir,
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, path_notify,
+	 const struct path *path, u64 mask, unsigned int obj_type)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, inode_alloc_security,
+	 struct inode *inode)
+LSM_HOOK(void, LSM_RET_VOID, 0, inode_free_security, struct inode *inode)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, 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, LSM_RET_NEG | LSM_RET_ZERO, inode_init_security_anon,
+	 struct inode *inode, const struct qstr *name,
+	 const struct inode *context_inode)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, inode_create, struct inode *dir,
+	 struct dentry *dentry, umode_t mode)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, inode_link,
+	 struct dentry *old_dentry, struct inode *dir,
 	 struct dentry *new_dentry)
-LSM_HOOK(int, 0, inode_unlink, struct inode *dir, struct dentry *dentry)
-LSM_HOOK(int, 0, inode_symlink, struct inode *dir, struct dentry *dentry,
-	 const char *old_name)
-LSM_HOOK(int, 0, inode_mkdir, struct inode *dir, struct dentry *dentry,
-	 umode_t mode)
-LSM_HOOK(int, 0, inode_rmdir, struct inode *dir, struct dentry *dentry)
-LSM_HOOK(int, 0, inode_mknod, struct inode *dir, struct dentry *dentry,
-	 umode_t mode, dev_t dev)
-LSM_HOOK(int, 0, inode_rename, struct inode *old_dir, struct dentry *old_dentry,
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, inode_unlink, struct inode *dir,
+	 struct dentry *dentry)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, inode_symlink, struct inode *dir,
+	 struct dentry *dentry, const char *old_name)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, inode_mkdir, struct inode *dir,
+	 struct dentry *dentry, umode_t mode)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, inode_rmdir, struct inode *dir,
+	 struct dentry *dentry)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, inode_mknod, struct inode *dir,
+	 struct dentry *dentry, umode_t mode, dev_t dev)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, inode_rename,
+	 struct inode *old_dir, struct dentry *old_dentry,
 	 struct inode *new_dir, struct dentry *new_dentry)
-LSM_HOOK(int, 0, inode_readlink, struct dentry *dentry)
-LSM_HOOK(int, 0, inode_follow_link, struct dentry *dentry, struct inode *inode,
-	 bool rcu)
-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 user_namespace *mnt_userns,
-	 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,
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, inode_readlink,
+	 struct dentry *dentry)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, inode_follow_link,
+	 struct dentry *dentry, struct inode *inode, bool rcu)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, inode_permission,
+	 struct inode *inode, int mask)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, inode_setattr,
+	 struct dentry *dentry, struct iattr *attr)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, inode_getattr,
+	 const struct path *path)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, inode_setxattr,
+	 struct user_namespace *mnt_userns, struct dentry *dentry,
+	 const char *name, const void *value, size_t size, int flags)
+LSM_HOOK(void, LSM_RET_VOID, 0, 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 user_namespace *mnt_userns,
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, inode_getxattr,
 	 struct dentry *dentry, const char *name)
-LSM_HOOK(int, 0, inode_need_killpriv, struct dentry *dentry)
-LSM_HOOK(int, 0, inode_killpriv, struct user_namespace *mnt_userns,
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, inode_listxattr,
 	 struct dentry *dentry)
-LSM_HOOK(int, -EOPNOTSUPP, inode_getsecurity, struct user_namespace *mnt_userns,
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, inode_removexattr,
+	 struct user_namespace *mnt_userns, struct dentry *dentry,
+	 const char *name)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO | LSM_RET_ONE | LSM_RET_GT_ONE,
+	 inode_need_killpriv, struct dentry *dentry)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, inode_killpriv,
+	 struct user_namespace *mnt_userns, struct dentry *dentry)
+LSM_HOOK(int, -EOPNOTSUPP,
+	 LSM_RET_NEG | LSM_RET_ZERO | LSM_RET_ONE | LSM_RET_GT_ONE,
+	 inode_getsecurity, struct user_namespace *mnt_userns,
 	 struct inode *inode, const char *name, void **buffer, bool alloc)
-LSM_HOOK(int, -EOPNOTSUPP, inode_setsecurity, struct inode *inode,
-	 const char *name, const void *value, size_t size, int flags)
-LSM_HOOK(int, 0, inode_listsecurity, struct inode *inode, char *buffer,
+LSM_HOOK(int, -EOPNOTSUPP, LSM_RET_NEG | LSM_RET_ZERO, inode_setsecurity,
+	 struct inode *inode, const char *name, const void *value, size_t size,
+	 int flags)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO | LSM_RET_ONE | LSM_RET_GT_ONE,
+	 inode_listsecurity, struct inode *inode, char *buffer,
 	 size_t buffer_size)
-LSM_HOOK(void, LSM_RET_VOID, inode_getsecid, struct inode *inode, u32 *secid)
-LSM_HOOK(int, 0, inode_copy_up, struct dentry *src, struct cred **new)
-LSM_HOOK(int, -EOPNOTSUPP, inode_copy_up_xattr, const char *name)
-LSM_HOOK(int, 0, kernfs_init_security, struct kernfs_node *kn_dir,
-	 struct kernfs_node *kn)
-LSM_HOOK(int, 0, file_permission, struct file *file, int mask)
-LSM_HOOK(int, 0, file_alloc_security, struct file *file)
-LSM_HOOK(void, LSM_RET_VOID, file_free_security, struct file *file)
-LSM_HOOK(int, 0, file_ioctl, struct file *file, unsigned int cmd,
-	 unsigned long arg)
-LSM_HOOK(int, 0, mmap_addr, unsigned long addr)
-LSM_HOOK(int, 0, mmap_file, struct file *file, unsigned long reqprot,
-	 unsigned long prot, unsigned long flags)
-LSM_HOOK(int, 0, file_mprotect, struct vm_area_struct *vma,
-	 unsigned long reqprot, unsigned long prot)
-LSM_HOOK(int, 0, file_lock, struct file *file, unsigned int cmd)
-LSM_HOOK(int, 0, file_fcntl, struct file *file, unsigned int cmd,
-	 unsigned long arg)
-LSM_HOOK(void, LSM_RET_VOID, file_set_fowner, struct file *file)
-LSM_HOOK(int, 0, file_send_sigiotask, struct task_struct *tsk,
-	 struct fown_struct *fown, int sig)
-LSM_HOOK(int, 0, file_receive, struct file *file)
-LSM_HOOK(int, 0, file_open, struct file *file)
-LSM_HOOK(int, 0, task_alloc, struct task_struct *task,
-	 unsigned long clone_flags)
-LSM_HOOK(void, LSM_RET_VOID, task_free, struct task_struct *task)
-LSM_HOOK(int, 0, cred_alloc_blank, struct cred *cred, gfp_t gfp)
-LSM_HOOK(void, LSM_RET_VOID, cred_free, struct cred *cred)
-LSM_HOOK(int, 0, cred_prepare, struct cred *new, const struct cred *old,
-	 gfp_t gfp)
-LSM_HOOK(void, LSM_RET_VOID, cred_transfer, struct cred *new,
+LSM_HOOK(void, LSM_RET_VOID, 0, inode_getsecid, struct inode *inode, u32 *secid)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, inode_copy_up, struct dentry *src,
+	 struct cred **new)
+LSM_HOOK(int, -EOPNOTSUPP, LSM_RET_NEG | LSM_RET_ZERO | LSM_RET_ONE,
+	 inode_copy_up_xattr, const char *name)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, kernfs_init_security,
+	 struct kernfs_node *kn_dir, struct kernfs_node *kn)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, file_permission, struct file *file,
+	 int mask)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, file_alloc_security,
+	 struct file *file)
+LSM_HOOK(void, LSM_RET_VOID, 0, file_free_security, struct file *file)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, file_ioctl, struct file *file,
+	 unsigned int cmd, unsigned long arg)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, mmap_addr, unsigned long addr)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, mmap_file, struct file *file,
+	 unsigned long reqprot, unsigned long prot, unsigned long flags)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, file_mprotect,
+	 struct vm_area_struct *vma, unsigned long reqprot, unsigned long prot)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, file_lock, struct file *file,
+	 unsigned int cmd)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, file_fcntl, struct file *file,
+	 unsigned int cmd, unsigned long arg)
+LSM_HOOK(void, LSM_RET_VOID, 0, file_set_fowner, struct file *file)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, file_send_sigiotask,
+	 struct task_struct *tsk, struct fown_struct *fown, int sig)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, file_receive, struct file *file)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, file_open, struct file *file)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, task_alloc,
+	 struct task_struct *task, unsigned long clone_flags)
+LSM_HOOK(void, LSM_RET_VOID, 0, task_free, struct task_struct *task)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, cred_alloc_blank,
+	 struct cred *cred, gfp_t gfp)
+LSM_HOOK(void, LSM_RET_VOID, 0, cred_free, struct cred *cred)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, cred_prepare, struct cred *new,
+	 const struct cred *old, gfp_t gfp)
+LSM_HOOK(void, LSM_RET_VOID, 0, cred_transfer, struct cred *new,
 	 const struct cred *old)
-LSM_HOOK(void, LSM_RET_VOID, cred_getsecid, const struct cred *c, u32 *secid)
-LSM_HOOK(int, 0, kernel_act_as, struct cred *new, u32 secid)
-LSM_HOOK(int, 0, kernel_create_files_as, struct cred *new, struct inode *inode)
-LSM_HOOK(int, 0, kernel_module_request, char *kmod_name)
-LSM_HOOK(int, 0, kernel_load_data, enum kernel_load_data_id id, bool contents)
-LSM_HOOK(int, 0, kernel_post_load_data, char *buf, loff_t size,
-	 enum kernel_load_data_id id, char *description)
-LSM_HOOK(int, 0, kernel_read_file, struct file *file,
-	 enum kernel_read_file_id id, bool contents)
-LSM_HOOK(int, 0, kernel_post_read_file, struct file *file, char *buf,
-	 loff_t size, enum kernel_read_file_id id)
-LSM_HOOK(int, 0, task_fix_setuid, struct cred *new, const struct cred *old,
-	 int flags)
-LSM_HOOK(int, 0, task_fix_setgid, struct cred *new, const struct cred * old,
-	 int flags)
-LSM_HOOK(int, 0, task_fix_setgroups, struct cred *new, const struct cred * old)
-LSM_HOOK(int, 0, task_setpgid, struct task_struct *p, pid_t pgid)
-LSM_HOOK(int, 0, task_getpgid, struct task_struct *p)
-LSM_HOOK(int, 0, task_getsid, struct task_struct *p)
-LSM_HOOK(void, LSM_RET_VOID, current_getsecid_subj, u32 *secid)
-LSM_HOOK(void, LSM_RET_VOID, task_getsecid_obj,
+LSM_HOOK(void, LSM_RET_VOID, 0, cred_getsecid, const struct cred *c, u32 *secid)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, kernel_act_as, struct cred *new,
+	 u32 secid)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, kernel_create_files_as,
+	 struct cred *new, struct inode *inode)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, kernel_module_request,
+	 char *kmod_name)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, kernel_load_data,
+	 enum kernel_load_data_id id, bool contents)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, kernel_post_load_data, char *buf,
+	 loff_t size, enum kernel_load_data_id id, char *description)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, kernel_read_file,
+	 struct file *file, enum kernel_read_file_id id, bool contents)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, kernel_post_read_file,
+	 struct file *file, char *buf, loff_t size, enum kernel_read_file_id id)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, task_fix_setuid, struct cred *new,
+	 const struct cred *old, int flags)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, task_fix_setgid, struct cred *new,
+	 const struct cred *old, int flags)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, task_fix_setgroups,
+	 struct cred *new, const struct cred *old)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, task_setpgid,
+	 struct task_struct *p, pid_t pgid)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, task_getpgid,
+	 struct task_struct *p)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, task_getsid, struct task_struct *p)
+LSM_HOOK(void, LSM_RET_VOID, 0, current_getsecid_subj, u32 *secid)
+LSM_HOOK(void, LSM_RET_VOID, 0, task_getsecid_obj,
 	 struct task_struct *p, u32 *secid)
-LSM_HOOK(int, 0, task_setnice, struct task_struct *p, int nice)
-LSM_HOOK(int, 0, task_setioprio, struct task_struct *p, int ioprio)
-LSM_HOOK(int, 0, task_getioprio, struct task_struct *p)
-LSM_HOOK(int, 0, task_prlimit, const struct cred *cred,
-	 const struct cred *tcred, unsigned int flags)
-LSM_HOOK(int, 0, task_setrlimit, struct task_struct *p, unsigned int resource,
-	 struct rlimit *new_rlim)
-LSM_HOOK(int, 0, task_setscheduler, struct task_struct *p)
-LSM_HOOK(int, 0, task_getscheduler, struct task_struct *p)
-LSM_HOOK(int, 0, task_movememory, struct task_struct *p)
-LSM_HOOK(int, 0, task_kill, struct task_struct *p, struct kernel_siginfo *info,
-	 int sig, const struct cred *cred)
-LSM_HOOK(int, -ENOSYS, task_prctl, int option, unsigned long arg2,
-	 unsigned long arg3, unsigned long arg4, unsigned long arg5)
-LSM_HOOK(void, LSM_RET_VOID, task_to_inode, struct task_struct *p,
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, task_setnice,
+	 struct task_struct *p, int nice)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, task_setioprio,
+	 struct task_struct *p, int ioprio)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, task_getioprio,
+	 struct task_struct *p)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, task_prlimit,
+	 const struct cred *cred, const struct cred *tcred, unsigned int flags)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, task_setrlimit,
+	 struct task_struct *p, unsigned int resource, struct rlimit *new_rlim)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, task_setscheduler,
+	 struct task_struct *p)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, task_getscheduler,
+	 struct task_struct *p)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, task_movememory,
+	 struct task_struct *p)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, task_kill, struct task_struct *p,
+	 struct kernel_siginfo *info, int sig, const struct cred *cred)
+LSM_HOOK(int, -ENOSYS,
+	 LSM_RET_NEG | LSM_RET_ZERO | LSM_RET_ONE | LSM_RET_GT_ONE, task_prctl,
+	 int option, unsigned long arg2, unsigned long arg3, unsigned long arg4,
+	 unsigned long arg5)
+LSM_HOOK(void, LSM_RET_VOID, 0, task_to_inode, struct task_struct *p,
 	 struct inode *inode)
-LSM_HOOK(int, 0, userns_create, const struct cred *cred)
-LSM_HOOK(int, 0, ipc_permission, struct kern_ipc_perm *ipcp, short flag)
-LSM_HOOK(void, LSM_RET_VOID, ipc_getsecid, struct kern_ipc_perm *ipcp,
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, userns_create,
+	 const struct cred *cred)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, ipc_permission,
+	 struct kern_ipc_perm *ipcp, short flag)
+LSM_HOOK(void, LSM_RET_VOID, 0, ipc_getsecid, struct kern_ipc_perm *ipcp,
 	 u32 *secid)
-LSM_HOOK(int, 0, msg_msg_alloc_security, struct msg_msg *msg)
-LSM_HOOK(void, LSM_RET_VOID, msg_msg_free_security, struct msg_msg *msg)
-LSM_HOOK(int, 0, msg_queue_alloc_security, struct kern_ipc_perm *perm)
-LSM_HOOK(void, LSM_RET_VOID, msg_queue_free_security,
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, msg_msg_alloc_security,
+	 struct msg_msg *msg)
+LSM_HOOK(void, LSM_RET_VOID, 0, msg_msg_free_security, struct msg_msg *msg)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, msg_queue_alloc_security,
+	 struct kern_ipc_perm *perm)
+LSM_HOOK(void, LSM_RET_VOID, 0, msg_queue_free_security,
+	 struct kern_ipc_perm *perm)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, msg_queue_associate,
+	 struct kern_ipc_perm *perm, int msqflg)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, msg_queue_msgctl,
+	 struct kern_ipc_perm *perm, int cmd)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, msg_queue_msgsnd,
+	 struct kern_ipc_perm *perm, struct msg_msg *msg, int msqflg)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, msg_queue_msgrcv,
+	 struct kern_ipc_perm *perm, struct msg_msg *msg,
+	 struct task_struct *target, long type, int mode)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, shm_alloc_security,
 	 struct kern_ipc_perm *perm)
-LSM_HOOK(int, 0, msg_queue_associate, struct kern_ipc_perm *perm, int msqflg)
-LSM_HOOK(int, 0, msg_queue_msgctl, struct kern_ipc_perm *perm, int cmd)
-LSM_HOOK(int, 0, msg_queue_msgsnd, struct kern_ipc_perm *perm,
-	 struct msg_msg *msg, int msqflg)
-LSM_HOOK(int, 0, msg_queue_msgrcv, struct kern_ipc_perm *perm,
-	 struct msg_msg *msg, struct task_struct *target, long type, int mode)
-LSM_HOOK(int, 0, shm_alloc_security, struct kern_ipc_perm *perm)
-LSM_HOOK(void, LSM_RET_VOID, shm_free_security, struct kern_ipc_perm *perm)
-LSM_HOOK(int, 0, shm_associate, struct kern_ipc_perm *perm, int shmflg)
-LSM_HOOK(int, 0, shm_shmctl, struct kern_ipc_perm *perm, int cmd)
-LSM_HOOK(int, 0, shm_shmat, struct kern_ipc_perm *perm, char __user *shmaddr,
-	 int shmflg)
-LSM_HOOK(int, 0, sem_alloc_security, struct kern_ipc_perm *perm)
-LSM_HOOK(void, LSM_RET_VOID, sem_free_security, struct kern_ipc_perm *perm)
-LSM_HOOK(int, 0, sem_associate, struct kern_ipc_perm *perm, int semflg)
-LSM_HOOK(int, 0, sem_semctl, struct kern_ipc_perm *perm, int cmd)
-LSM_HOOK(int, 0, sem_semop, struct kern_ipc_perm *perm, struct sembuf *sops,
-	 unsigned nsops, int alter)
-LSM_HOOK(int, 0, netlink_send, struct sock *sk, struct sk_buff *skb)
-LSM_HOOK(void, LSM_RET_VOID, d_instantiate, struct dentry *dentry,
+LSM_HOOK(void, LSM_RET_VOID, 0, shm_free_security, struct kern_ipc_perm *perm)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, shm_associate,
+	 struct kern_ipc_perm *perm, int shmflg)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, shm_shmctl,
+	 struct kern_ipc_perm *perm, int cmd)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, shm_shmat,
+	 struct kern_ipc_perm *perm, char __user *shmaddr, int shmflg)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sem_alloc_security,
+	 struct kern_ipc_perm *perm)
+LSM_HOOK(void, LSM_RET_VOID, 0, sem_free_security, struct kern_ipc_perm *perm)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sem_associate,
+	 struct kern_ipc_perm *perm, int semflg)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sem_semctl,
+	 struct kern_ipc_perm *perm, int cmd)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sem_semop,
+	 struct kern_ipc_perm *perm, struct sembuf *sops, unsigned nsops,
+	 int alter)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, netlink_send, struct sock *sk,
+	 struct sk_buff *skb)
+LSM_HOOK(void, LSM_RET_VOID, 0, d_instantiate, struct dentry *dentry,
 	 struct inode *inode)
-LSM_HOOK(int, -EINVAL, getprocattr, struct task_struct *p, const char *name,
-	 char **value)
-LSM_HOOK(int, -EINVAL, setprocattr, const char *name, void *value, size_t size)
-LSM_HOOK(int, 0, ismaclabel, const char *name)
-LSM_HOOK(int, -EOPNOTSUPP, secid_to_secctx, u32 secid, char **secdata,
-	 u32 *seclen)
-LSM_HOOK(int, 0, secctx_to_secid, const char *secdata, u32 seclen, u32 *secid)
-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,
-	 u32 *ctxlen)
+LSM_HOOK(int, -EINVAL,
+	 LSM_RET_NEG | LSM_RET_ZERO | LSM_RET_ONE | LSM_RET_GT_ONE, getprocattr,
+	 struct task_struct *p, const char *name, char **value)
+LSM_HOOK(int, -EINVAL,
+	 LSM_RET_NEG | LSM_RET_ZERO | LSM_RET_ONE | LSM_RET_GT_ONE, setprocattr,
+	 const char *name, void *value, size_t size)
+LSM_HOOK(int, 0, LSM_RET_ZERO | LSM_RET_ONE, ismaclabel, const char *name)
+LSM_HOOK(int, -EOPNOTSUPP, LSM_RET_NEG | LSM_RET_ZERO, secid_to_secctx,
+	 u32 secid, char **secdata, u32 *seclen)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, secctx_to_secid,
+	 const char *secdata, u32 seclen, u32 *secid)
+LSM_HOOK(void, LSM_RET_VOID, 0, release_secctx, char *secdata, u32 seclen)
+LSM_HOOK(void, LSM_RET_VOID, 0, inode_invalidate_secctx, struct inode *inode)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, inode_notifysecctx,
+	 struct inode *inode, void *ctx, u32 ctxlen)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, inode_setsecctx,
+	 struct dentry *dentry, void *ctx, u32 ctxlen)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, inode_getsecctx,
+	 struct inode *inode, void **ctx, u32 *ctxlen)
 
 #if defined(CONFIG_SECURITY) && defined(CONFIG_WATCH_QUEUE)
-LSM_HOOK(int, 0, post_notification, const struct cred *w_cred,
-	 const struct cred *cred, struct watch_notification *n)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, post_notification,
+	 const struct cred *w_cred, const struct cred *cred,
+	 struct watch_notification *n)
 #endif /* CONFIG_SECURITY && CONFIG_WATCH_QUEUE */
 
 #if defined(CONFIG_SECURITY) && defined(CONFIG_KEY_NOTIFICATIONS)
-LSM_HOOK(int, 0, watch_key, struct key *key)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, watch_key, struct key *key)
 #endif /* CONFIG_SECURITY && CONFIG_KEY_NOTIFICATIONS */
 
 #ifdef CONFIG_SECURITY_NETWORK
-LSM_HOOK(int, 0, unix_stream_connect, struct sock *sock, struct sock *other,
-	 struct sock *newsk)
-LSM_HOOK(int, 0, unix_may_send, struct socket *sock, struct socket *other)
-LSM_HOOK(int, 0, socket_create, int family, int type, int protocol, int kern)
-LSM_HOOK(int, 0, socket_post_create, struct socket *sock, int family, int type,
-	 int protocol, int kern)
-LSM_HOOK(int, 0, socket_socketpair, struct socket *socka, struct socket *sockb)
-LSM_HOOK(int, 0, socket_bind, struct socket *sock, struct sockaddr *address,
-	 int addrlen)
-LSM_HOOK(int, 0, socket_connect, struct socket *sock, struct sockaddr *address,
-	 int addrlen)
-LSM_HOOK(int, 0, socket_listen, struct socket *sock, int backlog)
-LSM_HOOK(int, 0, socket_accept, struct socket *sock, struct socket *newsock)
-LSM_HOOK(int, 0, socket_sendmsg, struct socket *sock, struct msghdr *msg,
-	 int size)
-LSM_HOOK(int, 0, socket_recvmsg, struct socket *sock, struct msghdr *msg,
-	 int size, int flags)
-LSM_HOOK(int, 0, socket_getsockname, struct socket *sock)
-LSM_HOOK(int, 0, socket_getpeername, struct socket *sock)
-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,
-	 char __user *optval, int __user *optlen, unsigned len)
-LSM_HOOK(int, 0, 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)
-LSM_HOOK(void, LSM_RET_VOID, sk_clone_security, const struct sock *sk,
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, unix_stream_connect,
+	 struct sock *sock, struct sock *other, struct sock *newsk)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, unix_may_send, struct socket *sock,
+	 struct socket *other)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, socket_create, int family,
+	 int type, int protocol, int kern)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, socket_post_create,
+	 struct socket *sock, int family, int type, int protocol, int kern)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, socket_socketpair,
+	 struct socket *socka, struct socket *sockb)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, socket_bind, struct socket *sock,
+	 struct sockaddr *address, int addrlen)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, socket_connect,
+	 struct socket *sock, struct sockaddr *address, int addrlen)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, socket_listen, struct socket *sock,
+	 int backlog)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, socket_accept, struct socket *sock,
+	 struct socket *newsock)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, socket_sendmsg,
+	 struct socket *sock, struct msghdr *msg, int size)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, socket_recvmsg,
+	 struct socket *sock, struct msghdr *msg, int size, int flags)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, socket_getsockname,
+	 struct socket *sock)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, socket_getpeername,
+	 struct socket *sock)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, socket_getsockopt,
+	 struct socket *sock, int level, int optname)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, socket_setsockopt,
+	 struct socket *sock, int level, int optname)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, socket_shutdown,
+	 struct socket *sock, int how)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, socket_sock_rcv_skb,
+	 struct sock *sk, struct sk_buff *skb)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, socket_getpeersec_stream,
+	 struct socket *sock, char __user *optval, int __user *optlen,
+	 unsigned len)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, socket_getpeersec_dgram,
+	 struct socket *sock, struct sk_buff *skb, u32 *secid)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sk_alloc_security, struct sock *sk,
+	 int family, gfp_t priority)
+LSM_HOOK(void, LSM_RET_VOID, 0, sk_free_security, struct sock *sk)
+LSM_HOOK(void, LSM_RET_VOID, 0, sk_clone_security, const struct sock *sk,
 	 struct sock *newsk)
-LSM_HOOK(void, LSM_RET_VOID, sk_getsecid, struct sock *sk, u32 *secid)
-LSM_HOOK(void, LSM_RET_VOID, sock_graft, struct sock *sk, struct socket *parent)
-LSM_HOOK(int, 0, inet_conn_request, const struct sock *sk, struct sk_buff *skb,
-	 struct request_sock *req)
-LSM_HOOK(void, LSM_RET_VOID, inet_csk_clone, struct sock *newsk,
+LSM_HOOK(void, LSM_RET_VOID, 0, sk_getsecid, struct sock *sk, u32 *secid)
+LSM_HOOK(void, LSM_RET_VOID, 0, sock_graft, struct sock *sk, struct socket *parent)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, inet_conn_request,
+	 const struct sock *sk, struct sk_buff *skb, struct request_sock *req)
+LSM_HOOK(void, LSM_RET_VOID, 0, inet_csk_clone, struct sock *newsk,
 	 const struct request_sock *req)
-LSM_HOOK(void, LSM_RET_VOID, inet_conn_established, struct sock *sk,
+LSM_HOOK(void, LSM_RET_VOID, 0, inet_conn_established, struct sock *sk,
 	 struct sk_buff *skb)
-LSM_HOOK(int, 0, secmark_relabel_packet, u32 secid)
-LSM_HOOK(void, LSM_RET_VOID, secmark_refcount_inc, void)
-LSM_HOOK(void, LSM_RET_VOID, secmark_refcount_dec, void)
-LSM_HOOK(void, LSM_RET_VOID, req_classify_flow, const struct request_sock *req,
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, secmark_relabel_packet, u32 secid)
+LSM_HOOK(void, LSM_RET_VOID, 0, secmark_refcount_inc, void)
+LSM_HOOK(void, LSM_RET_VOID, 0, secmark_refcount_dec, void)
+LSM_HOOK(void, LSM_RET_VOID, 0, req_classify_flow, const struct request_sock *req,
 	 struct flowi_common *flic)
-LSM_HOOK(int, 0, tun_dev_alloc_security, void **security)
-LSM_HOOK(void, LSM_RET_VOID, tun_dev_free_security, void *security)
-LSM_HOOK(int, 0, tun_dev_create, void)
-LSM_HOOK(int, 0, tun_dev_attach_queue, void *security)
-LSM_HOOK(int, 0, tun_dev_attach, struct sock *sk, void *security)
-LSM_HOOK(int, 0, tun_dev_open, void *security)
-LSM_HOOK(int, 0, sctp_assoc_request, struct sctp_association *asoc,
-	 struct sk_buff *skb)
-LSM_HOOK(int, 0, sctp_bind_connect, struct sock *sk, int optname,
-	 struct sockaddr *address, int addrlen)
-LSM_HOOK(void, LSM_RET_VOID, sctp_sk_clone, struct sctp_association *asoc,
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, tun_dev_alloc_security,
+	 void **security)
+LSM_HOOK(void, LSM_RET_VOID, 0, tun_dev_free_security, void *security)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, tun_dev_create, void)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, tun_dev_attach_queue,
+	 void *security)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, tun_dev_attach, struct sock *sk,
+	 void *security)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, tun_dev_open, void *security)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sctp_assoc_request,
+	 struct sctp_association *asoc, struct sk_buff *skb)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sctp_bind_connect, struct sock *sk,
+	 int optname, struct sockaddr *address, int addrlen)
+LSM_HOOK(void, LSM_RET_VOID, 0, sctp_sk_clone, struct sctp_association *asoc,
 	 struct sock *sk, struct sock *newsk)
-LSM_HOOK(int, 0, sctp_assoc_established, struct sctp_association *asoc,
-	 struct sk_buff *skb)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, sctp_assoc_established,
+	 struct sctp_association *asoc, struct sk_buff *skb)
 #endif /* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_INFINIBAND
-LSM_HOOK(int, 0, ib_pkey_access, void *sec, u64 subnet_prefix, u16 pkey)
-LSM_HOOK(int, 0, ib_endport_manage_subnet, void *sec, const char *dev_name,
-	 u8 port_num)
-LSM_HOOK(int, 0, ib_alloc_security, void **sec)
-LSM_HOOK(void, LSM_RET_VOID, ib_free_security, void *sec)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, ib_pkey_access, void *sec,
+	 u64 subnet_prefix, u16 pkey)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, ib_endport_manage_subnet,
+	 void *sec, const char *dev_name, u8 port_num)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, ib_alloc_security, void **sec)
+LSM_HOOK(void, LSM_RET_VOID, 0, ib_free_security, void *sec)
 #endif /* CONFIG_SECURITY_INFINIBAND */
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
-LSM_HOOK(int, 0, xfrm_policy_alloc_security, struct xfrm_sec_ctx **ctxp,
-	 struct xfrm_user_sec_ctx *sec_ctx, gfp_t gfp)
-LSM_HOOK(int, 0, xfrm_policy_clone_security, struct xfrm_sec_ctx *old_ctx,
-	 struct xfrm_sec_ctx **new_ctx)
-LSM_HOOK(void, LSM_RET_VOID, xfrm_policy_free_security,
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, xfrm_policy_alloc_security,
+	 struct xfrm_sec_ctx **ctxp, struct xfrm_user_sec_ctx *sec_ctx,
+	 gfp_t gfp)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, xfrm_policy_clone_security,
+	 struct xfrm_sec_ctx *old_ctx, struct xfrm_sec_ctx **new_ctx)
+LSM_HOOK(void, LSM_RET_VOID, 0, xfrm_policy_free_security,
+	 struct xfrm_sec_ctx *ctx)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, xfrm_policy_delete_security,
 	 struct xfrm_sec_ctx *ctx)
-LSM_HOOK(int, 0, xfrm_policy_delete_security, struct xfrm_sec_ctx *ctx)
-LSM_HOOK(int, 0, xfrm_state_alloc, struct xfrm_state *x,
-	 struct xfrm_user_sec_ctx *sec_ctx)
-LSM_HOOK(int, 0, xfrm_state_alloc_acquire, struct xfrm_state *x,
-	 struct xfrm_sec_ctx *polsec, u32 secid)
-LSM_HOOK(void, LSM_RET_VOID, xfrm_state_free_security, struct xfrm_state *x)
-LSM_HOOK(int, 0, xfrm_state_delete_security, struct xfrm_state *x)
-LSM_HOOK(int, 0, xfrm_policy_lookup, struct xfrm_sec_ctx *ctx, u32 fl_secid)
-LSM_HOOK(int, 1, xfrm_state_pol_flow_match, struct xfrm_state *x,
-	 struct xfrm_policy *xp, const struct flowi_common *flic)
-LSM_HOOK(int, 0, xfrm_decode_session, struct sk_buff *skb, u32 *secid,
-	 int ckall)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, xfrm_state_alloc,
+	 struct xfrm_state *x, struct xfrm_user_sec_ctx *sec_ctx)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, xfrm_state_alloc_acquire,
+	 struct xfrm_state *x, struct xfrm_sec_ctx *polsec, u32 secid)
+LSM_HOOK(void, LSM_RET_VOID, 0, xfrm_state_free_security, struct xfrm_state *x)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, xfrm_state_delete_security,
+	 struct xfrm_state *x)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, xfrm_policy_lookup,
+	 struct xfrm_sec_ctx *ctx, u32 fl_secid)
+LSM_HOOK(int, 1, LSM_RET_ZERO | LSM_RET_ONE, xfrm_state_pol_flow_match,
+	 struct xfrm_state *x, struct xfrm_policy *xp,
+	 const struct flowi_common *flic)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, xfrm_decode_session,
+	 struct sk_buff *skb, u32 *secid, int ckall)
 #endif /* CONFIG_SECURITY_NETWORK_XFRM */
 
 /* key management security hooks */
 #ifdef CONFIG_KEYS
-LSM_HOOK(int, 0, key_alloc, struct key *key, const struct cred *cred,
-	 unsigned long flags)
-LSM_HOOK(void, LSM_RET_VOID, key_free, struct key *key)
-LSM_HOOK(int, 0, key_permission, key_ref_t key_ref, const struct cred *cred,
-	 enum key_need_perm need_perm)
-LSM_HOOK(int, 0, key_getsecurity, struct key *key, char **_buffer)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, key_alloc, struct key *key,
+	 const struct cred *cred, unsigned long flags)
+LSM_HOOK(void, LSM_RET_VOID, 0, key_free, struct key *key)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, key_permission, key_ref_t key_ref,
+	 const struct cred *cred, enum key_need_perm need_perm)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO | LSM_RET_ONE | LSM_RET_GT_ONE,
+	 key_getsecurity, struct key *key, char **_buffer)
 #endif /* CONFIG_KEYS */
 
 #ifdef CONFIG_AUDIT
-LSM_HOOK(int, 0, audit_rule_init, u32 field, u32 op, char *rulestr,
-	 void **lsmrule)
-LSM_HOOK(int, 0, audit_rule_known, struct audit_krule *krule)
-LSM_HOOK(int, 0, audit_rule_match, u32 secid, u32 field, u32 op, void *lsmrule)
-LSM_HOOK(void, LSM_RET_VOID, audit_rule_free, void *lsmrule)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, audit_rule_init, u32 field, u32 op,
+	 char *rulestr, void **lsmrule)
+LSM_HOOK(int, 0, LSM_RET_ZERO | LSM_RET_ONE, audit_rule_known,
+	 struct audit_krule *krule)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO | LSM_RET_ONE, audit_rule_match,
+	 u32 secid, u32 field, u32 op, void *lsmrule)
+LSM_HOOK(void, LSM_RET_VOID, 0, audit_rule_free, void *lsmrule)
 #endif /* CONFIG_AUDIT */
 
 #ifdef CONFIG_BPF_SYSCALL
-LSM_HOOK(int, 0, bpf, int cmd, union bpf_attr *attr, unsigned int size)
-LSM_HOOK(int, 0, bpf_map, struct bpf_map *map, fmode_t fmode)
-LSM_HOOK(int, 0, bpf_prog, struct bpf_prog *prog)
-LSM_HOOK(int, 0, bpf_map_alloc_security, struct bpf_map *map)
-LSM_HOOK(void, LSM_RET_VOID, bpf_map_free_security, struct bpf_map *map)
-LSM_HOOK(int, 0, bpf_prog_alloc_security, struct bpf_prog_aux *aux)
-LSM_HOOK(void, LSM_RET_VOID, bpf_prog_free_security, struct bpf_prog_aux *aux)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, bpf, int cmd, union bpf_attr *attr,
+	 unsigned int size)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, bpf_map, struct bpf_map *map,
+	 fmode_t fmode)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, bpf_prog, struct bpf_prog *prog)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, bpf_map_alloc_security,
+	 struct bpf_map *map)
+LSM_HOOK(void, LSM_RET_VOID, 0, bpf_map_free_security, struct bpf_map *map)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, bpf_prog_alloc_security,
+	 struct bpf_prog_aux *aux)
+LSM_HOOK(void, LSM_RET_VOID, 0, bpf_prog_free_security, struct bpf_prog_aux *aux)
 #endif /* CONFIG_BPF_SYSCALL */
 
-LSM_HOOK(int, 0, locked_down, enum lockdown_reason what)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, locked_down,
+	 enum lockdown_reason what)
 
 #ifdef CONFIG_PERF_EVENTS
-LSM_HOOK(int, 0, perf_event_open, struct perf_event_attr *attr, int type)
-LSM_HOOK(int, 0, perf_event_alloc, struct perf_event *event)
-LSM_HOOK(void, LSM_RET_VOID, perf_event_free, struct perf_event *event)
-LSM_HOOK(int, 0, perf_event_read, struct perf_event *event)
-LSM_HOOK(int, 0, perf_event_write, struct perf_event *event)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, perf_event_open,
+	 struct perf_event_attr *attr, int type)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, perf_event_alloc,
+	 struct perf_event *event)
+LSM_HOOK(void, LSM_RET_VOID, 0, perf_event_free, struct perf_event *event)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, perf_event_read,
+	 struct perf_event *event)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, perf_event_write,
+	 struct perf_event *event)
 #endif /* CONFIG_PERF_EVENTS */
 
 #ifdef CONFIG_IO_URING
-LSM_HOOK(int, 0, uring_override_creds, const struct cred *new)
-LSM_HOOK(int, 0, uring_sqpoll, void)
-LSM_HOOK(int, 0, uring_cmd, struct io_uring_cmd *ioucmd)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, uring_override_creds,
+	 const struct cred *new)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, uring_sqpoll, void)
+LSM_HOOK(int, 0, LSM_RET_NEG | LSM_RET_ZERO, uring_cmd,
+	 struct io_uring_cmd *ioucmd)
 #endif /* CONFIG_IO_URING */
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index c0c570b7eabd..1911675a0da0 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1642,13 +1642,13 @@
  *
  */
 union security_list_options {
-	#define LSM_HOOK(RET, DEFAULT, NAME, ...) RET (*NAME)(__VA_ARGS__);
+	#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) RET (*NAME)(__VA_ARGS__);
 	#include "lsm_hook_defs.h"
 	#undef LSM_HOOK
 };
 
 struct security_hook_heads {
-	#define LSM_HOOK(RET, DEFAULT, NAME, ...) struct hlist_head NAME;
+	#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) struct hlist_head NAME;
 	#include "lsm_hook_defs.h"
 	#undef LSM_HOOK
 } __randomize_layout;
@@ -1683,6 +1683,11 @@ struct lsm_blob_sizes {
  */
 #define LSM_RET_VOID ((void) 0)
 
+#define LSM_RET_NEG	0x00000001
+#define LSM_RET_ZERO	0x00000002
+#define LSM_RET_ONE	0x00000004
+#define LSM_RET_GT_ONE	0x00000008
+
 /*
  * Initializing a security_hook_list structure takes
  * up a lot of space in a source file. This macro takes
diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
index d6c9b3705f24..37bcedf5a44e 100644
--- a/kernel/bpf/bpf_lsm.c
+++ b/kernel/bpf/bpf_lsm.c
@@ -21,7 +21,7 @@
 /* For every LSM hook that allows attachment of BPF programs, declare a nop
  * function where a BPF program can be attached.
  */
-#define LSM_HOOK(RET, DEFAULT, NAME, ...)	\
+#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...)	\
 noinline RET bpf_lsm_##NAME(__VA_ARGS__)	\
 {						\
 	return DEFAULT;				\
@@ -30,7 +30,8 @@ noinline RET bpf_lsm_##NAME(__VA_ARGS__)	\
 #include <linux/lsm_hook_defs.h>
 #undef LSM_HOOK
 
-#define LSM_HOOK(RET, DEFAULT, NAME, ...) BTF_ID(func, bpf_lsm_##NAME)
+#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \
+	BTF_ID(func, bpf_lsm_##NAME)
 BTF_SET_START(bpf_lsm_hooks)
 #include <linux/lsm_hook_defs.h>
 #undef LSM_HOOK
diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
index e5971fa74fd7..a2a3b2be345f 100644
--- a/security/bpf/hooks.c
+++ b/security/bpf/hooks.c
@@ -7,7 +7,7 @@
 #include <linux/bpf_lsm.h>
 
 static struct security_hook_list bpf_lsm_hooks[] __lsm_ro_after_init = {
-	#define LSM_HOOK(RET, DEFAULT, NAME, ...) \
+	#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \
 	LSM_HOOK_INIT(NAME, bpf_lsm_##NAME),
 	#include <linux/lsm_hook_defs.h>
 	#undef LSM_HOOK
diff --git a/security/security.c b/security/security.c
index 79d82cb6e469..4041d24e3283 100644
--- a/security/security.c
+++ b/security/security.c
@@ -371,7 +371,7 @@ int __init early_security_init(void)
 {
 	struct lsm_info *lsm;
 
-#define LSM_HOOK(RET, DEFAULT, NAME, ...) \
+#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \
 	INIT_HLIST_HEAD(&security_hook_heads.NAME);
 #include "linux/lsm_hook_defs.h"
 #undef LSM_HOOK
@@ -710,7 +710,7 @@ static int lsm_superblock_alloc(struct super_block *sb)
 #define DECLARE_LSM_RET_DEFAULT_void(DEFAULT, NAME)
 #define DECLARE_LSM_RET_DEFAULT_int(DEFAULT, NAME) \
 	static const int __maybe_unused LSM_RET_DEFAULT(NAME) = (DEFAULT);
-#define LSM_HOOK(RET, DEFAULT, NAME, ...) \
+#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \
 	DECLARE_LSM_RET_DEFAULT_##RET(DEFAULT, NAME)
 
 #include <linux/lsm_hook_defs.h>
-- 
2.25.1

Re: [RFC][PATCH 3/4] lsm: Redefine LSM_HOOK() macro to add return value flags as argument

[Paul Moore]

On Tue, Nov 15, 2022 at 12:58 PM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> From: Roberto Sassu <roberto.sassu@huawei.com>
>
> Define four return value flags (LSM_RET_NEG, LSM_RET_ZERO, LSM_RET_ONE,
> LSM_RET_GT_ONE), one for each interval of interest (< 0, = 0, = 1, > 1).
>
> Redefine the LSM_HOOK() macro to add return value flags as argument, and
> set the correct flags for each LSM hook.
>
> Implementors of new LSM hooks should do the same as well.
>
> Cc: stable@vger.kernel.org # 5.7.x
> Fixes: 9d3fdea789c8 ("bpf: lsm: Provide attachment points for BPF LSM programs")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  include/linux/bpf_lsm.h       |   2 +-
>  include/linux/lsm_hook_defs.h | 779 ++++++++++++++++++++--------------
>  include/linux/lsm_hooks.h     |   9 +-
>  kernel/bpf/bpf_lsm.c          |   5 +-
>  security/bpf/hooks.c          |   2 +-
>  security/security.c           |   4 +-
>  6 files changed, 466 insertions(+), 335 deletions(-)

Just a quick note here that even if we wanted to do something like
this, it is absolutely not -stable kernel material.  No way.

-- 
paul-moore.com

Re: [RFC][PATCH 3/4] lsm: Redefine LSM_HOOK() macro to add return value flags as argument

[Roberto Sassu]

On Tue, 2022-11-15 at 21:27 -0500, Paul Moore wrote:
> On Tue, Nov 15, 2022 at 12:58 PM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > Define four return value flags (LSM_RET_NEG, LSM_RET_ZERO, LSM_RET_ONE,
> > LSM_RET_GT_ONE), one for each interval of interest (< 0, = 0, = 1, > 1).
> > 
> > Redefine the LSM_HOOK() macro to add return value flags as argument, and
> > set the correct flags for each LSM hook.
> > 
> > Implementors of new LSM hooks should do the same as well.
> > 
> > Cc: stable@vger.kernel.org # 5.7.x
> > Fixes: 9d3fdea789c8 ("bpf: lsm: Provide attachment points for BPF LSM programs")
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >  include/linux/bpf_lsm.h       |   2 +-
> >  include/linux/lsm_hook_defs.h | 779 ++++++++++++++++++++--------------
> >  include/linux/lsm_hooks.h     |   9 +-
> >  kernel/bpf/bpf_lsm.c          |   5 +-
> >  security/bpf/hooks.c          |   2 +-
> >  security/security.c           |   4 +-
> >  6 files changed, 466 insertions(+), 335 deletions(-)
> 
> Just a quick note here that even if we wanted to do something like
> this, it is absolutely not -stable kernel material.  No way.

I was unsure about that. We need a proper fix for this issue that needs
to be backported to some kernels. I saw this more like a dependency.
But I agree with you that it would be unlikely that this patch is
applied to stable kernels.

For stable kernels, what it would be the proper way? We still need to
maintain an allow list of functions that allow a positive return value,
at least. Should it be in the eBPF code only?

Thanks

Roberto

Re: [RFC][PATCH 3/4] lsm: Redefine LSM_HOOK() macro to add return value flags as argument

[Paul Moore]

On Wed, Nov 16, 2022 at 3:11 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
> On Tue, 2022-11-15 at 21:27 -0500, Paul Moore wrote:
> > On Tue, Nov 15, 2022 at 12:58 PM Roberto Sassu
> > <roberto.sassu@huaweicloud.com> wrote:
> > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > >
> > > Define four return value flags (LSM_RET_NEG, LSM_RET_ZERO, LSM_RET_ONE,
> > > LSM_RET_GT_ONE), one for each interval of interest (< 0, = 0, = 1, > 1).
> > >
> > > Redefine the LSM_HOOK() macro to add return value flags as argument, and
> > > set the correct flags for each LSM hook.
> > >
> > > Implementors of new LSM hooks should do the same as well.
> > >
> > > Cc: stable@vger.kernel.org # 5.7.x
> > > Fixes: 9d3fdea789c8 ("bpf: lsm: Provide attachment points for BPF LSM programs")
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > ---
> > >  include/linux/bpf_lsm.h       |   2 +-
> > >  include/linux/lsm_hook_defs.h | 779 ++++++++++++++++++++--------------
> > >  include/linux/lsm_hooks.h     |   9 +-
> > >  kernel/bpf/bpf_lsm.c          |   5 +-
> > >  security/bpf/hooks.c          |   2 +-
> > >  security/security.c           |   4 +-
> > >  6 files changed, 466 insertions(+), 335 deletions(-)
> >
> > Just a quick note here that even if we wanted to do something like
> > this, it is absolutely not -stable kernel material.  No way.
>
> I was unsure about that. We need a proper fix for this issue that needs
> to be backported to some kernels. I saw this more like a dependency.
> But I agree with you that it would be unlikely that this patch is
> applied to stable kernels.
>
> For stable kernels, what it would be the proper way? We still need to
> maintain an allow list of functions that allow a positive return value,
> at least. Should it be in the eBPF code only?

Ideally the fix for -stable is the same as what is done for Linus'
kernel (ignoring backport fuzzing), so I would wait and see how that
ends up first.  However, if the patchset for Linus' tree is
particularly large and touches a lot of code, you may need to work on
something a bit more targeted to the specific problem.  I tend to be
more conservative than most kernel devs when it comes to -stable
patches, but if you can't backport the main upstream patchset, smaller
(both in terms of impact and lines changed) is almost always better.

-- 
paul-moore.com

Re: [RFC][PATCH 3/4] lsm: Redefine LSM_HOOK() macro to add return value flags as argument

[Greg KH]

On Wed, Nov 16, 2022 at 05:04:05PM -0500, Paul Moore wrote:
> On Wed, Nov 16, 2022 at 3:11 AM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> > On Tue, 2022-11-15 at 21:27 -0500, Paul Moore wrote:
> > > On Tue, Nov 15, 2022 at 12:58 PM Roberto Sassu
> > > <roberto.sassu@huaweicloud.com> wrote:
> > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > >
> > > > Define four return value flags (LSM_RET_NEG, LSM_RET_ZERO, LSM_RET_ONE,
> > > > LSM_RET_GT_ONE), one for each interval of interest (< 0, = 0, = 1, > 1).
> > > >
> > > > Redefine the LSM_HOOK() macro to add return value flags as argument, and
> > > > set the correct flags for each LSM hook.
> > > >
> > > > Implementors of new LSM hooks should do the same as well.
> > > >
> > > > Cc: stable@vger.kernel.org # 5.7.x
> > > > Fixes: 9d3fdea789c8 ("bpf: lsm: Provide attachment points for BPF LSM programs")
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > ---
> > > >  include/linux/bpf_lsm.h       |   2 +-
> > > >  include/linux/lsm_hook_defs.h | 779 ++++++++++++++++++++--------------
> > > >  include/linux/lsm_hooks.h     |   9 +-
> > > >  kernel/bpf/bpf_lsm.c          |   5 +-
> > > >  security/bpf/hooks.c          |   2 +-
> > > >  security/security.c           |   4 +-
> > > >  6 files changed, 466 insertions(+), 335 deletions(-)
> > >
> > > Just a quick note here that even if we wanted to do something like
> > > this, it is absolutely not -stable kernel material.  No way.
> >
> > I was unsure about that. We need a proper fix for this issue that needs
> > to be backported to some kernels. I saw this more like a dependency.
> > But I agree with you that it would be unlikely that this patch is
> > applied to stable kernels.
> >
> > For stable kernels, what it would be the proper way? We still need to
> > maintain an allow list of functions that allow a positive return value,
> > at least. Should it be in the eBPF code only?
> 
> Ideally the fix for -stable is the same as what is done for Linus'
> kernel (ignoring backport fuzzing), so I would wait and see how that
> ends up first.  However, if the patchset for Linus' tree is
> particularly large and touches a lot of code, you may need to work on
> something a bit more targeted to the specific problem.  I tend to be
> more conservative than most kernel devs when it comes to -stable
> patches, but if you can't backport the main upstream patchset, smaller
> (both in terms of impact and lines changed) is almost always better.

No, the mainline patch (what is in Linus's tree), is almost always
better and preferred for stable backports.  When you diverge, bugs
happen, almost every time, and it makes later fixes harder to backport
as well.

But first work on solving the problem in Linus's tree.  Don't worry
about stable trees until after the correct solution is merged.

thanks,

greg k-h

Re: [RFC][PATCH 3/4] lsm: Redefine LSM_HOOK() macro to add return value flags as argument

[Paul Moore]

On Thu, Nov 17, 2022 at 12:50 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Nov 16, 2022 at 05:04:05PM -0500, Paul Moore wrote:
> > On Wed, Nov 16, 2022 at 3:11 AM Roberto Sassu
> > <roberto.sassu@huaweicloud.com> wrote:
> > > On Tue, 2022-11-15 at 21:27 -0500, Paul Moore wrote:
> > > > On Tue, Nov 15, 2022 at 12:58 PM Roberto Sassu
> > > > <roberto.sassu@huaweicloud.com> wrote:
> > > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > >
> > > > > Define four return value flags (LSM_RET_NEG, LSM_RET_ZERO, LSM_RET_ONE,
> > > > > LSM_RET_GT_ONE), one for each interval of interest (< 0, = 0, = 1, > 1).
> > > > >
> > > > > Redefine the LSM_HOOK() macro to add return value flags as argument, and
> > > > > set the correct flags for each LSM hook.
> > > > >
> > > > > Implementors of new LSM hooks should do the same as well.
> > > > >
> > > > > Cc: stable@vger.kernel.org # 5.7.x
> > > > > Fixes: 9d3fdea789c8 ("bpf: lsm: Provide attachment points for BPF LSM programs")
> > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > ---
> > > > >  include/linux/bpf_lsm.h       |   2 +-
> > > > >  include/linux/lsm_hook_defs.h | 779 ++++++++++++++++++++--------------
> > > > >  include/linux/lsm_hooks.h     |   9 +-
> > > > >  kernel/bpf/bpf_lsm.c          |   5 +-
> > > > >  security/bpf/hooks.c          |   2 +-
> > > > >  security/security.c           |   4 +-
> > > > >  6 files changed, 466 insertions(+), 335 deletions(-)
> > > >
> > > > Just a quick note here that even if we wanted to do something like
> > > > this, it is absolutely not -stable kernel material.  No way.
> > >
> > > I was unsure about that. We need a proper fix for this issue that needs
> > > to be backported to some kernels. I saw this more like a dependency.
> > > But I agree with you that it would be unlikely that this patch is
> > > applied to stable kernels.
> > >
> > > For stable kernels, what it would be the proper way? We still need to
> > > maintain an allow list of functions that allow a positive return value,
> > > at least. Should it be in the eBPF code only?
> >
> > Ideally the fix for -stable is the same as what is done for Linus'
> > kernel (ignoring backport fuzzing), so I would wait and see how that
> > ends up first.  However, if the patchset for Linus' tree is
> > particularly large and touches a lot of code, you may need to work on
> > something a bit more targeted to the specific problem.  I tend to be
> > more conservative than most kernel devs when it comes to -stable
> > patches, but if you can't backport the main upstream patchset, smaller
> > (both in terms of impact and lines changed) is almost always better.
>
> No, the mainline patch (what is in Linus's tree), is almost always
> better and preferred for stable backports.  When you diverge, bugs
> happen, almost every time, and it makes later fixes harder to backport
> as well.
>
> But first work on solving the problem in Linus's tree.  Don't worry
> about stable trees until after the correct solution is merged.

Perhaps you missed my very first sentence where I mentioned exactly
the same things: solve it in Linus' tree first, backports of patches
in Linus' tree is ideal.

-- 
paul-moore.com

[RFC][PATCH 4/4] security: Enforce limitations on return values from LSMs

[Roberto Sassu]

From: Roberto Sassu <roberto.sassu@huawei.com>

LSMs should not be able to return arbitrary return values, as the callers
of the LSM infrastructure might not be ready to handle unexpected values
(e.g. positive values that are first converted to a pointer with ERR_PTR,
and then evaluated with IS_ERR()).

Modify call_int_hook() to call is_ret_value_allowed(), so that the return
value from each LSM for a given hook is checked. If for the interval the
return value falls into the corresponding flag is not set, change the
return value to the default value, just for the current LSM.

A misbehaving LSM would not have impact on the decision of other LSMs, as
the loop terminates whenever the return value is not zero.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/security.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/security/security.c b/security/security.c
index 4041d24e3283..cd417a8a0e65 100644
--- a/security/security.c
+++ b/security/security.c
@@ -716,6 +716,35 @@ static int lsm_superblock_alloc(struct super_block *sb)
 #include <linux/lsm_hook_defs.h>
 #undef LSM_HOOK
 
+/*
+ * The return value flags of the LSM hook are defined in linux/lsm_hook_defs.h
+ * and can be accessed with:
+ *
+ *	LSM_RET_FLAGS(<hook_name>)
+ *
+ * The macros below define static constants for the return value flags of each
+ * LSM hook.
+ */
+#define LSM_RET_FLAGS(NAME) (NAME##_ret_flags)
+#define DECLARE_LSM_RET_FLAGS(RET_FLAGS, NAME) \
+	static const u32 __maybe_unused LSM_RET_FLAGS(NAME) = (RET_FLAGS);
+#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \
+	DECLARE_LSM_RET_FLAGS(RET_FLAGS, NAME)
+
+#include <linux/lsm_hook_defs.h>
+#undef LSM_HOOK
+
+static bool is_ret_value_allowed(int ret, u32 ret_flags)
+{
+	if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) ||
+	    (ret == 0 && !(ret_flags & LSM_RET_ZERO)) ||
+	    (ret == 1 && !(ret_flags & LSM_RET_ONE)) ||
+	    (ret > 1 && !(ret_flags & LSM_RET_GT_ONE)))
+		return false;
+
+	return true;
+}
+
 /*
  * Hook list operation macros.
  *
@@ -741,6 +770,11 @@ static int lsm_superblock_alloc(struct super_block *sb)
 								\
 		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
 			RC = P->hook.FUNC(__VA_ARGS__);		\
+			if (!is_ret_value_allowed(RC, LSM_RET_FLAGS(FUNC))) { \
+				WARN_ONCE(1, "Illegal ret %d for " #FUNC " from %s, forcing %d\n", \
+					  RC, P->lsm, LSM_RET_DEFAULT(FUNC)); \
+				RC = LSM_RET_DEFAULT(FUNC);	\
+			}					\
 			if (RC != 0)				\
 				break;				\
 		}						\
-- 
2.25.1

Re: [RFC][PATCH 4/4] security: Enforce limitations on return values from LSMs

[Paul Moore]

On Tue, Nov 15, 2022 at 12:58 PM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> From: Roberto Sassu <roberto.sassu@huawei.com>
>
> LSMs should not be able to return arbitrary return values, as the callers
> of the LSM infrastructure might not be ready to handle unexpected values
> (e.g. positive values that are first converted to a pointer with ERR_PTR,
> and then evaluated with IS_ERR()).
>
> Modify call_int_hook() to call is_ret_value_allowed(), so that the return
> value from each LSM for a given hook is checked. If for the interval the
> return value falls into the corresponding flag is not set, change the
> return value to the default value, just for the current LSM.
>
> A misbehaving LSM would not have impact on the decision of other LSMs, as
> the loop terminates whenever the return value is not zero.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  security/security.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)

Casey touched on some of this in his reply to patch 0/4, but basically
I see this as a BPF LSM specific problem and not a generalized LSM
issue that should be addressed at the LSM layer.  Especially if the
solution involves incurring additional processing for every LSM hook
instantiation, regardless if a BPF LSM is present.  Reading your
overall patchset description I believe that you understand this too.

If you want to somehow instrument the LSM hook definitions (what I
believe to be the motivation behind patch 3/4) to indicate valid
return values for use by the BPF verifier, I think we could entertain
that, or at least discuss it further, but I'm not inclined to support
any runtime overhead at the LSM layer for a specific LSM.

-- 
paul-moore.com

Re: [RFC][PATCH 4/4] security: Enforce limitations on return values from LSMs

[Roberto Sassu]

On Tue, 2022-11-15 at 21:35 -0500, Paul Moore wrote:
> On Tue, Nov 15, 2022 at 12:58 PM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > LSMs should not be able to return arbitrary return values, as the callers
> > of the LSM infrastructure might not be ready to handle unexpected values
> > (e.g. positive values that are first converted to a pointer with ERR_PTR,
> > and then evaluated with IS_ERR()).
> > 
> > Modify call_int_hook() to call is_ret_value_allowed(), so that the return
> > value from each LSM for a given hook is checked. If for the interval the
> > return value falls into the corresponding flag is not set, change the
> > return value to the default value, just for the current LSM.
> > 
> > A misbehaving LSM would not have impact on the decision of other LSMs, as
> > the loop terminates whenever the return value is not zero.
> > 
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >  security/security.c | 34 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> 
> Casey touched on some of this in his reply to patch 0/4, but basically
> I see this as a BPF LSM specific problem and not a generalized LSM
> issue that should be addressed at the LSM layer.  Especially if the
> solution involves incurring additional processing for every LSM hook
> instantiation, regardless if a BPF LSM is present.  Reading your
> overall patchset description I believe that you understand this too.

Yes, I had this concern too. Thanks Paul and Casey for taking the time
to reply.

I liked the fact that the fix is extremely simple, but nevertheless it
should not impact the performance, if there are alternative ways. I
thought maybe we look at non-zero values, since the check is already
there. But it could be that there is an impact for it too (maybe for
audit_rule_match?).

> If you want to somehow instrument the LSM hook definitions (what I
> believe to be the motivation behind patch 3/4) to indicate valid
> return values for use by the BPF verifier, I think we could entertain
> that, or at least discuss it further, but I'm not inclined to support
> any runtime overhead at the LSM layer for a specific LSM.

Ok, yes. Patches 1-3 would help to keep in sync the LSM infrastructure
and eBPF, but it is not strictly needed. I could propose an eBPF-only
alternative to declare sets of functions per interval.

More or less, I developed an eBPF-based alternative also for patch 4.
It is just a proof of concept. Will propose it, to validate the idea.

Thanks

Roberto

[PoC][PATCH] bpf: Call return value check function in the JITed code

[Roberto Sassu]

From: Roberto Sassu <roberto.sassu@huawei.com>

eBPF allows certain types of eBPF programs to modify the return value of
the functions they attach to. This is used for example by BPF LSM to let
security modules make their decision on LSM hooks.

The JITed code looks like the following:

    ret = bpf_lsm_inode_permission_impl1(); // from a security module
    if (ret)
        goto out;

...

    ret = bpf_lsm_inode_permission_implN(); // from a security module
    if (ret)
        goto out;

    ret = bpf_lsm_inode_permission(); // in the kernel, returns DEFAULT
out:

If ret is not zero, the attachment points of BPF LSM are not executed. For
this reason, the return value check cannot be done there.

Instead, the idea is to use the LSM_HOOK() macro to define a per-hook check
function.

Whenever an eBPF program attaches to an LSM hook, the eBPF verifier
resolves the address of the check function (whose name is
bpf_lsm_<hook name>_ret()) and adds a call to that function just after the
out label. If the return value is illegal, the check function changes it
back to the default value defined by the LSM infrastructure:

...

out:
    ret = bpf_lsm_inode_permission_ret(ret);

In this way, an eBPF program cannot cause illegal return values to be sent
to BPF LSM, and to the callers of the LSM infrastructure.

This is just a PoC, to validate the idea and get an early feedback.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 arch/arm64/net/bpf_jit_comp.c |  7 ++++---
 arch/x86/net/bpf_jit_comp.c   | 17 ++++++++++++++++-
 include/linux/bpf.h           |  4 +++-
 kernel/bpf/bpf_lsm.c          | 20 ++++++++++++++++++++
 kernel/bpf/bpf_struct_ops.c   |  2 +-
 kernel/bpf/trampoline.c       |  6 ++++--
 kernel/bpf/verifier.c         | 28 ++++++++++++++++++++++++++--
 7 files changed, 74 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index 62f805f427b7..5412230c6935 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -1764,7 +1764,7 @@ static void restore_args(struct jit_ctx *ctx, int args_off, int nargs)
  */
 static int prepare_trampoline(struct jit_ctx *ctx, struct bpf_tramp_image *im,
 			      struct bpf_tramp_links *tlinks, void *orig_call,
-			      int nargs, u32 flags)
+			      void *ret_check_call, int nargs, u32 flags)
 {
 	int i;
 	int stack_size;
@@ -1963,7 +1963,7 @@ static int prepare_trampoline(struct jit_ctx *ctx, struct bpf_tramp_image *im,
 int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image,
 				void *image_end, const struct btf_func_model *m,
 				u32 flags, struct bpf_tramp_links *tlinks,
-				void *orig_call)
+				void *orig_call, void *ret_check_call)
 {
 	int i, ret;
 	int nargs = m->nr_args;
@@ -1983,7 +1983,8 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image,
 			return -ENOTSUPP;
 	}
 
-	ret = prepare_trampoline(&ctx, im, tlinks, orig_call, nargs, flags);
+	ret = prepare_trampoline(&ctx, im, tlinks, orig_call, ret_check_call,
+				 nargs, flags);
 	if (ret < 0)
 		return ret;
 
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index cec5195602bc..6cd727b4af0a 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -2123,7 +2123,7 @@ static int invoke_bpf_mod_ret(const struct btf_func_model *m, u8 **pprog,
 int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *image_end,
 				const struct btf_func_model *m, u32 flags,
 				struct bpf_tramp_links *tlinks,
-				void *func_addr)
+				void *func_addr, void *func_ret_check_addr)
 {
 	int ret, i, nr_args = m->nr_args, extra_nregs = 0;
 	int regs_off, ip_off, args_off, stack_size = nr_args * 8, run_ctx_off;
@@ -2280,6 +2280,21 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
 		for (i = 0; i < fmod_ret->nr_links; i++)
 			emit_cond_near_jump(&branches[i], prog, branches[i],
 					    X86_JNE);
+
+		if (func_ret_check_addr) {
+			emit_ldx(&prog, BPF_DW, BPF_REG_1, BPF_REG_FP, -8);
+
+			/* call ret check function */
+			if (emit_call(&prog, func_ret_check_addr, prog)) {
+				ret = -EINVAL;
+				goto cleanup;
+			}
+
+			/* remember return value in a stack for bpf prog to access */
+			emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -8);
+			memcpy(prog, x86_nops[5], X86_PATCH_SIZE);
+			prog += X86_PATCH_SIZE;
+		}
 	}
 
 	if (fexit->nr_links)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 49f9d2bec401..f3551f7bdc28 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -919,7 +919,7 @@ struct bpf_tramp_image;
 int arch_prepare_bpf_trampoline(struct bpf_tramp_image *tr, void *image, void *image_end,
 				const struct btf_func_model *m, u32 flags,
 				struct bpf_tramp_links *tlinks,
-				void *orig_call);
+				void *orig_call, void *ret_call);
 u64 notrace __bpf_prog_enter_sleepable_recur(struct bpf_prog *prog,
 					     struct bpf_tramp_run_ctx *run_ctx);
 void notrace __bpf_prog_exit_sleepable_recur(struct bpf_prog *prog, u64 start,
@@ -974,6 +974,7 @@ struct bpf_trampoline {
 	struct {
 		struct btf_func_model model;
 		void *addr;
+		void *ret_check_addr;
 		bool ftrace_managed;
 	} func;
 	/* if !NULL this is BPF_PROG_TYPE_EXT program that extends another BPF
@@ -994,6 +995,7 @@ struct bpf_trampoline {
 struct bpf_attach_target_info {
 	struct btf_func_model fmodel;
 	long tgt_addr;
+	long tgt_ret_check_addr;
 	const char *tgt_name;
 	const struct btf_type *tgt_type;
 };
diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
index 37bcedf5a44e..f7f25d0064dd 100644
--- a/kernel/bpf/bpf_lsm.c
+++ b/kernel/bpf/bpf_lsm.c
@@ -18,6 +18,17 @@
 #include <linux/ima.h>
 #include <linux/bpf-cgroup.h>
 
+static bool is_ret_value_allowed(int ret, u32 ret_flags)
+{
+	if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) ||
+	    (ret == 0 && !(ret_flags & LSM_RET_ZERO)) ||
+	    (ret == 1 && !(ret_flags & LSM_RET_ONE)) ||
+	    (ret > 1 && !(ret_flags & LSM_RET_GT_ONE)))
+		return false;
+
+	return true;
+}
+
 /* For every LSM hook that allows attachment of BPF programs, declare a nop
  * function where a BPF program can be attached.
  */
@@ -30,6 +41,15 @@ noinline RET bpf_lsm_##NAME(__VA_ARGS__)	\
 #include <linux/lsm_hook_defs.h>
 #undef LSM_HOOK
 
+#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...)	\
+noinline RET bpf_lsm_##NAME##_ret(int ret)	\
+{						\
+	return is_ret_value_allowed(ret, RET_FLAGS) ? ret : DEFAULT; \
+}
+
+#include <linux/lsm_hook_defs.h>
+#undef LSM_HOOK
+
 #define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \
 	BTF_ID(func, bpf_lsm_##NAME)
 BTF_SET_START(bpf_lsm_hooks)
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 84b2d9dba79a..22485f0df534 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -346,7 +346,7 @@ int bpf_struct_ops_prepare_trampoline(struct bpf_tramp_links *tlinks,
 	 */
 	flags = model->ret_size > 0 ? BPF_TRAMP_F_RET_FENTRY_RET : 0;
 	return arch_prepare_bpf_trampoline(NULL, image, image_end,
-					   model, flags, tlinks, NULL);
+					   model, flags, tlinks, NULL, NULL);
 }
 
 static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index d6395215b849..3c6821b3c08c 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -464,7 +464,8 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
 
 	err = arch_prepare_bpf_trampoline(im, im->image, im->image + PAGE_SIZE,
 					  &tr->func.model, tr->flags, tlinks,
-					  tr->func.addr);
+					  tr->func.addr,
+					  tr->func.ret_check_addr);
 	if (err < 0)
 		goto out;
 
@@ -802,6 +803,7 @@ struct bpf_trampoline *bpf_trampoline_get(u64 key,
 
 	memcpy(&tr->func.model, &tgt_info->fmodel, sizeof(tgt_info->fmodel));
 	tr->func.addr = (void *)tgt_info->tgt_addr;
+	tr->func.ret_check_addr = (void *)tgt_info->tgt_ret_check_addr;
 out:
 	mutex_unlock(&tr->mutex);
 	return tr;
@@ -1055,7 +1057,7 @@ int __weak
 arch_prepare_bpf_trampoline(struct bpf_tramp_image *tr, void *image, void *image_end,
 			    const struct btf_func_model *m, u32 flags,
 			    struct bpf_tramp_links *tlinks,
-			    void *orig_call)
+			    void *orig_call, void *ret_check_call)
 {
 	return -ENOTSUPP;
 }
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 5e74f460dfd0..1ad0fe5cefe9 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -14988,12 +14988,13 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
 {
 	bool prog_extension = prog->type == BPF_PROG_TYPE_EXT;
 	const char prefix[] = "btf_trace_";
-	int ret = 0, subprog = -1, i;
+	int ret = 0, subprog = -1, i, tname_len;
 	const struct btf_type *t;
 	bool conservative = true;
 	const char *tname;
+	char *tname_ret;
 	struct btf *btf;
-	long addr = 0;
+	long addr = 0, ret_check_addr = 0;
 
 	if (!btf_id) {
 		bpf_log(log, "Tracing programs must provide btf_id\n");
@@ -15168,6 +15169,28 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
 					tname);
 				return -ENOENT;
 			}
+
+			if (prog->expected_attach_type == BPF_LSM_MAC) {
+				tname_len = strlen(tname);
+				tname_ret = kmalloc(tname_len + 5, GFP_KERNEL);
+				if (!tname_ret) {
+					bpf_log(log,
+						"Cannot allocate memory for %s_ret string\n",
+						tname);
+					return -ENOMEM;
+				}
+
+				snprintf(tname_ret, tname_len + 5, "%s_ret", tname);
+				ret_check_addr = kallsyms_lookup_name(tname_ret);
+				kfree(tname_ret);
+
+				if (!ret_check_addr) {
+					bpf_log(log,
+						"Kernel symbol %s_ret not found\n",
+						tname);
+					return -ENOENT;
+				}
+			}
 		}
 
 		if (prog->aux->sleepable) {
@@ -15210,6 +15233,7 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
 		break;
 	}
 	tgt_info->tgt_addr = addr;
+	tgt_info->tgt_ret_check_addr = ret_check_addr;
 	tgt_info->tgt_name = tname;
 	tgt_info->tgt_type = t;
 	return 0;
-- 
2.25.1

Re: [PoC][PATCH] bpf: Call return value check function in the JITed code

[Alexei Starovoitov]

On Wed, Nov 16, 2022 at 7:48 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
> +static bool is_ret_value_allowed(int ret, u32 ret_flags)
> +{
> +       if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) ||
> +           (ret == 0 && !(ret_flags & LSM_RET_ZERO)) ||
> +           (ret == 1 && !(ret_flags & LSM_RET_ONE)) ||
> +           (ret > 1 && !(ret_flags & LSM_RET_GT_ONE)))
> +               return false;
> +
> +       return true;
> +}
> +
>  /* For every LSM hook that allows attachment of BPF programs, declare a nop
>   * function where a BPF program can be attached.
>   */
> @@ -30,6 +41,15 @@ noinline RET bpf_lsm_##NAME(__VA_ARGS__)     \
>  #include <linux/lsm_hook_defs.h>
>  #undef LSM_HOOK
>
> +#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...)   \
> +noinline RET bpf_lsm_##NAME##_ret(int ret)     \
> +{                                              \
> +       return is_ret_value_allowed(ret, RET_FLAGS) ? ret : DEFAULT; \
> +}
> +
> +#include <linux/lsm_hook_defs.h>
> +#undef LSM_HOOK
> +

because lsm hooks is mess of undocumented return values your
"solution" is to add hundreds of noninline functions
and hack the call into them in JITs ?!
That's an obvious no-go. Not sure why you bothered to implement it.

Re: [PoC][PATCH] bpf: Call return value check function in the JITed code

[Roberto Sassu]

On Wed, 2022-11-16 at 08:16 -0800, Alexei Starovoitov wrote:
> On Wed, Nov 16, 2022 at 7:48 AM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> > +static bool is_ret_value_allowed(int ret, u32 ret_flags)
> > +{
> > +       if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) ||
> > +           (ret == 0 && !(ret_flags & LSM_RET_ZERO)) ||
> > +           (ret == 1 && !(ret_flags & LSM_RET_ONE)) ||
> > +           (ret > 1 && !(ret_flags & LSM_RET_GT_ONE)))
> > +               return false;
> > +
> > +       return true;
> > +}
> > +
> >  /* For every LSM hook that allows attachment of BPF programs, declare a nop
> >   * function where a BPF program can be attached.
> >   */
> > @@ -30,6 +41,15 @@ noinline RET bpf_lsm_##NAME(__VA_ARGS__)     \
> >  #include <linux/lsm_hook_defs.h>
> >  #undef LSM_HOOK
> > 
> > +#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...)   \
> > +noinline RET bpf_lsm_##NAME##_ret(int ret)     \
> > +{                                              \
> > +       return is_ret_value_allowed(ret, RET_FLAGS) ? ret : DEFAULT; \
> > +}
> > +
> > +#include <linux/lsm_hook_defs.h>
> > +#undef LSM_HOOK
> > +
> 
> because lsm hooks is mess of undocumented return values your
> "solution" is to add hundreds of noninline functions
> and hack the call into them in JITs ?!

I revisited the documentation and checked each LSM hook one by one.
Hopefully, I completed it correctly, but I would review again (others
are also welcome to do it).

Not sure if there is a more efficient way. Do you have any idea?
Maybe we find a way to use only one check function (by reusing the
address of the attachment point?).

Regarding the JIT approach, I didn't find a reliable solution for using
just the verifier. As I wrote to you, there could be the case where the
range can include positive values, despite the possible return values
are zero and -EACCES.

# ./test_progs-no_alu32 -t libbpf_get_fd

*reg = {type = SCALAR_VALUE, off = 0, {range = 0, {map_ptr = 0x0
<fixed_percpu_data>, map_uid = 0}, {btf = 0x0 <fixed_percpu_data>,
btf_id = 0}, mem_size = 0, dynptr = {type = BPF_DYNPTR_TYPE_INVALID,
first_slot = false}, raw = {raw1 = 0, raw2 = 0}, subprogno = 0}, id =
0, 
  ref_obj_id = 0, var_off = {value = 0, mask = 18446744073709551603},
smin_value = -9223372036854775808, smax_value = 9223372036854775795,
umin_value = 0, umax_value = 18446744073709551603, s32_min_value =
-2147483648, s32_max_value = 2147483635, u32_min_value = 0, 
  u32_max_value = 4294967283, parent = 0x0 <fixed_percpu_data>, frameno
= 0, subreg_def = 0, live = REG_LIVE_WRITTEN, precise = false}

The JIT approach instead is 100% reliable, as you check the real value
to be returned to BPF LSM.

But of course, the performance will be worse this way. If you are able
to determine at verification time that an eBPF program is not going to
return illegal values, that would be better. I'm not sure it is
feasible.

Thanks

Roberto

Re: [PoC][PATCH] bpf: Call return value check function in the JITed code

[Alexei Starovoitov]

On Wed, Nov 16, 2022 at 8:41 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> On Wed, 2022-11-16 at 08:16 -0800, Alexei Starovoitov wrote:
> > On Wed, Nov 16, 2022 at 7:48 AM Roberto Sassu
> > <roberto.sassu@huaweicloud.com> wrote:
> > > +static bool is_ret_value_allowed(int ret, u32 ret_flags)
> > > +{
> > > +       if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) ||
> > > +           (ret == 0 && !(ret_flags & LSM_RET_ZERO)) ||
> > > +           (ret == 1 && !(ret_flags & LSM_RET_ONE)) ||
> > > +           (ret > 1 && !(ret_flags & LSM_RET_GT_ONE)))
> > > +               return false;
> > > +
> > > +       return true;
> > > +}
> > > +
> > >  /* For every LSM hook that allows attachment of BPF programs, declare a nop
> > >   * function where a BPF program can be attached.
> > >   */
> > > @@ -30,6 +41,15 @@ noinline RET bpf_lsm_##NAME(__VA_ARGS__)     \
> > >  #include <linux/lsm_hook_defs.h>
> > >  #undef LSM_HOOK
> > >
> > > +#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...)   \
> > > +noinline RET bpf_lsm_##NAME##_ret(int ret)     \
> > > +{                                              \
> > > +       return is_ret_value_allowed(ret, RET_FLAGS) ? ret : DEFAULT; \
> > > +}
> > > +
> > > +#include <linux/lsm_hook_defs.h>
> > > +#undef LSM_HOOK
> > > +
> >
> > because lsm hooks is mess of undocumented return values your
> > "solution" is to add hundreds of noninline functions
> > and hack the call into them in JITs ?!
>
> I revisited the documentation and checked each LSM hook one by one.
> Hopefully, I completed it correctly, but I would review again (others
> are also welcome to do it).
>
> Not sure if there is a more efficient way. Do you have any idea?
> Maybe we find a way to use only one check function (by reusing the
> address of the attachment point?).
>
> Regarding the JIT approach, I didn't find a reliable solution for using
> just the verifier. As I wrote to you, there could be the case where the
> range can include positive values, despite the possible return values
> are zero and -EACCES.

Didn't you find that there are only 12 or so odd return cases.
Maybe refactor some of them to something that the verifier can enforce
and denylist the rest ?

Also denylist those that Casey mentioned like security_secid_to_secctx ?

Re: [PoC][PATCH] bpf: Call return value check function in the JITed code

[Casey Schaufler]

On 11/16/2022 9:55 AM, Alexei Starovoitov wrote:
> On Wed, Nov 16, 2022 at 8:41 AM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
>> On Wed, 2022-11-16 at 08:16 -0800, Alexei Starovoitov wrote:
>>> On Wed, Nov 16, 2022 at 7:48 AM Roberto Sassu
>>> <roberto.sassu@huaweicloud.com> wrote:
>>>> +static bool is_ret_value_allowed(int ret, u32 ret_flags)
>>>> +{
>>>> +       if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) ||
>>>> +           (ret == 0 && !(ret_flags & LSM_RET_ZERO)) ||
>>>> +           (ret == 1 && !(ret_flags & LSM_RET_ONE)) ||
>>>> +           (ret > 1 && !(ret_flags & LSM_RET_GT_ONE)))
>>>> +               return false;
>>>> +
>>>> +       return true;
>>>> +}
>>>> +
>>>>  /* For every LSM hook that allows attachment of BPF programs, declare a nop
>>>>   * function where a BPF program can be attached.
>>>>   */
>>>> @@ -30,6 +41,15 @@ noinline RET bpf_lsm_##NAME(__VA_ARGS__)     \
>>>>  #include <linux/lsm_hook_defs.h>
>>>>  #undef LSM_HOOK
>>>>
>>>> +#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...)   \
>>>> +noinline RET bpf_lsm_##NAME##_ret(int ret)     \
>>>> +{                                              \
>>>> +       return is_ret_value_allowed(ret, RET_FLAGS) ? ret : DEFAULT; \
>>>> +}
>>>> +
>>>> +#include <linux/lsm_hook_defs.h>
>>>> +#undef LSM_HOOK
>>>> +
>>> because lsm hooks is mess of undocumented return values your
>>> "solution" is to add hundreds of noninline functions
>>> and hack the call into them in JITs ?!
>> I revisited the documentation and checked each LSM hook one by one.
>> Hopefully, I completed it correctly, but I would review again (others
>> are also welcome to do it).
>>
>> Not sure if there is a more efficient way. Do you have any idea?
>> Maybe we find a way to use only one check function (by reusing the
>> address of the attachment point?).
>>
>> Regarding the JIT approach, I didn't find a reliable solution for using
>> just the verifier. As I wrote to you, there could be the case where the
>> range can include positive values, despite the possible return values
>> are zero and -EACCES.
> Didn't you find that there are only 12 or so odd return cases.
> Maybe refactor some of them to something that the verifier can enforce
> and denylist the rest ?

Changing security_mumble() often requires changes in either VFS, audit or
networking code. Even simple changes can require extensive review and
difficult to obtain Acked-by's. It may be the correct approach, but it
won't be easy or quick.

> Also denylist those that Casey mentioned like security_secid_to_secctx ?

Identifying all the hooks that could be "dangerous" isn't an easy chore,
and some of the "dangerous" hooks are key to implementing classes of policy
and absolutely necessary for audit support.

Re: [PoC][PATCH] bpf: Call return value check function in the JITed code

[KP Singh]

On Wed, Nov 16, 2022 at 6:55 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Nov 16, 2022 at 8:41 AM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> >
> > On Wed, 2022-11-16 at 08:16 -0800, Alexei Starovoitov wrote:
> > > On Wed, Nov 16, 2022 at 7:48 AM Roberto Sassu
> > > <roberto.sassu@huaweicloud.com> wrote:
> > > > +static bool is_ret_value_allowed(int ret, u32 ret_flags)
> > > > +{
> > > > +       if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) ||
> > > > +           (ret == 0 && !(ret_flags & LSM_RET_ZERO)) ||
> > > > +           (ret == 1 && !(ret_flags & LSM_RET_ONE)) ||
> > > > +           (ret > 1 && !(ret_flags & LSM_RET_GT_ONE)))
> > > > +               return false;
> > > > +
> > > > +       return true;
> > > > +}
> > > > +
> > > >  /* For every LSM hook that allows attachment of BPF programs, declare a nop
> > > >   * function where a BPF program can be attached.
> > > >   */
> > > > @@ -30,6 +41,15 @@ noinline RET bpf_lsm_##NAME(__VA_ARGS__)     \
> > > >  #include <linux/lsm_hook_defs.h>
> > > >  #undef LSM_HOOK
> > > >
> > > > +#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...)   \
> > > > +noinline RET bpf_lsm_##NAME##_ret(int ret)     \
> > > > +{                                              \
> > > > +       return is_ret_value_allowed(ret, RET_FLAGS) ? ret : DEFAULT; \
> > > > +}
> > > > +
> > > > +#include <linux/lsm_hook_defs.h>
> > > > +#undef LSM_HOOK
> > > > +
> > >
> > > because lsm hooks is mess of undocumented return values your
> > > "solution" is to add hundreds of noninline functions
> > > and hack the call into them in JITs ?!
> >
> > I revisited the documentation and checked each LSM hook one by one.
> > Hopefully, I completed it correctly, but I would review again (others
> > are also welcome to do it).
> >
> > Not sure if there is a more efficient way. Do you have any idea?
> > Maybe we find a way to use only one check function (by reusing the
> > address of the attachment point?).
> >
> > Regarding the JIT approach, I didn't find a reliable solution for using
> > just the verifier. As I wrote to you, there could be the case where the
> > range can include positive values, despite the possible return values
> > are zero and -EACCES.
>
> Didn't you find that there are only 12 or so odd return cases.
> Maybe refactor some of them to something that the verifier can enforce
> and denylist the rest ?

+1

>
> Also denylist those that Casey mentioned like security_secid_to_secctx ?

Just replied to Casey's comment and I agree, these hooks should be denylisted.

Re: [PoC][PATCH] bpf: Call return value check function in the JITed code

[Paul Moore]

On Wed, Nov 16, 2022 at 2:04 PM KP Singh <kpsingh@kernel.org> wrote:
> On Wed, Nov 16, 2022 at 6:55 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Wed, Nov 16, 2022 at 8:41 AM Roberto Sassu
> > <roberto.sassu@huaweicloud.com> wrote:
> > >
> > > On Wed, 2022-11-16 at 08:16 -0800, Alexei Starovoitov wrote:
> > > > On Wed, Nov 16, 2022 at 7:48 AM Roberto Sassu
> > > > <roberto.sassu@huaweicloud.com> wrote:
> > > > > +static bool is_ret_value_allowed(int ret, u32 ret_flags)
> > > > > +{
> > > > > +       if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) ||
> > > > > +           (ret == 0 && !(ret_flags & LSM_RET_ZERO)) ||
> > > > > +           (ret == 1 && !(ret_flags & LSM_RET_ONE)) ||
> > > > > +           (ret > 1 && !(ret_flags & LSM_RET_GT_ONE)))
> > > > > +               return false;
> > > > > +
> > > > > +       return true;
> > > > > +}
> > > > > +
> > > > >  /* For every LSM hook that allows attachment of BPF programs, declare a nop
> > > > >   * function where a BPF program can be attached.
> > > > >   */
> > > > > @@ -30,6 +41,15 @@ noinline RET bpf_lsm_##NAME(__VA_ARGS__)     \
> > > > >  #include <linux/lsm_hook_defs.h>
> > > > >  #undef LSM_HOOK
> > > > >
> > > > > +#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...)   \
> > > > > +noinline RET bpf_lsm_##NAME##_ret(int ret)     \
> > > > > +{                                              \
> > > > > +       return is_ret_value_allowed(ret, RET_FLAGS) ? ret : DEFAULT; \
> > > > > +}
> > > > > +
> > > > > +#include <linux/lsm_hook_defs.h>
> > > > > +#undef LSM_HOOK
> > > > > +
> > > >
> > > > because lsm hooks is mess of undocumented return values your
> > > > "solution" is to add hundreds of noninline functions
> > > > and hack the call into them in JITs ?!
> > >
> > > I revisited the documentation and checked each LSM hook one by one.
> > > Hopefully, I completed it correctly, but I would review again (others
> > > are also welcome to do it).
> > >
> > > Not sure if there is a more efficient way. Do you have any idea?
> > > Maybe we find a way to use only one check function (by reusing the
> > > address of the attachment point?).
> > >
> > > Regarding the JIT approach, I didn't find a reliable solution for using
> > > just the verifier. As I wrote to you, there could be the case where the
> > > range can include positive values, despite the possible return values
> > > are zero and -EACCES.
> >
> > Didn't you find that there are only 12 or so odd return cases.
> > Maybe refactor some of them to something that the verifier can enforce
> > and denylist the rest ?
>
> +1

I'm not sure we want to refactor the LSM hooks right now, we've got
too much stuff in-progress which I consider higher value/priority.
While I'm generally in favor of improving the sanity of interfaces,
I'd much rather we resolve the IMA/EVM special cases and land the
stacking changes before we start playing with refactoring the hooks.
I know this is a bummer for the BPF folks, but the IMA/EVM and
stacking patches affect everybody.

-- 
paul-moore.com

Re: [PoC][PATCH] bpf: Call return value check function in the JITed code

[Roberto Sassu]

On Wed, 2022-11-16 at 09:55 -0800, Alexei Starovoitov wrote:
> On Wed, Nov 16, 2022 at 8:41 AM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> > On Wed, 2022-11-16 at 08:16 -0800, Alexei Starovoitov wrote:
> > > On Wed, Nov 16, 2022 at 7:48 AM Roberto Sassu
> > > <roberto.sassu@huaweicloud.com> wrote:
> > > > +static bool is_ret_value_allowed(int ret, u32 ret_flags)
> > > > +{
> > > > +       if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) ||
> > > > +           (ret == 0 && !(ret_flags & LSM_RET_ZERO)) ||
> > > > +           (ret == 1 && !(ret_flags & LSM_RET_ONE)) ||
> > > > +           (ret > 1 && !(ret_flags & LSM_RET_GT_ONE)))
> > > > +               return false;
> > > > +
> > > > +       return true;
> > > > +}
> > > > +
> > > >  /* For every LSM hook that allows attachment of BPF programs, declare a nop
> > > >   * function where a BPF program can be attached.
> > > >   */
> > > > @@ -30,6 +41,15 @@ noinline RET bpf_lsm_##NAME(__VA_ARGS__)     \
> > > >  #include <linux/lsm_hook_defs.h>
> > > >  #undef LSM_HOOK
> > > > 
> > > > +#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...)   \
> > > > +noinline RET bpf_lsm_##NAME##_ret(int ret)     \
> > > > +{                                              \
> > > > +       return is_ret_value_allowed(ret, RET_FLAGS) ? ret : DEFAULT; \
> > > > +}
> > > > +
> > > > +#include <linux/lsm_hook_defs.h>
> > > > +#undef LSM_HOOK
> > > > +
> > > 
> > > because lsm hooks is mess of undocumented return values your
> > > "solution" is to add hundreds of noninline functions
> > > and hack the call into them in JITs ?!
> > 
> > I revisited the documentation and checked each LSM hook one by one.
> > Hopefully, I completed it correctly, but I would review again (others
> > are also welcome to do it).
> > 
> > Not sure if there is a more efficient way. Do you have any idea?
> > Maybe we find a way to use only one check function (by reusing the
> > address of the attachment point?).
> > 
> > Regarding the JIT approach, I didn't find a reliable solution for using
> > just the verifier. As I wrote to you, there could be the case where the
> > range can include positive values, despite the possible return values
> > are zero and -EACCES.
> 
> Didn't you find that there are only 12 or so odd return cases.
> Maybe refactor some of them to something that the verifier can enforce
> and denylist the rest ?

Ok, went back to trying to enforce the return value on the verifier
side, assuming that for now we consider hooks that return zero or a
negative value.

I wanted to see if at least we are able to enforce that.

The biggest problem is which value of the register I should use, the 64
bit one or the 32 bit one?

We can have a look at test_libbpf_get_fd_by_id_opts. The default flavor
gives:

0000000000000000 <check_access>:
       0:	b4 00 00 00 00 00 00 00	w0 = 0
       1:	79 12 00 00 00 00 00 00	r2 = *(u64 *)(r1 + 0)
       2:	18 03 00 00 00 00 00 00 00 00 00 00 00 00 00 00	r3 = 0 ll
       4:	5d 32 05 00 00 00 00 00	if r2 != r3 goto +5 <LBB0_3>
       5:	79 11 08 00 00 00 00 00	r1 = *(u64 *)(r1 + 8)
       6:	57 01 00 00 02 00 00 00	r1 &= 2
       7:	b4 00 00 00 00 00 00 00	w0 = 0
       8:	15 01 01 00 00 00 00 00	if r1 == 0 goto +1 <LBB0_3>
       9:	b4 00 00 00 f3 ff ff ff	w0 = -13

smin_value = 0xfffffff3, smax_value = 0xfffffff3,
s32_min_value = 0xfffffff3, s32_max_value = 0xfffffff3,

I think it is because of this, in check_alu_op():

if (BPF_CLASS(insn->code) == BPF_ALU64) {
	__mark_reg_known(regs + insn->dst_reg,
			 insn->imm);
} else {
	__mark_reg_known(regs + insn->dst_reg,
			 (u32)insn->imm);
	}
}

So, here you have to use the 32 bit values. But, if you use the
no_alu32 flavor:

0000000000000000 <check_access>:
       0:	b7 00 00 00 00 00 00 00	r0 = 0
       1:	79 12 00 00 00 00 00 00	r2 = *(u64 *)(r1 + 0)
       2:	18 03 00 00 00 00 00 00 00 00 00 00 00 00 00 00	r3 = 0 ll
       4:	5d 32 04 00 00 00 00 00	if r2 != r3 goto +4 <LBB0_2>
       5:	79 10 08 00 00 00 00 00	r0 = *(u64 *)(r1 + 8)
       6:	67 00 00 00 3e 00 00 00	r0 <<= 62
       7:	c7 00 00 00 3f 00 00 00	r0 s>>= 63

smin_value = 0xffffffffffffffff, smax_value = 0x0,
s32_min_value = 0x80000000, s32_max_value = 0x7fffffff,

       8:	57 00 00 00 f3 ff ff ff	r0 &= -13

smin_value = 0xfffffffffffffff3, smax_value = 0x7fffffffffffffff,
s32_min_value = 0x80000000, s32_max_value = 0x7ffffff3,

I would have hoped to see:

smin_value = 0xfffffffffffffff3, smax_value = 0x0,

but it doesn't because of this, in scalar_min_max_and():

if (dst_reg->smin_value < 0 || smin_val < 0) {
	/* Lose signed bounds when ANDing negative numbers,
	 * ain't nobody got time for that.
	 */
	dst_reg->smin_value = S64_MIN;
	dst_reg->smax_value = S64_MAX;

Could we do an AND, if src_reg is known?

And what would be the right register value to use?

Thanks

Roberto

Re: [PoC][PATCH] bpf: Call return value check function in the JITed code

[Casey Schaufler]

On 11/16/2022 7:47 AM, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
>
> eBPF allows certain types of eBPF programs to modify the return value of
> the functions they attach to. This is used for example by BPF LSM to let
> security modules make their decision on LSM hooks.
>
> The JITed code looks like the following:
>
>     ret = bpf_lsm_inode_permission_impl1(); // from a security module
>     if (ret)
>         goto out;
>
> ..
>
>     ret = bpf_lsm_inode_permission_implN(); // from a security module
>     if (ret)
>         goto out;
>
>     ret = bpf_lsm_inode_permission(); // in the kernel, returns DEFAULT
> out:
>
> If ret is not zero, the attachment points of BPF LSM are not executed. For
> this reason, the return value check cannot be done there.
>
> Instead, the idea is to use the LSM_HOOK() macro to define a per-hook check
> function.
>
> Whenever an eBPF program attaches to an LSM hook, the eBPF verifier
> resolves the address of the check function (whose name is
> bpf_lsm_<hook name>_ret()) and adds a call to that function just after the
> out label. If the return value is illegal, the check function changes it
> back to the default value defined by the LSM infrastructure:
>
> ..
>
> out:
>     ret = bpf_lsm_inode_permission_ret(ret);

As I've mentioned elsewhere, the return value is a small part of
the problem you have with eBPF programs and the BPF LSM. Because
the LSM infrastructure is inconsistent with regard to return codes,
values returned in pointers and use of secids there is no uniform
mechanism that I can see to address the "legitimate return" problem.

Lets look at one of the ickyest interfaces we have, security_getprocattr().
It returns the size of a string that it has allocated. It puts the
pointer to the allocated buffer into a char **value that was passed to it.
If bpf_getprocattr() returns a positive number and sets value to NULL Bad
Things can happen. If the return value is greater than the size allocated
ditto. If it returns an error but allocates a string you get a memory leak.

security_secid_to_secctx() has to work in concert with security_release_secctx()
to do memory lifecycle management. If secid_to_secctx() allocates memory
release_secctx() has to free it, while if secid_to_secctx() doesn't allocate
memory it better not. (SELinux allocates memory, Smack does not. It's a real
distinction) Your return checker would need to understand a lot more about
the behavior of your eBPF programs than what value they return.

>
> In this way, an eBPF program cannot cause illegal return values to be sent
> to BPF LSM, and to the callers of the LSM infrastructure.
>
> This is just a PoC, to validate the idea and get an early feedback.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  arch/arm64/net/bpf_jit_comp.c |  7 ++++---
>  arch/x86/net/bpf_jit_comp.c   | 17 ++++++++++++++++-
>  include/linux/bpf.h           |  4 +++-
>  kernel/bpf/bpf_lsm.c          | 20 ++++++++++++++++++++
>  kernel/bpf/bpf_struct_ops.c   |  2 +-
>  kernel/bpf/trampoline.c       |  6 ++++--
>  kernel/bpf/verifier.c         | 28 ++++++++++++++++++++++++++--
>  7 files changed, 74 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index 62f805f427b7..5412230c6935 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c
> @@ -1764,7 +1764,7 @@ static void restore_args(struct jit_ctx *ctx, int args_off, int nargs)
>   */
>  static int prepare_trampoline(struct jit_ctx *ctx, struct bpf_tramp_image *im,
>  			      struct bpf_tramp_links *tlinks, void *orig_call,
> -			      int nargs, u32 flags)
> +			      void *ret_check_call, int nargs, u32 flags)
>  {
>  	int i;
>  	int stack_size;
> @@ -1963,7 +1963,7 @@ static int prepare_trampoline(struct jit_ctx *ctx, struct bpf_tramp_image *im,
>  int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image,
>  				void *image_end, const struct btf_func_model *m,
>  				u32 flags, struct bpf_tramp_links *tlinks,
> -				void *orig_call)
> +				void *orig_call, void *ret_check_call)
>  {
>  	int i, ret;
>  	int nargs = m->nr_args;
> @@ -1983,7 +1983,8 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image,
>  			return -ENOTSUPP;
>  	}
>  
> -	ret = prepare_trampoline(&ctx, im, tlinks, orig_call, nargs, flags);
> +	ret = prepare_trampoline(&ctx, im, tlinks, orig_call, ret_check_call,
> +				 nargs, flags);
>  	if (ret < 0)
>  		return ret;
>  
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index cec5195602bc..6cd727b4af0a 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -2123,7 +2123,7 @@ static int invoke_bpf_mod_ret(const struct btf_func_model *m, u8 **pprog,
>  int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *image_end,
>  				const struct btf_func_model *m, u32 flags,
>  				struct bpf_tramp_links *tlinks,
> -				void *func_addr)
> +				void *func_addr, void *func_ret_check_addr)
>  {
>  	int ret, i, nr_args = m->nr_args, extra_nregs = 0;
>  	int regs_off, ip_off, args_off, stack_size = nr_args * 8, run_ctx_off;
> @@ -2280,6 +2280,21 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
>  		for (i = 0; i < fmod_ret->nr_links; i++)
>  			emit_cond_near_jump(&branches[i], prog, branches[i],
>  					    X86_JNE);
> +
> +		if (func_ret_check_addr) {
> +			emit_ldx(&prog, BPF_DW, BPF_REG_1, BPF_REG_FP, -8);
> +
> +			/* call ret check function */
> +			if (emit_call(&prog, func_ret_check_addr, prog)) {
> +				ret = -EINVAL;
> +				goto cleanup;
> +			}
> +
> +			/* remember return value in a stack for bpf prog to access */
> +			emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -8);
> +			memcpy(prog, x86_nops[5], X86_PATCH_SIZE);
> +			prog += X86_PATCH_SIZE;
> +		}
>  	}
>  
>  	if (fexit->nr_links)
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 49f9d2bec401..f3551f7bdc28 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -919,7 +919,7 @@ struct bpf_tramp_image;
>  int arch_prepare_bpf_trampoline(struct bpf_tramp_image *tr, void *image, void *image_end,
>  				const struct btf_func_model *m, u32 flags,
>  				struct bpf_tramp_links *tlinks,
> -				void *orig_call);
> +				void *orig_call, void *ret_call);
>  u64 notrace __bpf_prog_enter_sleepable_recur(struct bpf_prog *prog,
>  					     struct bpf_tramp_run_ctx *run_ctx);
>  void notrace __bpf_prog_exit_sleepable_recur(struct bpf_prog *prog, u64 start,
> @@ -974,6 +974,7 @@ struct bpf_trampoline {
>  	struct {
>  		struct btf_func_model model;
>  		void *addr;
> +		void *ret_check_addr;
>  		bool ftrace_managed;
>  	} func;
>  	/* if !NULL this is BPF_PROG_TYPE_EXT program that extends another BPF
> @@ -994,6 +995,7 @@ struct bpf_trampoline {
>  struct bpf_attach_target_info {
>  	struct btf_func_model fmodel;
>  	long tgt_addr;
> +	long tgt_ret_check_addr;
>  	const char *tgt_name;
>  	const struct btf_type *tgt_type;
>  };
> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> index 37bcedf5a44e..f7f25d0064dd 100644
> --- a/kernel/bpf/bpf_lsm.c
> +++ b/kernel/bpf/bpf_lsm.c
> @@ -18,6 +18,17 @@
>  #include <linux/ima.h>
>  #include <linux/bpf-cgroup.h>
>  
> +static bool is_ret_value_allowed(int ret, u32 ret_flags)
> +{
> +	if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) ||
> +	    (ret == 0 && !(ret_flags & LSM_RET_ZERO)) ||
> +	    (ret == 1 && !(ret_flags & LSM_RET_ONE)) ||
> +	    (ret > 1 && !(ret_flags & LSM_RET_GT_ONE)))
> +		return false;
> +
> +	return true;
> +}
> +
>  /* For every LSM hook that allows attachment of BPF programs, declare a nop
>   * function where a BPF program can be attached.
>   */
> @@ -30,6 +41,15 @@ noinline RET bpf_lsm_##NAME(__VA_ARGS__)	\
>  #include <linux/lsm_hook_defs.h>
>  #undef LSM_HOOK
>  
> +#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...)	\
> +noinline RET bpf_lsm_##NAME##_ret(int ret)	\
> +{						\
> +	return is_ret_value_allowed(ret, RET_FLAGS) ? ret : DEFAULT; \
> +}
> +
> +#include <linux/lsm_hook_defs.h>
> +#undef LSM_HOOK
> +
>  #define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \
>  	BTF_ID(func, bpf_lsm_##NAME)
>  BTF_SET_START(bpf_lsm_hooks)
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index 84b2d9dba79a..22485f0df534 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -346,7 +346,7 @@ int bpf_struct_ops_prepare_trampoline(struct bpf_tramp_links *tlinks,
>  	 */
>  	flags = model->ret_size > 0 ? BPF_TRAMP_F_RET_FENTRY_RET : 0;
>  	return arch_prepare_bpf_trampoline(NULL, image, image_end,
> -					   model, flags, tlinks, NULL);
> +					   model, flags, tlinks, NULL, NULL);
>  }
>  
>  static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index d6395215b849..3c6821b3c08c 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -464,7 +464,8 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
>  
>  	err = arch_prepare_bpf_trampoline(im, im->image, im->image + PAGE_SIZE,
>  					  &tr->func.model, tr->flags, tlinks,
> -					  tr->func.addr);
> +					  tr->func.addr,
> +					  tr->func.ret_check_addr);
>  	if (err < 0)
>  		goto out;
>  
> @@ -802,6 +803,7 @@ struct bpf_trampoline *bpf_trampoline_get(u64 key,
>  
>  	memcpy(&tr->func.model, &tgt_info->fmodel, sizeof(tgt_info->fmodel));
>  	tr->func.addr = (void *)tgt_info->tgt_addr;
> +	tr->func.ret_check_addr = (void *)tgt_info->tgt_ret_check_addr;
>  out:
>  	mutex_unlock(&tr->mutex);
>  	return tr;
> @@ -1055,7 +1057,7 @@ int __weak
>  arch_prepare_bpf_trampoline(struct bpf_tramp_image *tr, void *image, void *image_end,
>  			    const struct btf_func_model *m, u32 flags,
>  			    struct bpf_tramp_links *tlinks,
> -			    void *orig_call)
> +			    void *orig_call, void *ret_check_call)
>  {
>  	return -ENOTSUPP;
>  }
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 5e74f460dfd0..1ad0fe5cefe9 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -14988,12 +14988,13 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
>  {
>  	bool prog_extension = prog->type == BPF_PROG_TYPE_EXT;
>  	const char prefix[] = "btf_trace_";
> -	int ret = 0, subprog = -1, i;
> +	int ret = 0, subprog = -1, i, tname_len;
>  	const struct btf_type *t;
>  	bool conservative = true;
>  	const char *tname;
> +	char *tname_ret;
>  	struct btf *btf;
> -	long addr = 0;
> +	long addr = 0, ret_check_addr = 0;
>  
>  	if (!btf_id) {
>  		bpf_log(log, "Tracing programs must provide btf_id\n");
> @@ -15168,6 +15169,28 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
>  					tname);
>  				return -ENOENT;
>  			}
> +
> +			if (prog->expected_attach_type == BPF_LSM_MAC) {
> +				tname_len = strlen(tname);
> +				tname_ret = kmalloc(tname_len + 5, GFP_KERNEL);
> +				if (!tname_ret) {
> +					bpf_log(log,
> +						"Cannot allocate memory for %s_ret string\n",
> +						tname);
> +					return -ENOMEM;
> +				}
> +
> +				snprintf(tname_ret, tname_len + 5, "%s_ret", tname);
> +				ret_check_addr = kallsyms_lookup_name(tname_ret);
> +				kfree(tname_ret);
> +
> +				if (!ret_check_addr) {
> +					bpf_log(log,
> +						"Kernel symbol %s_ret not found\n",
> +						tname);
> +					return -ENOENT;
> +				}
> +			}
>  		}
>  
>  		if (prog->aux->sleepable) {
> @@ -15210,6 +15233,7 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
>  		break;
>  	}
>  	tgt_info->tgt_addr = addr;
> +	tgt_info->tgt_ret_check_addr = ret_check_addr;
>  	tgt_info->tgt_name = tname;
>  	tgt_info->tgt_type = t;
>  	return 0;

Re: [PoC][PATCH] bpf: Call return value check function in the JITed code

[KP Singh]

On Wed, Nov 16, 2022 at 6:12 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 11/16/2022 7:47 AM, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> >
> > eBPF allows certain types of eBPF programs to modify the return value of
> > the functions they attach to. This is used for example by BPF LSM to let
> > security modules make their decision on LSM hooks.
> >
> > The JITed code looks like the following:
> >
> >     ret = bpf_lsm_inode_permission_impl1(); // from a security module
> >     if (ret)
> >         goto out;
> >
> > ..
> >
> >     ret = bpf_lsm_inode_permission_implN(); // from a security module
> >     if (ret)
> >         goto out;
> >
> >     ret = bpf_lsm_inode_permission(); // in the kernel, returns DEFAULT
> > out:
> >
> > If ret is not zero, the attachment points of BPF LSM are not executed. For
> > this reason, the return value check cannot be done there.
> >
> > Instead, the idea is to use the LSM_HOOK() macro to define a per-hook check
> > function.
> >
> > Whenever an eBPF program attaches to an LSM hook, the eBPF verifier
> > resolves the address of the check function (whose name is
> > bpf_lsm_<hook name>_ret()) and adds a call to that function just after the
> > out label. If the return value is illegal, the check function changes it
> > back to the default value defined by the LSM infrastructure:
> >
> > ..
> >
> > out:
> >     ret = bpf_lsm_inode_permission_ret(ret);
>
> As I've mentioned elsewhere, the return value is a small part of
> the problem you have with eBPF programs and the BPF LSM. Because
> the LSM infrastructure is inconsistent with regard to return codes,
> values returned in pointers and use of secids there is no uniform
> mechanism that I can see to address the "legitimate return" problem.
>
> Lets look at one of the ickyest interfaces we have, security_getprocattr().
> It returns the size of a string that it has allocated. It puts the
> pointer to the allocated buffer into a char **value that was passed to it.
> If bpf_getprocattr() returns a positive number and sets value to NULL Bad
> Things can happen. If the return value is greater than the size allocated
> ditto. If it returns an error but allocates a string you get a memory leak.

I think we should not need this hook in BPF. We can create a list of hooks that
we should not really expose via BPF.

>
> security_secid_to_secctx() has to work in concert with security_release_secctx()
> to do memory lifecycle management. If secid_to_secctx() allocates memory
> release_secctx() has to free it, while if secid_to_secctx() doesn't allocate
> memory it better not. (SELinux allocates memory, Smack does not. It's a real
> distinction) Your return checker would need to understand a lot more about
> the behavior of your eBPF programs than what value they return.

While this is possible to do using the BPF verifier (i.e. more detailed checks).

I don't see the value of BPF programs attaching to these hooks and we should
just not register the BPF LSM callbacks for these.

>
> >
> > In this way, an eBPF program cannot cause illegal return values to be sent
> > to BPF LSM, and to the callers of the LSM infrastructure.
> >
> > This is just a PoC, to validate the idea and get an early feedback.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >  arch/arm64/net/bpf_jit_comp.c |  7 ++++---
> >  arch/x86/net/bpf_jit_comp.c   | 17 ++++++++++++++++-
> >  include/linux/bpf.h           |  4 +++-
> >  kernel/bpf/bpf_lsm.c          | 20 ++++++++++++++++++++
> >  kernel/bpf/bpf_struct_ops.c   |  2 +-
> >  kernel/bpf/trampoline.c       |  6 ++++--
> >  kernel/bpf/verifier.c         | 28 ++++++++++++++++++++++++++--
> >  7 files changed, 74 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> > index 62f805f427b7..5412230c6935 100644
> > --- a/arch/arm64/net/bpf_jit_comp.c
> > +++ b/arch/arm64/net/bpf_jit_comp.c
> > @@ -1764,7 +1764,7 @@ static void restore_args(struct jit_ctx *ctx, int args_off, int nargs)
> >   */
> >  static int prepare_trampoline(struct jit_ctx *ctx, struct bpf_tramp_image *im,
> >                             struct bpf_tramp_links *tlinks, void *orig_call,
> > -                           int nargs, u32 flags)
> > +                           void *ret_check_call, int nargs, u32 flags)
> >  {
> >       int i;
> >       int stack_size;
> > @@ -1963,7 +1963,7 @@ static int prepare_trampoline(struct jit_ctx *ctx, struct bpf_tramp_image *im,
> >  int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image,
> >                               void *image_end, const struct btf_func_model *m,
> >                               u32 flags, struct bpf_tramp_links *tlinks,
> > -                             void *orig_call)
> > +                             void *orig_call, void *ret_check_call)
> >  {
> >       int i, ret;
> >       int nargs = m->nr_args;
> > @@ -1983,7 +1983,8 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image,
> >                       return -ENOTSUPP;
> >       }
> >
> > -     ret = prepare_trampoline(&ctx, im, tlinks, orig_call, nargs, flags);
> > +     ret = prepare_trampoline(&ctx, im, tlinks, orig_call, ret_check_call,
> > +                              nargs, flags);
> >       if (ret < 0)
> >               return ret;
> >
> > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > index cec5195602bc..6cd727b4af0a 100644
> > --- a/arch/x86/net/bpf_jit_comp.c
> > +++ b/arch/x86/net/bpf_jit_comp.c
> > @@ -2123,7 +2123,7 @@ static int invoke_bpf_mod_ret(const struct btf_func_model *m, u8 **pprog,
> >  int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *image_end,
> >                               const struct btf_func_model *m, u32 flags,
> >                               struct bpf_tramp_links *tlinks,
> > -                             void *func_addr)
> > +                             void *func_addr, void *func_ret_check_addr)
> >  {
> >       int ret, i, nr_args = m->nr_args, extra_nregs = 0;
> >       int regs_off, ip_off, args_off, stack_size = nr_args * 8, run_ctx_off;
> > @@ -2280,6 +2280,21 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
> >               for (i = 0; i < fmod_ret->nr_links; i++)
> >                       emit_cond_near_jump(&branches[i], prog, branches[i],
> >                                           X86_JNE);
> > +
> > +             if (func_ret_check_addr) {
> > +                     emit_ldx(&prog, BPF_DW, BPF_REG_1, BPF_REG_FP, -8);
> > +
> > +                     /* call ret check function */
> > +                     if (emit_call(&prog, func_ret_check_addr, prog)) {
> > +                             ret = -EINVAL;
> > +                             goto cleanup;
> > +                     }
> > +
> > +                     /* remember return value in a stack for bpf prog to access */
> > +                     emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -8);
> > +                     memcpy(prog, x86_nops[5], X86_PATCH_SIZE);
> > +                     prog += X86_PATCH_SIZE;
> > +             }
> >       }
> >
> >       if (fexit->nr_links)
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 49f9d2bec401..f3551f7bdc28 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -919,7 +919,7 @@ struct bpf_tramp_image;
> >  int arch_prepare_bpf_trampoline(struct bpf_tramp_image *tr, void *image, void *image_end,
> >                               const struct btf_func_model *m, u32 flags,
> >                               struct bpf_tramp_links *tlinks,
> > -                             void *orig_call);
> > +                             void *orig_call, void *ret_call);
> >  u64 notrace __bpf_prog_enter_sleepable_recur(struct bpf_prog *prog,
> >                                            struct bpf_tramp_run_ctx *run_ctx);
> >  void notrace __bpf_prog_exit_sleepable_recur(struct bpf_prog *prog, u64 start,
> > @@ -974,6 +974,7 @@ struct bpf_trampoline {
> >       struct {
> >               struct btf_func_model model;
> >               void *addr;
> > +             void *ret_check_addr;
> >               bool ftrace_managed;
> >       } func;
> >       /* if !NULL this is BPF_PROG_TYPE_EXT program that extends another BPF
> > @@ -994,6 +995,7 @@ struct bpf_trampoline {
> >  struct bpf_attach_target_info {
> >       struct btf_func_model fmodel;
> >       long tgt_addr;
> > +     long tgt_ret_check_addr;
> >       const char *tgt_name;
> >       const struct btf_type *tgt_type;
> >  };
> > diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> > index 37bcedf5a44e..f7f25d0064dd 100644
> > --- a/kernel/bpf/bpf_lsm.c
> > +++ b/kernel/bpf/bpf_lsm.c
> > @@ -18,6 +18,17 @@
> >  #include <linux/ima.h>
> >  #include <linux/bpf-cgroup.h>
> >
> > +static bool is_ret_value_allowed(int ret, u32 ret_flags)
> > +{
> > +     if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) ||
> > +         (ret == 0 && !(ret_flags & LSM_RET_ZERO)) ||
> > +         (ret == 1 && !(ret_flags & LSM_RET_ONE)) ||
> > +         (ret > 1 && !(ret_flags & LSM_RET_GT_ONE)))
> > +             return false;
> > +
> > +     return true;
> > +}
> > +
> >  /* For every LSM hook that allows attachment of BPF programs, declare a nop
> >   * function where a BPF program can be attached.
> >   */
> > @@ -30,6 +41,15 @@ noinline RET bpf_lsm_##NAME(__VA_ARGS__)   \
> >  #include <linux/lsm_hook_defs.h>
> >  #undef LSM_HOOK
> >
> > +#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \
> > +noinline RET bpf_lsm_##NAME##_ret(int ret)   \
> > +{                                            \
> > +     return is_ret_value_allowed(ret, RET_FLAGS) ? ret : DEFAULT; \
> > +}
> > +
> > +#include <linux/lsm_hook_defs.h>
> > +#undef LSM_HOOK
> > +
> >  #define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \
> >       BTF_ID(func, bpf_lsm_##NAME)
> >  BTF_SET_START(bpf_lsm_hooks)
> > diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> > index 84b2d9dba79a..22485f0df534 100644
> > --- a/kernel/bpf/bpf_struct_ops.c
> > +++ b/kernel/bpf/bpf_struct_ops.c
> > @@ -346,7 +346,7 @@ int bpf_struct_ops_prepare_trampoline(struct bpf_tramp_links *tlinks,
> >        */
> >       flags = model->ret_size > 0 ? BPF_TRAMP_F_RET_FENTRY_RET : 0;
> >       return arch_prepare_bpf_trampoline(NULL, image, image_end,
> > -                                        model, flags, tlinks, NULL);
> > +                                        model, flags, tlinks, NULL, NULL);
> >  }
> >
> >  static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
> > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> > index d6395215b849..3c6821b3c08c 100644
> > --- a/kernel/bpf/trampoline.c
> > +++ b/kernel/bpf/trampoline.c
> > @@ -464,7 +464,8 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
> >
> >       err = arch_prepare_bpf_trampoline(im, im->image, im->image + PAGE_SIZE,
> >                                         &tr->func.model, tr->flags, tlinks,
> > -                                       tr->func.addr);
> > +                                       tr->func.addr,
> > +                                       tr->func.ret_check_addr);
> >       if (err < 0)
> >               goto out;
> >
> > @@ -802,6 +803,7 @@ struct bpf_trampoline *bpf_trampoline_get(u64 key,
> >
> >       memcpy(&tr->func.model, &tgt_info->fmodel, sizeof(tgt_info->fmodel));
> >       tr->func.addr = (void *)tgt_info->tgt_addr;
> > +     tr->func.ret_check_addr = (void *)tgt_info->tgt_ret_check_addr;
> >  out:
> >       mutex_unlock(&tr->mutex);
> >       return tr;
> > @@ -1055,7 +1057,7 @@ int __weak
> >  arch_prepare_bpf_trampoline(struct bpf_tramp_image *tr, void *image, void *image_end,
> >                           const struct btf_func_model *m, u32 flags,
> >                           struct bpf_tramp_links *tlinks,
> > -                         void *orig_call)
> > +                         void *orig_call, void *ret_check_call)
> >  {
> >       return -ENOTSUPP;
> >  }
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 5e74f460dfd0..1ad0fe5cefe9 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -14988,12 +14988,13 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
> >  {
> >       bool prog_extension = prog->type == BPF_PROG_TYPE_EXT;
> >       const char prefix[] = "btf_trace_";
> > -     int ret = 0, subprog = -1, i;
> > +     int ret = 0, subprog = -1, i, tname_len;
> >       const struct btf_type *t;
> >       bool conservative = true;
> >       const char *tname;
> > +     char *tname_ret;
> >       struct btf *btf;
> > -     long addr = 0;
> > +     long addr = 0, ret_check_addr = 0;
> >
> >       if (!btf_id) {
> >               bpf_log(log, "Tracing programs must provide btf_id\n");
> > @@ -15168,6 +15169,28 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
> >                                       tname);
> >                               return -ENOENT;
> >                       }
> > +
> > +                     if (prog->expected_attach_type == BPF_LSM_MAC) {
> > +                             tname_len = strlen(tname);
> > +                             tname_ret = kmalloc(tname_len + 5, GFP_KERNEL);
> > +                             if (!tname_ret) {
> > +                                     bpf_log(log,
> > +                                             "Cannot allocate memory for %s_ret string\n",
> > +                                             tname);
> > +                                     return -ENOMEM;
> > +                             }
> > +
> > +                             snprintf(tname_ret, tname_len + 5, "%s_ret", tname);
> > +                             ret_check_addr = kallsyms_lookup_name(tname_ret);
> > +                             kfree(tname_ret);
> > +
> > +                             if (!ret_check_addr) {
> > +                                     bpf_log(log,
> > +                                             "Kernel symbol %s_ret not found\n",
> > +                                             tname);
> > +                                     return -ENOENT;
> > +                             }
> > +                     }
> >               }
> >
> >               if (prog->aux->sleepable) {
> > @@ -15210,6 +15233,7 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
> >               break;
> >       }
> >       tgt_info->tgt_addr = addr;
> > +     tgt_info->tgt_ret_check_addr = ret_check_addr;
> >       tgt_info->tgt_name = tname;
> >       tgt_info->tgt_type = t;
> >       return 0;

Re: [PoC][PATCH] bpf: Call return value check function in the JITed code

[Roberto Sassu]

On 11/16/2022 6:12 PM, Casey Schaufler wrote:
> On 11/16/2022 7:47 AM, Roberto Sassu wrote:
>> From: Roberto Sassu <roberto.sassu@huawei.com>
>>
>> eBPF allows certain types of eBPF programs to modify the return value of
>> the functions they attach to. This is used for example by BPF LSM to let
>> security modules make their decision on LSM hooks.
>>
>> The JITed code looks like the following:
>>
>>      ret = bpf_lsm_inode_permission_impl1(); // from a security module
>>      if (ret)
>>          goto out;
>>
>> ..
>>
>>      ret = bpf_lsm_inode_permission_implN(); // from a security module
>>      if (ret)
>>          goto out;
>>
>>      ret = bpf_lsm_inode_permission(); // in the kernel, returns DEFAULT
>> out:
>>
>> If ret is not zero, the attachment points of BPF LSM are not executed. For
>> this reason, the return value check cannot be done there.
>>
>> Instead, the idea is to use the LSM_HOOK() macro to define a per-hook check
>> function.
>>
>> Whenever an eBPF program attaches to an LSM hook, the eBPF verifier
>> resolves the address of the check function (whose name is
>> bpf_lsm_<hook name>_ret()) and adds a call to that function just after the
>> out label. If the return value is illegal, the check function changes it
>> back to the default value defined by the LSM infrastructure:
>>
>> ..
>>
>> out:
>>      ret = bpf_lsm_inode_permission_ret(ret);
> 
> As I've mentioned elsewhere, the return value is a small part of
> the problem you have with eBPF programs and the BPF LSM. Because
> the LSM infrastructure is inconsistent with regard to return codes,
> values returned in pointers and use of secids there is no uniform
> mechanism that I can see to address the "legitimate return" problem.
> 
> Lets look at one of the ickyest interfaces we have, security_getprocattr().
> It returns the size of a string that it has allocated. It puts the
> pointer to the allocated buffer into a char **value that was passed to it.
> If bpf_getprocattr() returns a positive number and sets value to NULL Bad
> Things can happen. If the return value is greater than the size allocated
> ditto. If it returns an error but allocates a string you get a memory leak.

I hope I understood how it works correctly, but you cannot modify 
directly data accessible from a pointer provided as parameter by the LSM 
hook you attach to. The pointer is treated as scalar value and the eBPF 
verifier detects any attempt to dereference as an illegal access. The 
only way to modify such data is through helpers that need to be properly 
declared to be usable by eBPF programs.

Also, if I'm not mistaken we have the limitation of five parameters per 
functions. Not sure what happens for hooks that have more than this.

> security_secid_to_secctx() has to work in concert with security_release_secctx()
> to do memory lifecycle management. If secid_to_secctx() allocates memory
> release_secctx() has to free it, while if secid_to_secctx() doesn't allocate
> memory it better not. (SELinux allocates memory, Smack does not. It's a real
> distinction) Your return checker would need to understand a lot more about
> the behavior of your eBPF programs than what value they return.

I see. Within an eBPF program we are able to pair allocation and free 
together. I guess something similar could be done for pairs of LSM hooks.

Roberto

>> In this way, an eBPF program cannot cause illegal return values to be sent
>> to BPF LSM, and to the callers of the LSM infrastructure.
>>
>> This is just a PoC, to validate the idea and get an early feedback.
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>> ---
>>   arch/arm64/net/bpf_jit_comp.c |  7 ++++---
>>   arch/x86/net/bpf_jit_comp.c   | 17 ++++++++++++++++-
>>   include/linux/bpf.h           |  4 +++-
>>   kernel/bpf/bpf_lsm.c          | 20 ++++++++++++++++++++
>>   kernel/bpf/bpf_struct_ops.c   |  2 +-
>>   kernel/bpf/trampoline.c       |  6 ++++--
>>   kernel/bpf/verifier.c         | 28 ++++++++++++++++++++++++++--
>>   7 files changed, 74 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
>> index 62f805f427b7..5412230c6935 100644
>> --- a/arch/arm64/net/bpf_jit_comp.c
>> +++ b/arch/arm64/net/bpf_jit_comp.c
>> @@ -1764,7 +1764,7 @@ static void restore_args(struct jit_ctx *ctx, int args_off, int nargs)
>>    */
>>   static int prepare_trampoline(struct jit_ctx *ctx, struct bpf_tramp_image *im,
>>   			      struct bpf_tramp_links *tlinks, void *orig_call,
>> -			      int nargs, u32 flags)
>> +			      void *ret_check_call, int nargs, u32 flags)
>>   {
>>   	int i;
>>   	int stack_size;
>> @@ -1963,7 +1963,7 @@ static int prepare_trampoline(struct jit_ctx *ctx, struct bpf_tramp_image *im,
>>   int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image,
>>   				void *image_end, const struct btf_func_model *m,
>>   				u32 flags, struct bpf_tramp_links *tlinks,
>> -				void *orig_call)
>> +				void *orig_call, void *ret_check_call)
>>   {
>>   	int i, ret;
>>   	int nargs = m->nr_args;
>> @@ -1983,7 +1983,8 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image,
>>   			return -ENOTSUPP;
>>   	}
>>   
>> -	ret = prepare_trampoline(&ctx, im, tlinks, orig_call, nargs, flags);
>> +	ret = prepare_trampoline(&ctx, im, tlinks, orig_call, ret_check_call,
>> +				 nargs, flags);
>>   	if (ret < 0)
>>   		return ret;
>>   
>> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
>> index cec5195602bc..6cd727b4af0a 100644
>> --- a/arch/x86/net/bpf_jit_comp.c
>> +++ b/arch/x86/net/bpf_jit_comp.c
>> @@ -2123,7 +2123,7 @@ static int invoke_bpf_mod_ret(const struct btf_func_model *m, u8 **pprog,
>>   int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *image_end,
>>   				const struct btf_func_model *m, u32 flags,
>>   				struct bpf_tramp_links *tlinks,
>> -				void *func_addr)
>> +				void *func_addr, void *func_ret_check_addr)
>>   {
>>   	int ret, i, nr_args = m->nr_args, extra_nregs = 0;
>>   	int regs_off, ip_off, args_off, stack_size = nr_args * 8, run_ctx_off;
>> @@ -2280,6 +2280,21 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
>>   		for (i = 0; i < fmod_ret->nr_links; i++)
>>   			emit_cond_near_jump(&branches[i], prog, branches[i],
>>   					    X86_JNE);
>> +
>> +		if (func_ret_check_addr) {
>> +			emit_ldx(&prog, BPF_DW, BPF_REG_1, BPF_REG_FP, -8);
>> +
>> +			/* call ret check function */
>> +			if (emit_call(&prog, func_ret_check_addr, prog)) {
>> +				ret = -EINVAL;
>> +				goto cleanup;
>> +			}
>> +
>> +			/* remember return value in a stack for bpf prog to access */
>> +			emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -8);
>> +			memcpy(prog, x86_nops[5], X86_PATCH_SIZE);
>> +			prog += X86_PATCH_SIZE;
>> +		}
>>   	}
>>   
>>   	if (fexit->nr_links)
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 49f9d2bec401..f3551f7bdc28 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -919,7 +919,7 @@ struct bpf_tramp_image;
>>   int arch_prepare_bpf_trampoline(struct bpf_tramp_image *tr, void *image, void *image_end,
>>   				const struct btf_func_model *m, u32 flags,
>>   				struct bpf_tramp_links *tlinks,
>> -				void *orig_call);
>> +				void *orig_call, void *ret_call);
>>   u64 notrace __bpf_prog_enter_sleepable_recur(struct bpf_prog *prog,
>>   					     struct bpf_tramp_run_ctx *run_ctx);
>>   void notrace __bpf_prog_exit_sleepable_recur(struct bpf_prog *prog, u64 start,
>> @@ -974,6 +974,7 @@ struct bpf_trampoline {
>>   	struct {
>>   		struct btf_func_model model;
>>   		void *addr;
>> +		void *ret_check_addr;
>>   		bool ftrace_managed;
>>   	} func;
>>   	/* if !NULL this is BPF_PROG_TYPE_EXT program that extends another BPF
>> @@ -994,6 +995,7 @@ struct bpf_trampoline {
>>   struct bpf_attach_target_info {
>>   	struct btf_func_model fmodel;
>>   	long tgt_addr;
>> +	long tgt_ret_check_addr;
>>   	const char *tgt_name;
>>   	const struct btf_type *tgt_type;
>>   };
>> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
>> index 37bcedf5a44e..f7f25d0064dd 100644
>> --- a/kernel/bpf/bpf_lsm.c
>> +++ b/kernel/bpf/bpf_lsm.c
>> @@ -18,6 +18,17 @@
>>   #include <linux/ima.h>
>>   #include <linux/bpf-cgroup.h>
>>   
>> +static bool is_ret_value_allowed(int ret, u32 ret_flags)
>> +{
>> +	if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) ||
>> +	    (ret == 0 && !(ret_flags & LSM_RET_ZERO)) ||
>> +	    (ret == 1 && !(ret_flags & LSM_RET_ONE)) ||
>> +	    (ret > 1 && !(ret_flags & LSM_RET_GT_ONE)))
>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>>   /* For every LSM hook that allows attachment of BPF programs, declare a nop
>>    * function where a BPF program can be attached.
>>    */
>> @@ -30,6 +41,15 @@ noinline RET bpf_lsm_##NAME(__VA_ARGS__)	\
>>   #include <linux/lsm_hook_defs.h>
>>   #undef LSM_HOOK
>>   
>> +#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...)	\
>> +noinline RET bpf_lsm_##NAME##_ret(int ret)	\
>> +{						\
>> +	return is_ret_value_allowed(ret, RET_FLAGS) ? ret : DEFAULT; \
>> +}
>> +
>> +#include <linux/lsm_hook_defs.h>
>> +#undef LSM_HOOK
>> +
>>   #define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \
>>   	BTF_ID(func, bpf_lsm_##NAME)
>>   BTF_SET_START(bpf_lsm_hooks)
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index 84b2d9dba79a..22485f0df534 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -346,7 +346,7 @@ int bpf_struct_ops_prepare_trampoline(struct bpf_tramp_links *tlinks,
>>   	 */
>>   	flags = model->ret_size > 0 ? BPF_TRAMP_F_RET_FENTRY_RET : 0;
>>   	return arch_prepare_bpf_trampoline(NULL, image, image_end,
>> -					   model, flags, tlinks, NULL);
>> +					   model, flags, tlinks, NULL, NULL);
>>   }
>>   
>>   static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
>> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
>> index d6395215b849..3c6821b3c08c 100644
>> --- a/kernel/bpf/trampoline.c
>> +++ b/kernel/bpf/trampoline.c
>> @@ -464,7 +464,8 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
>>   
>>   	err = arch_prepare_bpf_trampoline(im, im->image, im->image + PAGE_SIZE,
>>   					  &tr->func.model, tr->flags, tlinks,
>> -					  tr->func.addr);
>> +					  tr->func.addr,
>> +					  tr->func.ret_check_addr);
>>   	if (err < 0)
>>   		goto out;
>>   
>> @@ -802,6 +803,7 @@ struct bpf_trampoline *bpf_trampoline_get(u64 key,
>>   
>>   	memcpy(&tr->func.model, &tgt_info->fmodel, sizeof(tgt_info->fmodel));
>>   	tr->func.addr = (void *)tgt_info->tgt_addr;
>> +	tr->func.ret_check_addr = (void *)tgt_info->tgt_ret_check_addr;
>>   out:
>>   	mutex_unlock(&tr->mutex);
>>   	return tr;
>> @@ -1055,7 +1057,7 @@ int __weak
>>   arch_prepare_bpf_trampoline(struct bpf_tramp_image *tr, void *image, void *image_end,
>>   			    const struct btf_func_model *m, u32 flags,
>>   			    struct bpf_tramp_links *tlinks,
>> -			    void *orig_call)
>> +			    void *orig_call, void *ret_check_call)
>>   {
>>   	return -ENOTSUPP;
>>   }
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 5e74f460dfd0..1ad0fe5cefe9 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -14988,12 +14988,13 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
>>   {
>>   	bool prog_extension = prog->type == BPF_PROG_TYPE_EXT;
>>   	const char prefix[] = "btf_trace_";
>> -	int ret = 0, subprog = -1, i;
>> +	int ret = 0, subprog = -1, i, tname_len;
>>   	const struct btf_type *t;
>>   	bool conservative = true;
>>   	const char *tname;
>> +	char *tname_ret;
>>   	struct btf *btf;
>> -	long addr = 0;
>> +	long addr = 0, ret_check_addr = 0;
>>   
>>   	if (!btf_id) {
>>   		bpf_log(log, "Tracing programs must provide btf_id\n");
>> @@ -15168,6 +15169,28 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
>>   					tname);
>>   				return -ENOENT;
>>   			}
>> +
>> +			if (prog->expected_attach_type == BPF_LSM_MAC) {
>> +				tname_len = strlen(tname);
>> +				tname_ret = kmalloc(tname_len + 5, GFP_KERNEL);
>> +				if (!tname_ret) {
>> +					bpf_log(log,
>> +						"Cannot allocate memory for %s_ret string\n",
>> +						tname);
>> +					return -ENOMEM;
>> +				}
>> +
>> +				snprintf(tname_ret, tname_len + 5, "%s_ret", tname);
>> +				ret_check_addr = kallsyms_lookup_name(tname_ret);
>> +				kfree(tname_ret);
>> +
>> +				if (!ret_check_addr) {
>> +					bpf_log(log,
>> +						"Kernel symbol %s_ret not found\n",
>> +						tname);
>> +					return -ENOENT;
>> +				}
>> +			}
>>   		}
>>   
>>   		if (prog->aux->sleepable) {
>> @@ -15210,6 +15233,7 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
>>   		break;
>>   	}
>>   	tgt_info->tgt_addr = addr;
>> +	tgt_info->tgt_ret_check_addr = ret_check_addr;
>>   	tgt_info->tgt_name = tname;
>>   	tgt_info->tgt_type = t;
>>   	return 0;

Re: [PoC][PATCH] bpf: Call return value check function in the JITed code

[Roberto Sassu]

On Fri, 2022-11-18 at 09:44 +0100, Roberto Sassu wrote:
> On 11/16/2022 6:12 PM, Casey Schaufler wrote:
> > On 11/16/2022 7:47 AM, Roberto Sassu wrote:
> > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > 
> > > eBPF allows certain types of eBPF programs to modify the return value of
> > > the functions they attach to. This is used for example by BPF LSM to let
> > > security modules make their decision on LSM hooks.
> > > 
> > > The JITed code looks like the following:
> > > 
> > >      ret = bpf_lsm_inode_permission_impl1(); // from a security module
> > >      if (ret)
> > >          goto out;
> > > 
> > > ..
> > > 
> > >      ret = bpf_lsm_inode_permission_implN(); // from a security module
> > >      if (ret)
> > >          goto out;
> > > 
> > >      ret = bpf_lsm_inode_permission(); // in the kernel, returns DEFAULT
> > > out:
> > > 
> > > If ret is not zero, the attachment points of BPF LSM are not executed. For
> > > this reason, the return value check cannot be done there.
> > > 
> > > Instead, the idea is to use the LSM_HOOK() macro to define a per-hook check
> > > function.
> > > 
> > > Whenever an eBPF program attaches to an LSM hook, the eBPF verifier
> > > resolves the address of the check function (whose name is
> > > bpf_lsm_<hook name>_ret()) and adds a call to that function just after the
> > > out label. If the return value is illegal, the check function changes it
> > > back to the default value defined by the LSM infrastructure:
> > > 
> > > ..
> > > 
> > > out:
> > >      ret = bpf_lsm_inode_permission_ret(ret);
> > 
> > As I've mentioned elsewhere, the return value is a small part of
> > the problem you have with eBPF programs and the BPF LSM. Because
> > the LSM infrastructure is inconsistent with regard to return codes,
> > values returned in pointers and use of secids there is no uniform
> > mechanism that I can see to address the "legitimate return" problem.
> > 
> > Lets look at one of the ickyest interfaces we have, security_getprocattr().
> > It returns the size of a string that it has allocated. It puts the
> > pointer to the allocated buffer into a char **value that was passed to it.
> > If bpf_getprocattr() returns a positive number and sets value to NULL Bad
> > Things can happen. If the return value is greater than the size allocated
> > ditto. If it returns an error but allocates a string you get a memory leak.
> 
> I hope I understood how it works correctly, but you cannot modify 
> directly data accessible from a pointer provided as parameter by the LSM 
> hook you attach to. The pointer is treated as scalar value and the eBPF 
> verifier detects any attempt to dereference as an illegal access. The 
> only way to modify such data is through helpers that need to be properly 
> declared to be usable by eBPF programs.

I wanted to double check about accessing the LSM hook arguments from an
eBPF program. I checked what it could prevent to access them.

First, in kernel/bpf/btf.c:

if (!btf_type_is_struct(t)) {
	bpf_log(log,
		"func '%s' arg%d type %s is not a struct\n",

If the argument is not a struct, it is not accessible.


Second, if a btf_struct_access method has not been defined for a
structure, only read can be done (kernel/bpf/verifier.c):

if (env->ops->btf_struct_access) {
	ret = env->ops->btf_struct_access(...);
} else {
	if (atype != BPF_READ) {
		verbose(env, "only read is supported\n");
		return -EACCES;
	}

I found four:

net/bpf/bpf_dummy_struct_ops.c: .btf_struct_access =
bpf_dummy_ops_btf_struct_access,
net/core/filter.c:      .btf_struct_access      =
tc_cls_act_btf_struct_access,
net/core/filter.c:      .btf_struct_access      =
xdp_btf_struct_access,
net/ipv4/bpf_tcp_ca.c:  .btf_struct_access      =
bpf_tcp_ca_btf_struct_access,

Anything else?

Thanks

Roberto

Re: [RFC][PATCH 4/4] security: Enforce limitations on return values from LSMs

[Paul Moore]

On Wed, Nov 16, 2022 at 9:37 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
> On Tue, 2022-11-15 at 21:35 -0500, Paul Moore wrote:
> > If you want to somehow instrument the LSM hook definitions (what I
> > believe to be the motivation behind patch 3/4) to indicate valid
> > return values for use by the BPF verifier, I think we could entertain
> > that, or at least discuss it further, but I'm not inclined to support
> > any runtime overhead at the LSM layer for a specific LSM.
>
> Ok, yes. Patches 1-3 would help to keep in sync the LSM infrastructure
> and eBPF, but it is not strictly needed. I could propose an eBPF-only
> alternative to declare sets of functions per interval.
>
> More or less, I developed an eBPF-based alternative also for patch 4.
> It is just a proof of concept. Will propose it, to validate the idea.

Thanks, I think that might be the best approach.  Also, please
resubmit patches 1/4 and 2/4 with those small changes; those are nice
improvements that just need a couple of small tweaks to be acceptable
:)

-- 
paul-moore.com

Re: [RFC][PATCH 0/4] security: Ensure LSMs return expected values

[Casey Schaufler]

On 11/15/2022 9:56 AM, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
>
> LSMs should follow the conventions stated in include/linux/lsm_hooks.h for
> return values, as these conventions are followed by callers of the LSM
> infrastructure for error handling.
>
> The ability of an LSM to return arbitrary values could cause big troubles.
> For LSMs aiming at being upstreamed, this event is unlikely, as each LSM is
> carefully reviewed and it won't be accepted if it does not meet the return
> value conventions. However, the recent introduction of BPF LSM allows
> security modules (not part of the kernel) to inject arbitrary values,
> without BPF LSM verifying them.
>
> The initial idea was to fix BPF LSM itself. However, due to technical
> difficulties to determine the precise interval of return values from a
> static code analysis of eBPF programs, the new approach was to put the
> fix in the LSM infrastructure, so that all LSMs can benefit from this work
> as well.

The LSM infrastructure is a hodgepodge of inconsistently defined functions.
That makes them difficult to deal with in any sort of consistent way.
Your attempt to make them more rational is laudable, but falls short of
being useful, while slowing all security processing to no end.

I don't consider this a "fix", and I don't see how it benefits any LSM
other than BPF. Adding return code checks into the infrastructure code is
insufficient. If someone adds a bpf_secid_to_secctx() eBPF program you're
going to have a whole new set of problems that this clutter isn't going to
help with at all. Return values are an important part of the problem, but
nowhere near the whole of it.

Please take this back into the BPF security module, where it belongs.
BPF isn't playing by the admittedly complex, inconsistent and arbitrary
rules of the LSM infrastructure. Your proposal doesn't change that, nor
does it sufficiently change the infrastructure to make BPF safe.

>
> The biggest problem of allowing arbitrary return values is when an LSM
> returns a positive value, instead of a negative value, as it could be
> converted to a pointer. Since such pointer escapes the IS_ERR() check, its
> use later in the code can cause unpredictable consequences (e.g. invalid
> memory access).
>
> Another problem is returning zero when an LSM is supposed to have done some
> operations. For example, the inode_init_security hook expects that their
> implementations return zero only if they set the fields of the new xattr to
> be added to the new inode. Otherwise, other kernel subsystems might
> encounter unexpected conditions leading to a crash (e.g.
> evm_protected_xattr_common() getting NULL as argument). This problem is
> addressed separately in another patch set.
>
> Finally, there are LSM hooks which are supposed to return just 1 as
> positive value, or non-negative values. Also in these cases, although it
> seems less critical, it is safer to return to callers of the LSM
> infrastructure more precisely what they expect.
>
> Patches 1 and 2 ensure that the documentation of LSM return values is
> complete and accurate. Then, patch 3 introduces four flags (LSM_RET_NEG,
> LSM_RET_ZERO, LSM_RET_ONE, LSM_RET_GT_ONE), one for each interval of
> interest (< 0, = 0, = 1, > 1), and sets the correct flags for each LSM
> hook. Finally, patch 4 verifies for each return value from LSMs that it is
> an expected one.
>
> Roberto Sassu (4):
>   lsm: Clarify documentation of vm_enough_memory hook
>   lsm: Add missing return values doc in lsm_hooks.h and fix formatting
>   lsm: Redefine LSM_HOOK() macro to add return value flags as argument
>   security: Enforce limitations on return values from LSMs
>
>  include/linux/bpf_lsm.h       |   2 +-
>  include/linux/lsm_hook_defs.h | 779 ++++++++++++++++++++--------------
>  include/linux/lsm_hooks.h     | 136 ++++--
>  kernel/bpf/bpf_lsm.c          |   5 +-
>  security/bpf/hooks.c          |   2 +-
>  security/security.c           |  38 +-
>  6 files changed, 589 insertions(+), 373 deletions(-)
>