Re: [PATCH] mac80211: aes_ccm: move struct aead_req off the stack

[Ard Biesheuvel]

> On 14 Oct 2016, at 14:46, Johannes Berg  wrote:
> 
> 
>> 
>> Is the aad[] actually reused? I would assume it only affects the mac
>> on encryption, and the verification on decryption but I don't think
>> we actually need it back from the crypto routines.
> 
> I don't think it's reused.
> 
>> Exactly what you said above :-) My patch only touches CCM but as you
>> said,
>> 
>> """
>> 'Also there's B_0/J_0 for CCM/GCM, and the 'zero' thing that GMAC
>> has.
>> """
> 
> Ah, but we can/should do the same for the others, no?
> 

Yes, but then we end up kmalloc/kfreeing chunks of 16 bytes, which is actually 
another problem.

I still think we are not violating the api by putting aead_req on the stack 
(but herbert should confirm). The aad[] issue does violate the api, so it 
deserves a separate fix imo

Re: [PATCH] mac80211: aes_ccm: move struct aead_req off the stack

[Ard Biesheuvel]

> On 14 Oct 2016, at 14:46, Johannes Berg  wrote:
> 
> 
>> 
>> Is the aad[] actually reused? I would assume it only affects the mac
>> on encryption, and the verification on decryption but I don't think
>> we actually need it back from the crypto routines.
> 
> I don't think it's reused.
> 
>> Exactly what you said above :-) My patch only touches CCM but as you
>> said,
>> 
>> """
>> 'Also there's B_0/J_0 for CCM/GCM, and the 'zero' thing that GMAC
>> has.
>> """
> 
> Ah, but we can/should do the same for the others, no?
> 

Yes, but then we end up kmalloc/kfreeing chunks of 16 bytes, which is actually 
another problem.

I still think we are not violating the api by putting aead_req on the stack 
(but herbert should confirm). The aad[] issue does violate the api, so it 
deserves a separate fix imo

Re: [PATCH] mac80211: aes_ccm: move struct aead_req off the stack

[Johannes Berg]

> 
> Is the aad[] actually reused? I would assume it only affects the mac
> on encryption, and the verification on decryption but I don't think
> we actually need it back from the crypto routines.

I don't think it's reused.

> Exactly what you said above :-) My patch only touches CCM but as you
> said,
> 
> """
> 'Also there's B_0/J_0 for CCM/GCM, and the 'zero' thing that GMAC
> has.
> """

Ah, but we can/should do the same for the others, no?

johannes

Re: [PATCH] mac80211: aes_ccm: move struct aead_req off the stack

[Johannes Berg]

> 
> Is the aad[] actually reused? I would assume it only affects the mac
> on encryption, and the verification on decryption but I don't think
> we actually need it back from the crypto routines.

I don't think it's reused.

> Exactly what you said above :-) My patch only touches CCM but as you
> said,
> 
> """
> 'Also there's B_0/J_0 for CCM/GCM, and the 'zero' thing that GMAC
> has.
> """

Ah, but we can/should do the same for the others, no?

johannes

Re: [PATCH] mac80211: aes_ccm: move struct aead_req off the stack

[Johannes Berg]

On Fri, 2016-10-14 at 14:13 +0100, Ard Biesheuvel wrote:
> 
> > But if we allocate things anyway, is it worth expending per-CPU
> > buffers on these?
> 
> Ehmm, maybe not. I could spin a v2 that allocates a bigger buffer,
> and copies aad[] into it as well

Copies in/out, I guess. Also there's B_0/J_0 for CCM/GCM, and the
'zero' thing that GMAC has.

> That does not help the other algos though

What do you mean?

johannes

Re: [PATCH] mac80211: aes_ccm: move struct aead_req off the stack

[Johannes Berg]

On Fri, 2016-10-14 at 14:13 +0100, Ard Biesheuvel wrote:
> 
> > But if we allocate things anyway, is it worth expending per-CPU
> > buffers on these?
> 
> Ehmm, maybe not. I could spin a v2 that allocates a bigger buffer,
> and copies aad[] into it as well

Copies in/out, I guess. Also there's B_0/J_0 for CCM/GCM, and the
'zero' thing that GMAC has.

> That does not help the other algos though

What do you mean?

johannes

Re: [PATCH] mac80211: aes_ccm: move struct aead_req off the stack

[Ard Biesheuvel]

On 14 October 2016 at 14:15, Johannes Berg  wrote:
> On Fri, 2016-10-14 at 14:13 +0100, Ard Biesheuvel wrote:
>>
>> > But if we allocate things anyway, is it worth expending per-CPU
>> > buffers on these?
>>
>> Ehmm, maybe not. I could spin a v2 that allocates a bigger buffer,
>> and copies aad[] into it as well
>
> Copies in/out, I guess. Also there's B_0/J_0 for CCM/GCM, and the
> 'zero' thing that GMAC has.
>

Is the aad[] actually reused? I would assume it only affects the mac
on encryption, and the verification on decryption but I don't think we
actually need it back from the crypto routines.

>> That does not help the other algos though
>
> What do you mean?
>

Exactly what you said above :-) My patch only touches CCM but as you said,

"""
'Also there's B_0/J_0 for CCM/GCM, and the 'zero' thing that GMAC has.
"""

Re: [PATCH] mac80211: aes_ccm: move struct aead_req off the stack

[Ard Biesheuvel]

On 14 October 2016 at 14:15, Johannes Berg  wrote:
> On Fri, 2016-10-14 at 14:13 +0100, Ard Biesheuvel wrote:
>>
>> > But if we allocate things anyway, is it worth expending per-CPU
>> > buffers on these?
>>
>> Ehmm, maybe not. I could spin a v2 that allocates a bigger buffer,
>> and copies aad[] into it as well
>
> Copies in/out, I guess. Also there's B_0/J_0 for CCM/GCM, and the
> 'zero' thing that GMAC has.
>

Is the aad[] actually reused? I would assume it only affects the mac
on encryption, and the verification on decryption but I don't think we
actually need it back from the crypto routines.

>> That does not help the other algos though
>
> What do you mean?
>

Exactly what you said above :-) My patch only touches CCM but as you said,

"""
'Also there's B_0/J_0 for CCM/GCM, and the 'zero' thing that GMAC has.
"""

Re: [PATCH] mac80211: aes_ccm: move struct aead_req off the stack

[Ard Biesheuvel]

On 14 October 2016 at 14:10, Johannes Berg  wrote:
>
>> So use kzalloc
>
> Do we really need kzalloc()? We have things on the stack right now, and
> don't initialize, so surely we don't really need to zero things?
>
>> This only addresses one half of the problem. The other problem, i.e.,
>> the fact that the aad[] array lives on the stack of the caller, is
>> handled adequately imo by the change proposed by Johannes.
>
> But if we allocate things anyway, is it worth expending per-CPU buffers
> on these?
>

Ehmm, maybe not. I could spin a v2 that allocates a bigger buffer, and
copies aad[] into it as well
That does not help the other algos though

Re: [PATCH] mac80211: aes_ccm: move struct aead_req off the stack

[Ard Biesheuvel]

On 14 October 2016 at 14:10, Johannes Berg  wrote:
>
>> So use kzalloc
>
> Do we really need kzalloc()? We have things on the stack right now, and
> don't initialize, so surely we don't really need to zero things?
>
>> This only addresses one half of the problem. The other problem, i.e.,
>> the fact that the aad[] array lives on the stack of the caller, is
>> handled adequately imo by the change proposed by Johannes.
>
> But if we allocate things anyway, is it worth expending per-CPU buffers
> on these?
>

Ehmm, maybe not. I could spin a v2 that allocates a bigger buffer, and
copies aad[] into it as well
That does not help the other algos though

Re: [PATCH] mac80211: aes_ccm: move struct aead_req off the stack

[Johannes Berg]

On Fri, 2016-10-14 at 15:10 +0200, Johannes Berg wrote:
> > 
> > So use kzalloc
> 
> Do we really need kzalloc()? We have things on the stack right now,
> and don't initialize, so surely we don't really need to zero things? 

Err, never mind, I'm an idiot - we *do* initialize to 0, of course.

johannes

Re: [PATCH] mac80211: aes_ccm: move struct aead_req off the stack

[Johannes Berg]

On Fri, 2016-10-14 at 15:10 +0200, Johannes Berg wrote:
> > 
> > So use kzalloc
> 
> Do we really need kzalloc()? We have things on the stack right now,
> and don't initialize, so surely we don't really need to zero things? 

Err, never mind, I'm an idiot - we *do* initialize to 0, of course.

johannes

Re: [PATCH] mac80211: aes_ccm: move struct aead_req off the stack

[Johannes Berg]

> So use kzalloc

Do we really need kzalloc()? We have things on the stack right now, and
don't initialize, so surely we don't really need to zero things?

> This only addresses one half of the problem. The other problem, i.e.,
> the fact that the aad[] array lives on the stack of the caller, is
> handled adequately imo by the change proposed by Johannes.

But if we allocate things anyway, is it worth expending per-CPU buffers
on these?

johannes

Re: [PATCH] mac80211: aes_ccm: move struct aead_req off the stack

[Johannes Berg]

> So use kzalloc

Do we really need kzalloc()? We have things on the stack right now, and
don't initialize, so surely we don't really need to zero things?

> This only addresses one half of the problem. The other problem, i.e.,
> the fact that the aad[] array lives on the stack of the caller, is
> handled adequately imo by the change proposed by Johannes.

But if we allocate things anyway, is it worth expending per-CPU buffers
on these?

johannes

[PATCH] mac80211: aes_ccm: move struct aead_req off the stack

[Ard Biesheuvel]

Some CCM implementations (such as the generic CCM wrapper in crypto/)
use scatterlists to map fields of struct aead_req. This means these
data structures cannot live in the vmalloc area, which means that in
the near future, they can no longer live on the stack either.

Given that these data structures have implementation specific context fields,
it really depends on the particular driver whether this issue is likely to
occur or not, and so it seems best to simply move the entire data structure
into the direct mapped kernel heap.

So use kzalloc/kfree to allocate and free the data structures. This pattern
already exists in the IPsec ESP driver, but in the future, we may need to
improve upon this by either moving the request into the SKB, or using a
slab cache to allocate/free the data structures.

Signed-off-by: Ard Biesheuvel 
---

This only addresses one half of the problem. The other problem, i.e., the
fact that the aad[] array lives on the stack of the caller, is handled
adequately imo by the change proposed by Johannes.

 net/mac80211/aes_ccm.c | 24 ++--
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
index 7663c28ba353..a0ae8cebbe4e 100644
--- a/net/mac80211/aes_ccm.c
+++ b/net/mac80211/aes_ccm.c
@@ -23,13 +23,10 @@ void ieee80211_aes_ccm_encrypt(struct crypto_aead *tfm, u8 
*b_0, u8 *aad,
   size_t mic_len)
 {
struct scatterlist sg[3];
+   struct aead_request *aead_req;
 
-   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;
-
-   memset(aead_req, 0, sizeof(aead_req_data));
+   aead_req = kzalloc(sizeof(struct aead_request) +
+  crypto_aead_reqsize(tfm), GFP_ATOMIC);
 
sg_init_table(sg, 3);
sg_set_buf([0], [2], be16_to_cpup((__be16 *)aad));
@@ -41,6 +38,7 @@ void ieee80211_aes_ccm_encrypt(struct crypto_aead *tfm, u8 
*b_0, u8 *aad,
aead_request_set_ad(aead_req, sg[0].length);
 
crypto_aead_encrypt(aead_req);
+   kfree(aead_req);
 }
 
 int ieee80211_aes_ccm_decrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
@@ -48,15 +46,14 @@ int ieee80211_aes_ccm_decrypt(struct crypto_aead *tfm, u8 
*b_0, u8 *aad,
  size_t mic_len)
 {
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;
+   struct aead_request *aead_req;
+   int err;
 
if (data_len == 0)
return -EINVAL;
 
-   memset(aead_req, 0, sizeof(aead_req_data));
+   aead_req = kzalloc(sizeof(struct aead_request) +
+  crypto_aead_reqsize(tfm), GFP_ATOMIC);
 
sg_init_table(sg, 3);
sg_set_buf([0], [2], be16_to_cpup((__be16 *)aad));
@@ -67,7 +64,10 @@ int ieee80211_aes_ccm_decrypt(struct crypto_aead *tfm, u8 
*b_0, u8 *aad,
aead_request_set_crypt(aead_req, sg, sg, data_len + mic_len, b_0);
aead_request_set_ad(aead_req, sg[0].length);
 
-   return crypto_aead_decrypt(aead_req);
+   err = crypto_aead_decrypt(aead_req);
+   kfree(aead_req);
+
+   return err;
 }
 
 struct crypto_aead *ieee80211_aes_key_setup_encrypt(const u8 key[],
-- 
2.7.4

[PATCH] mac80211: aes_ccm: move struct aead_req off the stack

[Ard Biesheuvel]

Some CCM implementations (such as the generic CCM wrapper in crypto/)
use scatterlists to map fields of struct aead_req. This means these
data structures cannot live in the vmalloc area, which means that in
the near future, they can no longer live on the stack either.

Given that these data structures have implementation specific context fields,
it really depends on the particular driver whether this issue is likely to
occur or not, and so it seems best to simply move the entire data structure
into the direct mapped kernel heap.

So use kzalloc/kfree to allocate and free the data structures. This pattern
already exists in the IPsec ESP driver, but in the future, we may need to
improve upon this by either moving the request into the SKB, or using a
slab cache to allocate/free the data structures.

Signed-off-by: Ard Biesheuvel 
---

This only addresses one half of the problem. The other problem, i.e., the
fact that the aad[] array lives on the stack of the caller, is handled
adequately imo by the change proposed by Johannes.

 net/mac80211/aes_ccm.c | 24 ++--
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
index 7663c28ba353..a0ae8cebbe4e 100644
--- a/net/mac80211/aes_ccm.c
+++ b/net/mac80211/aes_ccm.c
@@ -23,13 +23,10 @@ void ieee80211_aes_ccm_encrypt(struct crypto_aead *tfm, u8 
*b_0, u8 *aad,
   size_t mic_len)
 {
struct scatterlist sg[3];
+   struct aead_request *aead_req;
 
-   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;
-
-   memset(aead_req, 0, sizeof(aead_req_data));
+   aead_req = kzalloc(sizeof(struct aead_request) +
+  crypto_aead_reqsize(tfm), GFP_ATOMIC);
 
sg_init_table(sg, 3);
sg_set_buf([0], [2], be16_to_cpup((__be16 *)aad));
@@ -41,6 +38,7 @@ void ieee80211_aes_ccm_encrypt(struct crypto_aead *tfm, u8 
*b_0, u8 *aad,
aead_request_set_ad(aead_req, sg[0].length);
 
crypto_aead_encrypt(aead_req);
+   kfree(aead_req);
 }
 
 int ieee80211_aes_ccm_decrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
@@ -48,15 +46,14 @@ int ieee80211_aes_ccm_decrypt(struct crypto_aead *tfm, u8 
*b_0, u8 *aad,
  size_t mic_len)
 {
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;
+   struct aead_request *aead_req;
+   int err;
 
if (data_len == 0)
return -EINVAL;
 
-   memset(aead_req, 0, sizeof(aead_req_data));
+   aead_req = kzalloc(sizeof(struct aead_request) +
+  crypto_aead_reqsize(tfm), GFP_ATOMIC);
 
sg_init_table(sg, 3);
sg_set_buf([0], [2], be16_to_cpup((__be16 *)aad));
@@ -67,7 +64,10 @@ int ieee80211_aes_ccm_decrypt(struct crypto_aead *tfm, u8 
*b_0, u8 *aad,
aead_request_set_crypt(aead_req, sg, sg, data_len + mic_len, b_0);
aead_request_set_ad(aead_req, sg[0].length);
 
-   return crypto_aead_decrypt(aead_req);
+   err = crypto_aead_decrypt(aead_req);
+   kfree(aead_req);
+
+   return err;
 }
 
 struct crypto_aead *ieee80211_aes_key_setup_encrypt(const u8 key[],
-- 
2.7.4