| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c6ece90a-a9bd-4a77-960a-a9c8e44dd564@huaweicloud.com>
Date: Sat, 20 Jul 2024 17:29:10 +0800
From: Xu Kuohai <xukuohai@...weicloud.com>
To: Paul Moore <paul@...l-moore.com>, bpf@...r.kernel.org,
netdev@...r.kernel.org, linux-security-module@...r.kernel.org,
linux-kselftest@...r.kernel.org, linux-integrity@...r.kernel.org,
selinux@...r.kernel.org
Cc: Alexei Starovoitov <ast@...nel.org>, Andrii Nakryiko <andrii@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Martin KaFai Lau <martin.lau@...ux.dev>, Eduard Zingerman
<eddyz87@...il.com>, Song Liu <song@...nel.org>,
Yonghong Song <yonghong.song@...ux.dev>,
John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>,
Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
Matt Bobrowski <mattbobrowski@...gle.com>,
Brendan Jackman <jackmanb@...omium.org>, James Morris <jmorris@...ei.org>,
"Serge E . Hallyn" <serge@...lyn.com>,
Khadija Kamran <kamrankhadijadj@...il.com>,
Casey Schaufler <casey@...aufler-ca.com>,
Ondrej Mosnacek <omosnace@...hat.com>, Kees Cook <keescook@...omium.org>,
John Johansen <john.johansen@...onical.com>,
Lukas Bulwahn <lukas.bulwahn@...il.com>,
Roberto Sassu <roberto.sassu@...wei.com>,
Shung-Hsi Yu <shung-hsi.yu@...e.com>, Edward Cree <ecree.xilinx@...il.com>,
Alexander Viro <viro@...iv.linux.org.uk>,
Christian Brauner <brauner@...nel.org>,
Trond Myklebust <trond.myklebust@...merspace.com>,
Anna Schumaker <anna@...nel.org>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Stephen Smalley <stephen.smalley.work@...il.com>
Subject: Re: [PATCH v4 4/20] lsm: Refactor return value of LSM hook
inode_listsecurity
On 7/19/2024 10:08 AM, Paul Moore wrote:
> On Jul 11, 2024 Xu Kuohai <xukuohai@...weicloud.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@...wei.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
>
Powered by blists - more mailing lists