* [PATCH] crypto: algif_aead - skip SGL entries with NULL page
@ 2017-11-10 7:29 Stephan Müller
2017-11-10 9:24 ` Stephan Müller
0 siblings, 1 reply; 6+ messages in thread
From: Stephan Müller @ 2017-11-10 7:29 UTC (permalink / raw)
To: herbert; +Cc: linux-crypto
The TX SGL may contain SGL entries that are assigned a NULL page. This
may happen if a multi-stage AIO operation is performed where the data
for each stage is pointed to by one SGL entry. Upon completion of that
stage, af_alg_pull_tsgl will assign NULL to the SGL entry.
The NULL cipher used to copy the AAD from TX SGL to the destination
buffer, however, cannot handle the case where the SGL starts with an SGL
entry having a NULL page. Thus, the code needs to advance the start
pointer into the SGL to the first non-NULL entry.
This fixes a crash visible on Intel x86 32 bit using the libkcapi test
suite.
Fixes: 72548b093ee38 ("crypto: algif_aead - copy AAD from src to dst")
Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
crypto/algif_aead.c | 24 +++++++++++++++++-------
1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 6cdd4fb08335..250816971edf 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -101,10 +101,10 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr *msg,
struct aead_tfm *aeadc = pask->private;
struct crypto_aead *tfm = aeadc->aead;
struct crypto_skcipher *null_tfm = aeadc->null_tfm;
- unsigned int as = crypto_aead_authsize(tfm);
+ unsigned int i, as = crypto_aead_authsize(tfm);
struct af_alg_async_req *areq;
struct af_alg_tsgl *tsgl;
- struct scatterlist *src;
+ struct scatterlist *rsgl_src, *tsgl_src = NULL;
int err = 0;
size_t used = 0; /* [in] TX bufs to be en/decrypted */
size_t outlen = 0; /* [out] RX bufs produced by kernel */
@@ -179,6 +179,16 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr *msg,
processed = used + ctx->aead_assoclen;
tsgl = list_first_entry(&ctx->tsgl_list, struct af_alg_tsgl, list);
+ for (i = 0; i < tsgl->cur; i++) {
+ if (!sg_page(tsgl->sg + i))
+ continue;
+ tsgl_src = tsgl->sg + i;
+ break;
+ }
+ if (!tsgl_src) {
+ err = -EFAULT;
+ goto free;
+ }
/*
* Copy of AAD from source to destination
@@ -194,7 +204,7 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr *msg,
*/
/* Use the RX SGL as source (and destination) for crypto op. */
- src = areq->first_rsgl.sgl.sg;
+ rsgl_src = areq->first_rsgl.sgl.sg;
if (ctx->enc) {
/*
@@ -207,7 +217,7 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr *msg,
* v v
* RX SGL: AAD || PT || Tag
*/
- err = crypto_aead_copy_sgl(null_tfm, tsgl->sg,
+ err = crypto_aead_copy_sgl(null_tfm, tsgl_src,
areq->first_rsgl.sgl.sg, processed);
if (err)
goto free;
@@ -225,7 +235,7 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr *msg,
*/
/* Copy AAD || CT to RX SGL buffer for in-place operation. */
- err = crypto_aead_copy_sgl(null_tfm, tsgl->sg,
+ err = crypto_aead_copy_sgl(null_tfm, tsgl_src,
areq->first_rsgl.sgl.sg, outlen);
if (err)
goto free;
@@ -257,11 +267,11 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr *msg,
areq->tsgl);
} else
/* no RX SGL present (e.g. authentication only) */
- src = areq->tsgl;
+ rsgl_src = areq->tsgl;
}
/* Initialize the crypto operation */
- aead_request_set_crypt(&areq->cra_u.aead_req, src,
+ aead_request_set_crypt(&areq->cra_u.aead_req, rsgl_src,
areq->first_rsgl.sgl.sg, used, ctx->iv);
aead_request_set_ad(&areq->cra_u.aead_req, ctx->aead_assoclen);
aead_request_set_tfm(&areq->cra_u.aead_req, tfm);
--
2.13.6
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] crypto: algif_aead - skip SGL entries with NULL page
2017-11-10 7:29 [PATCH] crypto: algif_aead - skip SGL entries with NULL page Stephan Müller
@ 2017-11-10 9:24 ` Stephan Müller
2017-11-10 9:51 ` [PATCH v2] " Stephan Müller
0 siblings, 1 reply; 6+ messages in thread
From: Stephan Müller @ 2017-11-10 9:24 UTC (permalink / raw)
To: herbert; +Cc: linux-crypto
Am Freitag, 10. November 2017, 08:29:40 CET schrieb Stephan Müller:
Hi,
> The TX SGL may contain SGL entries that are assigned a NULL page. This
> may happen if a multi-stage AIO operation is performed where the data
> for each stage is pointed to by one SGL entry. Upon completion of that
> stage, af_alg_pull_tsgl will assign NULL to the SGL entry.
>
> The NULL cipher used to copy the AAD from TX SGL to the destination
> buffer, however, cannot handle the case where the SGL starts with an SGL
> entry having a NULL page. Thus, the code needs to advance the start
> pointer into the SGL to the first non-NULL entry.
>
> This fixes a crash visible on Intel x86 32 bit using the libkcapi test
> suite.
This one still has an issue with zero input. I will send a fix shortly.
Ciao
Stephan
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] crypto: algif_aead - skip SGL entries with NULL page
2017-11-10 9:24 ` Stephan Müller
@ 2017-11-10 9:51 ` Stephan Müller
2017-11-10 10:04 ` [PATCH v3] " Stephan Müller
0 siblings, 1 reply; 6+ messages in thread
From: Stephan Müller @ 2017-11-10 9:51 UTC (permalink / raw)
To: herbert; +Cc: linux-crypto
The TX SGL may contain SGL entries that are assigned a NULL page. This
may happen if a multi-stage AIO operation is performed where the data
for each stage is pointed to by one SGL entry. Upon completion of that
stage, af_alg_pull_tsgl will assign NULL to the SGL entry.
The NULL cipher used to copy the AAD from TX SGL to the destination
buffer, however, cannot handle the case where the SGL starts with an SGL
entry having a NULL page. Thus, the code needs to advance the start
pointer into the SGL to the first non-NULL entry.
This fixes a crash visible on Intel x86 32 bit using the libkcapi test
suite.
Fixes: 72548b093ee38 ("crypto: algif_aead - copy AAD from src to dst")
Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
crypto/algif_aead.c | 31 ++++++++++++++++++++++---------
1 file changed, 22 insertions(+), 9 deletions(-)
diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 6cdd4fb08335..bea39102184d 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -101,10 +101,10 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr *msg,
struct aead_tfm *aeadc = pask->private;
struct crypto_aead *tfm = aeadc->aead;
struct crypto_skcipher *null_tfm = aeadc->null_tfm;
- unsigned int as = crypto_aead_authsize(tfm);
+ unsigned int i, as = crypto_aead_authsize(tfm);
struct af_alg_async_req *areq;
- struct af_alg_tsgl *tsgl;
- struct scatterlist *src;
+ struct af_alg_tsgl *tsgl, *tmp;
+ struct scatterlist *rsgl_src, *tsgl_src = NULL;
int err = 0;
size_t used = 0; /* [in] TX bufs to be en/decrypted */
size_t outlen = 0; /* [out] RX bufs produced by kernel */
@@ -178,7 +178,20 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr *msg,
}
processed = used + ctx->aead_assoclen;
- tsgl = list_first_entry(&ctx->tsgl_list, struct af_alg_tsgl, list);
+ list_for_each_entry_safe(tsgl, tmp, &ctx->tsgl_list, list) {
+ for (i = 0; i < tsgl->cur; i++) {
+ struct scatterlist *process_sg = tsgl->sg + i;
+
+ if (!(process_sg->length) || !sg_page(process_sg))
+ continue;
+ tsgl_src = process_sg;
+ break;
+ }
+ }
+ if (processed && !tsgl_src) {
+ err = -EFAULT;
+ goto free;
+ }
/*
* Copy of AAD from source to destination
@@ -194,7 +207,7 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr *msg,
*/
/* Use the RX SGL as source (and destination) for crypto op. */
- src = areq->first_rsgl.sgl.sg;
+ rsgl_src = areq->first_rsgl.sgl.sg;
if (ctx->enc) {
/*
@@ -207,7 +220,7 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr *msg,
* v v
* RX SGL: AAD || PT || Tag
*/
- err = crypto_aead_copy_sgl(null_tfm, tsgl->sg,
+ err = crypto_aead_copy_sgl(null_tfm, tsgl_src,
areq->first_rsgl.sgl.sg, processed);
if (err)
goto free;
@@ -225,7 +238,7 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr *msg,
*/
/* Copy AAD || CT to RX SGL buffer for in-place operation. */
- err = crypto_aead_copy_sgl(null_tfm, tsgl->sg,
+ err = crypto_aead_copy_sgl(null_tfm, tsgl_src,
areq->first_rsgl.sgl.sg, outlen);
if (err)
goto free;
@@ -257,11 +270,11 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr *msg,
areq->tsgl);
} else
/* no RX SGL present (e.g. authentication only) */
- src = areq->tsgl;
+ rsgl_src = areq->tsgl;
}
/* Initialize the crypto operation */
- aead_request_set_crypt(&areq->cra_u.aead_req, src,
+ aead_request_set_crypt(&areq->cra_u.aead_req, rsgl_src,
areq->first_rsgl.sgl.sg, used, ctx->iv);
aead_request_set_ad(&areq->cra_u.aead_req, ctx->aead_assoclen);
aead_request_set_tfm(&areq->cra_u.aead_req, tfm);
--
2.13.6
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3] crypto: algif_aead - skip SGL entries with NULL page
2017-11-10 9:51 ` [PATCH v2] " Stephan Müller
@ 2017-11-10 10:04 ` Stephan Müller
2017-11-24 7:37 ` Herbert Xu
0 siblings, 1 reply; 6+ messages in thread
From: Stephan Müller @ 2017-11-10 10:04 UTC (permalink / raw)
To: herbert; +Cc: linux-crypto
Hi Herbert,
I missed the termination of the outer loop of list_for_each_entry_safe.
The patch was tested on x86 64 and 32 bit environments.
---8<---
The TX SGL may contain SGL entries that are assigned a NULL page. This
may happen if a multi-stage AIO operation is performed where the data
for each stage is pointed to by one SGL entry. Upon completion of that
stage, af_alg_pull_tsgl will assign NULL to the SGL entry.
The NULL cipher used to copy the AAD from TX SGL to the destination
buffer, however, cannot handle the case where the SGL starts with an SGL
entry having a NULL page. Thus, the code needs to advance the start
pointer into the SGL to the first non-NULL entry.
This fixes a crash visible on Intel x86 32 bit using the libkcapi test
suite.
Fixes: 72548b093ee38 ("crypto: algif_aead - copy AAD from src to dst")
Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
crypto/algif_aead.c | 33 ++++++++++++++++++++++++---------
1 file changed, 24 insertions(+), 9 deletions(-)
diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 6cdd4fb08335..7822e2fecb0b 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -101,10 +101,10 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr *msg,
struct aead_tfm *aeadc = pask->private;
struct crypto_aead *tfm = aeadc->aead;
struct crypto_skcipher *null_tfm = aeadc->null_tfm;
- unsigned int as = crypto_aead_authsize(tfm);
+ unsigned int i, as = crypto_aead_authsize(tfm);
struct af_alg_async_req *areq;
- struct af_alg_tsgl *tsgl;
- struct scatterlist *src;
+ struct af_alg_tsgl *tsgl, *tmp;
+ struct scatterlist *rsgl_src, *tsgl_src = NULL;
int err = 0;
size_t used = 0; /* [in] TX bufs to be en/decrypted */
size_t outlen = 0; /* [out] RX bufs produced by kernel */
@@ -178,7 +178,22 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr *msg,
}
processed = used + ctx->aead_assoclen;
- tsgl = list_first_entry(&ctx->tsgl_list, struct af_alg_tsgl, list);
+ list_for_each_entry_safe(tsgl, tmp, &ctx->tsgl_list, list) {
+ for (i = 0; i < tsgl->cur; i++) {
+ struct scatterlist *process_sg = tsgl->sg + i;
+
+ if (!(process_sg->length) || !sg_page(process_sg))
+ continue;
+ tsgl_src = process_sg;
+ break;
+ }
+ if (tsgl_src)
+ break;
+ }
+ if (processed && !tsgl_src) {
+ err = -EFAULT;
+ goto free;
+ }
/*
* Copy of AAD from source to destination
@@ -194,7 +209,7 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr *msg,
*/
/* Use the RX SGL as source (and destination) for crypto op. */
- src = areq->first_rsgl.sgl.sg;
+ rsgl_src = areq->first_rsgl.sgl.sg;
if (ctx->enc) {
/*
@@ -207,7 +222,7 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr *msg,
* v v
* RX SGL: AAD || PT || Tag
*/
- err = crypto_aead_copy_sgl(null_tfm, tsgl->sg,
+ err = crypto_aead_copy_sgl(null_tfm, tsgl_src,
areq->first_rsgl.sgl.sg, processed);
if (err)
goto free;
@@ -225,7 +240,7 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr *msg,
*/
/* Copy AAD || CT to RX SGL buffer for in-place operation. */
- err = crypto_aead_copy_sgl(null_tfm, tsgl->sg,
+ err = crypto_aead_copy_sgl(null_tfm, tsgl_src,
areq->first_rsgl.sgl.sg, outlen);
if (err)
goto free;
@@ -257,11 +272,11 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr *msg,
areq->tsgl);
} else
/* no RX SGL present (e.g. authentication only) */
- src = areq->tsgl;
+ rsgl_src = areq->tsgl;
}
/* Initialize the crypto operation */
- aead_request_set_crypt(&areq->cra_u.aead_req, src,
+ aead_request_set_crypt(&areq->cra_u.aead_req, rsgl_src,
areq->first_rsgl.sgl.sg, used, ctx->iv);
aead_request_set_ad(&areq->cra_u.aead_req, ctx->aead_assoclen);
aead_request_set_tfm(&areq->cra_u.aead_req, tfm);
--
2.13.6
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3] crypto: algif_aead - skip SGL entries with NULL page
2017-11-10 10:04 ` [PATCH v3] " Stephan Müller
@ 2017-11-24 7:37 ` Herbert Xu
2017-11-24 16:09 ` Stephan Mueller
0 siblings, 1 reply; 6+ messages in thread
From: Herbert Xu @ 2017-11-24 7:37 UTC (permalink / raw)
To: Stephan Müller; +Cc: linux-crypto
On Fri, Nov 10, 2017 at 11:04:52AM +0100, Stephan Müller wrote:
> Hi Herbert,
>
> I missed the termination of the outer loop of list_for_each_entry_safe.
>
> The patch was tested on x86 64 and 32 bit environments.
>
> ---8<---
>
> The TX SGL may contain SGL entries that are assigned a NULL page. This
> may happen if a multi-stage AIO operation is performed where the data
> for each stage is pointed to by one SGL entry. Upon completion of that
> stage, af_alg_pull_tsgl will assign NULL to the SGL entry.
>
> The NULL cipher used to copy the AAD from TX SGL to the destination
> buffer, however, cannot handle the case where the SGL starts with an SGL
> entry having a NULL page. Thus, the code needs to advance the start
> pointer into the SGL to the first non-NULL entry.
>
> This fixes a crash visible on Intel x86 32 bit using the libkcapi test
> suite.
>
> Fixes: 72548b093ee38 ("crypto: algif_aead - copy AAD from src to dst")
> Signed-off-by: Stephan Mueller <smueller@chronox.de>
Patch applied. Thanks.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] crypto: algif_aead - skip SGL entries with NULL page
2017-11-24 7:37 ` Herbert Xu
@ 2017-11-24 16:09 ` Stephan Mueller
0 siblings, 0 replies; 6+ messages in thread
From: Stephan Mueller @ 2017-11-24 16:09 UTC (permalink / raw)
To: Herbert Xu; +Cc: linux-crypto
Am Freitag, 24. November 2017, 08:37:28 CET schrieb Herbert Xu:
Hi Herbert,
> On Fri, Nov 10, 2017 at 11:04:52AM +0100, Stephan Müller wrote:
> > Hi Herbert,
> >
> > I missed the termination of the outer loop of list_for_each_entry_safe.
> >
> > The patch was tested on x86 64 and 32 bit environments.
> >
> > ---8<---
> >
> > The TX SGL may contain SGL entries that are assigned a NULL page. This
> > may happen if a multi-stage AIO operation is performed where the data
> > for each stage is pointed to by one SGL entry. Upon completion of that
> > stage, af_alg_pull_tsgl will assign NULL to the SGL entry.
> >
> > The NULL cipher used to copy the AAD from TX SGL to the destination
> > buffer, however, cannot handle the case where the SGL starts with an SGL
> > entry having a NULL page. Thus, the code needs to advance the start
> > pointer into the SGL to the first non-NULL entry.
> >
> > This fixes a crash visible on Intel x86 32 bit using the libkcapi test
> > suite.
> >
> > Fixes: 72548b093ee38 ("crypto: algif_aead - copy AAD from src to dst")
> > Signed-off-by: Stephan Mueller <smueller@chronox.de>
>
> Patch applied. Thanks.
Would it make sense to feed it to stable?
Ciao
Stephan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-11-24 16:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-10 7:29 [PATCH] crypto: algif_aead - skip SGL entries with NULL page Stephan Müller
2017-11-10 9:24 ` Stephan Müller
2017-11-10 9:51 ` [PATCH v2] " Stephan Müller
2017-11-10 10:04 ` [PATCH v3] " Stephan Müller
2017-11-24 7:37 ` Herbert Xu
2017-11-24 16:09 ` Stephan Mueller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.