| Message ID | 20220802144236.1481779-1-mszeredi@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfs_getxattr_alloc(): don't allocate buf on failure | expand |
On Tue, Aug 02, 2022 at 04:42:36PM +0200, Miklos Szeredi wrote: > Some callers of vfs_getxattr_alloc() assume that on failure the allocated > buffer does not need to be freed. > > Callers could be fixed, but fixing the semantics of vfs_getxattr_alloc() is > simpler and makes sure that this class of bugs does not occur again. > > Reported-and-tested-by: syzbot+942d5390db2d9624ced8@syzkaller.appspotmail.com > Fixes: 1601fbad2b14 ("xattr: define vfs_getxattr_alloc and vfs_xattr_cmp") > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > --- > fs/xattr.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/fs/xattr.c b/fs/xattr.c > index e8dd03e4561e..1800cfa97411 100644 > --- a/fs/xattr.c > +++ b/fs/xattr.c > @@ -383,7 +383,10 @@ vfs_getxattr_alloc(struct user_namespace *mnt_userns, struct dentry *dentry, > } > > error = handler->get(handler, dentry, inode, name, value, error); > - *xattr_value = value; > + if (error < 0 && value != *xattr_value) > + kfree(value); > + else > + *xattr_value = value; > return error; > } Think what happens if it had been called with non-NULL *xattr_value, found that it needed realloc, had krealloc() succeed (and free the original), only to fail in ->get(). Your variant will leave *xattr_value pointing to already freed object, with no way for the caller to tell that from failure before it got to krealloc(). IOW, that's unusable for callers with preallocated buffer - in particular, ones that call that thing in a loop.
On Tue, Aug 02, 2022 at 04:12:31PM +0100, Al Viro wrote: > On Tue, Aug 02, 2022 at 04:42:36PM +0200, Miklos Szeredi wrote: > > Some callers of vfs_getxattr_alloc() assume that on failure the allocated > > buffer does not need to be freed. > > > > Callers could be fixed, but fixing the semantics of vfs_getxattr_alloc() is > > simpler and makes sure that this class of bugs does not occur again. > > > > Reported-and-tested-by: syzbot+942d5390db2d9624ced8@syzkaller.appspotmail.com > > Fixes: 1601fbad2b14 ("xattr: define vfs_getxattr_alloc and vfs_xattr_cmp") > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > > --- > > fs/xattr.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/fs/xattr.c b/fs/xattr.c > > index e8dd03e4561e..1800cfa97411 100644 > > --- a/fs/xattr.c > > +++ b/fs/xattr.c > > @@ -383,7 +383,10 @@ vfs_getxattr_alloc(struct user_namespace *mnt_userns, struct dentry *dentry, > > } > > > > error = handler->get(handler, dentry, inode, name, value, error); > > - *xattr_value = value; > > + if (error < 0 && value != *xattr_value) > > + kfree(value); > > + else > > + *xattr_value = value; > > return error; > > } > > Think what happens if it had been called with non-NULL *xattr_value, > found that it needed realloc, had krealloc() succeed (and free the > original), only to fail in ->get(). > > Your variant will leave *xattr_value pointing to already freed > object, with no way for the caller to tell that from failure before > it got to krealloc(). > > IOW, that's unusable for callers with preallocated buffer - in > particular, ones that call that thing in a loop. FWIW, if we change calling conventions so that in some cases caller need not kfree() whatever's in *xattr_value, about the only variant I see is to have the damn thing freed and replaced with NULL on *all* failure exits. Might or might not make sense, not sure...
On Tue, 2 Aug 2022 at 17:29, Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Tue, Aug 02, 2022 at 04:12:31PM +0100, Al Viro wrote: > > On Tue, Aug 02, 2022 at 04:42:36PM +0200, Miklos Szeredi wrote: > > > Some callers of vfs_getxattr_alloc() assume that on failure the allocated > > > buffer does not need to be freed. > > > > > > Callers could be fixed, but fixing the semantics of vfs_getxattr_alloc() is > > > simpler and makes sure that this class of bugs does not occur again. > > > > > > Reported-and-tested-by: syzbot+942d5390db2d9624ced8@syzkaller.appspotmail.com > > > Fixes: 1601fbad2b14 ("xattr: define vfs_getxattr_alloc and vfs_xattr_cmp") > > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > > > --- > > > fs/xattr.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/fs/xattr.c b/fs/xattr.c > > > index e8dd03e4561e..1800cfa97411 100644 > > > --- a/fs/xattr.c > > > +++ b/fs/xattr.c > > > @@ -383,7 +383,10 @@ vfs_getxattr_alloc(struct user_namespace *mnt_userns, struct dentry *dentry, > > > } > > > > > > error = handler->get(handler, dentry, inode, name, value, error); > > > - *xattr_value = value; > > > + if (error < 0 && value != *xattr_value) > > > + kfree(value); > > > + else > > > + *xattr_value = value; > > > return error; > > > } > > > > Think what happens if it had been called with non-NULL *xattr_value, > > found that it needed realloc, had krealloc() succeed (and free the > > original), only to fail in ->get(). > > > > Your variant will leave *xattr_value pointing to already freed > > object, with no way for the caller to tell that from failure before > > it got to krealloc(). > > > > IOW, that's unusable for callers with preallocated buffer - in > > particular, ones that call that thing in a loop. > > FWIW, if we change calling conventions so that in some cases caller > need not kfree() whatever's in *xattr_value, about the only variant > I see is to have the damn thing freed and replaced with NULL on > *all* failure exits. Might or might not make sense, not sure... The minimal semantic change that would fix all buggy callers is to change the non-loop case (i.e. when called only once with a xattr_value pointing to NULL. In the looping case callers correctly handle the error, since an explicit kfree in the helper is unexpected. Posted v2. Thanks, Miklos
diff --git a/fs/xattr.c b/fs/xattr.c index e8dd03e4561e..1800cfa97411 100644 --- a/fs/xattr.c +++ b/fs/xattr.c @@ -383,7 +383,10 @@ vfs_getxattr_alloc(struct user_namespace *mnt_userns, struct dentry *dentry, } error = handler->get(handler, dentry, inode, name, value, error); - *xattr_value = value; + if (error < 0 && value != *xattr_value) + kfree(value); + else + *xattr_value = value; return error; }
Some callers of vfs_getxattr_alloc() assume that on failure the allocated buffer does not need to be freed. Callers could be fixed, but fixing the semantics of vfs_getxattr_alloc() is simpler and makes sure that this class of bugs does not occur again. Reported-and-tested-by: syzbot+942d5390db2d9624ced8@syzkaller.appspotmail.com Fixes: 1601fbad2b14 ("xattr: define vfs_getxattr_alloc and vfs_xattr_cmp") Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> --- fs/xattr.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)