2015-05-21 10:44:20

by Herbert Xu

[permalink] [raw]
Subject: [PATCH 7/7] mac80211: Switch to new AEAD interface

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 <[email protected]>
---

net/mac80211/aes_ccm.c | 30 ++++++++++++++----------------
net/mac80211/aes_gcm.c | 30 ++++++++++++++----------------
net/mac80211/aes_gmac.c | 14 +++++---------
3 files changed, 33 insertions(+), 41 deletions(-)

diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
index 70d53da..42575ef 100644
--- a/net/mac80211/aes_ccm.c
+++ b/net/mac80211/aes_ccm.c
@@ -22,7 +22,7 @@ void ieee80211_aes_ccm_encrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
u8 *data, size_t data_len, u8 *mic,
size_t mic_len)
{
- struct scatterlist assoc, pt, ct[2];
+ struct scatterlist sg[3];

char aead_req_data[sizeof(struct aead_request) +
crypto_aead_reqsize(tfm)]
@@ -31,15 +31,14 @@ void ieee80211_aes_ccm_encrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,

memset(aead_req, 0, sizeof(aead_req_data));

- sg_init_one(&pt, data, data_len);
- sg_init_one(&assoc, &aad[2], be16_to_cpup((__be16 *)aad));
- sg_init_table(ct, 2);
- sg_set_buf(&ct[0], data, data_len);
- sg_set_buf(&ct[1], mic, mic_len);
+ sg_init_table(sg, 3);
+ sg_set_buf(&sg[0], &aad[2], be16_to_cpup((__be16 *)aad));
+ sg_set_buf(&sg[1], data, data_len);
+ sg_set_buf(&sg[2], mic, mic_len);

aead_request_set_tfm(aead_req, tfm);
- aead_request_set_assoc(aead_req, &assoc, assoc.length);
- aead_request_set_crypt(aead_req, &pt, ct, data_len, b_0);
+ aead_request_set_crypt(aead_req, sg, sg, data_len, b_0);
+ aead_request_set_ad(aead_req, sg[0].length, 0);

crypto_aead_encrypt(aead_req);
}
@@ -48,7 +47,7 @@ int ieee80211_aes_ccm_decrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
u8 *data, size_t data_len, u8 *mic,
size_t mic_len)
{
- struct scatterlist assoc, pt, ct[2];
+ struct scatterlist sg[3];
char aead_req_data[sizeof(struct aead_request) +
crypto_aead_reqsize(tfm)]
__aligned(__alignof__(struct aead_request));
@@ -59,15 +58,14 @@ int ieee80211_aes_ccm_decrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,

memset(aead_req, 0, sizeof(aead_req_data));

- sg_init_one(&pt, data, data_len);
- sg_init_one(&assoc, &aad[2], be16_to_cpup((__be16 *)aad));
- sg_init_table(ct, 2);
- sg_set_buf(&ct[0], data, data_len);
- sg_set_buf(&ct[1], mic, mic_len);
+ sg_init_table(sg, 3);
+ sg_set_buf(&sg[0], &aad[2], be16_to_cpup((__be16 *)aad));
+ sg_set_buf(&sg[1], data, data_len);
+ sg_set_buf(&sg[2], mic, mic_len);

aead_request_set_tfm(aead_req, tfm);
- aead_request_set_assoc(aead_req, &assoc, assoc.length);
- aead_request_set_crypt(aead_req, ct, &pt, data_len + mic_len, b_0);
+ aead_request_set_crypt(aead_req, sg, sg, data_len + mic_len, b_0);
+ aead_request_set_ad(aead_req, sg[0].length, 0);

return crypto_aead_decrypt(aead_req);
}
diff --git a/net/mac80211/aes_gcm.c b/net/mac80211/aes_gcm.c
index b91c9d7..12dcd66 100644
--- a/net/mac80211/aes_gcm.c
+++ b/net/mac80211/aes_gcm.c
@@ -18,7 +18,7 @@
void ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
u8 *data, size_t data_len, u8 *mic)
{
- struct scatterlist assoc, pt, ct[2];
+ struct scatterlist sg[3];

char aead_req_data[sizeof(struct aead_request) +
crypto_aead_reqsize(tfm)]
@@ -27,15 +27,14 @@ void ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,

memset(aead_req, 0, sizeof(aead_req_data));

- sg_init_one(&pt, data, data_len);
- sg_init_one(&assoc, &aad[2], be16_to_cpup((__be16 *)aad));
- sg_init_table(ct, 2);
- sg_set_buf(&ct[0], data, data_len);
- sg_set_buf(&ct[1], mic, IEEE80211_GCMP_MIC_LEN);
+ sg_init_table(sg, 3);
+ sg_set_buf(&sg[0], &aad[2], be16_to_cpup((__be16 *)aad));
+ sg_set_buf(&sg[1], data, data_len);
+ sg_set_buf(&sg[2], mic, IEEE80211_GCMP_MIC_LEN);

aead_request_set_tfm(aead_req, tfm);
- aead_request_set_assoc(aead_req, &assoc, assoc.length);
- aead_request_set_crypt(aead_req, &pt, ct, data_len, j_0);
+ aead_request_set_crypt(aead_req, sg, sg, data_len, j_0);
+ aead_request_set_ad(aead_req, sg[0].length, 0);

crypto_aead_encrypt(aead_req);
}
@@ -43,7 +42,7 @@ void ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
int ieee80211_aes_gcm_decrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
u8 *data, size_t data_len, u8 *mic)
{
- struct scatterlist assoc, pt, ct[2];
+ struct scatterlist sg[3];
char aead_req_data[sizeof(struct aead_request) +
crypto_aead_reqsize(tfm)]
__aligned(__alignof__(struct aead_request));
@@ -54,16 +53,15 @@ int ieee80211_aes_gcm_decrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,

memset(aead_req, 0, sizeof(aead_req_data));

- sg_init_one(&pt, data, data_len);
- sg_init_one(&assoc, &aad[2], be16_to_cpup((__be16 *)aad));
- sg_init_table(ct, 2);
- sg_set_buf(&ct[0], data, data_len);
- sg_set_buf(&ct[1], mic, IEEE80211_GCMP_MIC_LEN);
+ sg_init_table(sg, 3);
+ sg_set_buf(&sg[0], &aad[2], be16_to_cpup((__be16 *)aad));
+ sg_set_buf(&sg[1], data, data_len);
+ sg_set_buf(&sg[2], mic, IEEE80211_GCMP_MIC_LEN);

aead_request_set_tfm(aead_req, tfm);
- aead_request_set_assoc(aead_req, &assoc, assoc.length);
- aead_request_set_crypt(aead_req, ct, &pt,
+ aead_request_set_crypt(aead_req, sg, sg,
data_len + IEEE80211_GCMP_MIC_LEN, j_0);
+ aead_request_set_ad(aead_req, sg[0].length, 0);

return crypto_aead_decrypt(aead_req);
}
diff --git a/net/mac80211/aes_gmac.c b/net/mac80211/aes_gmac.c
index c34b06ca..7eee32b 100644
--- a/net/mac80211/aes_gmac.c
+++ b/net/mac80211/aes_gmac.c
@@ -24,34 +24,30 @@
int ieee80211_aes_gmac(struct crypto_aead *tfm, const u8 *aad, u8 *nonce,
const u8 *data, size_t data_len, u8 *mic)
{
- struct scatterlist sg[3], ct[1];
+ struct scatterlist sg[3];
char aead_req_data[sizeof(struct aead_request) +
crypto_aead_reqsize(tfm)]
__aligned(__alignof__(struct aead_request));
struct aead_request *aead_req = (void *)aead_req_data;
- u8 zero[GMAC_MIC_LEN], iv[AES_BLOCK_SIZE];
+ u8 iv[AES_BLOCK_SIZE];

if (data_len < GMAC_MIC_LEN)
return -EINVAL;

memset(aead_req, 0, sizeof(aead_req_data));

- memset(zero, 0, GMAC_MIC_LEN);
sg_init_table(sg, 3);
sg_set_buf(&sg[0], aad, AAD_LEN);
sg_set_buf(&sg[1], data, data_len - GMAC_MIC_LEN);
- sg_set_buf(&sg[2], zero, GMAC_MIC_LEN);
+ sg_set_buf(&sg[2], mic, GMAC_MIC_LEN);

memcpy(iv, nonce, GMAC_NONCE_LEN);
memset(iv + GMAC_NONCE_LEN, 0, sizeof(iv) - GMAC_NONCE_LEN);
iv[AES_BLOCK_SIZE - 1] = 0x01;

- sg_init_table(ct, 1);
- sg_set_buf(&ct[0], mic, GMAC_MIC_LEN);
-
aead_request_set_tfm(aead_req, tfm);
- aead_request_set_assoc(aead_req, sg, AAD_LEN + data_len);
- aead_request_set_crypt(aead_req, NULL, ct, 0, iv);
+ aead_request_set_crypt(aead_req, sg, sg, 0, iv);
+ aead_request_set_ad(aead_req, AAD_LEN + data_len, 0);

crypto_aead_encrypt(aead_req);



2015-05-21 11:20:49

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH 7/7] mac80211: Switch to new AEAD interface

On Thu, 2015-05-21 at 18:44 +0800, Herbert Xu wrote:
> 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.

Looks fine - want me to run any tests on it?

johannes

2015-05-21 11:50:27

by Herbert Xu

[permalink] [raw]
Subject: Re: [PATCH 7/7] mac80211: Switch to new AEAD interface

On Thu, May 21, 2015 at 01:20:49PM +0200, Johannes Berg wrote:
> On Thu, 2015-05-21 at 18:44 +0800, Herbert Xu wrote:
> > 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.
>
> Looks fine - want me to run any tests on it?

That would be great!

However, they depend on a series which has not been merged into
cryptodev yet so you'll need to apply the following pathces first:

https://www.mail-archive.com/[email protected]/msg14270.html

Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2015-05-21 12:17:49

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH 7/7] mac80211: Switch to new AEAD interface

On Thu, 2015-05-21 at 19:50 +0800, Herbert Xu wrote:
> On Thu, May 21, 2015 at 01:20:49PM +0200, Johannes Berg wrote:
> > On Thu, 2015-05-21 at 18:44 +0800, Herbert Xu wrote:
> > > 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.
> >
> > Looks fine - want me to run any tests on it?
>
> That would be great!
>
> However, they depend on a series which has not been merged into
> cryptodev yet so you'll need to apply the following pathces first:
>
> https://www.mail-archive.com/[email protected]/msg14270.html

Do you have a branch somewhere with all of that?

johannes

2015-05-22 04:11:53

by Herbert Xu

[permalink] [raw]
Subject: Re: [PATCH 7/7] mac80211: Switch to new AEAD interface

On Thu, May 21, 2015 at 02:17:44PM +0200, Johannes Berg wrote:
>
> Do you have a branch somewhere with all of that?

OK the prerequisite patches are now in cryptodev.

Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2015-05-22 07:32:33

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH 7/7] mac80211: Switch to new AEAD interface

On Thu, 2015-05-21 at 18:44 +0800, Herbert Xu wrote:
> 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.

The CCM and GCM part seems to work, but GMAC causes a kernel crash:

[ 26.143579] BUG: unable to handle kernel NULL pointer dereference at (null)
[ 26.144406] IP: [<ffffffff811d9e7d>] scatterwalk_map_and_copy+0x3d/0xd0
[ 26.145071] PGD da3a067 PUD d9ee067 PMD 0
[ 26.145514] Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
[ 26.146146] CPU: 1 PID: 661 Comm: hostapd Not tainted 4.0.0+ #860
[ 26.146746] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
[ 26.148333] task: ffff88000d9a4a20 ti: ffff880000070000 task.ti: ffff880000070000
[ 26.149625] RIP: 0010:[<ffffffff811d9e7d>] [<ffffffff811d9e7d>] scatterwalk_map_and_copy+0x3d/0xd0
[ 26.151223] RSP: 0018:ffff8800000733b8 EFLAGS: 00010246
[ 26.152156] RAX: 0000000000000000 RBX: 0000000000000010 RCX: 000077ff80000000
[ 26.153396] RDX: 0000000080000000 RSI: 0000000000000000 RDI: ffff8800000733c8
[ 26.153481] RBP: ffff880000073428 R08: 0000000000000001 R09: 0000000000000010
[ 26.153481] R10: 0000000000000010 R11: 0000000000000012 R12: 0000000000000001
[ 26.153481] R13: ffff8800000735f8 R14: 0000000000000000 R15: 0000000000000030
[ 26.153481] FS: 00007f20eee60700(0000) GS:ffff88000f600000(0000) knlGS:0000000000000000
[ 26.153481] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 26.153481] CR2: 0000000000000000 CR3: 000000000da2a000 CR4: 00000000000007a0
[ 26.153481] Stack:
[ 26.153481] 0000000000000000 0000000000000030 ffff8800000733d8 ffffffff811e05c6
[ 26.153481] ffff8800000733f8 ffffffff811df815 ffff8800000735f8 ffff880000073598
[ 26.153481] ffff880000073408 ffffffff811dfc86 ffff880000073438 ffff8800000735f8
[ 26.153481] Call Trace:
[ 26.153481] [<ffffffff811e05c6>] ? shash_async_final+0x16/0x20
[ 26.153481] [<ffffffff811df815>] ? crypto_ahash_op+0x25/0x60
[ 26.153481] [<ffffffff811dfc86>] ? crypto_ahash_final+0x16/0x20
[ 26.153481] [<ffffffff811e3608>] gcm_enc_copy_hash+0x28/0x30
[ 26.153481] [<ffffffff811e36fc>] crypto_gcm_encrypt+0xec/0x100
[ 26.153481] [<ffffffff811e3610>] ? gcm_enc_copy_hash+0x30/0x30
[ 26.153481] [<ffffffff811da875>] old_crypt+0xc5/0xe0
[ 26.153481] [<ffffffff811da8cd>] old_encrypt+0x1d/0x20
[ 26.153481] [<ffffffff814b688b>] ieee80211_aes_gmac+0x21b/0x230
[ 26.153481] [<ffffffff811e3710>] ? crypto_gcm_encrypt+0x100/0x100
[ 26.153481] [<ffffffff811e2f10>] ? __gcm_hash_final_done+0x60/0x60
[ 26.153481] [<ffffffff814b66a4>] ? ieee80211_aes_gmac+0x34/0x230
[ 26.153481] [<ffffffff81498621>] ieee80211_crypto_aes_gmac_encrypt+0x191/0x1a0
[ 26.153481] [<ffffffff8153b794>] ieee80211_tx_h_encrypt+0x67/0x77
[ 26.153481] [<ffffffff814cd496>] invoke_tx_handlers+0xe6/0x1b0

johannes

2015-05-22 07:41:33

by Herbert Xu

[permalink] [raw]
Subject: Re: [PATCH 7/7] mac80211: Switch to new AEAD interface

On Fri, May 22, 2015 at 09:32:28AM +0200, Johannes Berg wrote:
>
> The CCM and GCM part seems to work, but GMAC causes a kernel crash:

Awesome :)

> [ 26.143579] BUG: unable to handle kernel NULL pointer dereference at (null)
> [ 26.144406] IP: [<ffffffff811d9e7d>] scatterwalk_map_and_copy+0x3d/0xd0
> [ 26.145071] PGD da3a067 PUD d9ee067 PMD 0
> [ 26.145514] Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
> [ 26.146146] CPU: 1 PID: 661 Comm: hostapd Not tainted 4.0.0+ #860
> [ 26.146746] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
> [ 26.148333] task: ffff88000d9a4a20 ti: ffff880000070000 task.ti: ffff880000070000
> [ 26.149625] RIP: 0010:[<ffffffff811d9e7d>] [<ffffffff811d9e7d>] scatterwalk_map_and_copy+0x3d/0xd0
> [ 26.151223] RSP: 0018:ffff8800000733b8 EFLAGS: 00010246
> [ 26.152156] RAX: 0000000000000000 RBX: 0000000000000010 RCX: 000077ff80000000
> [ 26.153396] RDX: 0000000080000000 RSI: 0000000000000000 RDI: ffff8800000733c8
> [ 26.153481] RBP: ffff880000073428 R08: 0000000000000001 R09: 0000000000000010
> [ 26.153481] R10: 0000000000000010 R11: 0000000000000012 R12: 0000000000000001
> [ 26.153481] R13: ffff8800000735f8 R14: 0000000000000000 R15: 0000000000000030
> [ 26.153481] FS: 00007f20eee60700(0000) GS:ffff88000f600000(0000) knlGS:0000000000000000
> [ 26.153481] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 26.153481] CR2: 0000000000000000 CR3: 000000000da2a000 CR4: 00000000000007a0
> [ 26.153481] Stack:
> [ 26.153481] 0000000000000000 0000000000000030 ffff8800000733d8 ffffffff811e05c6
> [ 26.153481] ffff8800000733f8 ffffffff811df815 ffff8800000735f8 ffff880000073598
> [ 26.153481] ffff880000073408 ffffffff811dfc86 ffff880000073438 ffff8800000735f8
> [ 26.153481] Call Trace:
> [ 26.153481] [<ffffffff811e05c6>] ? shash_async_final+0x16/0x20
> [ 26.153481] [<ffffffff811df815>] ? crypto_ahash_op+0x25/0x60
> [ 26.153481] [<ffffffff811dfc86>] ? crypto_ahash_final+0x16/0x20
> [ 26.153481] [<ffffffff811e3608>] gcm_enc_copy_hash+0x28/0x30
> [ 26.153481] [<ffffffff811e36fc>] crypto_gcm_encrypt+0xec/0x100
> [ 26.153481] [<ffffffff811e3610>] ? gcm_enc_copy_hash+0x30/0x30
> [ 26.153481] [<ffffffff811da875>] old_crypt+0xc5/0xe0
> [ 26.153481] [<ffffffff811da8cd>] old_encrypt+0x1d/0x20
> [ 26.153481] [<ffffffff814b688b>] ieee80211_aes_gmac+0x21b/0x230
> [ 26.153481] [<ffffffff811e3710>] ? crypto_gcm_encrypt+0x100/0x100
> [ 26.153481] [<ffffffff811e2f10>] ? __gcm_hash_final_done+0x60/0x60
> [ 26.153481] [<ffffffff814b66a4>] ? ieee80211_aes_gmac+0x34/0x230
> [ 26.153481] [<ffffffff81498621>] ieee80211_crypto_aes_gmac_encrypt+0x191/0x1a0
> [ 26.153481] [<ffffffff8153b794>] ieee80211_tx_h_encrypt+0x67/0x77
> [ 26.153481] [<ffffffff814cd496>] invoke_tx_handlers+0xe6/0x1b0

Did this have a code section at the end? Without it it's difficult
to pin-point the crash because your compiler produces different
output than mine.

Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2015-05-22 07:43:31

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH 7/7] mac80211: Switch to new AEAD interface

On Fri, 2015-05-22 at 15:41 +0800, Herbert Xu wrote:

> Did this have a code section at the end? Without it it's difficult
> to pin-point the crash because your compiler produces different
> output than mine.

Oops, sorry, of course - I was running in a VM :)

[ 26.143579] BUG: unable to handle kernel NULL pointer dereference at (null)
[ 26.144406] IP: [<ffffffff811d9e7d>] scatterwalk_map_and_copy+0x3d/0xd0
[ 26.145071] PGD da3a067 PUD d9ee067 PMD 0
[ 26.145514] Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
[ 26.146146] CPU: 1 PID: 661 Comm: hostapd Not tainted 4.0.0+ #860
[ 26.146746] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
[ 26.148333] task: ffff88000d9a4a20 ti: ffff880000070000 task.ti: ffff880000070000
[ 26.149625] RIP: 0010:[<ffffffff811d9e7d>] [<ffffffff811d9e7d>] scatterwalk_map_and_copy+0x3d/0xd0
[ 26.151223] RSP: 0018:ffff8800000733b8 EFLAGS: 00010246
[ 26.152156] RAX: 0000000000000000 RBX: 0000000000000010 RCX: 000077ff80000000
[ 26.153396] RDX: 0000000080000000 RSI: 0000000000000000 RDI: ffff8800000733c8
[ 26.153481] RBP: ffff880000073428 R08: 0000000000000001 R09: 0000000000000010
[ 26.153481] R10: 0000000000000010 R11: 0000000000000012 R12: 0000000000000001
[ 26.153481] R13: ffff8800000735f8 R14: 0000000000000000 R15: 0000000000000030
[ 26.153481] FS: 00007f20eee60700(0000) GS:ffff88000f600000(0000) knlGS:0000000000000000
[ 26.153481] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 26.153481] CR2: 0000000000000000 CR3: 000000000da2a000 CR4: 00000000000007a0
[ 26.153481] Stack:
[ 26.153481] 0000000000000000 0000000000000030 ffff8800000733d8 ffffffff811e05c6
[ 26.153481] ffff8800000733f8 ffffffff811df815 ffff8800000735f8 ffff880000073598
[ 26.153481] ffff880000073408 ffffffff811dfc86 ffff880000073438 ffff8800000735f8
[ 26.153481] Call Trace:
[ 26.153481] [<ffffffff811e05c6>] ? shash_async_final+0x16/0x20
[ 26.153481] [<ffffffff811df815>] ? crypto_ahash_op+0x25/0x60
[ 26.153481] [<ffffffff811dfc86>] ? crypto_ahash_final+0x16/0x20
[ 26.153481] [<ffffffff811e3608>] gcm_enc_copy_hash+0x28/0x30
[ 26.153481] [<ffffffff811e36fc>] crypto_gcm_encrypt+0xec/0x100
[ 26.153481] [<ffffffff811e3610>] ? gcm_enc_copy_hash+0x30/0x30
[ 26.153481] [<ffffffff811da875>] old_crypt+0xc5/0xe0
[ 26.153481] [<ffffffff811da8cd>] old_encrypt+0x1d/0x20
[ 26.153481] [<ffffffff814b688b>] ieee80211_aes_gmac+0x21b/0x230
[...]
[ 26.153481] [<ffffffff81543dee>] system_call_fastpath+0x12/0x76
[ 26.153481] Code: 89 e5 41 55 49 89 fd 41 54 48 8d 7d a0 45 89 c4 53 89 cb 48 83 ec 58 e8 12 ff ff ff ba 00 00 00 80 48 b9 00 00 00 80 ff 77 00 00 <48> 8b 30 48 83 e6 fc 4c 01 ea 48 0f 42 0d 81 31 63 00 48 01 ca
[ 26.153481] RIP [<ffffffff811d9e7d>] scatterwalk_map_and_copy
+0x3d/0xd0
[ 26.153481] RSP <ffff8800000733b8>
[ 26.153481] CR2: 0000000000000000
[ 26.153481] ---[ end trace b6af799d0103eb26 ]---

johannes

2015-05-22 08:05:44

by Herbert Xu

[permalink] [raw]
Subject: Re: [PATCH 7/7] mac80211: Switch to new AEAD interface

On Fri, May 22, 2015 at 09:43:28AM +0200, Johannes Berg wrote:
>
> Oops, sorry, of course - I was running in a VM :)

Thanks!

Does this patch on top help?

diff --git a/net/mac80211/aes_gmac.c b/net/mac80211/aes_gmac.c
index 7eee32b..133be53 100644
--- a/net/mac80211/aes_gmac.c
+++ b/net/mac80211/aes_gmac.c
@@ -24,22 +24,24 @@
int ieee80211_aes_gmac(struct crypto_aead *tfm, const u8 *aad, u8 *nonce,
const u8 *data, size_t data_len, u8 *mic)
{
- struct scatterlist sg[3];
+ struct scatterlist sg[4];
char aead_req_data[sizeof(struct aead_request) +
crypto_aead_reqsize(tfm)]
__aligned(__alignof__(struct aead_request));
struct aead_request *aead_req = (void *)aead_req_data;
- u8 iv[AES_BLOCK_SIZE];
+ u8 zero[GMAC_MIC_LEN], iv[AES_BLOCK_SIZE];

if (data_len < GMAC_MIC_LEN)
return -EINVAL;

memset(aead_req, 0, sizeof(aead_req_data));

- sg_init_table(sg, 3);
+ memset(zero, 0, GMAC_MIC_LEN);
+ sg_init_table(sg, 4);
sg_set_buf(&sg[0], aad, AAD_LEN);
sg_set_buf(&sg[1], data, data_len - GMAC_MIC_LEN);
- sg_set_buf(&sg[2], mic, GMAC_MIC_LEN);
+ sg_set_buf(&sg[2], zero, GMAC_MIC_LEN);
+ sg_set_buf(&sg[3], mic, GMAC_MIC_LEN);

memcpy(iv, nonce, GMAC_NONCE_LEN);
memset(iv + GMAC_NONCE_LEN, 0, sizeof(iv) - GMAC_NONCE_LEN);
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2015-05-22 08:18:03

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH 7/7] mac80211: Switch to new AEAD interface

On Fri, 2015-05-22 at 16:05 +0800, Herbert Xu wrote:
> On Fri, May 22, 2015 at 09:43:28AM +0200, Johannes Berg wrote:
> >
> > Oops, sorry, of course - I was running in a VM :)
>
> Thanks!
>
> Does this patch on top help?

Yep, that fixes things.

johannes

2015-05-22 08:19:43

by Herbert Xu

[permalink] [raw]
Subject: Re: [PATCH 7/7] mac80211: Switch to new AEAD interface

On Fri, May 22, 2015 at 10:18:03AM +0200, Johannes Berg wrote:
>
> Yep, that fixes things.

Great I will respin the patches.

Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2015-06-01 13:21:50

by Stephan Müller

[permalink] [raw]
Subject: Re: [PATCH 7/7] mac80211: Switch to new AEAD interface

Am Donnerstag, 21. Mai 2015, 13:20:49 schrieb Johannes Berg:

Hi Johannes,

> On Thu, 2015-05-21 at 18:44 +0800, Herbert Xu wrote:
> > 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.
>
> Looks fine - want me to run any tests on it?

Just a short question on ieee80211_aes_ccm_encrypt, ieee80211_aes_ccm_decrypt,
ieee80211_aes_gcm_encrypt, ieee80211_aes_gcm_decrypt, ieee80211_aes_gmac: can
the aad parameter of these functions be zero?

--
Ciao
Stephan

2015-06-01 13:42:41

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH 7/7] mac80211: Switch to new AEAD interface

On Mon, 2015-06-01 at 15:21 +0200, Stephan Mueller wrote:

> Just a short question on ieee80211_aes_ccm_encrypt, ieee80211_aes_ccm_decrypt,
> ieee80211_aes_gcm_encrypt, ieee80211_aes_gcm_decrypt, ieee80211_aes_gmac: can
> the aad parameter of these functions be zero?

What do you mean by "zero"? The pointer itself can clearly never be
NULL.

The contents, now, that's a more interesting question. I believe it can
never be all zeroes, since association request frames are not
encrypted/protected and thus at least one byte in here must be non-zero.
The MAC addresses are also very likely non-zero, but technically
00:00:00:00:00:00 is a valid MAC address I believe.

johannes

2015-06-01 13:49:58

by Stephan Müller

[permalink] [raw]
Subject: Re: [PATCH 7/7] mac80211: Switch to new AEAD interface

Am Montag, 1. Juni 2015, 15:42:41 schrieb Johannes Berg:

Hi Johannes,

>On Mon, 2015-06-01 at 15:21 +0200, Stephan Mueller wrote:
>> Just a short question on ieee80211_aes_ccm_encrypt,
>> ieee80211_aes_ccm_decrypt, ieee80211_aes_gcm_encrypt,
>> ieee80211_aes_gcm_decrypt, ieee80211_aes_gmac: can the aad parameter of
>> these functions be zero?
>
>What do you mean by "zero"? The pointer itself can clearly never be
>NULL.

Thanks for clarifying: indeed I mean the value of the pointer, not the pointer
itself :-)
>
>The contents, now, that's a more interesting question. I believe it can
>never be all zeroes, since association request frames are not
>encrypted/protected and thus at least one byte in here must be non-zero.
>The MAC addresses are also very likely non-zero, but technically
>00:00:00:00:00:00 is a valid MAC address I believe.

So, even when having a malicious AP, that value is never zero? The driver of
the question is the following code in the patch set:

+ sg_set_buf(&sg[0], &aad[2], be16_to_cpup((__be16 *)aad));

...

+ aead_request_set_crypt(aead_req, sg, sg, data_len, b_0);

...

crypto_aead_encrypt(aead_req);


When I played around with the aead_request_set_crypt, I saw a crash in the
scatterlist handling of the crypto API when the first SGL entry has a zero
length.

Ciao
Stephan

2015-06-01 14:05:19

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH 7/7] mac80211: Switch to new AEAD interface

On Mon, 2015-06-01 at 15:49 +0200, Stephan Mueller wrote:

> >The contents, now, that's a more interesting question. I believe it can
> >never be all zeroes, since association request frames are not
> >encrypted/protected and thus at least one byte in here must be non-zero.
> >The MAC addresses are also very likely non-zero, but technically
> >00:00:00:00:00:00 is a valid MAC address I believe.
>
> So, even when having a malicious AP, that value is never zero? The driver of
> the question is the following code in the patch set:
>
> + sg_set_buf(&sg[0], &aad[2], be16_to_cpup((__be16 *)aad));
>
> ...
>
> + aead_request_set_crypt(aead_req, sg, sg, data_len, b_0);
>
> ...
>
> crypto_aead_encrypt(aead_req);
>
>
> When I played around with the aead_request_set_crypt, I saw a crash in the
> scatterlist handling of the crypto API when the first SGL entry has a zero
> length.

Wait, I guess that's a *third* way for this to be "zero" a valid pointer
but zero length data?

Oh, no - you're referring to the CCM/GCM cases only, I guess, i.e. this
part:

- sg_init_one(&assoc, &aad[2], be16_to_cpup((__be16 *)aad));
+ sg_set_buf(&sg[0], &aad[2], be16_to_cpup((__be16 *)aad));

I was looking at GMAC and that has a constant for the length :-)

Ok - here the length is kinda passed a part of the AAD buffer, but this
is really just some arcane code that should be fixed to use a proper
struct. The value there, even though it is __be16 and looks like it came
from the data, is actually created locally, see ccmp_special_blocks()
and gcmp_special_blocks().

johannes

2015-06-01 14:35:32

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH 7/7] mac80211: Switch to new AEAD interface

On Mon, 2015-06-01 at 16:05 +0200, Johannes Berg wrote:

> Ok - here the length is kinda passed a part of the AAD buffer, but this
> is really just some arcane code that should be fixed to use a proper
> struct. The value there, even though it is __be16 and looks like it came
> from the data, is actually created locally, see ccmp_special_blocks()
> and gcmp_special_blocks().

IOW, I think something like this would make sense:

(but I'll hold it until after Herbert's patches I guess)

>From 20bd0e92ab0d7ef545687da762228622bcdabeec Mon Sep 17 00:00:00 2001
From: Johannes Berg <[email protected]>
Date: Mon, 1 Jun 2015 16:33:11 +0200
Subject: [PATCH] mac80211: move AAD length out of AAD buffer

The code currently passes the AAD buffer as a __be16 with the
length, followed by the actual data, but doesn't use a struct
or make this explicit in any other way, so it's confusing.

Change the code to pass the AAD length explicity outside of
the buffer.

Reported-by: Stephan Mueller <[email protected]>
Signed-off-by: Johannes Berg <[email protected]>
---
net/mac80211/aes_ccm.c | 18 +++++++-------
net/mac80211/aes_ccm.h | 14 ++++++-----
net/mac80211/aes_gcm.c | 10 ++++----
net/mac80211/aes_gcm.h | 6 +++--
net/mac80211/wpa.c | 64 +++++++++++++++++++++++++++-----------------------
5 files changed, 62 insertions(+), 50 deletions(-)

diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
index 208df7c0b6ea..b6e2f096127a 100644
--- a/net/mac80211/aes_ccm.c
+++ b/net/mac80211/aes_ccm.c
@@ -19,9 +19,10 @@
#include "key.h"
#include "aes_ccm.h"

-void ieee80211_aes_ccm_encrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
- u8 *data, size_t data_len, u8 *mic,
- size_t mic_len)
+void ieee80211_aes_ccm_encrypt(struct crypto_aead *tfm, u8 *b_0,
+ u8 *aad, size_t aad_len,
+ u8 *data, size_t data_len,
+ u8 *mic, size_t mic_len)
{
struct scatterlist assoc, pt, ct[2];

@@ -33,7 +34,7 @@ void ieee80211_aes_ccm_encrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
memset(aead_req, 0, sizeof(aead_req_data));

sg_init_one(&pt, data, data_len);
- sg_init_one(&assoc, &aad[2], be16_to_cpup((__be16 *)aad));
+ sg_init_one(&assoc, aad, aad_len);
sg_init_table(ct, 2);
sg_set_buf(&ct[0], data, data_len);
sg_set_buf(&ct[1], mic, mic_len);
@@ -45,9 +46,10 @@ void ieee80211_aes_ccm_encrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
crypto_aead_encrypt(aead_req);
}

-int ieee80211_aes_ccm_decrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
- u8 *data, size_t data_len, u8 *mic,
- size_t mic_len)
+int ieee80211_aes_ccm_decrypt(struct crypto_aead *tfm, u8 *b_0,
+ u8 *aad, size_t aad_len,
+ u8 *data, size_t data_len,
+ u8 *mic, size_t mic_len)
{
struct scatterlist assoc, pt, ct[2];
char aead_req_data[sizeof(struct aead_request) +
@@ -61,7 +63,7 @@ int ieee80211_aes_ccm_decrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
memset(aead_req, 0, sizeof(aead_req_data));

sg_init_one(&pt, data, data_len);
- sg_init_one(&assoc, &aad[2], be16_to_cpup((__be16 *)aad));
+ sg_init_one(&assoc, aad, aad_len);
sg_init_table(ct, 2);
sg_set_buf(&ct[0], data, data_len);
sg_set_buf(&ct[1], mic, mic_len);
diff --git a/net/mac80211/aes_ccm.h b/net/mac80211/aes_ccm.h
index 6a73d1e4d186..bfe355e4a680 100644
--- a/net/mac80211/aes_ccm.h
+++ b/net/mac80211/aes_ccm.h
@@ -15,12 +15,14 @@
struct crypto_aead *ieee80211_aes_key_setup_encrypt(const u8 key[],
size_t key_len,
size_t mic_len);
-void ieee80211_aes_ccm_encrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
- u8 *data, size_t data_len, u8 *mic,
- size_t mic_len);
-int ieee80211_aes_ccm_decrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
- u8 *data, size_t data_len, u8 *mic,
- size_t mic_len);
+void ieee80211_aes_ccm_encrypt(struct crypto_aead *tfm, u8 *b_0,
+ u8 *aad, size_t aad_len,
+ u8 *data, size_t data_len,
+ u8 *mic, size_t mic_len);
+int ieee80211_aes_ccm_decrypt(struct crypto_aead *tfm, u8 *b_0,
+ u8 *aad, size_t aad_len,
+ u8 *data, size_t data_len,
+ u8 *mic, size_t mic_len);
void ieee80211_aes_key_free(struct crypto_aead *tfm);

#endif /* AES_CCM_H */
diff --git a/net/mac80211/aes_gcm.c b/net/mac80211/aes_gcm.c
index fd278bbe1b0d..fb6823c5e381 100644
--- a/net/mac80211/aes_gcm.c
+++ b/net/mac80211/aes_gcm.c
@@ -16,7 +16,8 @@
#include "key.h"
#include "aes_gcm.h"

-void ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
+void ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0,
+ u8 *aad, size_t aad_len,
u8 *data, size_t data_len, u8 *mic)
{
struct scatterlist assoc, pt, ct[2];
@@ -29,7 +30,7 @@ void ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
memset(aead_req, 0, sizeof(aead_req_data));

sg_init_one(&pt, data, data_len);
- sg_init_one(&assoc, &aad[2], be16_to_cpup((__be16 *)aad));
+ sg_init_one(&assoc, aad, aad_len);
sg_init_table(ct, 2);
sg_set_buf(&ct[0], data, data_len);
sg_set_buf(&ct[1], mic, IEEE80211_GCMP_MIC_LEN);
@@ -41,7 +42,8 @@ void ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
crypto_aead_encrypt(aead_req);
}

-int ieee80211_aes_gcm_decrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
+int ieee80211_aes_gcm_decrypt(struct crypto_aead *tfm, u8 *j_0,
+ u8 *aad, size_t aad_len,
u8 *data, size_t data_len, u8 *mic)
{
struct scatterlist assoc, pt, ct[2];
@@ -56,7 +58,7 @@ int ieee80211_aes_gcm_decrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
memset(aead_req, 0, sizeof(aead_req_data));

sg_init_one(&pt, data, data_len);
- sg_init_one(&assoc, &aad[2], be16_to_cpup((__be16 *)aad));
+ sg_init_one(&assoc, aad, aad_len);
sg_init_table(ct, 2);
sg_set_buf(&ct[0], data, data_len);
sg_set_buf(&ct[1], mic, IEEE80211_GCMP_MIC_LEN);
diff --git a/net/mac80211/aes_gcm.h b/net/mac80211/aes_gcm.h
index 1347fda6b76a..67ca10e3e7a4 100644
--- a/net/mac80211/aes_gcm.h
+++ b/net/mac80211/aes_gcm.h
@@ -11,9 +11,11 @@

#include <linux/crypto.h>

-void ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
+void ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0,
+ u8 *aad, size_t aad_len,
u8 *data, size_t data_len, u8 *mic);
-int ieee80211_aes_gcm_decrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
+int ieee80211_aes_gcm_decrypt(struct crypto_aead *tfm, u8 *j_0,
+ u8 *aad, size_t aad_len,
u8 *data, size_t data_len, u8 *mic);
struct crypto_aead *ieee80211_aes_gcm_key_setup_encrypt(const u8 key[],
size_t key_len);
diff --git a/net/mac80211/wpa.c b/net/mac80211/wpa.c
index 9d63d93c836e..b32c043b48b1 100644
--- a/net/mac80211/wpa.c
+++ b/net/mac80211/wpa.c
@@ -304,7 +304,7 @@ ieee80211_crypto_tkip_decrypt(struct ieee80211_rx_data *rx)
}


-static void ccmp_special_blocks(struct sk_buff *skb, u8 *pn, u8 *b_0, u8 *aad)
+static u16 ccmp_special_blocks(struct sk_buff *skb, u8 *pn, u8 *b_0, u8 *aad)
{
__le16 mask_fc;
int a4_included, mgmt;
@@ -352,22 +352,23 @@ static void ccmp_special_blocks(struct sk_buff *skb, u8 *pn, u8 *b_0, u8 *aad)

/* AAD (extra authenticate-only data) / masked 802.11 header
* FC | A1 | A2 | A3 | SC | [A4] | [QC] */
- put_unaligned_be16(len_a, &aad[0]);
- put_unaligned(mask_fc, (__le16 *)&aad[2]);
- memcpy(&aad[4], &hdr->addr1, 3 * ETH_ALEN);
+ put_unaligned(mask_fc, (__le16 *)aad);
+ memcpy(&aad[2], &hdr->addr1, 3 * ETH_ALEN);

/* Mask Seq#, leave Frag# */
- aad[22] = *((u8 *) &hdr->seq_ctrl) & 0x0f;
- aad[23] = 0;
+ aad[20] = *((u8 *) &hdr->seq_ctrl) & 0x0f;
+ aad[21] = 0;

if (a4_included) {
- memcpy(&aad[24], hdr->addr4, ETH_ALEN);
- aad[30] = qos_tid;
- aad[31] = 0;
+ memcpy(&aad[22], hdr->addr4, ETH_ALEN);
+ aad[28] = qos_tid;
+ aad[29] = 0;
} else {
- memset(&aad[24], 0, ETH_ALEN + IEEE80211_QOS_CTL_LEN);
- aad[24] = qos_tid;
+ memset(&aad[22], 0, ETH_ALEN + IEEE80211_QOS_CTL_LEN);
+ aad[22] = qos_tid;
}
+
+ return len_a;
}


@@ -407,6 +408,7 @@ static int ccmp_encrypt_skb(struct ieee80211_tx_data *tx, struct sk_buff *skb,
u64 pn64;
u8 aad[2 * AES_BLOCK_SIZE];
u8 b_0[AES_BLOCK_SIZE];
+ size_t aad_len;

if (info->control.hw_key &&
!(info->control.hw_key->flags & IEEE80211_KEY_FLAG_GENERATE_IV) &&
@@ -460,8 +462,8 @@ static int ccmp_encrypt_skb(struct ieee80211_tx_data *tx, struct sk_buff *skb,
return 0;

pos += IEEE80211_CCMP_HDR_LEN;
- ccmp_special_blocks(skb, pn, b_0, aad);
- ieee80211_aes_ccm_encrypt(key->u.ccmp.tfm, b_0, aad, pos, len,
+ aad_len = ccmp_special_blocks(skb, pn, b_0, aad);
+ ieee80211_aes_ccm_encrypt(key->u.ccmp.tfm, b_0, aad, aad_len, pos, len,
skb_put(skb, mic_len), mic_len);

return 0;
@@ -529,10 +531,10 @@ ieee80211_crypto_ccmp_decrypt(struct ieee80211_rx_data *rx,
u8 aad[2 * AES_BLOCK_SIZE];
u8 b_0[AES_BLOCK_SIZE];
/* hardware didn't decrypt/verify MIC */
- ccmp_special_blocks(skb, pn, b_0, aad);
+ size_t aad_len = ccmp_special_blocks(skb, pn, b_0, aad);

if (ieee80211_aes_ccm_decrypt(
- key->u.ccmp.tfm, b_0, aad,
+ key->u.ccmp.tfm, b_0, aad, aad_len,
skb->data + hdrlen + IEEE80211_CCMP_HDR_LEN,
data_len,
skb->data + skb->len - mic_len, mic_len))
@@ -550,7 +552,7 @@ ieee80211_crypto_ccmp_decrypt(struct ieee80211_rx_data *rx,
return RX_CONTINUE;
}

-static void gcmp_special_blocks(struct sk_buff *skb, u8 *pn, u8 *j_0, u8 *aad)
+static u16 gcmp_special_blocks(struct sk_buff *skb, u8 *pn, u8 *j_0, u8 *aad)
{
__le16 mask_fc;
u8 qos_tid;
@@ -565,7 +567,6 @@ static void gcmp_special_blocks(struct sk_buff *skb, u8 *pn, u8 *j_0, u8 *aad)
/* AAD (extra authenticate-only data) / masked 802.11 header
* FC | A1 | A2 | A3 | SC | [A4] | [QC]
*/
- put_unaligned_be16(ieee80211_hdrlen(hdr->frame_control) - 2, &aad[0]);
/* Mask FC: zero subtype b4 b5 b6 (if not mgmt)
* Retry, PwrMgt, MoreData; set Protected
*/
@@ -576,12 +577,12 @@ static void gcmp_special_blocks(struct sk_buff *skb, u8 *pn, u8 *j_0, u8 *aad)
mask_fc &= ~cpu_to_le16(0x0070);
mask_fc |= cpu_to_le16(IEEE80211_FCTL_PROTECTED);

- put_unaligned(mask_fc, (__le16 *)&aad[2]);
- memcpy(&aad[4], &hdr->addr1, 3 * ETH_ALEN);
+ put_unaligned(mask_fc, (__le16 *)aad);
+ memcpy(&aad[2], &hdr->addr1, 3 * ETH_ALEN);

/* Mask Seq#, leave Frag# */
- aad[22] = *((u8 *)&hdr->seq_ctrl) & 0x0f;
- aad[23] = 0;
+ aad[20] = *((u8 *)&hdr->seq_ctrl) & 0x0f;
+ aad[21] = 0;

if (ieee80211_is_data_qos(hdr->frame_control))
qos_tid = *ieee80211_get_qos_ctl(hdr) &
@@ -590,13 +591,15 @@ static void gcmp_special_blocks(struct sk_buff *skb, u8 *pn, u8 *j_0, u8 *aad)
qos_tid = 0;

if (ieee80211_has_a4(hdr->frame_control)) {
- memcpy(&aad[24], hdr->addr4, ETH_ALEN);
- aad[30] = qos_tid;
- aad[31] = 0;
+ memcpy(&aad[22], hdr->addr4, ETH_ALEN);
+ aad[28] = qos_tid;
+ aad[29] = 0;
} else {
- memset(&aad[24], 0, ETH_ALEN + IEEE80211_QOS_CTL_LEN);
- aad[24] = qos_tid;
+ memset(&aad[22], 0, ETH_ALEN + IEEE80211_QOS_CTL_LEN);
+ aad[22] = qos_tid;
}
+
+ return ieee80211_hdrlen(hdr->frame_control) - 2;
}

static inline void gcmp_pn2hdr(u8 *hdr, const u8 *pn, int key_id)
@@ -632,6 +635,7 @@ static int gcmp_encrypt_skb(struct ieee80211_tx_data *tx, struct sk_buff *skb)
u64 pn64;
u8 aad[2 * AES_BLOCK_SIZE];
u8 j_0[AES_BLOCK_SIZE];
+ size_t aad_len;

if (info->control.hw_key &&
!(info->control.hw_key->flags & IEEE80211_KEY_FLAG_GENERATE_IV) &&
@@ -686,8 +690,8 @@ static int gcmp_encrypt_skb(struct ieee80211_tx_data *tx, struct sk_buff *skb)
return 0;

pos += IEEE80211_GCMP_HDR_LEN;
- gcmp_special_blocks(skb, pn, j_0, aad);
- ieee80211_aes_gcm_encrypt(key->u.gcmp.tfm, j_0, aad, pos, len,
+ aad_len = gcmp_special_blocks(skb, pn, j_0, aad);
+ ieee80211_aes_gcm_encrypt(key->u.gcmp.tfm, j_0, aad, aad_len, pos, len,
skb_put(skb, IEEE80211_GCMP_MIC_LEN));

return 0;
@@ -752,10 +756,10 @@ ieee80211_crypto_gcmp_decrypt(struct ieee80211_rx_data *rx)
u8 aad[2 * AES_BLOCK_SIZE];
u8 j_0[AES_BLOCK_SIZE];
/* hardware didn't decrypt/verify MIC */
- gcmp_special_blocks(skb, pn, j_0, aad);
+ size_t aad_len = gcmp_special_blocks(skb, pn, j_0, aad);

if (ieee80211_aes_gcm_decrypt(
- key->u.gcmp.tfm, j_0, aad,
+ key->u.gcmp.tfm, j_0, aad, aad_len,
skb->data + hdrlen + IEEE80211_GCMP_HDR_LEN,
data_len,
skb->data + skb->len - IEEE80211_GCMP_MIC_LEN))
--
2.1.4

2015-06-01 15:36:58

by Stephan Müller

[permalink] [raw]
Subject: Re: [PATCH 7/7] mac80211: Switch to new AEAD interface

Am Montag, 1. Juni 2015, 16:35:26 schrieb Johannes Berg:

Hi Johannes,

>
>IOW, I think something like this would make sense:
>

That looks definitely cleaner :-)

Though, my main concern was just to ensure that the aad length value is not
zero.


Ciao
Stephan

2015-06-02 09:15:43

by Jouni Malinen

[permalink] [raw]
Subject: Re: [PATCH 7/7] mac80211: Switch to new AEAD interface

On Mon, Jun 01, 2015 at 05:36:58PM +0200, Stephan Mueller wrote:
> Am Montag, 1. Juni 2015, 16:35:26 schrieb Johannes Berg:
> >IOW, I think something like this would make sense:
>
> That looks definitely cleaner :-)

Indeed.. That AAD length-in-the-buffer design came from the over ten
year old code that was optimized to cover the CCM construction with the
same buffer and that was not cleaned up when this was converted to use
cryptoapi couple of years ago.

> Though, my main concern was just to ensure that the aad length value is not
> zero.

It won't be in IEEE 802.11 use cases. The exact length depends on the
IEEE 802.11 frame type, but AAD is constructed in a way that it is
normally a bit over 20 octets while allowing CCM to fit the related
operations into two AES blocks.

--
Jouni Malinen PGP id EFC895FA