On Fri, 2 May 2014, Mateusz Guzik wrote:
> Date: Fri, 2 May 2014 13:53:11 +0200
> From: Mateusz Guzik <mguzik@xxxxxxxxxx>
> To: Lukáš Czerner <lczerner@xxxxxxxxxx>
> Cc: Benjamin LaHaise <bcrl@xxxxxxxxx>, torvalds@xxxxxxxxxxxxxxxxxxxx,
> linux-aio@xxxxxxxxx, linux-fsdevel@xxxxxxxxxxxxxxx,
> stable@xxxxxxxxxxxxxxx, Leon Yu <chianglungyu@xxxxxxxxx>
> 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@xxxxxxxxx>
> > > To: torvalds@xxxxxxxxxxxxxxxxxxxx
> > > Cc: linux-aio@xxxxxxxxx, linux-fsdevel@xxxxxxxxxxxxxxx,
> > > stable@xxxxxxxxxxxxxxx, Leon Yu <chianglungyu@xxxxxxxxx>
> > > 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@xxxxxxxxx>
> > > Signed-off-by: Benjamin LaHaise <bcrl@xxxxxxxxx>
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > ---
> > > 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
>
>