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.
