Re: [PATCH bpf-next v4 03/20] lsm: Refactor return value of LSM hook inode_getsecurity

Xu Kuohai
2024-07-13T08:07:29Z

On 7/12/2024 9:31 PM, Simon Horman wrote:
> On Thu, Jul 11, 2024 at 07:18:51PM +0800, Xu Kuohai wrote:
>> From: Xu Kuohai <xukuohai@huawei.com>
>>
>> To be consistent with most LSM hooks, convert the return value of
>> hook inode_getsecurity to 0 or a negative error code.
>>
>> Before:
>> - Hook inode_getsecurity returns size of buffer on success or a
>> negative error code on failure.
>>
>> After:
>> - Hook inode_getsecurity returns 0 on success or a negative error
>> code on failure. An output parameter @len is introduced to hold
>> the buffer size on success.
>>
>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
>> ---
>> fs/xattr.c | 19 ++++++++++---------
>> include/linux/lsm_hook_defs.h | 3 ++-
>> include/linux/security.h | 12 ++++++------
>> security/commoncap.c | 9 ++++++---
>> security/security.c | 11 ++++++-----
>> security/selinux/hooks.c | 16 ++++++----------
>> security/smack/smack_lsm.c | 14 +++++++-------
>> 7 files changed, 43 insertions(+), 41 deletions(-)
>>
>> diff --git a/fs/xattr.c b/fs/xattr.c
>> index f8b643f91a98..f4e3bedf7272 100644
>> --- a/fs/xattr.c
>> +++ b/fs/xattr.c
>> @@ -339,27 +339,28 @@ xattr_getsecurity(struct mnt_idmap *idmap, struct inode *inode,
>> const char *name, void *value, size_t size)
>> {
>> void *buffer = NULL;
>> - ssize_t len;
>> + int error;
>> + u32 len;
>>
>> if (!value || !size) {
>> - len = security_inode_getsecurity(idmap, inode, name,
>> - &buffer, false);
>> + error = security_inode_getsecurity(idmap, inode, name,
>> + false, &buffer, &len);
>> goto out_noalloc;
>> }
>>
>> - len = security_inode_getsecurity(idmap, inode, name, &buffer,
>> - true);
>> - if (len < 0)
>> - return len;
>> + error = security_inode_getsecurity(idmap, inode, name, true,
>> + &buffer, &len);
>> + if (error)
>> + return error;
>> if (size < len) {
>> - len = -ERANGE;
>> + error = -ERANGE;
>> goto out;
>> }
>> memcpy(value, buffer, len);
>> out:
>> kfree(buffer);
>> out_noalloc:
>> - return len;
>> + return error < 0 ? error : len;
>
> Hi Xu Kuohai,
>
> len is an unsigned 32-bit entity, but the return type of this function
> is an unsigned value (ssize_t). So in theory, if len is very large,
> a negative error value error will be returned.
>
>> }
>
> Similarly for the handling of nattr in lsm_get_self_attr in
> lsm_syscalls.c in a subsequent patch.
>
> Flagged by Smatch.
>
> ...
>
>

OK, indeed there is no standardization that says ssize_t must be wider than u32.
I'll look into adding overflow checks. Thanks.