Re: [PATCH 1/2] btrfs: fix sparse address space warnings

[Omar Sandoval <osandov@osandov.com>]

On Sun, Sep 28, 2014 at 01:48:11AM -0700, Omar Sandoval wrote:
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index 6528aa6..e0be577 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -515,7 +515,8 @@ static int write_buf(struct file *filp, const void *buf, u32 len, loff_t *off)
>  	set_fs(KERNEL_DS);
>  
>  	while (pos < len) {
> -		ret = vfs_write(filp, (char *)buf + pos, len - pos, off);
> +		ret = vfs_write(filp, (__force const char __user *)buf + pos,
> +				len - pos, off);
>  		/* TODO handle that correctly */
>  		/*if (ret == -ERESTARTSYS) {
>  			continue;

Actually, looking at this now, it looks like this is just an open-coded
kernel_write. I think this could be made a bit cleaner by using that instead;
the tradeoff is that each call to kernel_write will do the address space
flip-flop, so if the write gets split up into many calls, there'd be some
slight overhead. That's probably a microoptimization, but I think it's worth
looking into making kernel_read and kernel_write handle the retry logic.

It looks like Oren Laadan submitted a patch doing exactly that as part of the
checkpoint/restart patch series: https://lkml.org/lkml/2010/5/6/142. I've added
an email address that I found for him.

>From Josef's response way back in 2010:
> > +static ssize_t _kernel_write(struct file *file, loff_t offset,
> > +			     const char __user *ubuf, size_t count)
> > +{
> > +	ssize_t nwrite;
> > +	size_t nleft;
> > +	loff_t pos = offset;
> > +
> > +	for (nleft = count; nleft; nleft -= nwrite) {
> > +		nwrite = vfs_write(file, ubuf, nleft, &pos);
> > +		if (nwrite < 0) {
> > +			if (nwrite == -EAGAIN) {
> > +				nwrite = 0;
> > +				continue;
> > +			} else
> > +				return nwrite;
> > +		}
> > +		ubuf += nwrite;
> > +	}
> > +	return count - nleft;
> > +}
> 
> I'm not entirely sure if this can happen, but if vfs_write doesn't write
> anything, but doesn't have an error, we could end up in an infinite loop.  Like
> I said I'm not sure if thats even possible, but its definitely one of those
> things that if it is possible some random security guy is going to figure out
> how to exploit it at some point down the line.

Did anyone ever come up with a good answer for this?

-- 
Omar