| Message ID | 1487861534-14429-1-git-send-email-agruenba@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. I guess the idea is that ERANGE means "the xattr value doesn't fit into the buffer you gave me", and EF2BIG 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). Anyway, feel free to add Reviewed-by: J. Bruce Fields <bfields@redhat.com> but I wouldn't say no to more documentation. --b. > if (error > 0) { > if ((strcmp(kname, XATTR_NAME_POSIX_ACL_ACCESS) == 0) || > (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0)) > posix_acl_fix_xattr_to_user(kvalue, size); > if (size && copy_to_user(value, kvalue, error)) > error = -EFAULT; > - } else if (error == -ERANGE && size >= XATTR_SIZE_MAX) { > - /* The file system tried to returned a value bigger > - than XATTR_SIZE_MAX bytes. Not possible. */ > - error = -E2BIG; > } > > kvfree(kvalue); > @@ -620,14 +622,16 @@ listxattr(struct dentry *d, char __user *list, size_t size) > } > > error = vfs_listxattr(d, klist, size); > - if (error > 0) { > - if (size && copy_to_user(list, klist, error)) > - error = -EFAULT; > - } else if (error == -ERANGE && size >= XATTR_LIST_MAX) { > + if (error > XATTR_LIST_MAX || > + (error == -ERANGE && size >= XATTR_LIST_MAX)) { > /* The file system tried to returned a list bigger > than XATTR_LIST_MAX bytes. Not possible. */ > error = -E2BIG; > } > + if (error > 0) { > + if (size && copy_to_user(list, klist, error)) > + error = -EFAULT; > + } > > kvfree(klist); > > -- > 2.7.4 >
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
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); + 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; + } if (error > 0) { if ((strcmp(kname, XATTR_NAME_POSIX_ACL_ACCESS) == 0) || (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0)) posix_acl_fix_xattr_to_user(kvalue, size); if (size && copy_to_user(value, kvalue, error)) error = -EFAULT; - } else if (error == -ERANGE && size >= XATTR_SIZE_MAX) { - /* The file system tried to returned a value bigger - than XATTR_SIZE_MAX bytes. Not possible. */ - error = -E2BIG; } kvfree(kvalue); @@ -620,14 +622,16 @@ listxattr(struct dentry *d, char __user *list, size_t size) } error = vfs_listxattr(d, klist, size); - if (error > 0) { - if (size && copy_to_user(list, klist, error)) - error = -EFAULT; - } else if (error == -ERANGE && size >= XATTR_LIST_MAX) { + if (error > XATTR_LIST_MAX || + (error == -ERANGE && size >= XATTR_LIST_MAX)) { /* The file system tried to returned a list bigger than XATTR_LIST_MAX bytes. Not possible. */ error = -E2BIG; } + if (error > 0) { + if (size && copy_to_user(list, klist, error)) + error = -EFAULT; + } kvfree(klist);
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(-)