[PATCH] vfs_getxattr_alloc(): don't allocate buf on failure

[PATCH] vfs_getxattr_alloc(): don't allocate buf on failure

[Miklos Szeredi]

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;
 }
 
-- 
2.35.3

Re: [PATCH] vfs_getxattr_alloc(): don't allocate buf on failure

[Al Viro]

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.

Re: [PATCH] vfs_getxattr_alloc(): don't allocate buf on failure

[Al Viro]

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...

Re: [PATCH] vfs_getxattr_alloc(): don't allocate buf on failure

[Miklos Szeredi]

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