Patchwork crypto: AF_ALG - remove SGL end indicator when chaining

login
register
mail settings
Submitter Stephan Mueller
Date Aug. 30, 2017, 4:59 p.m.
Message ID <1913314.pET67Fvxje@positron.chronox.de>
Download mbox | patch
Permalink /patch/9930217/
State Not Applicable
Delegated to: Herbert Xu
Headers show

Comments

Stephan Mueller - Aug. 30, 2017, 4:59 p.m.
The SGL is MAX_SGL_ENTS + 1 in size. The last SG entry is used for the
chaining and is properly updated with the sg_chain invocation. During
the filling-in of the initial SG entries, sg_mark_end is called for each
SG entry. This is appropriate as long as no additional SGL is chained
with the current SGL. However, when a new SGL is chained and the last
SG entry is updated with sg_chain, the last but one entry still contains
the end marker from the sg_mark_end. This end marker must be removed as
otherwise a walk of the chained SGLs will cause a NULL pointer
dereference at the last but one SG entry, because sg_next will return
NULL.

Fixes: 8ff590903d5fc ("crypto: algif_skcipher - User-space interface
for skcipher operations")
CC: <stable@vger.kernel.org>
CC: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/algif_skcipher.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Herbert Xu - Sept. 20, 2017, 8:32 a.m.
On Wed, Aug 30, 2017 at 06:59:07PM +0200, Stephan Müller wrote:
> The SGL is MAX_SGL_ENTS + 1 in size. The last SG entry is used for the
> chaining and is properly updated with the sg_chain invocation. During
> the filling-in of the initial SG entries, sg_mark_end is called for each
> SG entry. This is appropriate as long as no additional SGL is chained
> with the current SGL. However, when a new SGL is chained and the last
> SG entry is updated with sg_chain, the last but one entry still contains
> the end marker from the sg_mark_end. This end marker must be removed as
> otherwise a walk of the chained SGLs will cause a NULL pointer
> dereference at the last but one SG entry, because sg_next will return
> NULL.
> 
> Fixes: 8ff590903d5fc ("crypto: algif_skcipher - User-space interface
> for skcipher operations")
> CC: <stable@vger.kernel.org>
> CC: Herbert Xu <herbert@gondor.apana.org.au>
> Signed-off-by: Stephan Mueller <smueller@chronox.de>

Hmm, this patch does not apply against the current tree.  Is this
a stable-only patch?

Cheers,
Stephan Mueller - Sept. 20, 2017, 1:47 p.m.
Am Mittwoch, 20. September 2017, 10:32:09 CEST schrieb Herbert Xu:

Hi Herbert,

> 
> Hmm, this patch does not apply against the current tree.  Is this
> a stable-only patch?

This would be a stable-only patch. With the overhauling of the AF_ALG memory 
handling, this is a no-issue any more.

Thanks

Ciao
Stephan
Greg Kroah-Hartman - Sept. 20, 2017, 5:31 p.m.
On Wed, Sep 20, 2017 at 03:47:46PM +0200, Stephan Mueller wrote:
> Am Mittwoch, 20. September 2017, 10:32:09 CEST schrieb Herbert Xu:
> 
> Hi Herbert,
> 
> > 
> > Hmm, this patch does not apply against the current tree.  Is this
> > a stable-only patch?
> 
> This would be a stable-only patch. With the overhauling of the AF_ALG memory 
> handling, this is a no-issue any more.

If you want this as a stable-only patch, you need to resend it and
justify it a bunch as to why it isn't in Linus's tree as well.

thanks,

greg k-h
Stephan Mueller - Sept. 21, 2017, 6:43 a.m.
Am Mittwoch, 20. September 2017, 19:31:33 CEST schrieb Greg KH:

Hi Herbert,

> On Wed, Sep 20, 2017 at 03:47:46PM +0200, Stephan Mueller wrote:
> > Am Mittwoch, 20. September 2017, 10:32:09 CEST schrieb Herbert Xu:
> > 
> > Hi Herbert,
> > 
> > > Hmm, this patch does not apply against the current tree.  Is this
> > > a stable-only patch?
> > 
> > This would be a stable-only patch. With the overhauling of the AF_ALG
> > memory handling, this is a no-issue any more.
> 
> If you want this as a stable-only patch, you need to resend it and
> justify it a bunch as to why it isn't in Linus's tree as well.

Would you push it or shall I send it?

Thanks

Ciao
Stephan
Herbert Xu - Sept. 21, 2017, 8:04 a.m.
On Thu, Sep 21, 2017 at 08:43:28AM +0200, Stephan Mueller wrote:
> Am Mittwoch, 20. September 2017, 19:31:33 CEST schrieb Greg KH:
> 
> Hi Herbert,
> 
> > On Wed, Sep 20, 2017 at 03:47:46PM +0200, Stephan Mueller wrote:
> > > Am Mittwoch, 20. September 2017, 10:32:09 CEST schrieb Herbert Xu:
> > > 
> > > Hi Herbert,
> > > 
> > > > Hmm, this patch does not apply against the current tree.  Is this
> > > > a stable-only patch?
> > > 
> > > This would be a stable-only patch. With the overhauling of the AF_ALG
> > > memory handling, this is a no-issue any more.
> > 
> > If you want this as a stable-only patch, you need to resend it and
> > justify it a bunch as to why it isn't in Linus's tree as well.
> 
> Would you push it or shall I send it?

Please resend it with details as to why this isn't needed on the
mainline kernel, i.e., due to the new code-base which has addressed
the bug in a different way but is too invasive for stable.

Thanks,

Patch

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 43839b00fe6c..62449a8f14ce 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -139,8 +139,10 @@  static int skcipher_alloc_sgl(struct sock *sk)
 		sg_init_table(sgl->sg, MAX_SGL_ENTS + 1);
 		sgl->cur = 0;
 
-		if (sg)
+		if (sg) {
 			sg_chain(sg, MAX_SGL_ENTS + 1, sgl->sg);
+			sg_unmark_end(sg + (MAX_SGL_ENTS - 1));
+		}
 
 		list_add_tail(&sgl->list, &ctx->tsgl);
 	}