Re: [PATCH 2/2] aio: fix potential leak in aio_run_iocb().

[Mateusz Guzik <mguzik@redhat.com>]

On Fri, May 02, 2014 at 10:56:32AM +0200, Lukáš Czerner wrote:
> On Thu, 1 May 2014, Benjamin LaHaise wrote:
> 
> > Date: Thu, 1 May 2014 09:07:09 -0400
> > From: Benjamin LaHaise <bcrl@kvack.org>
> > To: torvalds@linux-foundation.org
> > Cc: linux-aio@kvack.org, linux-fsdevel@vger.kernel.org,
> >     stable@vger.kernel.org, Leon Yu <chianglungyu@gmail.com>
> > Subject: [PATCH 2/2] aio: fix potential leak in aio_run_iocb().
> > 
> > iovec should be reclaimed whenever caller of rw_copy_check_uvector() returns,
> > but it doesn't hold when failure happens right after aio_setup_vectored_rw().
> > 
> > Fix that in a such way to avoid hairy goto.
> 
> As I already replied to Leon,
> 
> this does not seem right.
> 
> > 
> > Signed-off-by: Leon Yu <chianglungyu@gmail.com>
> > Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>
> > Cc: stable@vger.kernel.org
> > ---
> >  fs/aio.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/aio.c b/fs/aio.c
> > index 2adbb03..a0ed6c7 100644
> > --- a/fs/aio.c
> > +++ b/fs/aio.c
> > @@ -1327,10 +1327,8 @@ rw_common:
> >  						&iovec, compat)
> >  			: aio_setup_single_vector(req, rw, buf, &nr_segs,
> >  						  iovec);
> > -		if (ret)
> > -			return ret;
> > -
> > -		ret = rw_verify_area(rw, file, &req->ki_pos, req->ki_nbytes);
> 
> here ret could be possibly set to a positive number.
> 

How?

                ret = (opcode == IOCB_CMD_PREADV ||
                       opcode == IOCB_CMD_PWRITEV)
                        ? aio_setup_vectored_rw(req, rw, buf, &nr_segs,
                                                &iovec, compat)
                        : aio_setup_single_vector(req, rw, buf, &nr_segs,
                                                  iovec);

Where aio_setup_vectored_rw:
        if (ret < 0)
                return ret;
[..]
	return 0;


and aio_setup_single_vector:
        if (unlikely(!access_ok(!rw, buf, kiocb->ki_nbytes)))
                return -EFAULT;
[..]
	        return 0;

Both functions are returning ssize_t, thus it's either 0 on success or
negative on failure.

"if (ret)" replaced by "if (ret < 0)" should indeed set off alarm bells,
but turns it turns out to be fine here.

> > +		if (!ret)
> > +			ret = rw_verify_area(rw, file, &req->ki_pos, req->ki_nbytes);
> >  		if (ret < 0) {
> 

So this check is fine and cleanup will be called.

However, there is a yet to be merged patch which fixes actual problem
which is weird rw_copy_check_uvector semantics:
https://lkml.org/lkml/2014/4/25/778

rendering this patch unnecessary

-- 
Mateusz Guzik

--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>