Message ID | 20240711111908.3817636-5-xukuohai@huaweicloud.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Paul Moore |
Headers | show |
Series | Add return value range check for BPF LSM | expand |
On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote: > > To be consistent with most LSM hooks, convert the return value of > hook inode_listsecurity to 0 or a negative error code. > > Before: > - Hook inode_listsecurity returns number of bytes used/required on > success or a negative error code on failure. > > After: > - Hook inode_listsecurity returns 0 on success or a negative error > code on failure. An output parameter @bytes is introduced to hold > the number of bytes used/required on success. > > Signed-off-by: Xu Kuohai <xukuohai@huawei.com> > --- > fs/nfs/nfs4proc.c | 5 ++++- > fs/xattr.c | 5 ++++- > include/linux/lsm_hook_defs.h | 2 +- > include/linux/security.h | 7 ++++--- > net/socket.c | 9 +++++---- > security/security.c | 29 +++++++++++++++++++++++++---- > security/selinux/hooks.c | 8 +++++--- > security/smack/smack_lsm.c | 6 ++++-- > 8 files changed, 52 insertions(+), 19 deletions(-) > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index a691fa10b3e9..6d75758ba3d5 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -7848,10 +7848,13 @@ static int nfs4_xattr_get_nfs4_label(const struct xattr_handler *handler, > static ssize_t > nfs4_listxattr_nfs4_label(struct inode *inode, char *list, size_t list_len) > { > + size_t bytes; > int len = 0; > > if (nfs_server_capable(inode, NFS_CAP_SECURITY_LABEL)) { > - len = security_inode_listsecurity(inode, list, list_len); > + len = security_inode_listsecurity(inode, list, list_len, &bytes); > + if (!len) > + len = bytes; > if (len >= 0 && list_len && len > list_len) > return -ERANGE; > } See my comments below. > diff --git a/fs/xattr.c b/fs/xattr.c > index f4e3bedf7272..ab7d7123a016 100644 > --- a/fs/xattr.c > +++ b/fs/xattr.c > @@ -485,6 +485,7 @@ vfs_listxattr(struct dentry *dentry, char *list, size_t size) > { > struct inode *inode = d_inode(dentry); > ssize_t error; > + size_t bytes; > > error = security_inode_listxattr(dentry); > if (error) > @@ -493,7 +494,9 @@ vfs_listxattr(struct dentry *dentry, char *list, size_t size) > if (inode->i_op->listxattr) { > error = inode->i_op->listxattr(dentry, list, size); > } else { > - error = security_inode_listsecurity(inode, list, size); > + error = security_inode_listsecurity(inode, list, size, &bytes); > + if (!error) > + error = bytes; > if (size && error > size) > error = -ERANGE; More on this below, but since the buffer length is fixed we are already going to have to do a length comparison in the LSMs, why not do the check and return -ERANGE there? > diff --git a/net/socket.c b/net/socket.c > index e416920e9399..43f0e3c9a6e0 100644 > --- a/net/socket.c > +++ b/net/socket.c > @@ -571,12 +571,13 @@ static struct socket *sockfd_lookup_light(int fd, int *err, int *fput_needed) > static ssize_t sockfs_listxattr(struct dentry *dentry, char *buffer, > size_t size) > { > - ssize_t len; > + int err; > + size_t len; > ssize_t used = 0; > > - len = security_inode_listsecurity(d_inode(dentry), buffer, size); > - if (len < 0) > - return len; > + err = security_inode_listsecurity(d_inode(dentry), buffer, size, &len); > + if (err < 0) > + return err; > used += len; > if (buffer) { > if (size < used) It doesn't show in the patch/diff, but if the LSM hook handles the length comparison we can simplify the -ERANGE code in sockfs_listxattr(). > diff --git a/security/security.c b/security/security.c > index 614f14cbfff7..26eea8f4cd74 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -2597,20 +2597,41 @@ int security_inode_setsecurity(struct inode *inode, const char *name, > * @inode: inode > * @buffer: buffer > * @buffer_size: size of buffer > + * @bytes: number of bytes used/required > * > * Copy the extended attribute names for the security labels associated with > * @inode into @buffer. The maximum size of @buffer is specified by > * @buffer_size. @buffer may be NULL to request the size of the buffer > * required. > * > - * Return: Returns number of bytes used/required on success. > + * Return: Returns 0 on success or a negative error code on failure. > */ > int security_inode_listsecurity(struct inode *inode, > - char *buffer, size_t buffer_size) > + char *buffer, size_t buffer_size, > + size_t *bytes) > { > + int rc; > + size_t used; > + struct security_hook_list *hp; > + > if (unlikely(IS_PRIVATE(inode))) > - return 0; > - return call_int_hook(inode_listsecurity, inode, buffer, buffer_size); > + return *bytes = 0; > + > + used = 0; > + hlist_for_each_entry(hp, &security_hook_heads.inode_listsecurity, > + list) { > + rc = hp->hook.inode_listsecurity(inode, buffer, buffer_size, > + &used); > + if (rc < 0) > + return rc; > + if (used != 0) > + break; > + } > + > + *bytes = used; > + > + return 0; > + > } > EXPORT_SYMBOL(security_inode_listsecurity); For reasons associated with the static_call work, we really need to limit ourselves to the call_{int,void}_hook() macros on any new code. The good news is that I think we can do that here as the existing code isn't multi-LSM friendly. > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 70792bba24d9..5dedd3917d57 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -3481,16 +3481,18 @@ static int selinux_inode_setsecurity(struct inode *inode, const char *name, > return 0; > } > > -static int selinux_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size) > +static int selinux_inode_listsecurity(struct inode *inode, char *buffer, > + size_t buffer_size, size_t *bytes) > { > const int len = sizeof(XATTR_NAME_SELINUX); > > if (!selinux_initialized()) > - return 0; > + return *bytes = 0; > > if (buffer && len <= buffer_size) > memcpy(buffer, XATTR_NAME_SELINUX, len); > - return len; > + *bytes = len; > + return 0; > } Let's do something like below so we can catch -ERANGE in the LSMs themselves. if (!selinux_initialized()) return *bytes = 0; *bytes = sizeof(XATTR_NAME_SELINUX); if (len > buffer_size); return -ERANGE; if (buffer) memcpy(buffer, XATTR_NAME_SELINUX, *bytes); return 0; > static void selinux_inode_getsecid(struct inode *inode, u32 *secid) > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c > index e7a5f6fd9a2d..6f73906bf7ea 100644 > --- a/security/smack/smack_lsm.c > +++ b/security/smack/smack_lsm.c > @@ -1611,16 +1611,18 @@ static int smack_inode_getsecurity(struct mnt_idmap *idmap, > * @inode: the object > * @buffer: where they go > * @buffer_size: size of buffer > + * @bytes: number of data bytes in buffer > */ > static int smack_inode_listsecurity(struct inode *inode, char *buffer, > - size_t buffer_size) > + size_t buffer_size, size_t *bytes) > { > int len = sizeof(XATTR_NAME_SMACK); > > if (buffer != NULL && len <= buffer_size) > memcpy(buffer, XATTR_NAME_SMACK, len); > > - return len; > + *bytes = len; > + return 0; > } A similar approach could be used here. -- paul-moore.com
On 7/19/2024 10:08 AM, Paul Moore wrote: > On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote: >> >> To be consistent with most LSM hooks, convert the return value of >> hook inode_listsecurity to 0 or a negative error code. >> >> Before: >> - Hook inode_listsecurity returns number of bytes used/required on >> success or a negative error code on failure. >> >> After: >> - Hook inode_listsecurity returns 0 on success or a negative error >> code on failure. An output parameter @bytes is introduced to hold >> the number of bytes used/required on success. >> >> Signed-off-by: Xu Kuohai <xukuohai@huawei.com> >> --- >> fs/nfs/nfs4proc.c | 5 ++++- >> fs/xattr.c | 5 ++++- >> include/linux/lsm_hook_defs.h | 2 +- >> include/linux/security.h | 7 ++++--- >> net/socket.c | 9 +++++---- >> security/security.c | 29 +++++++++++++++++++++++++---- >> security/selinux/hooks.c | 8 +++++--- >> security/smack/smack_lsm.c | 6 ++++-- >> 8 files changed, 52 insertions(+), 19 deletions(-) >> >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >> index a691fa10b3e9..6d75758ba3d5 100644 >> --- a/fs/nfs/nfs4proc.c >> +++ b/fs/nfs/nfs4proc.c >> @@ -7848,10 +7848,13 @@ static int nfs4_xattr_get_nfs4_label(const struct xattr_handler *handler, >> static ssize_t >> nfs4_listxattr_nfs4_label(struct inode *inode, char *list, size_t list_len) >> { >> + size_t bytes; >> int len = 0; >> >> if (nfs_server_capable(inode, NFS_CAP_SECURITY_LABEL)) { >> - len = security_inode_listsecurity(inode, list, list_len); >> + len = security_inode_listsecurity(inode, list, list_len, &bytes); >> + if (!len) >> + len = bytes; >> if (len >= 0 && list_len && len > list_len) >> return -ERANGE; >> } > > See my comments below. > >> diff --git a/fs/xattr.c b/fs/xattr.c >> index f4e3bedf7272..ab7d7123a016 100644 >> --- a/fs/xattr.c >> +++ b/fs/xattr.c >> @@ -485,6 +485,7 @@ vfs_listxattr(struct dentry *dentry, char *list, size_t size) >> { >> struct inode *inode = d_inode(dentry); >> ssize_t error; >> + size_t bytes; >> >> error = security_inode_listxattr(dentry); >> if (error) >> @@ -493,7 +494,9 @@ vfs_listxattr(struct dentry *dentry, char *list, size_t size) >> if (inode->i_op->listxattr) { >> error = inode->i_op->listxattr(dentry, list, size); >> } else { >> - error = security_inode_listsecurity(inode, list, size); >> + error = security_inode_listsecurity(inode, list, size, &bytes); >> + if (!error) >> + error = bytes; >> if (size && error > size) >> error = -ERANGE; > > More on this below, but since the buffer length is fixed we are > already going to have to do a length comparison in the LSMs, why not > do the check and return -ERANGE there? > Sounds great, thanks >> diff --git a/net/socket.c b/net/socket.c >> index e416920e9399..43f0e3c9a6e0 100644 >> --- a/net/socket.c >> +++ b/net/socket.c >> @@ -571,12 +571,13 @@ static struct socket *sockfd_lookup_light(int fd, int *err, int *fput_needed) >> static ssize_t sockfs_listxattr(struct dentry *dentry, char *buffer, >> size_t size) >> { >> - ssize_t len; >> + int err; >> + size_t len; >> ssize_t used = 0; >> >> - len = security_inode_listsecurity(d_inode(dentry), buffer, size); >> - if (len < 0) >> - return len; >> + err = security_inode_listsecurity(d_inode(dentry), buffer, size, &len); >> + if (err < 0) >> + return err; >> used += len; >> if (buffer) { >> if (size < used) > > It doesn't show in the patch/diff, but if the LSM hook handles the length > comparison we can simplify the -ERANGE code in sockfs_listxattr(). > Will do >> diff --git a/security/security.c b/security/security.c >> index 614f14cbfff7..26eea8f4cd74 100644 >> --- a/security/security.c >> +++ b/security/security.c >> @@ -2597,20 +2597,41 @@ int security_inode_setsecurity(struct inode *inode, const char *name, >> * @inode: inode >> * @buffer: buffer >> * @buffer_size: size of buffer >> + * @bytes: number of bytes used/required >> * >> * Copy the extended attribute names for the security labels associated with >> * @inode into @buffer. The maximum size of @buffer is specified by >> * @buffer_size. @buffer may be NULL to request the size of the buffer >> * required. >> * >> - * Return: Returns number of bytes used/required on success. >> + * Return: Returns 0 on success or a negative error code on failure. >> */ >> int security_inode_listsecurity(struct inode *inode, >> - char *buffer, size_t buffer_size) >> + char *buffer, size_t buffer_size, >> + size_t *bytes) >> { >> + int rc; >> + size_t used; >> + struct security_hook_list *hp; >> + >> if (unlikely(IS_PRIVATE(inode))) >> - return 0; >> - return call_int_hook(inode_listsecurity, inode, buffer, buffer_size); >> + return *bytes = 0; >> + >> + used = 0; >> + hlist_for_each_entry(hp, &security_hook_heads.inode_listsecurity, >> + list) { >> + rc = hp->hook.inode_listsecurity(inode, buffer, buffer_size, >> + &used); >> + if (rc < 0) >> + return rc; >> + if (used != 0) >> + break; >> + } >> + >> + *bytes = used; >> + >> + return 0; >> + >> } >> EXPORT_SYMBOL(security_inode_listsecurity); > > For reasons associated with the static_call work, we really need to > limit ourselves to the call_{int,void}_hook() macros on any new code. > The good news is that I think we can do that here as the existing > code isn't multi-LSM friendly. > Thanks for pointing that out. Good to know the current code is acceptable. >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >> index 70792bba24d9..5dedd3917d57 100644 >> --- a/security/selinux/hooks.c >> +++ b/security/selinux/hooks.c >> @@ -3481,16 +3481,18 @@ static int selinux_inode_setsecurity(struct inode *inode, const char *name, >> return 0; >> } >> >> -static int selinux_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size) >> +static int selinux_inode_listsecurity(struct inode *inode, char *buffer, >> + size_t buffer_size, size_t *bytes) >> { >> const int len = sizeof(XATTR_NAME_SELINUX); >> >> if (!selinux_initialized()) >> - return 0; >> + return *bytes = 0; >> >> if (buffer && len <= buffer_size) >> memcpy(buffer, XATTR_NAME_SELINUX, len); >> - return len; >> + *bytes = len; >> + return 0; >> } > > Let's do something like below so we can catch -ERANGE in the LSMs > themselves. > > if (!selinux_initialized()) > return *bytes = 0; > > *bytes = sizeof(XATTR_NAME_SELINUX); > if (len > buffer_size); > return -ERANGE; > if (buffer) > memcpy(buffer, XATTR_NAME_SELINUX, *bytes); > > return 0; > Will do >> static void selinux_inode_getsecid(struct inode *inode, u32 *secid) >> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c >> index e7a5f6fd9a2d..6f73906bf7ea 100644 >> --- a/security/smack/smack_lsm.c >> +++ b/security/smack/smack_lsm.c >> @@ -1611,16 +1611,18 @@ static int smack_inode_getsecurity(struct mnt_idmap *idmap, >> * @inode: the object >> * @buffer: where they go >> * @buffer_size: size of buffer >> + * @bytes: number of data bytes in buffer >> */ >> static int smack_inode_listsecurity(struct inode *inode, char *buffer, >> - size_t buffer_size) >> + size_t buffer_size, size_t *bytes) >> { >> int len = sizeof(XATTR_NAME_SMACK); >> >> if (buffer != NULL && len <= buffer_size) >> memcpy(buffer, XATTR_NAME_SMACK, len); >> >> - return len; >> + *bytes = len; >> + return 0; >> } > > A similar approach could be used here. > Will do > -- > paul-moore.com >
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index a691fa10b3e9..6d75758ba3d5 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -7848,10 +7848,13 @@ static int nfs4_xattr_get_nfs4_label(const struct xattr_handler *handler, static ssize_t nfs4_listxattr_nfs4_label(struct inode *inode, char *list, size_t list_len) { + size_t bytes; int len = 0; if (nfs_server_capable(inode, NFS_CAP_SECURITY_LABEL)) { - len = security_inode_listsecurity(inode, list, list_len); + len = security_inode_listsecurity(inode, list, list_len, &bytes); + if (!len) + len = bytes; if (len >= 0 && list_len && len > list_len) return -ERANGE; } diff --git a/fs/xattr.c b/fs/xattr.c index f4e3bedf7272..ab7d7123a016 100644 --- a/fs/xattr.c +++ b/fs/xattr.c @@ -485,6 +485,7 @@ vfs_listxattr(struct dentry *dentry, char *list, size_t size) { struct inode *inode = d_inode(dentry); ssize_t error; + size_t bytes; error = security_inode_listxattr(dentry); if (error) @@ -493,7 +494,9 @@ vfs_listxattr(struct dentry *dentry, char *list, size_t size) if (inode->i_op->listxattr) { error = inode->i_op->listxattr(dentry, list, size); } else { - error = security_inode_listsecurity(inode, list, size); + error = security_inode_listsecurity(inode, list, size, &bytes); + if (!error) + error = bytes; if (size && error > size) error = -ERANGE; } diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index 4f056f2613af..1b7761ae2777 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -174,7 +174,7 @@ LSM_HOOK(int, -EOPNOTSUPP, inode_getsecurity, struct mnt_idmap *idmap, 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, - size_t buffer_size) + size_t buffer_size, size_t *bytes) 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, struct dentry *src, diff --git a/include/linux/security.h b/include/linux/security.h index b6d296d21438..0ed53e232c4d 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -395,7 +395,7 @@ int security_inode_getsecurity(struct mnt_idmap *idmap, struct inode *inode, const char *name, bool alloc, void **buffer, u32 *len); int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags); -int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size); +int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size, size_t *bytes); void security_inode_getsecid(struct inode *inode, u32 *secid); int security_inode_copy_up(struct dentry *src, struct cred **new); int security_inode_copy_up_xattr(struct dentry *src, const char *name); @@ -1007,9 +1007,10 @@ static inline int security_inode_setsecurity(struct inode *inode, const char *na return -EOPNOTSUPP; } -static inline int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size) +static inline int security_inode_listsecurity(struct inode *inode, char *buffer, + size_t buffer_size, size_t *bytes) { - return 0; + return *bytes = 0; } static inline void security_inode_getsecid(struct inode *inode, u32 *secid) diff --git a/net/socket.c b/net/socket.c index e416920e9399..43f0e3c9a6e0 100644 --- a/net/socket.c +++ b/net/socket.c @@ -571,12 +571,13 @@ static struct socket *sockfd_lookup_light(int fd, int *err, int *fput_needed) static ssize_t sockfs_listxattr(struct dentry *dentry, char *buffer, size_t size) { - ssize_t len; + int err; + size_t len; ssize_t used = 0; - len = security_inode_listsecurity(d_inode(dentry), buffer, size); - if (len < 0) - return len; + err = security_inode_listsecurity(d_inode(dentry), buffer, size, &len); + if (err < 0) + return err; used += len; if (buffer) { if (size < used) diff --git a/security/security.c b/security/security.c index 614f14cbfff7..26eea8f4cd74 100644 --- a/security/security.c +++ b/security/security.c @@ -2597,20 +2597,41 @@ int security_inode_setsecurity(struct inode *inode, const char *name, * @inode: inode * @buffer: buffer * @buffer_size: size of buffer + * @bytes: number of bytes used/required * * Copy the extended attribute names for the security labels associated with * @inode into @buffer. The maximum size of @buffer is specified by * @buffer_size. @buffer may be NULL to request the size of the buffer * required. * - * Return: Returns number of bytes used/required on success. + * Return: Returns 0 on success or a negative error code on failure. */ int security_inode_listsecurity(struct inode *inode, - char *buffer, size_t buffer_size) + char *buffer, size_t buffer_size, + size_t *bytes) { + int rc; + size_t used; + struct security_hook_list *hp; + if (unlikely(IS_PRIVATE(inode))) - return 0; - return call_int_hook(inode_listsecurity, inode, buffer, buffer_size); + return *bytes = 0; + + used = 0; + hlist_for_each_entry(hp, &security_hook_heads.inode_listsecurity, + list) { + rc = hp->hook.inode_listsecurity(inode, buffer, buffer_size, + &used); + if (rc < 0) + return rc; + if (used != 0) + break; + } + + *bytes = used; + + return 0; + } EXPORT_SYMBOL(security_inode_listsecurity); diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 70792bba24d9..5dedd3917d57 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3481,16 +3481,18 @@ static int selinux_inode_setsecurity(struct inode *inode, const char *name, return 0; } -static int selinux_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size) +static int selinux_inode_listsecurity(struct inode *inode, char *buffer, + size_t buffer_size, size_t *bytes) { const int len = sizeof(XATTR_NAME_SELINUX); if (!selinux_initialized()) - return 0; + return *bytes = 0; if (buffer && len <= buffer_size) memcpy(buffer, XATTR_NAME_SELINUX, len); - return len; + *bytes = len; + return 0; } static void selinux_inode_getsecid(struct inode *inode, u32 *secid) diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index e7a5f6fd9a2d..6f73906bf7ea 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -1611,16 +1611,18 @@ static int smack_inode_getsecurity(struct mnt_idmap *idmap, * @inode: the object * @buffer: where they go * @buffer_size: size of buffer + * @bytes: number of data bytes in buffer */ static int smack_inode_listsecurity(struct inode *inode, char *buffer, - size_t buffer_size) + size_t buffer_size, size_t *bytes) { int len = sizeof(XATTR_NAME_SMACK); if (buffer != NULL && len <= buffer_size) memcpy(buffer, XATTR_NAME_SMACK, len); - return len; + *bytes = len; + return 0; } /**