From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mateusz Guzik Subject: Re: [PATCH 2/2] aio: fix potential leak in aio_run_iocb(). Date: Fri, 2 May 2014 13:53:11 +0200 Message-ID: <20140502115310.GA13207@mguzik.redhat.com> References: <20140501130709.GL28959@kvack.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Cc: Benjamin LaHaise , torvalds@linux-foundation.org, linux-aio@kvack.org, linux-fsdevel@vger.kernel.org, stable@vger.kernel.org, Leon Yu To: =?utf-8?B?THVrw6HFoQ==?= Czerner Return-path: Content-Disposition: inline In-Reply-To: Sender: owner-linux-aio@kvack.org List-Id: linux-fsdevel.vger.kernel.org On Fri, May 02, 2014 at 10:56:32AM +0200, Luk=C3=A1=C5=A1 Czerner wrote: > On Thu, 1 May 2014, Benjamin LaHaise wrote: >=20 > > Date: Thu, 1 May 2014 09:07:09 -0400 > > From: Benjamin LaHaise > > To: torvalds@linux-foundation.org > > Cc: linux-aio@kvack.org, linux-fsdevel@vger.kernel.org, > > stable@vger.kernel.org, Leon Yu > > Subject: [PATCH 2/2] aio: fix potential leak in aio_run_iocb(). > >=20 > > iovec should be reclaimed whenever caller of rw_copy_check_uvector() = returns, > > but it doesn't hold when failure happens right after aio_setup_vector= ed_rw(). > >=20 > > Fix that in a such way to avoid hairy goto. >=20 > As I already replied to Leon, >=20 > this does not seem right. >=20 > >=20 > > Signed-off-by: Leon Yu > > Signed-off-by: Benjamin LaHaise > > Cc: stable@vger.kernel.org > > --- > > fs/aio.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > >=20 > > 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 =3D rw_verify_area(rw, file, &req->ki_pos, req->ki_nbytes); >=20 > here ret could be possibly set to a positive number. >=20 How? ret =3D (opcode =3D=3D IOCB_CMD_PREADV || opcode =3D=3D 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 =3D rw_verify_area(rw, file, &req->ki_pos, req->ki_nbytes); > > if (ret < 0) { >=20 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 --=20 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: aart@kvack.org