From 3b87d9f436b6893503f43f487d3bc6091d9db178 Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Mon, 17 Oct 2022 14:08:09 -0400 Subject: [PATCH 01/13] fs: edit a comment made in bad taste I know nobody likes a buzzkill, but I figure it's best to keep the bad jokes appropriate for small children. Signed-off-by: Paul Moore --- include/linux/fs_context.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/fs_context.h b/include/linux/fs_context.h index 13fa6f3df8e4..6fbf49cc10e4 100644 --- a/include/linux/fs_context.h +++ b/include/linux/fs_context.h @@ -99,7 +99,7 @@ struct fs_context { const struct cred *cred; /* The mounter's credentials */ struct p_log log; /* Logging buffer */ const char *source; /* The source name (eg. dev path) */ - void *security; /* Linux S&M options */ + void *security; /* LSM options */ void *s_fs_info; /* Proposed s_fs_info */ unsigned int sb_flags; /* Proposed superblock flags (SB_*) */ unsigned int sb_flags_mask; /* Superblock flags that were changed */ From 3437d67a8d92a87aaba9107ceff6b4cc33b7cb94 Mon Sep 17 00:00:00 2001 From: Gaosheng Cui Date: Tue, 25 Oct 2022 20:57:44 +0800 Subject: [PATCH 02/13] lsm: remove obsoleted comments for security hooks Remove the following obsoleted comments for security hooks: 1. sb_copy_data, the hook function has been removed since commit 5b4002391153 ("LSM: turn sb_eat_lsm_opts() into a method"). 2. sb_parse_opts_str, the hook function has been removed since commit 757cbe597fe8 ("LSM: new method: ->sb_add_mnt_opt()"). They are obsoleted comments, so remove them. Signed-off-by: Gaosheng Cui Reviewed-by: Casey Schaufler [PM: subj line tweaks] Signed-off-by: Paul Moore --- include/linux/lsm_hooks.h | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 4ec80b96c22e..9a7e69aff3d1 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -136,15 +136,6 @@ * @flags contains the mount flags. * @data contains the filesystem-specific data. * Return 0 if permission is granted. - * @sb_copy_data: - * Allow mount option data to be copied prior to parsing by the filesystem, - * so that the security module can extract security-specific mount - * options cleanly (a filesystem may modify the data e.g. with strsep()). - * This also allows the original mount data to be stripped of security- - * specific options to avoid having to make filesystems aware of them. - * @orig the original mount data copied from userspace. - * @copy copied data which will be passed to the security module. - * Returns 0 if the copy was successful. * @sb_mnt_opts_compat: * Determine if the new mount options in @mnt_opts are allowed given * the existing mounted filesystem at @sb. @@ -180,10 +171,6 @@ * 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 * @move_mount: * Check permission before a mount is moved. * @from_path indicates the mount that is going to be moved. From 610b17b05c5c682fbb8fefedae1aacaab412eac3 Mon Sep 17 00:00:00 2001 From: Bo Liu Date: Tue, 25 Oct 2022 21:06:34 -0400 Subject: [PATCH 03/13] audit: Fix some kernel-doc warnings The current code provokes some kernel-doc warnings: security/lsm_audit.c:198: warning: Function parameter or member 'ab' not described in 'dump_common_audit_data' Signed-off-by: Bo Liu [PM: description line wrap] Signed-off-by: Paul Moore --- security/lsm_audit.c | 1 + 1 file changed, 1 insertion(+) diff --git a/security/lsm_audit.c b/security/lsm_audit.c index 75cc3f8d2a42..a7355b4b9bb8 100644 --- a/security/lsm_audit.c +++ b/security/lsm_audit.c @@ -190,6 +190,7 @@ static inline void print_ipv4_addr(struct audit_buffer *ab, __be32 addr, /** * dump_common_audit_data - helper to dump common audit data + * @ab : the audit buffer * @a : common audit data * */ From b10b9c342f7571f287fd422be5d5c0beb26ba974 Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Mon, 10 Oct 2022 12:31:21 -0400 Subject: [PATCH 04/13] lsm: make security_socket_getpeersec_stream() sockptr_t safe Commit 4ff09db1b79b ("bpf: net: Change sk_getsockopt() to take the sockptr_t argument") made it possible to call sk_getsockopt() with both user and kernel address space buffers through the use of the sockptr_t type. Unfortunately at the time of conversion the security_socket_getpeersec_stream() LSM hook was written to only accept userspace buffers, and in a desire to avoid having to change the LSM hook the commit author simply passed the sockptr_t's userspace buffer pointer. Since the only sk_getsockopt() callers at the time of conversion which used kernel sockptr_t buffers did not allow SO_PEERSEC, and hence the security_socket_getpeersec_stream() hook, this was acceptable but also very fragile as future changes presented the possibility of silently passing kernel space pointers to the LSM hook. There are several ways to protect against this, including careful code review of future commits, but since relying on code review to catch bugs is a recipe for disaster and the upstream eBPF maintainer is "strongly against defensive programming", this patch updates the LSM hook, and all of the implementations to support sockptr_t and safely handle both user and kernel space buffers. Acked-by: Casey Schaufler Acked-by: John Johansen Signed-off-by: Paul Moore --- include/linux/lsm_hook_defs.h | 2 +- include/linux/lsm_hooks.h | 4 ++-- include/linux/security.h | 11 +++++++---- net/core/sock.c | 3 ++- security/apparmor/lsm.c | 29 +++++++++++++---------------- security/security.c | 6 +++--- security/selinux/hooks.c | 13 ++++++------- security/smack/smack_lsm.c | 19 ++++++++++--------- 8 files changed, 44 insertions(+), 43 deletions(-) diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index ec119da1d89b..6abde829b6e5 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -302,7 +302,7 @@ 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) + sockptr_t optval, sockptr_t optlen, unsigned int 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) diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 9a7e69aff3d1..2831efebde69 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -949,8 +949,8 @@ * SO_GETPEERSEC. For tcp sockets this can be meaningful if the * socket is associated with an ipsec SA. * @sock is the local socket. - * @optval userspace memory where the security state is to be copied. - * @optlen userspace int where the module should copy the actual length + * @optval memory where the security state is to be copied. + * @optlen memory where the module should copy the actual length * of the security state. * @len as input is the maximum length to copy to userspace provided * by the caller. diff --git a/include/linux/security.h b/include/linux/security.h index ca1b7109c0db..0e419c595cee 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -31,6 +31,7 @@ #include #include #include +#include struct linux_binprm; struct cred; @@ -1411,8 +1412,8 @@ int security_socket_getsockopt(struct socket *sock, int level, int optname); int security_socket_setsockopt(struct socket *sock, int level, int optname); int security_socket_shutdown(struct socket *sock, int how); int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb); -int security_socket_getpeersec_stream(struct socket *sock, char __user *optval, - int __user *optlen, unsigned len); +int security_socket_getpeersec_stream(struct socket *sock, sockptr_t optval, + sockptr_t optlen, unsigned int len); int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid); int security_sk_alloc(struct sock *sk, int family, gfp_t priority); void security_sk_free(struct sock *sk); @@ -1548,8 +1549,10 @@ static inline int security_sock_rcv_skb(struct sock *sk, return 0; } -static inline int security_socket_getpeersec_stream(struct socket *sock, char __user *optval, - int __user *optlen, unsigned len) +static inline int security_socket_getpeersec_stream(struct socket *sock, + sockptr_t optval, + sockptr_t optlen, + unsigned int len) { return -ENOPROTOOPT; } diff --git a/net/core/sock.c b/net/core/sock.c index a3ba0358c77c..c9c6af85d9c0 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -1793,7 +1793,8 @@ int sk_getsockopt(struct sock *sk, int level, int optname, break; case SO_PEERSEC: - return security_socket_getpeersec_stream(sock, optval.user, optlen.user, len); + return security_socket_getpeersec_stream(sock, + optval, optlen, len); case SO_MARK: v.val = sk->sk_mark; diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index f56070270c69..89e84ef54e8e 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -1103,11 +1103,10 @@ static struct aa_label *sk_peer_label(struct sock *sk) * Note: for tcp only valid if using ipsec or cipso on lan */ static int apparmor_socket_getpeersec_stream(struct socket *sock, - char __user *optval, - int __user *optlen, + sockptr_t optval, sockptr_t optlen, unsigned int len) { - char *name; + char *name = NULL; int slen, error = 0; struct aa_label *label; struct aa_label *peer; @@ -1124,23 +1123,21 @@ static int apparmor_socket_getpeersec_stream(struct socket *sock, /* don't include terminating \0 in slen, it breaks some apps */ if (slen < 0) { error = -ENOMEM; - } else { - if (slen > len) { - error = -ERANGE; - } else if (copy_to_user(optval, name, slen)) { - error = -EFAULT; - goto out; - } - if (put_user(slen, optlen)) - error = -EFAULT; -out: - kfree(name); - + goto done; + } + if (slen > len) { + error = -ERANGE; + goto done_len; } + if (copy_to_sockptr(optval, name, slen)) + error = -EFAULT; +done_len: + if (copy_to_sockptr(optlen, &slen, sizeof(slen))) + error = -EFAULT; done: end_current_label_crit_section(label); - + kfree(name); return error; } diff --git a/security/security.c b/security/security.c index 79d82cb6e469..f27c885ee98d 100644 --- a/security/security.c +++ b/security/security.c @@ -2267,11 +2267,11 @@ int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb) } EXPORT_SYMBOL(security_sock_rcv_skb); -int security_socket_getpeersec_stream(struct socket *sock, char __user *optval, - int __user *optlen, unsigned len) +int security_socket_getpeersec_stream(struct socket *sock, sockptr_t optval, + sockptr_t optlen, unsigned int len) { return call_int_hook(socket_getpeersec_stream, -ENOPROTOOPT, sock, - optval, optlen, len); + optval, optlen, len); } int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index f553c370397e..0bdddeba90a6 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -5119,11 +5119,12 @@ static int selinux_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb) return err; } -static int selinux_socket_getpeersec_stream(struct socket *sock, char __user *optval, - int __user *optlen, unsigned len) +static int selinux_socket_getpeersec_stream(struct socket *sock, + sockptr_t optval, sockptr_t optlen, + unsigned int len) { int err = 0; - char *scontext; + char *scontext = NULL; u32 scontext_len; struct sk_security_struct *sksec = sock->sk->sk_security; u32 peer_sid = SECSID_NULL; @@ -5139,17 +5140,15 @@ static int selinux_socket_getpeersec_stream(struct socket *sock, char __user *op &scontext_len); if (err) return err; - if (scontext_len > len) { err = -ERANGE; goto out_len; } - if (copy_to_user(optval, scontext, scontext_len)) + if (copy_to_sockptr(optval, scontext, scontext_len)) err = -EFAULT; - out_len: - if (put_user(scontext_len, optlen)) + if (copy_to_sockptr(optlen, &scontext_len, sizeof(scontext_len))) err = -EFAULT; kfree(scontext); return err; diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index b6306d71c908..2bd7fadf7fb4 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -4006,12 +4006,12 @@ static int smack_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb) * returns zero on success, an error code otherwise */ static int smack_socket_getpeersec_stream(struct socket *sock, - char __user *optval, - int __user *optlen, unsigned len) + sockptr_t optval, sockptr_t optlen, + unsigned int len) { struct socket_smack *ssp; char *rcp = ""; - int slen = 1; + u32 slen = 1; int rc = 0; ssp = sock->sk->sk_security; @@ -4019,15 +4019,16 @@ static int smack_socket_getpeersec_stream(struct socket *sock, rcp = ssp->smk_packet->smk_known; slen = strlen(rcp) + 1; } - - if (slen > len) + if (slen > len) { rc = -ERANGE; - else if (copy_to_user(optval, rcp, slen) != 0) - rc = -EFAULT; + goto out_len; + } - if (put_user(slen, optlen) != 0) + if (copy_to_sockptr(optval, rcp, slen)) + rc = -EFAULT; +out_len: + if (copy_to_sockptr(optlen, &slen, sizeof(slen))) rc = -EFAULT; - return rc; } From 3b4c7bc01727e3a465759236eeac03d0dd686da3 Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Fri, 4 Nov 2022 13:52:42 +0100 Subject: [PATCH 05/13] xattr: use rbtree for simple_xattrs A while ago Vasily reported that it is possible to set a large number of xattrs on inodes of filesystems that make use of the simple xattr infrastructure. This includes all kernfs-based filesystems that support xattrs (e.g., cgroupfs and tmpfs). Both cgroupfs and tmpfs can be mounted by unprivileged users in unprivileged containers and root in an unprivileged container can set an unrestricted number of security.* xattrs and privileged users can also set unlimited trusted.* xattrs. As there are apparently users that have a fairly large number of xattrs we should scale a bit better. Other xattrs such as user.* are restricted for kernfs-based instances to a fairly limited number. Using a simple linked list protected by a spinlock used for set, get, and list operations doesn't scale well if users use a lot of xattrs even if it's not a crazy number. There's no need to bring in the big guns like rhashtables or rw semaphores for this. An rbtree with a rwlock, or limited rcu semanics and seqlock is enough. It scales within the constraints we are working in. By far the most common operation is getting an xattr. Setting xattrs should be a moderately rare operation. And listxattr() often only happens when copying xattrs between files or together with the contents to a new file. Holding a lock across listxattr() is unproblematic because it doesn't list the values of xattrs. It can only be used to list the names of all xattrs set on a file. And the number of xattr names that can be listed with listxattr() is limited to XATTR_LIST_MAX aka 65536 bytes. If a larger buffer is passed then vfs_listxattr() caps it to XATTR_LIST_MAX and if more xattr names are found it will return -E2BIG. In short, the maximum amount of memory that can be retrieved via listxattr() is limited. Of course, the API is broken as documented on xattr(7) already. In the future we might want to address this but for now this is the world we live in and have lived for a long time. But it does indeed mean that once an application goes over XATTR_LIST_MAX limit of xattrs set on an inode it isn't possible to copy the file and include its xattrs in the copy unless the caller knows all xattrs or limits the copy of the xattrs to important ones it knows by name (At least for tmpfs, and kernfs-based filesystems. Other filesystems might provide ways of achieving this.). Bonus of this port to rbtree+rwlock is that we shrink the memory consumption for users of the simple xattr infrastructure. Also add proper kernel documentation to all the functions. A big thanks to Paul for his comments. Cc: Vasily Averin Cc: "Paul E. McKenney" Acked-by: Roman Gushchin Acked-by: Paul E. McKenney Signed-off-by: Christian Brauner (Microsoft) --- fs/xattr.c | 317 +++++++++++++++++++++++++++++++++--------- include/linux/xattr.h | 38 ++--- mm/shmem.c | 2 +- 3 files changed, 260 insertions(+), 97 deletions(-) diff --git a/fs/xattr.c b/fs/xattr.c index 61107b6bbed2..402d9d43fde0 100644 --- a/fs/xattr.c +++ b/fs/xattr.c @@ -992,8 +992,29 @@ const char *xattr_full_name(const struct xattr_handler *handler, } EXPORT_SYMBOL(xattr_full_name); -/* - * Allocate new xattr and copy in the value; but leave the name to callers. +/** + * free_simple_xattr - free an xattr object + * @xattr: the xattr object + * + * Free the xattr object. Can handle @xattr being NULL. + */ +static inline void free_simple_xattr(struct simple_xattr *xattr) +{ + if (xattr) + kfree(xattr->name); + kvfree(xattr); +} + +/** + * simple_xattr_alloc - allocate new xattr object + * @value: value of the xattr object + * @size: size of @value + * + * Allocate a new xattr object and initialize respective members. The caller is + * responsible for handling the name of the xattr. + * + * Return: On success a new xattr object is returned. On failure NULL is + * returned. */ struct simple_xattr *simple_xattr_alloc(const void *value, size_t size) { @@ -1014,20 +1035,69 @@ struct simple_xattr *simple_xattr_alloc(const void *value, size_t size) return new_xattr; } -/* - * xattr GET operation for in-memory/pseudo filesystems +/** + * rbtree_simple_xattr_cmp - compare xattr name with current rbtree xattr entry + * @key: xattr name + * @node: current node + * + * Compare the xattr name with the xattr name attached to @node in the rbtree. + * + * Return: Negative value if continuing left, positive if continuing right, 0 + * if the xattr attached to @node matches @key. + */ +static int rbtree_simple_xattr_cmp(const void *key, const struct rb_node *node) +{ + const char *xattr_name = key; + const struct simple_xattr *xattr; + + xattr = rb_entry(node, struct simple_xattr, rb_node); + return strcmp(xattr->name, xattr_name); +} + +/** + * rbtree_simple_xattr_node_cmp - compare two xattr rbtree nodes + * @new_node: new node + * @node: current node + * + * Compare the xattr attached to @new_node with the xattr attached to @node. + * + * Return: Negative value if continuing left, positive if continuing right, 0 + * if the xattr attached to @new_node matches the xattr attached to @node. + */ +static int rbtree_simple_xattr_node_cmp(struct rb_node *new_node, + const struct rb_node *node) +{ + struct simple_xattr *xattr; + xattr = rb_entry(new_node, struct simple_xattr, rb_node); + return rbtree_simple_xattr_cmp(xattr->name, node); +} + +/** + * simple_xattr_get - get an xattr object + * @xattrs: the header of the xattr object + * @name: the name of the xattr to retrieve + * @buffer: the buffer to store the value into + * @size: the size of @buffer + * + * Try to find and retrieve the xattr object associated with @name. + * If @buffer is provided store the value of @xattr in @buffer + * otherwise just return the length. The size of @buffer is limited + * to XATTR_SIZE_MAX which currently is 65536. + * + * Return: On success the length of the xattr value is returned. On error a + * negative error code is returned. */ int simple_xattr_get(struct simple_xattrs *xattrs, const char *name, void *buffer, size_t size) { - struct simple_xattr *xattr; + struct simple_xattr *xattr = NULL; + struct rb_node *rbp; int ret = -ENODATA; - spin_lock(&xattrs->lock); - list_for_each_entry(xattr, &xattrs->head, list) { - if (strcmp(name, xattr->name)) - continue; - + read_lock(&xattrs->lock); + rbp = rb_find(name, &xattrs->rb_root, rbtree_simple_xattr_cmp); + if (rbp) { + xattr = rb_entry(rbp, struct simple_xattr, rb_node); ret = xattr->size; if (buffer) { if (size < xattr->size) @@ -1035,34 +1105,44 @@ int simple_xattr_get(struct simple_xattrs *xattrs, const char *name, else memcpy(buffer, xattr->value, xattr->size); } - break; } - spin_unlock(&xattrs->lock); + read_unlock(&xattrs->lock); return ret; } /** - * simple_xattr_set - xattr SET operation for in-memory/pseudo filesystems - * @xattrs: target simple_xattr list - * @name: name of the extended attribute - * @value: value of the xattr. If %NULL, will remove the attribute. - * @size: size of the new xattr - * @flags: %XATTR_{CREATE|REPLACE} - * @removed_size: returns size of the removed xattr, -1 if none removed + * simple_xattr_set - set an xattr object + * @xattrs: the header of the xattr object + * @name: the name of the xattr to retrieve + * @value: the value to store along the xattr + * @size: the size of @value + * @flags: the flags determining how to set the xattr + * @removed_size: the size of the removed xattr * - * %XATTR_CREATE is set, the xattr shouldn't exist already; otherwise fails - * with -EEXIST. If %XATTR_REPLACE is set, the xattr should exist; - * otherwise, fails with -ENODATA. + * Set a new xattr object. + * If @value is passed a new xattr object will be allocated. If XATTR_REPLACE + * is specified in @flags a matching xattr object for @name must already exist. + * If it does it will be replaced with the new xattr object. If it doesn't we + * fail. If XATTR_CREATE is specified and a matching xattr does already exist + * we fail. If it doesn't we create a new xattr. If @flags is zero we simply + * insert the new xattr replacing any existing one. * - * Returns 0 on success, -errno on failure. + * If @value is empty and a matching xattr object is found we delete it if + * XATTR_REPLACE is specified in @flags or @flags is zero. + * + * If @value is empty and no matching xattr object for @name is found we do + * nothing if XATTR_CREATE is specified in @flags or @flags is zero. For + * XATTR_REPLACE we fail as mentioned above. + * + * Return: On success zero and on error a negative error code is returned. */ int simple_xattr_set(struct simple_xattrs *xattrs, const char *name, const void *value, size_t size, int flags, ssize_t *removed_size) { - struct simple_xattr *xattr; - struct simple_xattr *new_xattr = NULL; - int err = 0; + struct simple_xattr *xattr = NULL, *new_xattr = NULL; + struct rb_node *parent = NULL, **rbp; + int err = 0, ret; if (removed_size) *removed_size = -1; @@ -1075,42 +1155,68 @@ int simple_xattr_set(struct simple_xattrs *xattrs, const char *name, new_xattr->name = kstrdup(name, GFP_KERNEL); if (!new_xattr->name) { - kvfree(new_xattr); + free_simple_xattr(new_xattr); return -ENOMEM; } } - spin_lock(&xattrs->lock); - list_for_each_entry(xattr, &xattrs->head, list) { - if (!strcmp(name, xattr->name)) { - if (flags & XATTR_CREATE) { - xattr = new_xattr; - err = -EEXIST; - } else if (new_xattr) { - list_replace(&xattr->list, &new_xattr->list); - if (removed_size) - *removed_size = xattr->size; - } else { - list_del(&xattr->list); - if (removed_size) - *removed_size = xattr->size; - } - goto out; - } + write_lock(&xattrs->lock); + rbp = &xattrs->rb_root.rb_node; + while (*rbp) { + parent = *rbp; + ret = rbtree_simple_xattr_cmp(name, *rbp); + if (ret < 0) + rbp = &(*rbp)->rb_left; + else if (ret > 0) + rbp = &(*rbp)->rb_right; + else + xattr = rb_entry(*rbp, struct simple_xattr, rb_node); + if (xattr) + break; } - if (flags & XATTR_REPLACE) { - xattr = new_xattr; - err = -ENODATA; - } else { - list_add(&new_xattr->list, &xattrs->head); - xattr = NULL; - } -out: - spin_unlock(&xattrs->lock); + if (xattr) { - kfree(xattr->name); - kvfree(xattr); + /* Fail if XATTR_CREATE is requested and the xattr exists. */ + if (flags & XATTR_CREATE) { + err = -EEXIST; + goto out_unlock; + } + + if (new_xattr) + rb_replace_node(&xattr->rb_node, &new_xattr->rb_node, + &xattrs->rb_root); + else + rb_erase(&xattr->rb_node, &xattrs->rb_root); + if (!err && removed_size) + *removed_size = xattr->size; + } else { + /* Fail if XATTR_REPLACE is requested but no xattr is found. */ + if (flags & XATTR_REPLACE) { + err = -ENODATA; + goto out_unlock; + } + + /* + * If XATTR_CREATE or no flags are specified together with a + * new value simply insert it. + */ + if (new_xattr) { + rb_link_node(&new_xattr->rb_node, parent, rbp); + rb_insert_color(&new_xattr->rb_node, &xattrs->rb_root); + } + + /* + * If XATTR_CREATE or no flags are specified and neither an + * old or new xattr exist then we don't need to do anything. + */ } + +out_unlock: + write_unlock(&xattrs->lock); + if (err) + free_simple_xattr(new_xattr); + else + free_simple_xattr(xattr); return err; } @@ -1134,14 +1240,31 @@ static int xattr_list_one(char **buffer, ssize_t *remaining_size, return 0; } -/* - * xattr LIST operation for in-memory/pseudo filesystems +/** + * simple_xattr_list - list all xattr objects + * @inode: inode from which to get the xattrs + * @xattrs: the header of the xattr object + * @buffer: the buffer to store all xattrs into + * @size: the size of @buffer + * + * List all xattrs associated with @inode. If @buffer is NULL we returned + * the required size of the buffer. If @buffer is provided we store the + * xattrs value into it provided it is big enough. + * + * Note, the number of xattr names that can be listed with listxattr(2) is + * limited to XATTR_LIST_MAX aka 65536 bytes. If a larger buffer is passed + * then vfs_listxattr() caps it to XATTR_LIST_MAX and if more xattr names + * are found it will return -E2BIG. + * + * Return: On success the required size or the size of the copied xattrs is + * returned. On error a negative error code is returned. */ ssize_t simple_xattr_list(struct inode *inode, struct simple_xattrs *xattrs, char *buffer, size_t size) { bool trusted = capable(CAP_SYS_ADMIN); struct simple_xattr *xattr; + struct rb_node *rbp; ssize_t remaining_size = size; int err = 0; @@ -1162,8 +1285,10 @@ ssize_t simple_xattr_list(struct inode *inode, struct simple_xattrs *xattrs, } #endif - spin_lock(&xattrs->lock); - list_for_each_entry(xattr, &xattrs->head, list) { + read_lock(&xattrs->lock); + for (rbp = rb_first(&xattrs->rb_root); rbp; rbp = rb_next(rbp)) { + xattr = rb_entry(rbp, struct simple_xattr, rb_node); + /* skip "trusted." attributes for unprivileged callers */ if (!trusted && xattr_is_trusted(xattr->name)) continue; @@ -1172,18 +1297,76 @@ ssize_t simple_xattr_list(struct inode *inode, struct simple_xattrs *xattrs, if (err) break; } - spin_unlock(&xattrs->lock); + read_unlock(&xattrs->lock); return err ? err : size - remaining_size; } -/* - * Adds an extended attribute to the list +/** + * rbtree_simple_xattr_less - compare two xattr rbtree nodes + * @new_node: new node + * @node: current node + * + * Compare the xattr attached to @new_node with the xattr attached to @node. + * Note that this function technically tolerates duplicate entries. + * + * Return: True if insertion point in the rbtree is found. */ -void simple_xattr_list_add(struct simple_xattrs *xattrs, - struct simple_xattr *new_xattr) +static bool rbtree_simple_xattr_less(struct rb_node *new_node, + const struct rb_node *node) { - spin_lock(&xattrs->lock); - list_add(&new_xattr->list, &xattrs->head); - spin_unlock(&xattrs->lock); + return rbtree_simple_xattr_node_cmp(new_node, node) < 0; +} + +/** + * simple_xattr_add - add xattr objects + * @xattrs: the header of the xattr object + * @new_xattr: the xattr object to add + * + * Add an xattr object to @xattrs. This assumes no replacement or removal + * of matching xattrs is wanted. Should only be called during inode + * initialization when a few distinct initial xattrs are supposed to be set. + */ +void simple_xattr_add(struct simple_xattrs *xattrs, + struct simple_xattr *new_xattr) +{ + write_lock(&xattrs->lock); + rb_add(&new_xattr->rb_node, &xattrs->rb_root, rbtree_simple_xattr_less); + write_unlock(&xattrs->lock); +} + +/** + * simple_xattrs_init - initialize new xattr header + * @xattrs: header to initialize + * + * Initialize relevant fields of a an xattr header. + */ +void simple_xattrs_init(struct simple_xattrs *xattrs) +{ + xattrs->rb_root = RB_ROOT; + rwlock_init(&xattrs->lock); +} + +/** + * simple_xattrs_free - free xattrs + * @xattrs: xattr header whose xattrs to destroy + * + * Destroy all xattrs in @xattr. When this is called no one can hold a + * reference to any of the xattrs anymore. + */ +void simple_xattrs_free(struct simple_xattrs *xattrs) +{ + struct rb_node *rbp; + + rbp = rb_first(&xattrs->rb_root); + while (rbp) { + struct simple_xattr *xattr; + struct rb_node *rbp_next; + + rbp_next = rb_next(rbp); + xattr = rb_entry(rbp, struct simple_xattr, rb_node); + rb_erase(&xattr->rb_node, &xattrs->rb_root); + free_simple_xattr(xattr); + rbp = rbp_next; + } } diff --git a/include/linux/xattr.h b/include/linux/xattr.h index 4c379d23ec6e..b559c6bbcad0 100644 --- a/include/linux/xattr.h +++ b/include/linux/xattr.h @@ -80,48 +80,28 @@ static inline const char *xattr_prefix(const struct xattr_handler *handler) } struct simple_xattrs { - struct list_head head; - spinlock_t lock; + struct rb_root rb_root; + rwlock_t lock; }; struct simple_xattr { - struct list_head list; + struct rb_node rb_node; char *name; size_t size; char value[]; }; -/* - * initialize the simple_xattrs structure - */ -static inline void simple_xattrs_init(struct simple_xattrs *xattrs) -{ - INIT_LIST_HEAD(&xattrs->head); - spin_lock_init(&xattrs->lock); -} - -/* - * free all the xattrs - */ -static inline void simple_xattrs_free(struct simple_xattrs *xattrs) -{ - struct simple_xattr *xattr, *node; - - list_for_each_entry_safe(xattr, node, &xattrs->head, list) { - kfree(xattr->name); - kvfree(xattr); - } -} - +void simple_xattrs_init(struct simple_xattrs *xattrs); +void simple_xattrs_free(struct simple_xattrs *xattrs); struct simple_xattr *simple_xattr_alloc(const void *value, size_t size); int simple_xattr_get(struct simple_xattrs *xattrs, const char *name, void *buffer, size_t size); int simple_xattr_set(struct simple_xattrs *xattrs, const char *name, const void *value, size_t size, int flags, ssize_t *removed_size); -ssize_t simple_xattr_list(struct inode *inode, struct simple_xattrs *xattrs, char *buffer, - size_t size); -void simple_xattr_list_add(struct simple_xattrs *xattrs, - struct simple_xattr *new_xattr); +ssize_t simple_xattr_list(struct inode *inode, struct simple_xattrs *xattrs, + char *buffer, size_t size); +void simple_xattr_add(struct simple_xattrs *xattrs, + struct simple_xattr *new_xattr); #endif /* _LINUX_XATTR_H */ diff --git a/mm/shmem.c b/mm/shmem.c index 8280a5cb48df..2872e6607b2c 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -3255,7 +3255,7 @@ static int shmem_initxattrs(struct inode *inode, memcpy(new_xattr->name + XATTR_SECURITY_PREFIX_LEN, xattr->name, len); - simple_xattr_list_add(&info->xattrs, new_xattr); + simple_xattr_add(&info->xattrs, new_xattr); } return 0; From 86ef3c735ec81566cd38174ed5e90e9721473064 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Tue, 1 Nov 2022 17:05:29 -0700 Subject: [PATCH 06/13] LSM: Better reporting of actual LSMs at boot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Enhance the details reported by "lsm.debug" in several ways: - report contents of "security=" - report contents of "CONFIG_LSM" - report contents of "lsm=" - report any early LSM details - whitespace-align the output of similar phases for easier visual parsing - change "disabled" to more accurate "skipped" - explain what "skipped" and "ignored" mean in a parenthetical Upgrade the "security= is ignored" warning from pr_info to pr_warn, and include full arguments list to make the cause even more clear. Replace static "Security Framework initializing" pr_info with specific list of the resulting order of enabled LSMs. For example, if the kernel is built with: CONFIG_SECURITY_SELINUX=y CONFIG_SECURITY_APPARMOR=y CONFIG_SECURITY_LOADPIN=y CONFIG_SECURITY_YAMA=y CONFIG_SECURITY_SAFESETID=y CONFIG_SECURITY_LOCKDOWN_LSM=y CONFIG_SECURITY_LANDLOCK=y CONFIG_INTEGRITY=y CONFIG_BPF_LSM=y CONFIG_DEFAULT_SECURITY_APPARMOR=y CONFIG_LSM="landlock,lockdown,yama,loadpin,safesetid,integrity,selinux, smack,tomoyo,apparmor,bpf" Booting without options will show: LSM: initializing lsm=lockdown,capability,landlock,yama,loadpin, safesetid,integrity,selinux,bpf landlock: Up and running. Yama: becoming mindful. LoadPin: ready to pin (currently not enforcing) SELinux: Initializing. LSM support for eBPF active Boot with "lsm.debug" will show: LSM: legacy security= *unspecified* LSM: CONFIG_LSM=landlock,lockdown,yama,loadpin,safesetid,integrity, selinux,smack,tomoyo,apparmor,bpf LSM: boot arg lsm= *unspecified* LSM: early started: lockdown (enabled) LSM: first ordered: capability (enabled) LSM: builtin ordered: landlock (enabled) LSM: builtin ignored: lockdown (not built into kernel) LSM: builtin ordered: yama (enabled) LSM: builtin ordered: loadpin (enabled) LSM: builtin ordered: safesetid (enabled) LSM: builtin ordered: integrity (enabled) LSM: builtin ordered: selinux (enabled) LSM: builtin ignored: smack (not built into kernel) LSM: builtin ignored: tomoyo (not built into kernel) LSM: builtin ordered: apparmor (enabled) LSM: builtin ordered: bpf (enabled) LSM: exclusive chosen: selinux LSM: exclusive disabled: apparmor LSM: initializing lsm=lockdown,capability,landlock,yama,loadpin, safesetid,integrity,selinux,bpf LSM: cred blob size = 32 LSM: file blob size = 16 LSM: inode blob size = 72 LSM: ipc blob size = 8 LSM: msg_msg blob size = 4 LSM: superblock blob size = 80 LSM: task blob size = 8 LSM: initializing capability LSM: initializing landlock landlock: Up and running. LSM: initializing yama Yama: becoming mindful. LSM: initializing loadpin LoadPin: ready to pin (currently not enforcing) LSM: initializing safesetid LSM: initializing integrity LSM: initializing selinux SELinux: Initializing. LSM: initializing bpf LSM support for eBPF active And some examples of how the lsm.debug ordering report changes... With "lsm.debug security=selinux": LSM: legacy security=selinux LSM: CONFIG_LSM=landlock,lockdown,yama,loadpin,safesetid,integrity, selinux,smack,tomoyo,apparmor,bpf LSM: boot arg lsm= *unspecified* LSM: early started: lockdown (enabled) LSM: first ordered: capability (enabled) LSM: security=selinux disabled: apparmor (only one legacy major LSM) LSM: builtin ordered: landlock (enabled) LSM: builtin ignored: lockdown (not built into kernel) LSM: builtin ordered: yama (enabled) LSM: builtin ordered: loadpin (enabled) LSM: builtin ordered: safesetid (enabled) LSM: builtin ordered: integrity (enabled) LSM: builtin ordered: selinux (enabled) LSM: builtin ignored: smack (not built into kernel) LSM: builtin ignored: tomoyo (not built into kernel) LSM: builtin ordered: apparmor (disabled) LSM: builtin ordered: bpf (enabled) LSM: exclusive chosen: selinux LSM: initializing lsm=lockdown,capability,landlock,yama,loadpin, safesetid,integrity,selinux,bpf With "lsm.debug lsm=integrity,selinux,loadpin,crabability,bpf, loadpin,loadpin": LSM: legacy security= *unspecified* LSM: CONFIG_LSM=landlock,lockdown,yama,loadpin,safesetid,integrity, selinux,smack,tomoyo,apparmor,bpf LSM: boot arg lsm=integrity,selinux,loadpin,capability,bpf,loadpin, loadpin LSM: early started: lockdown (enabled) LSM: first ordered: capability (enabled) LSM: cmdline ordered: integrity (enabled) LSM: cmdline ordered: selinux (enabled) LSM: cmdline ordered: loadpin (enabled) LSM: cmdline ignored: crabability (not built into kernel) LSM: cmdline ordered: bpf (enabled) LSM: cmdline skipped: apparmor (not in requested order) LSM: cmdline skipped: yama (not in requested order) LSM: cmdline skipped: safesetid (not in requested order) LSM: cmdline skipped: landlock (not in requested order) LSM: exclusive chosen: selinux LSM: initializing lsm=lockdown,capability,integrity,selinux,loadpin,bpf Cc: Paul Moore Cc: James Morris Cc: "Serge E. Hallyn" Cc: linux-security-module@vger.kernel.org Signed-off-by: Kees Cook Acked-by: Casey Schaufler Acked-by: Mickaël Salaün [PM: line wrapped commit description] Signed-off-by: Paul Moore --- security/security.c | 45 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 36 insertions(+), 9 deletions(-) diff --git a/security/security.c b/security/security.c index f27c885ee98d..d713d84d82d6 100644 --- a/security/security.c +++ b/security/security.c @@ -161,8 +161,8 @@ static void __init append_ordered_lsm(struct lsm_info *lsm, const char *from) lsm->enabled = &lsm_enabled_true; ordered_lsms[last_lsm++] = lsm; - init_debug("%s ordering: %s (%sabled)\n", from, lsm->name, - is_enabled(lsm) ? "en" : "dis"); + init_debug("%s ordered: %s (%s)\n", from, lsm->name, + is_enabled(lsm) ? "enabled" : "disabled"); } /* Is an LSM allowed to be initialized? */ @@ -224,7 +224,7 @@ static void __init prepare_lsm(struct lsm_info *lsm) if (enabled) { if ((lsm->flags & LSM_FLAG_EXCLUSIVE) && !exclusive) { exclusive = lsm; - init_debug("exclusive chosen: %s\n", lsm->name); + init_debug("exclusive chosen: %s\n", lsm->name); } lsm_set_blob_sizes(lsm->blobs); @@ -252,7 +252,7 @@ static void __init ordered_lsm_parse(const char *order, const char *origin) /* LSM_ORDER_FIRST is always first. */ for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) { if (lsm->order == LSM_ORDER_FIRST) - append_ordered_lsm(lsm, "first"); + append_ordered_lsm(lsm, " first"); } /* Process "security=", if given. */ @@ -270,7 +270,7 @@ static void __init ordered_lsm_parse(const char *order, const char *origin) if ((major->flags & LSM_FLAG_LEGACY_MAJOR) && strcmp(major->name, chosen_major_lsm) != 0) { set_enabled(major, false); - init_debug("security=%s disabled: %s\n", + init_debug("security=%s disabled: %s (only one legacy major LSM)\n", chosen_major_lsm, major->name); } } @@ -291,7 +291,8 @@ static void __init ordered_lsm_parse(const char *order, const char *origin) } if (!found) - init_debug("%s ignored: %s\n", origin, name); + init_debug("%s ignored: %s (not built into kernel)\n", + origin, name); } /* Process "security=", if given. */ @@ -309,7 +310,8 @@ static void __init ordered_lsm_parse(const char *order, const char *origin) if (exists_ordered_lsm(lsm)) continue; set_enabled(lsm, false); - init_debug("%s disabled: %s\n", origin, lsm->name); + init_debug("%s skipped: %s (not in requested order)\n", + origin, lsm->name); } kfree(sep); @@ -320,6 +322,24 @@ static void __init lsm_early_task(struct task_struct *task); static int lsm_append(const char *new, char **result); +static void __init report_lsm_order(void) +{ + struct lsm_info **lsm, *early; + int first = 0; + + pr_info("initializing lsm="); + + /* Report each enabled LSM name, comma separated. */ + for (early = __start_early_lsm_info; early < __end_early_lsm_info; early++) + if (is_enabled(early)) + pr_cont("%s%s", first++ == 0 ? "" : ",", early->name); + for (lsm = ordered_lsms; *lsm; lsm++) + if (is_enabled(*lsm)) + pr_cont("%s%s", first++ == 0 ? "" : ",", (*lsm)->name); + + pr_cont("\n"); +} + static void __init ordered_lsm_init(void) { struct lsm_info **lsm; @@ -329,7 +349,8 @@ static void __init ordered_lsm_init(void) if (chosen_lsm_order) { if (chosen_major_lsm) { - pr_info("security= is ignored because it is superseded by lsm=\n"); + pr_warn("security=%s is ignored because it is superseded by lsm=%s\n", + chosen_major_lsm, chosen_lsm_order); chosen_major_lsm = NULL; } ordered_lsm_parse(chosen_lsm_order, "cmdline"); @@ -339,6 +360,8 @@ static void __init ordered_lsm_init(void) for (lsm = ordered_lsms; *lsm; lsm++) prepare_lsm(*lsm); + report_lsm_order(); + init_debug("cred blob size = %d\n", blob_sizes.lbs_cred); init_debug("file blob size = %d\n", blob_sizes.lbs_file); init_debug("inode blob size = %d\n", blob_sizes.lbs_inode); @@ -395,13 +418,17 @@ int __init security_init(void) { struct lsm_info *lsm; - pr_info("Security Framework initializing\n"); + init_debug("legacy security=%s\n", chosen_major_lsm ?: " *unspecified*"); + init_debug(" CONFIG_LSM=%s\n", builtin_lsm_order); + init_debug("boot arg lsm=%s\n", chosen_lsm_order ?: " *unspecified*"); /* * Append the names of the early LSM modules now that kmalloc() is * available */ for (lsm = __start_early_lsm_info; lsm < __end_early_lsm_info; lsm++) { + init_debug(" early started: %s (%s)\n", lsm->name, + is_enabled(lsm) ? "enabled" : "disabled"); if (lsm->enabled) lsm_append(lsm->name, &lsm_names); } From e68bfbd3b3c3a0ec3cf8c230996ad8cabe90322f Mon Sep 17 00:00:00 2001 From: Wang Weiyang Date: Tue, 25 Oct 2022 19:31:01 +0800 Subject: [PATCH 07/13] device_cgroup: Roll back to original exceptions after copy failure When add the 'a *:* rwm' entry to devcgroup A's whitelist, at first A's exceptions will be cleaned and A's behavior is changed to DEVCG_DEFAULT_ALLOW. Then parent's exceptions will be copyed to A's whitelist. If copy failure occurs, just return leaving A to grant permissions to all devices. And A may grant more permissions than parent. Backup A's whitelist and recover original exceptions after copy failure. Cc: stable@vger.kernel.org Fixes: 4cef7299b478 ("device_cgroup: add proper checking when changing default behavior") Signed-off-by: Wang Weiyang Reviewed-by: Aristeu Rozanski Signed-off-by: Paul Moore --- security/device_cgroup.c | 35 ++++++++++++++++++++++++++++++----- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/security/device_cgroup.c b/security/device_cgroup.c index a9f8c63a96d1..bef2b9285fb3 100644 --- a/security/device_cgroup.c +++ b/security/device_cgroup.c @@ -82,6 +82,17 @@ free_and_exit: return -ENOMEM; } +static void dev_exceptions_move(struct list_head *dest, struct list_head *orig) +{ + struct dev_exception_item *ex, *tmp; + + lockdep_assert_held(&devcgroup_mutex); + + list_for_each_entry_safe(ex, tmp, orig, list) { + list_move_tail(&ex->list, dest); + } +} + /* * called under devcgroup_mutex */ @@ -604,11 +615,13 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup, int count, rc = 0; struct dev_exception_item ex; struct dev_cgroup *parent = css_to_devcgroup(devcgroup->css.parent); + struct dev_cgroup tmp_devcgrp; if (!capable(CAP_SYS_ADMIN)) return -EPERM; memset(&ex, 0, sizeof(ex)); + memset(&tmp_devcgrp, 0, sizeof(tmp_devcgrp)); b = buffer; switch (*b) { @@ -620,15 +633,27 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup, if (!may_allow_all(parent)) return -EPERM; - dev_exception_clean(devcgroup); - devcgroup->behavior = DEVCG_DEFAULT_ALLOW; - if (!parent) + if (!parent) { + devcgroup->behavior = DEVCG_DEFAULT_ALLOW; + dev_exception_clean(devcgroup); break; + } - rc = dev_exceptions_copy(&devcgroup->exceptions, - &parent->exceptions); + INIT_LIST_HEAD(&tmp_devcgrp.exceptions); + rc = dev_exceptions_copy(&tmp_devcgrp.exceptions, + &devcgroup->exceptions); if (rc) return rc; + dev_exception_clean(devcgroup); + rc = dev_exceptions_copy(&devcgroup->exceptions, + &parent->exceptions); + if (rc) { + dev_exceptions_move(&devcgroup->exceptions, + &tmp_devcgrp.exceptions); + return rc; + } + devcgroup->behavior = DEVCG_DEFAULT_ALLOW; + dev_exception_clean(&tmp_devcgrp); break; case DEVCG_DENY: if (css_has_online_children(&devcgroup->css)) From f6fbd8cbf3ed1915a7b957f2801f7c306a686c08 Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Wed, 9 Nov 2022 14:14:35 -0500 Subject: [PATCH 08/13] lsm,fs: fix vfs_getxattr_alloc() return type and caller error paths The vfs_getxattr_alloc() function currently returns a ssize_t value despite the fact that it only uses int values internally for return values. Fix this by converting vfs_getxattr_alloc() to return an int type and adjust the callers as necessary. As part of these caller modifications, some of the callers are fixed to properly free the xattr value buffer on both success and failure to ensure that memory is not leaked in the failure case. Reviewed-by: Serge Hallyn Reviewed-by: Mimi Zohar Signed-off-by: Paul Moore --- fs/xattr.c | 5 +++-- include/linux/xattr.h | 6 +++--- security/apparmor/domain.c | 3 +-- security/commoncap.c | 22 ++++++++++------------ security/integrity/evm/evm_crypto.c | 5 +++-- security/integrity/evm/evm_main.c | 7 +++++-- security/integrity/ima/ima.h | 5 +++-- security/integrity/ima/ima_appraise.c | 6 +++--- security/integrity/ima/ima_main.c | 6 ++++-- security/integrity/ima/ima_template_lib.c | 11 +++++------ 10 files changed, 40 insertions(+), 36 deletions(-) diff --git a/fs/xattr.c b/fs/xattr.c index 61107b6bbed2..f38fd969b5fc 100644 --- a/fs/xattr.c +++ b/fs/xattr.c @@ -354,11 +354,12 @@ out_noalloc: * vfs_getxattr_alloc - allocate memory, if necessary, before calling getxattr * * Allocate memory, if not already allocated, or re-allocate correct size, - * before retrieving the extended attribute. + * before retrieving the extended attribute. The xattr value buffer should + * always be freed by the caller, even on error. * * Returns the result of alloc, if failed, or the getxattr operation. */ -ssize_t +int vfs_getxattr_alloc(struct user_namespace *mnt_userns, struct dentry *dentry, const char *name, char **xattr_value, size_t xattr_size, gfp_t flags) diff --git a/include/linux/xattr.h b/include/linux/xattr.h index 4c379d23ec6e..2218a9645b89 100644 --- a/include/linux/xattr.h +++ b/include/linux/xattr.h @@ -68,9 +68,9 @@ int __vfs_removexattr_locked(struct user_namespace *, struct dentry *, int vfs_removexattr(struct user_namespace *, struct dentry *, const char *); ssize_t generic_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size); -ssize_t vfs_getxattr_alloc(struct user_namespace *mnt_userns, - struct dentry *dentry, const char *name, - char **xattr_value, size_t size, gfp_t flags); +int vfs_getxattr_alloc(struct user_namespace *mnt_userns, + struct dentry *dentry, const char *name, + char **xattr_value, size_t size, gfp_t flags); int xattr_supported_namespace(struct inode *inode, const char *prefix); diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c index 91689d34d281..04a818d51604 100644 --- a/security/apparmor/domain.c +++ b/security/apparmor/domain.c @@ -311,10 +311,9 @@ static int aa_xattrs_match(const struct linux_binprm *bprm, struct aa_profile *profile, unsigned int state) { int i; - ssize_t size; struct dentry *d; char *value = NULL; - int value_size = 0, ret = profile->xattr_count; + int size, value_size = 0, ret = profile->xattr_count; if (!bprm || !profile->xattr_count) return 0; diff --git a/security/commoncap.c b/security/commoncap.c index 5fc8986c3c77..d4fc89095513 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -350,14 +350,14 @@ static __u32 sansflags(__u32 m) return m & ~VFS_CAP_FLAGS_EFFECTIVE; } -static bool is_v2header(size_t size, const struct vfs_cap_data *cap) +static bool is_v2header(int size, const struct vfs_cap_data *cap) { if (size != XATTR_CAPS_SZ_2) return false; return sansflags(le32_to_cpu(cap->magic_etc)) == VFS_CAP_REVISION_2; } -static bool is_v3header(size_t size, const struct vfs_cap_data *cap) +static bool is_v3header(int size, const struct vfs_cap_data *cap) { if (size != XATTR_CAPS_SZ_3) return false; @@ -379,7 +379,7 @@ int cap_inode_getsecurity(struct user_namespace *mnt_userns, struct inode *inode, const char *name, void **buffer, bool alloc) { - int size, ret; + int size; kuid_t kroot; u32 nsmagic, magic; uid_t root, mappedroot; @@ -395,20 +395,18 @@ int cap_inode_getsecurity(struct user_namespace *mnt_userns, dentry = d_find_any_alias(inode); if (!dentry) return -EINVAL; - - size = sizeof(struct vfs_ns_cap_data); - ret = (int)vfs_getxattr_alloc(mnt_userns, dentry, XATTR_NAME_CAPS, - &tmpbuf, size, GFP_NOFS); + size = vfs_getxattr_alloc(mnt_userns, dentry, XATTR_NAME_CAPS, &tmpbuf, + sizeof(struct vfs_ns_cap_data), GFP_NOFS); dput(dentry); - - if (ret < 0 || !tmpbuf) - return ret; + /* gcc11 complains if we don't check for !tmpbuf */ + if (size < 0 || !tmpbuf) + goto out_free; fs_ns = inode->i_sb->s_user_ns; cap = (struct vfs_cap_data *) tmpbuf; - if (is_v2header((size_t) ret, cap)) { + if (is_v2header(size, cap)) { root = 0; - } else if (is_v3header((size_t) ret, cap)) { + } else if (is_v3header(size, cap)) { nscap = (struct vfs_ns_cap_data *) tmpbuf; root = le32_to_cpu(nscap->rootid); } else { diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c index 708de9656bbd..fa5ff13fa8c9 100644 --- a/security/integrity/evm/evm_crypto.c +++ b/security/integrity/evm/evm_crypto.c @@ -335,14 +335,15 @@ static int evm_is_immutable(struct dentry *dentry, struct inode *inode) (char **)&xattr_data, 0, GFP_NOFS); if (rc <= 0) { if (rc == -ENODATA) - return 0; - return rc; + rc = 0; + goto out; } if (xattr_data->type == EVM_XATTR_PORTABLE_DIGSIG) rc = 1; else rc = 0; +out: kfree(xattr_data); return rc; } diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 23d484e05e6f..bce72e80fd12 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -519,14 +519,17 @@ static int evm_xattr_change(struct user_namespace *mnt_userns, rc = vfs_getxattr_alloc(&init_user_ns, dentry, xattr_name, &xattr_data, 0, GFP_NOFS); - if (rc < 0) - return 1; + if (rc < 0) { + rc = 1; + goto out; + } if (rc == xattr_value_len) rc = !!memcmp(xattr_value, xattr_data, rc); else rc = 1; +out: kfree(xattr_data); return rc; } diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index be965a8715e4..03b440921e61 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -326,7 +326,7 @@ enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint, enum hash_algo ima_get_hash_algo(const struct evm_ima_xattr_data *xattr_value, int xattr_len); int ima_read_xattr(struct dentry *dentry, - struct evm_ima_xattr_data **xattr_value); + struct evm_ima_xattr_data **xattr_value, int xattr_len); #else static inline int ima_check_blacklist(struct integrity_iint_cache *iint, @@ -372,7 +372,8 @@ ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value, int xattr_len) } static inline int ima_read_xattr(struct dentry *dentry, - struct evm_ima_xattr_data **xattr_value) + struct evm_ima_xattr_data **xattr_value, + int xattr_len) { return 0; } diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 3e0fbbd99534..88ffb15ca0e2 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -221,12 +221,12 @@ enum hash_algo ima_get_hash_algo(const struct evm_ima_xattr_data *xattr_value, } int ima_read_xattr(struct dentry *dentry, - struct evm_ima_xattr_data **xattr_value) + struct evm_ima_xattr_data **xattr_value, int xattr_len) { - ssize_t ret; + int ret; ret = vfs_getxattr_alloc(&init_user_ns, dentry, XATTR_NAME_IMA, - (char **)xattr_value, 0, GFP_NOFS); + (char **)xattr_value, xattr_len, GFP_NOFS); if (ret == -EOPNOTSUPP) ret = 0; return ret; diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 040b03ddc1c7..0226899947d6 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -293,7 +293,8 @@ static int process_measurement(struct file *file, const struct cred *cred, /* HASH sets the digital signature and update flags, nothing else */ if ((action & IMA_HASH) && !(test_bit(IMA_DIGSIG, &iint->atomic_flags))) { - xattr_len = ima_read_xattr(file_dentry(file), &xattr_value); + xattr_len = ima_read_xattr(file_dentry(file), + &xattr_value, xattr_len); if ((xattr_value && xattr_len > 2) && (xattr_value->type == EVM_IMA_XATTR_DIGSIG)) set_bit(IMA_DIGSIG, &iint->atomic_flags); @@ -316,7 +317,8 @@ static int process_measurement(struct file *file, const struct cred *cred, if ((action & IMA_APPRAISE_SUBMASK) || strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0) { /* read 'security.ima' */ - xattr_len = ima_read_xattr(file_dentry(file), &xattr_value); + xattr_len = ima_read_xattr(file_dentry(file), + &xattr_value, xattr_len); /* * Read the appended modsig if allowed by the policy, and allow diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c index 7bf9b1507220..4564faae7d67 100644 --- a/security/integrity/ima/ima_template_lib.c +++ b/security/integrity/ima/ima_template_lib.c @@ -601,16 +601,15 @@ int ima_eventevmsig_init(struct ima_event_data *event_data, rc = vfs_getxattr_alloc(&init_user_ns, file_dentry(event_data->file), XATTR_NAME_EVM, (char **)&xattr_data, 0, GFP_NOFS); - if (rc <= 0) - return 0; - - if (xattr_data->type != EVM_XATTR_PORTABLE_DIGSIG) { - kfree(xattr_data); - return 0; + if (rc <= 0 || xattr_data->type != EVM_XATTR_PORTABLE_DIGSIG) { + rc = 0; + goto out; } rc = ima_write_template_field_data((char *)xattr_data, rc, DATA_FMT_HEX, field_data); + +out: kfree(xattr_data); return rc; } From 572302af1258459e124437b8f3369357447afac7 Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Thu, 10 Nov 2022 10:46:35 +0100 Subject: [PATCH 09/13] reiserfs: Add missing calls to reiserfs_security_free() Commit 57fe60df6241 ("reiserfs: add atomic addition of selinux attributes during inode creation") defined reiserfs_security_free() to free the name and value of a security xattr allocated by the active LSM through security_old_inode_init_security(). However, this function is not called in the reiserfs code. Thus, add a call to reiserfs_security_free() whenever reiserfs_security_init() is called, and initialize value to NULL, to avoid to call kfree() on an uninitialized pointer. Finally, remove the kfree() for the xattr name, as it is not allocated anymore. Fixes: 57fe60df6241 ("reiserfs: add atomic addition of selinux attributes during inode creation") Cc: stable@vger.kernel.org Cc: Jeff Mahoney Cc: Tetsuo Handa Reported-by: Mimi Zohar Reported-by: Tetsuo Handa Signed-off-by: Roberto Sassu Reviewed-by: Mimi Zohar Signed-off-by: Paul Moore --- fs/reiserfs/namei.c | 4 ++++ fs/reiserfs/xattr_security.c | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/fs/reiserfs/namei.c b/fs/reiserfs/namei.c index 3d7a35d6a18b..b916859992ec 100644 --- a/fs/reiserfs/namei.c +++ b/fs/reiserfs/namei.c @@ -696,6 +696,7 @@ static int reiserfs_create(struct user_namespace *mnt_userns, struct inode *dir, out_failed: reiserfs_write_unlock(dir->i_sb); + reiserfs_security_free(&security); return retval; } @@ -779,6 +780,7 @@ static int reiserfs_mknod(struct user_namespace *mnt_userns, struct inode *dir, out_failed: reiserfs_write_unlock(dir->i_sb); + reiserfs_security_free(&security); return retval; } @@ -878,6 +880,7 @@ static int reiserfs_mkdir(struct user_namespace *mnt_userns, struct inode *dir, retval = journal_end(&th); out_failed: reiserfs_write_unlock(dir->i_sb); + reiserfs_security_free(&security); return retval; } @@ -1194,6 +1197,7 @@ static int reiserfs_symlink(struct user_namespace *mnt_userns, retval = journal_end(&th); out_failed: reiserfs_write_unlock(parent_dir->i_sb); + reiserfs_security_free(&security); return retval; } diff --git a/fs/reiserfs/xattr_security.c b/fs/reiserfs/xattr_security.c index 8965c8e5e172..857a65b05726 100644 --- a/fs/reiserfs/xattr_security.c +++ b/fs/reiserfs/xattr_security.c @@ -50,6 +50,7 @@ int reiserfs_security_init(struct inode *dir, struct inode *inode, int error; sec->name = NULL; + sec->value = NULL; /* Don't add selinux attributes on xattrs - they'll never get used */ if (IS_PRIVATE(dir)) @@ -95,7 +96,6 @@ int reiserfs_security_write(struct reiserfs_transaction_handle *th, void reiserfs_security_free(struct reiserfs_security_handle *sec) { - kfree(sec->name); kfree(sec->value); sec->name = NULL; sec->value = NULL; From a79168a0c00d710420c1758f6c38df89e12f0763 Mon Sep 17 00:00:00 2001 From: Brian Foster Date: Wed, 30 Nov 2022 08:41:01 -0800 Subject: [PATCH 10/13] fs/remap_range: avoid spurious writeback on zero length request generic_remap_checks() can reduce the effective request length (i.e., after the reflink extend to EOF case is handled) down to zero. If this occurs, __generic_remap_file_range_prep() proceeds through dio serialization, file mapping flush calls, and may invoke file_modified() before returning back to the filesystem caller, all of which immediately check for len == 0 and return. While this is mostly harmless, it is spurious and not completely without side effect. A filemap write call can submit I/O (but not wait on it) when the specified end byte precedes the start but happens to land on the same aligned page boundary, which can occur from __generic_remap_file_range_prep() when len is 0. The dedupe path already has a len == 0 check to break out before doing range comparisons. Lift this check a bit earlier in the function to cover the general case of len == 0 and avoid the unnecessary work. While here, account for the case where generic_remap_check_len() may also reduce length to zero. Signed-off-by: Brian Foster Reviewed-by: Darrick J. Wong Signed-off-by: Darrick J. Wong --- fs/remap_range.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/fs/remap_range.c b/fs/remap_range.c index 654912d06862..8741ecd616a7 100644 --- a/fs/remap_range.c +++ b/fs/remap_range.c @@ -304,7 +304,7 @@ __generic_remap_file_range_prep(struct file *file_in, loff_t pos_in, /* Check that we don't violate system file offset limits. */ ret = generic_remap_checks(file_in, pos_in, file_out, pos_out, len, remap_flags); - if (ret) + if (ret || *len == 0) return ret; /* Wait for the completion of any pending IOs on both files */ @@ -328,9 +328,6 @@ __generic_remap_file_range_prep(struct file *file_in, loff_t pos_in, if (remap_flags & REMAP_FILE_DEDUP) { bool is_same = false; - if (*len == 0) - return 0; - if (!IS_DAX(inode_in)) ret = vfs_dedupe_file_range_compare(file_in, pos_in, file_out, pos_out, *len, &is_same); @@ -348,7 +345,7 @@ __generic_remap_file_range_prep(struct file *file_in, loff_t pos_in, ret = generic_remap_check_len(inode_in, inode_out, pos_out, len, remap_flags); - if (ret) + if (ret || *len == 0) return ret; /* If can't alter the file contents, we're done. */ From 361dd63ed5f1103ff21e22408a142a7d40804527 Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Mon, 28 Nov 2022 15:42:39 +0100 Subject: [PATCH 11/13] lsm: Clarify documentation of vm_enough_memory hook 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 if all LSMs return a positive value __vm_enough_memory() will be called with cap_sys_admin set. If at least one LSM returns 0 or negative, it will be called with cap_sys_admin cleared. Signed-off-by: Roberto Sassu Signed-off-by: Paul Moore --- include/linux/lsm_hooks.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 2831efebde69..c35e260efd8c 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -1398,7 +1398,11 @@ * 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 the LSM infrastructure to the + * caller. If all LSMs return a positive value, __vm_enough_memory() will + * be called with cap_sys_admin set. If at least one LSM returns 0 or + * negative, __vm_enough_memory() will be called with cap_sys_admin + * cleared. * * @ismaclabel: * Check if the extended attribute specified by @name From 8eb687bc806932fc65de4cf60c4ecf913182231d Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Mon, 28 Nov 2022 15:42:40 +0100 Subject: [PATCH 12/13] lsm: Add/fix return values in lsm_hooks.h and fix formatting 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, adjust semicolons and add the period at the end of the parameter description. Finally, move the description of gfp parameter of the xfrm_policy_alloc_security hook together with the others. Signed-off-by: Roberto Sassu [PM: /replaces./replaced./] Signed-off-by: Paul Moore --- include/linux/lsm_hooks.h | 221 ++++++++++++++++++++++++-------------- 1 file changed, 138 insertions(+), 83 deletions(-) diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index c35e260efd8c..bad3b6baad86 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -92,13 +92,17 @@ * 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 * should return 0; otherwise it should return -ENOPARAM to pass it on to * the filesystem. * @fc indicates the filesystem context. - * @param The parameter + * @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 on success, negative values on failure. * @sb_statfs: * Check permission before obtaining filesystem statistics for the @mnt * mountpoint. @@ -139,19 +144,21 @@ * @sb_mnt_opts_compat: * Determine if the new mount options in @mnt_opts are allowed given * the existing mounted filesystem at @sb. - * @sb superblock being compared - * @mnt_opts new mount options + * @sb superblock being compared. + * @mnt_opts new mount options. * Return 0 if options are compatible. * @sb_remount: * Extracts security system specific mount options and verifies no changes * are being made to those options. - * @sb superblock being remounted + * @sb superblock being remounted. * @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 on success, negative values on failure. * @sb_umount: * Check permission before the @mnt file system is unmounted. * @mnt contains the mounted file system. @@ -165,27 +172,31 @@ * Return 0 if permission is granted. * @sb_set_mnt_opts: * 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 + * @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 + * @oldsb old superblock which contain information to clone. + * @newsb new superblock which needs filled in. + * Return 0 on success, error on failure. * @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. * @dentry dentry to use in calculating the context. * @mode mode used to determine resource type. - * @name name of the last path component used to create file + * @name name of the last path component used to create file. * @xattr_name pointer to place the pointer to security xattr name. * Caller does not have to free the resulting pointer. Its * 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 on success, negative values on failure. * @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 @@ -193,9 +204,10 @@ * passed in creds and not the creds of the caller. * @dentry dentry to use in calculating the context. * @mode mode used to determine resource type. - * @name name of the last path component used to create file - * @old creds which should be used for context calculation - * @new creds to modify + * @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 on success, error on failure. * * * Security hooks for inode operations. @@ -223,7 +235,7 @@ * then it should return -EOPNOTSUPP to skip this processing. * @inode contains the inode structure of the newly created inode. * @dir contains the inode structure of the parent directory. - * @qstr contains the last path component of the new object + * @qstr contains the last path component of the new object. * @name will be set to the allocated name suffix (e.g. selinux). * @value will be set to the allocated attribute value. * @len will be set to the length of the value. @@ -234,9 +246,9 @@ * Set up the incore security field for the new anonymous inode * and return whether the inode creation is permitted by the security * module or not. - * @inode contains the inode structure - * @name name of the anonymous inode class - * @context_inode optional related inode + * @inode contains the inode structure. + * @name name of the anonymous inode class. + * @context_inode optional related inode. * Returns 0 on success, -EACCES if the security module denies the * creation of this inode, or another -errno upon other errors. * @inode_create: @@ -352,7 +364,7 @@ * mode is specified in @mode. * @path contains the path structure of the file to change the mode. * @mode contains the new DAC's permission, which is a bitmask of - * constants from + * constants from . * Return 0 if permission is granted. * @path_chown: * Check for permission to change owner/group of a file or directory. @@ -367,6 +379,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. @@ -374,7 +387,7 @@ * @inode_follow_link: * Check permission to follow a symbolic link when looking up a pathname. * @dentry contains the dentry structure for the link. - * @inode contains the inode, which itself is not stable in RCU-walk + * @inode contains the inode, which itself is not stable in RCU-walk. * @rcu indicates whether we are in RCU-walk mode. * Return 0 if permission is granted. * @inode_permission: @@ -426,9 +439,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 @@ -451,7 +464,7 @@ * @inode_killpriv: * The setuid bit is being removed. Remove similar security labels. * Called with the dentry->d_inode->i_mutex held. - * @mnt_userns: user namespace of the mount + * @mnt_userns: user namespace of the mount. * @dentry is the dentry being changed. * Return 0 on success. If error is returned, then the operation * causing setuid bit removal is failed. @@ -478,20 +491,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 + * @kn_dir the parent kernfs node. + * @kn the new child kernfs node. + * Return 0 if permission is granted. * * Security hooks for file operations * @@ -530,11 +545,11 @@ * simple integer value. When @arg represents a user space pointer, it * should never be used by the security module. * Return 0 if permission is granted. - * @mmap_addr : + * @mmap_addr: * Check permissions for a mmap operation at @addr. * @addr contains virtual address that will be used for the operation. * Return 0 if permission is granted. - * @mmap_file : + * @mmap_file: * Check permissions for a mmap operation. The @file may be NULL, e.g. * if mapping anonymous memory. * @file contains the file structure for file to map (may be NULL). @@ -589,6 +604,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. * @@ -606,6 +622,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. @@ -614,6 +631,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. @@ -625,7 +643,7 @@ * @kernel_act_as: * Set the credentials for a kernel service to act as (subjective context). * @new points to the credentials to be modified. - * @secid specifies the security ID to be set + * @secid specifies the security ID to be set. * The current task must be the one that nominated @secid. * Return 0 if successful. * @kernel_create_files_as: @@ -638,19 +656,19 @@ * @kernel_module_request: * Ability to trigger the kernel to automatically upcall to userspace for * userspace to load a kernel module with the given name. - * @kmod_name name of the module requested by the kernel + * @kmod_name name of the module requested by the kernel. * Return 0 if successful. * @kernel_load_data: * Load data provided by userspace. - * @id kernel load data identifier + * @id kernel load data identifier. * @contents if a subsequent @kernel_post_load_data will be called. * Return 0 if permission is granted. * @kernel_post_load_data: * Load data provided by a non-file source (usually userspace buffer). * @buf pointer to buffer containing the data contents. * @size length of the data contents. - * @id kernel load data identifier - * @description a text description of what was loaded, @id-specific + * @id kernel load data identifier. + * @description a text description of what was loaded, @id-specific. * Return 0 if permission is granted. * This must be paired with a prior @kernel_load_data call that had * @contents set to true. @@ -658,7 +676,7 @@ * Read a file specified by userspace. * @file contains the file structure pointing to the file being read * by the kernel. - * @id kernel read file identifier + * @id kernel read file identifier. * @contents if a subsequent @kernel_post_read_file will be called. * Return 0 if permission is granted. * @kernel_post_read_file: @@ -667,7 +685,7 @@ * by the kernel. * @buf pointer to buffer containing the file contents. * @size length of the file contents. - * @id kernel read file identifier + * @id kernel read file identifier. * This must be paired with a prior @kernel_read_file call that had * @contents set to true. * Return 0 if permission is granted. @@ -677,7 +695,7 @@ * indicates which of the set*uid system calls invoked this hook. If * @new is the set of credentials that will be installed. Modifications * should be made to this rather than to @current->cred. - * @old is the set of credentials that are being replaces + * @old is the set of credentials that are being replaced. * @flags contains one of the LSM_SETID_* values. * Return 0 on success. * @task_fix_setgid: @@ -729,7 +747,7 @@ * @task_setioprio: * Check permission before setting the ioprio value of @p to @ioprio. * @p contains the task_struct of process. - * @ioprio contains the new ioprio value + * @ioprio contains the new ioprio value. * Return 0 if permission is granted. * @task_getioprio: * Check permission before getting the ioprio value of @p. @@ -860,6 +878,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. @@ -943,6 +962,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 @@ -970,6 +990,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: @@ -982,17 +1003,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: @@ -1003,21 +1026,25 @@ * @tun_dev_free_security: * This hook allows a module to free the security structure for a TUN * device. - * @security pointer to the TUN device's security structure + * @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 * @@ -1050,6 +1077,7 @@ * to the security module. * @asoc pointer to sctp association structure. * @skb pointer to skbuff of association packet. + * Return 0 if permission is granted. * * Security hooks for Infiniband * @@ -1058,15 +1086,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. @@ -1078,10 +1108,11 @@ * Database used by the XFRM system. * @sec_ctx contains the security context information being provided by * the user-level policy update program (e.g., setkey). + * @gfp is to specify the context for the allocation. * Allocate a security structure to the xp->security field; the security * field is initialized to NULL when the xfrm_policy is allocated. - * Return 0 if operation was successful (memory to allocate, legal context) - * @gfp is to specify the context for the allocation + * Return 0 if operation was successful (memory to allocate, legal + * context). * @xfrm_policy_clone_security: * @old_ctx contains an existing xfrm_sec_ctx. * @new_ctxp contains a new xfrm_sec_ctx being cloned from old. @@ -1089,11 +1120,12 @@ * information from the old_ctx structure. * Return 0 if operation was successful (memory to allocate). * @xfrm_policy_free_security: - * @ctx contains the xfrm_sec_ctx + * @ctx contains the xfrm_sec_ctx. * Deallocate xp->security. * @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. @@ -1119,6 +1151,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. @@ -1147,7 +1180,7 @@ * Permit allocation of a key and assign security data. Note that key does * not have a serial number assigned at this point. * @key points to the key. - * @flags is the allocation flags + * @flags is the allocation flags. * Return 0 if permission is granted, -ve error otherwise. * @key_free: * Notification of destruction; free security data. @@ -1177,8 +1210,8 @@ * * @ipc_permission: * Check permissions for access to IPC - * @ipcp contains the kernel IPC permission structure - * @flag contains the desired (requested) permission set + * @ipcp contains the kernel IPC permission structure. + * @flag contains the desired (requested) permission set. * Return 0 if permission is granted. * @ipc_getsecid: * Get the secid associated with the ipc object. @@ -1323,15 +1356,18 @@ * to @to. * @from contains the struct cred for the sending process. * @to contains the struct cred for the receiving process. + * Return 0 if permission is granted. * @binder_transfer_binder: * Check whether @from is allowed to transfer a binder reference to @to. * @from contains the struct cred for the sending process. * @to contains the struct cred for the receiving process. + * Return 0 if permission is granted. * @binder_transfer_file: * Check whether @from is allowed to transfer @file to @to. * @from contains the struct cred for the sending process. * @file contains the struct file being transferred. * @to contains the struct cred for the receiving process. + * Return 0 if permission is granted. * * @ptrace_access_check: * Check permission before allowing the current process to trace the @@ -1373,26 +1409,29 @@ * Check whether the @tsk process has the @cap capability in the indicated * credentials. * @cred contains the credentials to use. - * @ns contains the user namespace we want the capability in + * @ns contains the user namespace we want the capability in. * @cap contains the capability . - * @opts contains options for the capable check + * @opts contains options for the capable check . * Return 0 if the capability is granted for @tsk. * @quotactl: - * Check whether the quotactl syscall is allowed for this @sb. + * Check whether the quotactl syscall is allowed for this @sb. + * Return 0 if permission is granted. * @quota_on: - * Check whether QUOTAON is allowed for this @dentry. + * Check whether QUOTAON is allowed for this @dentry. + * Return 0 if permission is granted. * @syslog: * Check permission before accessing the kernel message ring or changing * logging to the console. * See the syslog(2) manual page for an explanation of the @type values. - * @type contains the SYSLOG_ACTION_* constant from + * @type contains the SYSLOG_ACTION_* constant from + * . * Return 0 if permission is granted. * @settime: * Check permission to change the system time. * struct timespec64 is defined in and timezone * is defined in - * @ts contains new time - * @tz contains new timezone + * @ts contains new time. + * @tz contains new timezone. * Return 0 if permission is granted. * @vm_enough_memory: * Check permissions for allocating a new virtual mapping. @@ -1420,11 +1459,13 @@ * @secid contains the security ID. * @secdata contains the pointer that stores the converted security * context. - * @seclen pointer which contains the length of the data + * @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. @@ -1461,7 +1502,7 @@ * @audit_rule_free: * Deallocate the LSM audit rule structure previously allocated by * audit_rule_init. - * @lsmrule contains the allocated rule + * @lsmrule contains the allocated rule. * * @inode_invalidate_secctx: * Notify the security module that it must revalidate the security context @@ -1478,6 +1519,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 @@ -1491,6 +1533,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 @@ -1498,6 +1541,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: * @@ -1505,13 +1549,15 @@ * Check to see if a watch notification can be posted to a particular * queue. * @w_cred: The credentials of the whoever set the watch. - * @cred: The event-triggerer's credentials - * @n: The notification being posted + * @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. @@ -1520,65 +1566,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 { From 577cc1434e4cc1342c3df6d6a3c85136ab335c81 Mon Sep 17 00:00:00 2001 From: Roberto Sassu Date: Fri, 9 Dec 2022 09:29:35 +0100 Subject: [PATCH 13/13] lsm: Fix description of fs_context_parse_param The fs_context_parse_param hook already has a description, which seems the right one according to the code. Fixes: 8eb687bc8069 ("lsm: Add/fix return values in lsm_hooks.h and fix formatting") Signed-off-by: Roberto Sassu Signed-off-by: Paul Moore --- include/linux/lsm_hooks.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index bad3b6baad86..20e70132584c 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -100,9 +100,6 @@ * 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. *