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@xxxxxxxxxxxxxxxxxxxxxxxxx > > Fixes: 1601fbad2b14 ("xattr: define vfs_getxattr_alloc and vfs_xattr_cmp") > > Signed-off-by: Miklos Szeredi <mszeredi@xxxxxxxxxx> > > --- > > 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...