| Message ID | 20171128090252.GB23413@zzz.localdomain (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
Am Dienstag, 28. November 2017, 10:02:52 CET schrieb Eric Biggers: Hi Eric, > --- > crypto/af_alg.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/crypto/af_alg.c b/crypto/af_alg.c > index 358749c38894..415a54ced4d6 100644 > --- a/crypto/af_alg.c > +++ b/crypto/af_alg.c > @@ -672,14 +672,15 @@ void af_alg_free_areq_sgls(struct af_alg_async_req > *areq) } > > tsgl = areq->tsgl; > - for_each_sg(tsgl, sg, areq->tsgl_entries, i) { > - if (!sg_page(sg)) > - continue; > - put_page(sg_page(sg)); > - } > + if (tsgl) { > + for_each_sg(tsgl, sg, areq->tsgl_entries, i) { > + if (!sg_page(sg)) > + continue; > + put_page(sg_page(sg)); > + } > > - if (areq->tsgl && areq->tsgl_entries) Why do you want to remove the check for areq->tsgl_entries? I know in the current code that cannot happen. But it should be caught in case of a programming error. Thus, should we add a BUG_ON(!areq->tsgl_entries)? Otherwise: Reviewed-by: Stephan Mueller <smueller@chronox.de> Ciao Stephan
On Tue, Nov 28, 2017 at 10:10:55AM +0100, Stephan Mueller wrote: > > diff --git a/crypto/af_alg.c b/crypto/af_alg.c > > index 358749c38894..415a54ced4d6 100644 > > --- a/crypto/af_alg.c > > +++ b/crypto/af_alg.c > > @@ -672,14 +672,15 @@ void af_alg_free_areq_sgls(struct af_alg_async_req > > *areq) } > > > > tsgl = areq->tsgl; > > - for_each_sg(tsgl, sg, areq->tsgl_entries, i) { > > - if (!sg_page(sg)) > > - continue; > > - put_page(sg_page(sg)); > > - } > > + if (tsgl) { > > + for_each_sg(tsgl, sg, areq->tsgl_entries, i) { > > + if (!sg_page(sg)) > > + continue; > > + put_page(sg_page(sg)); > > + } > > > > - if (areq->tsgl && areq->tsgl_entries) > > Why do you want to remove the check for areq->tsgl_entries? I know in the > current code that cannot happen. But it should be caught in case of a > programming error. > > Thus, should we add a BUG_ON(!areq->tsgl_entries)? > sock_kfree_s() works even if the size is 0. So there's no reason to check. Eric
On Tue, Nov 28, 2017 at 01:02:52AM -0800, Eric Biggers wrote: > > >From 1a7a7f86f09c50652f1fff75b8d3a32712826b32 Mon Sep 17 00:00:00 2001 > From: Eric Biggers <ebiggers@google.com> > Date: Tue, 28 Nov 2017 00:46:24 -0800 > Subject: [PATCH] crypto: af_alg - fix NULL pointer dereference in > af_alg_free_areq_sgls() > > If allocating the ->tsgl member of 'struct af_alg_async_req' failed, > during cleanup we dereferenced the NULL ->tsgl pointer in > af_alg_free_areq_sgls(), because ->tsgl_entries was nonzero. > > Fix it by only freeing the ->tsgl list if it is non-NULL. > > This affected both algif_skcipher and algif_aead. > > Fixes: e870456d8e7c ("crypto: algif_skcipher - overhaul memory management") > Fixes: d887c52d6ae4 ("crypto: algif_aead - overhaul memory management") > Reported-by: syzbot <syzkaller@googlegroups.com> > Cc: <stable@vger.kernel.org> # v4.14+ > Signed-off-by: Eric Biggers <ebiggers@google.com> Patch applied. Thanks.
On Wed, Nov 29, 2017 at 04:23:53PM +1100, Herbert Xu wrote: > On Tue, Nov 28, 2017 at 01:02:52AM -0800, Eric Biggers wrote: > > > > >From 1a7a7f86f09c50652f1fff75b8d3a32712826b32 Mon Sep 17 00:00:00 2001 > > From: Eric Biggers <ebiggers@google.com> > > Date: Tue, 28 Nov 2017 00:46:24 -0800 > > Subject: [PATCH] crypto: af_alg - fix NULL pointer dereference in > > af_alg_free_areq_sgls() > > > > If allocating the ->tsgl member of 'struct af_alg_async_req' failed, > > during cleanup we dereferenced the NULL ->tsgl pointer in > > af_alg_free_areq_sgls(), because ->tsgl_entries was nonzero. > > > > Fix it by only freeing the ->tsgl list if it is non-NULL. > > > > This affected both algif_skcipher and algif_aead. > > > > Fixes: e870456d8e7c ("crypto: algif_skcipher - overhaul memory management") > > Fixes: d887c52d6ae4 ("crypto: algif_aead - overhaul memory management") > > Reported-by: syzbot <syzkaller@googlegroups.com> > > Cc: <stable@vger.kernel.org> # v4.14+ > > Signed-off-by: Eric Biggers <ebiggers@google.com> > > Patch applied. Thanks. Herbert, if it's not too late can you fix the subject? It got split into two lines: commit 887207ed9e5812ed9239b6d07185a2d35dda91db Author: Eric Biggers <ebiggers@google.com> Date: Tue Nov 28 00:46:24 2017 -0800 crypto: af_alg - fix NULL pointer dereference in af_alg_free_areq_sgls() If allocating the ->tsgl member of 'struct af_alg_async_req' failed, during cleanup we dereferenced the NULL ->tsgl pointer in af_alg_free_areq_sgls(), because ->tsgl_entries was nonzero. Fix it by only freeing the ->tsgl list if it is non-NULL. This affected both algif_skcipher and algif_aead. Fixes: e870456d8e7c ("crypto: algif_skcipher - overhaul memory management") Fixes: d887c52d6ae4 ("crypto: algif_aead - overhaul memory management") Reported-by: syzbot <syzkaller@googlegroups.com> Cc: <stable@vger.kernel.org> # v4.14+ Signed-off-by: Eric Biggers <ebiggers@google.com> Reviewed-by: Stephan Mueller <smueller@chronox.de> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
On Wed, Nov 29, 2017 at 11:51:09AM -0800, Eric Biggers wrote: > > Herbert, if it's not too late can you fix the subject? It got split into two > lines: Sorry, it's already pushed out with other patches sitting on top of it. Cheers,
diff --git a/crypto/af_alg.c b/crypto/af_alg.c index 358749c38894..415a54ced4d6 100644 --- a/crypto/af_alg.c +++ b/crypto/af_alg.c @@ -672,14 +672,15 @@ void af_alg_free_areq_sgls(struct af_alg_async_req *areq) } tsgl = areq->tsgl; - for_each_sg(tsgl, sg, areq->tsgl_entries, i) { - if (!sg_page(sg)) - continue; - put_page(sg_page(sg)); - } + if (tsgl) { + for_each_sg(tsgl, sg, areq->tsgl_entries, i) { + if (!sg_page(sg)) + continue; + put_page(sg_page(sg)); + } - if (areq->tsgl && areq->tsgl_entries) sock_kfree_s(sk, tsgl, areq->tsgl_entries * sizeof(*tsgl)); + } } EXPORT_SYMBOL_GPL(af_alg_free_areq_sgls);