diff mbox

[v2,13/13] crypto: algif_aead - Switch to new AEAD interface

Message ID E1YviMC-00010E-Uk@gondolin.me.apana.org.au (mailing list archive)
State Changes Requested
Delegated to: Herbert Xu
Headers show

Commit Message

Herbert Xu May 22, 2015, 8:31 a.m. UTC 
This patch makes use of the new AEAD interface which uses a single
SG list instead of separate lists for the AD and plain text.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 crypto/algif_aead.c |   61 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 36 insertions(+), 25 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Stephan Mueller May 22, 2015, 8:59 p.m. UTC | #1 
Am Freitag, 22. Mai 2015, 16:31:04 schrieb Herbert Xu:

Hi Herbert,

> This patch makes use of the new AEAD interface which uses a single
> SG list instead of separate lists for the AD and plain text.

Using an up-to date tree with the full set of patches of this patch set, I get 
the following oops.

It can easily be reproduced by using [1]: go to libkcapi/test/ and compile 
with make. Then execute ./test.sh

[1] http://www.chronox.de/libkcapi.html



[   22.680910] BUG: unable to handle kernel NULL pointer dereference at           
(null)
[   22.680915] IP: [<          (null)>]           (null)
[   22.680917] PGD 3c62e067 PUD 3b28e067 PMD 0 
[   22.680919] Oops: 0010 [#1] SMP 
[   22.680921] Modules linked in: seqiv ccm gcm crypto_null algif_aead 
algif_skcipher sha512_ssse3 sha512_generic mcryptd sha1_ssse3 sha1_generic 
crypto_user des3_ede_x86_64 des_generic algif_hash af_alg 
nf_conntrack_netbios_ns nf_conntrack_broadcast ip6t_rpfilter ip6t_REJECT 
nf_reject_ipv6 nf_conntrack_ipv6 nf_defrag_ipv6 nf_conntrack_ipv4 
nf_defrag_ipv4 xt_conntrack nf_conntrack cfg80211 ebtable_nat ebtable_broute 
bridge stp llc ebtable_filter ebtables ip6table_mangle ip6table_security 
ip6table_raw ip6table_filter ip6_tables iptable_mangle iptable_security 
iptable_raw crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel 
aesni_intel aes_x86_64 glue_helper ablk_helper joydev microcode virtio_console 
serio_raw virtio_balloon pcspkr i2c_piix4 acpi_cpufreq qxl drm_kms_helper ttm 
drm virtio_net
[   22.680948]  virtio_blk virtio_pci virtio_ring virtio
[   22.680952] CPU: 1 PID: 1889 Comm: kcapi Not tainted 4.0.0+ #122
[   22.680954] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[   22.680955] task: ffff88003c08cc80 ti: ffff88003b300000 task.ti: 
ffff88003b300000
[   22.680956] RIP: 0010:[<0000000000000000>]  [<          (null)>]           
(null)
[   22.680958] RSP: 0018:ffff88003b303ce0  EFLAGS: 00010282
[   22.680959] RAX: ffffffffa02f5080 RBX: ffffffffa0138b20 RCX: 
0000000000000001
[   22.680960] RDX: 0000000000000001 RSI: ffffffffa02f5368 RDI: 
ffff88003b303cf8
[   22.680961] RBP: ffff88003b303d88 R08: 0000000000000000 R09: 
ffffea0000ecbd00
[   22.680962] R10: ffffffff810676b4 R11: ffff88003c275240 R12: 
ffff88003b1ff200
[   22.680963] R13: 00000000fffffffe R14: ffffffffa02f5080 R15: 
0000000000000203
[   22.680965] FS:  00007fade1fe8700(0000) GS:ffff88003fd00000(0000) 
knlGS:0000000000000000
[   22.680966] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   22.680967] CR2: 0000000000000000 CR3: 000000003bdc9000 CR4: 
00000000000407e0
[   22.680971] Stack:
[   22.680973]  ffffffff812b7e6d 0002000c00000003 0000020f00000203 
ffff88003b303cec
[   22.680975]  ffff88003b303d14 0000000000000000 00010044812b49c4 
2d36303134636672
[   22.680977]  6e7365612d6d6367 0000000000000069 0000000000000000 
0000000000000000
[   22.680979] Call Trace:
[   22.680984]  [<ffffffff812b7e6d>] ? crypto_nivaead_default+0x14d/0x1a0
[   22.680986]  [<ffffffff812b7f5a>] crypto_lookup_aead+0x9a/0xf0
[   22.680989]  [<ffffffff812b4f33>] crypto_alloc_tfm+0x63/0x130
[   22.680992]  [<ffffffff81193dd1>] ? kmem_cache_alloc_trace+0x1f1/0x230
[   22.680994]  [<ffffffff812b7fc9>] crypto_alloc_aead+0x19/0x20
[   22.680996]  [<ffffffffa02d638e>] aead_bind+0xe/0x10 [algif_aead]
[   22.680999]  [<ffffffffa02848d0>] alg_bind+0x60/0x130 [af_alg]
[   22.681003]  [<ffffffff81561f68>] SYSC_bind+0xb8/0xf0
[   22.681003]  [<ffffffff811c7eb5>] ? fd_install+0x25/0x30
[   22.681003]  [<ffffffff81562850>] ? SyS_socket+0x90/0xd0
[   22.681003]  [<ffffffff8104a0f7>] ? trace_do_page_fault+0x37/0xb0
[   22.681003]  [<ffffffff81562ade>] SyS_bind+0xe/0x10
[   22.681003]  [<ffffffff81687f6e>] system_call_fastpath+0x12/0x71
[   22.681003] Code:  Bad RIP value.
[   22.681003] RIP  [<          (null)>]           (null)
[   22.681003]  RSP <ffff88003b303ce0>
[   22.681003] CR2: 0000000000000000
[   22.681053] ---[ end trace c1a8ba963ebab541 ]---
Stephan Mueller May 22, 2015, 9:04 p.m. UTC | #2 
Am Freitag, 22. Mai 2015, 22:59:34 schrieb Stephan Mueller:

Hi Stephan,

> Am Freitag, 22. Mai 2015, 16:31:04 schrieb Herbert Xu:
> 
> Hi Herbert,
> 
> > This patch makes use of the new AEAD interface which uses a single
> > SG list instead of separate lists for the AD and plain text.
> 
> Using an up-to date tree with the full set of patches of this patch set, I
> get the following oops.
> 
> It can easily be reproduced by using [1]: go to libkcapi/test/ and compile
> with make. Then execute ./test.sh
> 
> [1] http://www.chronox.de/libkcapi.html

Note, gcm(aes) looks good. Only rfc4106(gcm(aes)) causes the crash.
Herbert Xu May 22, 2015, 9:54 p.m. UTC | #3 
On Fri, May 22, 2015 at 11:04:39PM +0200, Stephan Mueller wrote:
> Am Freitag, 22. Mai 2015, 22:59:34 schrieb Stephan Mueller:
> 
> Hi Stephan,
> 
> > Am Freitag, 22. Mai 2015, 16:31:04 schrieb Herbert Xu:
> > 
> > Hi Herbert,
> > 
> > > This patch makes use of the new AEAD interface which uses a single
> > > SG list instead of separate lists for the AD and plain text.
> > 
> > Using an up-to date tree with the full set of patches of this patch set, I
> > get the following oops.
> > 
> > It can easily be reproduced by using [1]: go to libkcapi/test/ and compile
> > with make. Then execute ./test.sh
> > 
> > [1] http://www.chronox.de/libkcapi.html
> 
> Note, gcm(aes) looks good. Only rfc4106(gcm(aes)) causes the crash.

Thanks for the report!

The crash is because ablkcipher/aead are still using tmpl->alloc
and I forgot about them.

The following two patches will fix the crash by making them call
tmpl->create if it is set.

Cheers,
Stephan Mueller May 23, 2015, 6:04 p.m. UTC | #4 
Am Freitag, 22. Mai 2015, 16:31:04 schrieb Herbert Xu:

Hi Herbert,

> This patch makes use of the new AEAD interface which uses a single
> SG list instead of separate lists for the AD and plain text.

After applying your additional patch, the "normal" AEAD operation works.

But with long messages (16 filled pages), I get the following. To test, simply 
use [1], cd libkcapi/test, compile and execute ./kcapi -y



[   59.441841] BUG: unable to handle kernel NULL pointer dereference at 
000000000000000c
[   59.441853] IP: [<ffffffff812b6d78>] scatterwalk_ffwd+0x28/0xd0
[   59.441866] PGD 78ad6067 PUD 799f5067 PMD 0 
[   59.441874] Oops: 0000 [#1] SMP 
[   59.441880] Modules linked in: ansi_cprng drbg algif_rng ccm gcm algif_aead 
algif_skcipher sha512_ssse3 sha512_generic mcryptd sha1_ssse3 sha1_generic 
crypto_user des3_ede_x86_64 des_generic cmac algif_hash af_alg 
nf_conntrack_netbios_ns nf_conntrack_broadcast ip6t_rpfilter ip6t_REJECT 
nf_reject_ipv6 nf_conntrack_ipv6 nf_defrag_ipv6 nf_conntrack_ipv4 
nf_defrag_ipv4 xt_conntrack nf_conntrack cfg80211 ebtable_nat ebtable_broute 
bridge stp llc ebtable_filter ebtables ip6table_mangle ip6table_security 
ip6table_raw ip6table_filter ip6_tables iptable_mangle iptable_security 
iptable_raw crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel 
aesni_intel aes_x86_64 glue_helper ablk_helper microcode joydev pcspkr 
serio_raw virtio_balloon i2c_piix4 acpi_cpufreq virtio_net qxl virtio_blk 
drm_kms_helper
[   59.441958]  ttm drm virtio_pci virtio_ring virtio
[   59.441970] CPU: 1 PID: 2338 Comm: kcapi Not tainted 4.0.0+ #220
[   59.441975] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
1.7.5-20140709_153950- 04/01/2014
[   59.441995] task: ffff88007aa1e600 ti: ffff880035998000 task.ti: 
ffff880035998000
[   59.441999] RIP: 0010:[<ffffffff812b6d78>]  [<ffffffff812b6d78>] 
scatterwalk_ffwd+0x28/0xd0
[   59.442007] RSP: 0018:ffff88003599ba98  EFLAGS: 00010202
[   59.442007] RAX: 0000000000000000 RBX: 0000000000006fe0 RCX: 
ffffea0001eaa500
[   59.442007] RDX: 0000000000001000 RSI: ffff88003599bb38 RDI: 
ffff88003599bc18
[   59.442007] RBP: ffff88003599baa8 R08: ffff88003599bcf8 R09: 
0000000000000000
[   59.442007] R10: 0000000000000000 R11: 0000000000001000 R12: 
ffff88007b802d90
[   59.442007] R13: ffff88007b3f3c40 R14: ffff88007b802d50 R15: 
ffff88007b800000
[   59.442007] FS:  00007f6cf9da6700(0000) GS:ffff88007fd00000(0000) 
knlGS:0000000000000000
[   59.442007] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   59.442007] CR2: 000000000000000c CR3: 00000000799a6000 CR4: 
00000000000407e0
[   59.442007] Stack:
[   59.442007]  ffff88007b802cf0 ffffffffa02f6380 ffff88003599bad8 
ffffffff812b7b40
[   59.442007]  ffff88007b802cb0 ffff88007b800008 0000000000000000 
ffff88007aa04000
[   59.442007]  ffff88003599bae8 ffffffff812b7c0d ffff88003599bd88 
ffffffffa02e5252
[   59.442007] Call Trace:
[   59.442007]  [<ffffffffa02f6380>] ? crypto_ccm_decrypt+0x350/0x350 [ccm]
[   59.442007]  [<ffffffff812b7b40>] old_crypt+0x50/0xe0
[   59.442007]  [<ffffffff812b7c0d>] old_encrypt+0x1d/0x20
[   59.442007]  [<ffffffffa02e5252>] aead_recvmsg+0x702/0x862 [algif_aead]
[   59.442007]  [<ffffffff8114a672>] ? __alloc_pages_nodemask+0x1a2/0x9d0
[   59.442007]  [<ffffffff81687b7a>] ? _raw_spin_unlock_bh+0x1a/0x20
[   59.442007]  [<ffffffffa02e4849>] ? aead_sendmsg+0x429/0x4c0 [algif_aead]
[   59.442007]  [<ffffffff81561528>] sock_recvmsg+0x38/0x50
[   59.442007]  [<ffffffff815615c8>] sock_read_iter+0x88/0xd0
[   59.442007]  [<ffffffff811a9990>] __vfs_read+0x90/0xc0
[   59.442007]  [<ffffffff811aa12a>] vfs_read+0x8a/0x140
[   59.442007]  [<ffffffff811aab56>] SyS_read+0x46/0xb0
[   59.442007]  [<ffffffff8168812e>] system_call_fastpath+0x12/0x71
[   59.442007] Code: 0f 1f 00 66 66 66 66 90 55 85 d2 48 89 f0 48 89 e5 41 54 
53 89 d3 74 28 8b 56 0c 49 89 fc 39 d3 73 10 eb 27 0f 1f 80 00 00 00 00 <8b> 
50 0c 39 da 77 19 29 d3 48 89 c7 e8 87 a9 05 00 85 db 75 eb 
[   59.442007] RIP  [<ffffffff812b6d78>] scatterwalk_ffwd+0x28/0xd0
[   59.442007]  RSP <ffff88003599ba98>
[   59.442007] CR2: 000000000000000c
[   59.442368] ---[ end trace 09389ca31f370515 ]---
diff mbox

Patch

diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 53702e9..5674a33 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -26,7 +26,7 @@ 
 
 struct aead_sg_list {
 	unsigned int cur;
-	struct scatterlist sg[ALG_MAX_PAGES];
+	struct scatterlist sg[ALG_MAX_PAGES + 1];
 };
 
 struct aead_ctx {
@@ -357,7 +357,8 @@  static int aead_recvmsg(struct socket *sock, struct msghdr *msg, size_t ignored,
 	unsigned as = crypto_aead_authsize(crypto_aead_reqtfm(&ctx->aead_req));
 	struct aead_sg_list *sgl = &ctx->tsgl;
 	struct scatterlist *sg = NULL;
-	struct scatterlist assoc[ALG_MAX_PAGES];
+	struct scatterlist dstbuf[ALG_MAX_PAGES + 1];
+	struct scatterlist *dst = dstbuf;
 	size_t assoclen = 0;
 	unsigned int i = 0;
 	int err = -EINVAL;
@@ -453,7 +454,7 @@  static int aead_recvmsg(struct socket *sock, struct msghdr *msg, size_t ignored,
 	if (usedpages < outlen)
 		goto unlock;
 
-	sg_init_table(assoc, ALG_MAX_PAGES);
+	sg_mark_end(sgl->sg + sgl->cur);
 	assoclen = ctx->aead_assoclen;
 	/*
 	 * Split scatterlist into two: first part becomes AD, second part
@@ -465,35 +466,45 @@  static int aead_recvmsg(struct socket *sock, struct msghdr *msg, size_t ignored,
 		sg = sgl->sg + i;
 		if (sg->length <= assoclen) {
 			/* AD is larger than one page */
-			sg_set_page(assoc + i, sg_page(sg),
+			sg_set_page(dst + i, sg_page(sg),
 				    sg->length, sg->offset);
 			assoclen -= sg->length;
-			if (i >= ctx->tsgl.cur)
-				goto unlock;
-		} else if (!assoclen) {
-			/* current page is to start of plaintext / ciphertext */
-			if (i)
-				/* AD terminates at page boundary */
-				sg_mark_end(assoc + i - 1);
-			else
-				/* AD size is zero */
-				sg_mark_end(assoc);
-			break;
-		} else {
+			continue;
+		}
+
+		if (assoclen) {
 			/* AD does not terminate at page boundary */
-			sg_set_page(assoc + i, sg_page(sg),
+			sg_set_page(dst + i, sg_page(sg),
 				    assoclen, sg->offset);
-			sg_mark_end(assoc + i);
-			/* plaintext / ciphertext starts after AD */
-			sg->length -= assoclen;
-			sg->offset += assoclen;
-			break;
+			assoclen = 0;
+			i++;
 		}
+
+		break;
 	}
 
-	aead_request_set_assoc(&ctx->aead_req, assoc, ctx->aead_assoclen);
-	aead_request_set_crypt(&ctx->aead_req, sg, ctx->rsgl[0].sg, used,
-			       ctx->iv);
+	/* This should never happen because of aead_sufficient_data. */
+	if (WARN_ON_ONCE(assoclen))
+		goto unlock;
+
+	/* current page is the start of plaintext / ciphertext */
+	if (!i)
+		/* AD size is zero */
+		dst = ctx->rsgl[0].sg;
+	else if (outlen)
+		/* AD size is non-zero */
+		scatterwalk_crypto_chain(
+			dst, ctx->rsgl[0].sg,
+			sg_page(ctx->rsgl[0].sg) == sg_page(dst + i - 1) &&
+			ctx->rsgl[0].sg[0].offset == dst[i - 1].offset +
+						     dst[i - 1].length,
+			i + 1);
+	else
+		/* AD only */
+		sg_mark_end(dst + i);
+
+	aead_request_set_crypt(&ctx->aead_req, sgl->sg, dst, used, ctx->iv);
+	aead_request_set_ad(&ctx->aead_req, ctx->aead_assoclen, 0);
 
 	err = af_alg_wait_for_completion(ctx->enc ?
 					 crypto_aead_encrypt(&ctx->aead_req) :