Re: [PATCH] xattr: Additional maximum size check

[Andreas Gruenbacher <agruenba@redhat.com>]

On Thu, Feb 23, 2017 at 9:34 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> On Thu, Feb 23, 2017 at 03:52:14PM +0100, Andreas Gruenbacher wrote:
>> When querying for the buffer size required, a filesystem's getxattr
>> xattr handler or listxattr iop may return a value bigger than the
>> maximum size limit.  However, the VFS will never return oversize
>> buffers, so cast such values to -E2BIG.
>>
>> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
>> ---
>>  fs/xattr.c | 20 ++++++++++++--------
>>  1 file changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/xattr.c b/fs/xattr.c
>> index 7e3317c..c19a163 100644
>> --- a/fs/xattr.c
>> +++ b/fs/xattr.c
>> @@ -537,16 +537,18 @@ getxattr(struct dentry *d, const char __user *name, void __user *value,
>>       }
>>
>>       error = vfs_getxattr(d, kname, kvalue, size);
>
> We have this just above:
>
>         if (size) {
>                 if (size > XATTR_SIZE_MAX)
>                         size = XATTR_SIZE_MAX;
>
> So this test:
>
>> +     if (error > XATTR_SIZE_MAX ||
>> +         (error == -ERANGE && size >= XATTR_SIZE_MAX)) {
>> +             /* The file system tried to returned a value bigger
>> +                than XATTR_SIZE_MAX bytes. Not possible. */
>> +             error = -E2BIG;
>> +     }
>
> is equivalent to:
>
>         if (error > XATTR_SIZE_MAX ||
>             (error = -ERANGE && size == XATTR_SIZE_MAX))
>
> That's kind of a subtle case.

True.

> I guess the idea is that ERANGE means
> "the xattr value doesn't fit into the buffer you gave me", and E2BIG
> means "the xattr value couldn't fit in any buffer you could possible
> give me".  And if the filesystem isn't satisfied even with a
> maximum-sized buffer then clearly we're in the second case.  OK, got it.
> Still, this is a little subtle.  I don't see EF2BIG in my copy of
> getxattr(2).

It's not in any of my copies of getxattr(2) either yet, but it will be soon :)

> Anyway, feel free to add
>
>         Reviewed-by: J. Bruce Fields <bfields@redhat.com>
>
> but I wouldn't say no to more documentation.

Thanks,
Andreas