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

["Lukáš Czerner" <lczerner@redhat.com>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3230 bytes --]

On Fri, 2 May 2014, Mateusz Guzik wrote:

> Date: Fri, 2 May 2014 13:53:11 +0200
> From: Mateusz Guzik <mguzik@redhat.com>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: Benjamin LaHaise <bcrl@kvack.org>, torvalds@linux-foundation.org,
>     linux-aio@kvack.org, linux-fsdevel@vger.kernel.org,
>     stable@vger.kernel.org, Leon Yu <chianglungyu@gmail.com>
> Subject: Re: [PATCH 2/2] aio: fix potential leak in aio_run_iocb().
> 
> 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;

ah right, it replaces the return value. Ignore me then.

-Lukas

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