Linux wireless drivers development
 help / color / mirror / Atom feed
* Re: [PATCH v4] mac80211: move extra crypto data off the stack
From: Ard Biesheuvel @ 2016-10-17 10:02 UTC (permalink / raw)
  To: Johannes Berg
  Cc: <linux-wireless@vger.kernel.org>, Sergey Senozhatsky,
	<netdev@vger.kernel.org>, Herbert Xu
In-Reply-To: <1476698041.19992.19.camel@sipsolutions.net>



> On 17 Oct 2016, at 10:54, Johannes Berg <johannes@sipsolutions.net> wrote:=

>=20
>=20
>>> Well, if your other patch to make it OK to be on-stack would be
>>> applied instead, this wouldn't make much sense either :)
>>>=20
>>=20
>> Yes but that one only fixes ccm not gcm
>=20
> Yes, but we can do the same for GCM, no?
>=20

No, not really. ccm happens to use aes with the same key for the mac and the=
 encryption. gcm uses an another algo entirely for the mac

>>> In this particular patch, we could reduce the size of the struct,
>>> but I
>>> don't actually think it'll make a difference to go from 48 to 36
>>> bytes,
>>> given alignment etc., so I think I'll just leave it as is.
>>>=20
>>=20
>> I understand you are in a hurry, but this is unlikely to be the only
>> such issue. I will propose an 'auxdata' feature for the crypto api
>> that can be used here, but also for any other occurrence where client
>> data assoiciated with the request can no longer be allocated on the
>> stack
>=20
> No objections. I'll merge this anyway today I think, reverting is easy
> later.
>=20

ok fair enough=

^ permalink raw reply

* Re: [PATCH v4] mac80211: move extra crypto data off the stack
From: Johannes Berg @ 2016-10-17  9:54 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: <linux-wireless@vger.kernel.org>, Sergey Senozhatsky,
	<netdev@vger.kernel.org>, Herbert Xu
In-Reply-To: <67161BCC-596E-4DDE-A58E-9195BB5570C2@linaro.org>


> > Well, if your other patch to make it OK to be on-stack would be
> > applied instead, this wouldn't make much sense either :)
> > 
> 
> Yes but that one only fixes ccm not gcm

Yes, but we can do the same for GCM, no?

> > In this particular patch, we could reduce the size of the struct,
> > but I
> > don't actually think it'll make a difference to go from 48 to 36
> > bytes,
> > given alignment etc., so I think I'll just leave it as is.
> > 
> 
> I understand you are in a hurry, but this is unlikely to be the only
> such issue. I will propose an 'auxdata' feature for the crypto api
> that can be used here, but also for any other occurrence where client
> data assoiciated with the request can no longer be allocated on the
> stack

No objections. I'll merge this anyway today I think, reverting is easy
later.

johannes

^ permalink raw reply

* Re: [PATCH v4] mac80211: move extra crypto data off the stack
From: Ard Biesheuvel @ 2016-10-17  9:52 UTC (permalink / raw)
  To: Johannes Berg
  Cc: <linux-wireless@vger.kernel.org>, Sergey Senozhatsky,
	<netdev@vger.kernel.org>, Herbert Xu
In-Reply-To: <1476696918.19992.15.camel@sipsolutions.net>



> On 17 Oct 2016, at 10:35, Johannes Berg <johannes@sipsolutions.net> wrote:=

>=20
>> On Mon, 2016-10-17 at 10:30 +0100, Ard Biesheuvel wrote:
>>=20
>> Yes. But as I replied, setting the req size is not supported atm,
>> although it is reasonable to demand a way to allocate additional data
>> in the request specifically for this issue. So let's proceed with the
>> aead_request_alloc/free patch, but I would like to propose something
>> on the API side to address this particular issue
>=20
> Well, if your other patch to make it OK to be on-stack would be applied
> instead, this wouldn't make much sense either :)
>=20

Yes but that one only fixes ccm not gcm

> In this particular patch, we could reduce the size of the struct, but I
> don't actually think it'll make a difference to go from 48 to 36 bytes,
> given alignment etc., so I think I'll just leave it as is.
>=20

I understand you are in a hurry, but this is unlikely to be the only such is=
sue. I will propose an 'auxdata' feature for the crypto api that can be used=
 here, but also for any other occurrence where client data assoiciated with t=
he request can no longer be allocated on the stack=

^ permalink raw reply

* Re: [PATCH v4] mac80211: move extra crypto data off the stack
From: Ard Biesheuvel @ 2016-10-17  9:49 UTC (permalink / raw)
  To: Johannes Berg
  Cc: <linux-wireless@vger.kernel.org>, Sergey Senozhatsky,
	<netdev@vger.kernel.org>, Herbert Xu
In-Reply-To: <1476696918.19992.15.camel@sipsolutions.net>



> On 17 Oct 2016, at 10:35, Johannes Berg <johannes@sipsolutions.net> wrote:
> 
>> On Mon, 2016-10-17 at 10:30 +0100, Ard Biesheuvel wrote:
>> 
>> Yes. But as I replied, setting the req size is not supported atm,
>> although it is reasonable to demand a way to allocate additional data
>> in the request specifically for this issue. So let's proceed with the
>> aead_request_alloc/free patch, but I would like to propose something
>> on the API side to address this particular issue
> 
> Well, if your other patch to make it OK to be on-stack would be applied
> instead, this wouldn't make much sense either :)
> 
> In this particular patch, we could reduce the size of the struct, but I
> don't actually think it'll make a difference to go from 48 to 36 bytes,
> given alignment etc., so I think I'll just leave it as is.
> 
> johannes

^ permalink raw reply

* Re: [PATCHv5 2/2] mac80211: fix A-MSDU outer SA/DA
From: Johannes Berg @ 2016-10-17  9:44 UTC (permalink / raw)
  To: Michael Braun; +Cc: linux-wireless, projekt-wlan, Felix Fietkau
In-Reply-To: <1476530899-24241-2-git-send-email-michael-dev@fami-braun.de>

On Sat, 2016-10-15 at 13:28 +0200, Michael Braun wrote:
> According to IEEE 802.11-2012 section 8.3.2 table 8-19, the outer
> SA/DA
> of A-MSDU frames need to be changed depending on FromDS/ToDS values.

Also applied,

> +	if (bssid && ieee80211_has_fromds(hdr->frame_control))
> +		memcpy(h_80211_src, bssid, ETH_ALEN);
> +
> +	if (bssid && ieee80211_has_tods(hdr->frame_control))
> +		memcpy(h_80211_dst, bssid, ETH_ALEN);

but I also changed these to ether_addr_copy()

johannes

^ permalink raw reply

* Re: [PATCHv5 1/2] mac80211: avoid extra memcpy in A-MSDU head creation
From: Johannes Berg @ 2016-10-17  9:41 UTC (permalink / raw)
  To: Michael Braun; +Cc: linux-wireless, projekt-wlan, Felix Fietkau
In-Reply-To: <1476530899-24241-1-git-send-email-michael-dev@fami-braun.de>

On Sat, 2016-10-15 at 13:28 +0200, Michael Braun wrote:
> Signed-off-by: Michael Braun <michael-dev@fami-braun.de>
> 
Applied.

johannes

^ permalink raw reply

* Re: [PATCH] mac80211_hwsim: suggest nl80211 instead of wext driver in documentation
From: Johannes Berg @ 2016-10-17  9:39 UTC (permalink / raw)
  To: Linus Lüssing, linux-wireless, Jouni Malinen
  Cc: David S . Miller, Jonathan Corbet, netdev, linux-kernel,
	linux-doc
In-Reply-To: <20161016223933.9484-1-linus.luessing@c0d3.blue>

On Mon, 2016-10-17 at 00:39 +0200, Linus Lüssing wrote:
> For mac80211_hwsim interfaces, suggest to use wpa_supplicant with the
> more modern, netlink based driver instead of wext.

Makes sense, applied.

> Actually, I wasn't even able to make a connection with the
> configuration
> files and information provided in
> Documentation/networking/mac80211_hwsim/{README,hostapd.conf/wpa_supp
> licant.conf}
> 
This less so, we even have a few test cases we run regularly, but I
don't know. Maybe there's something special in those configuration
files that we don't test for otherwise.

johannes

^ permalink raw reply

* Re: [PATCH v4] mac80211: move extra crypto data off the stack
From: Johannes Berg @ 2016-10-17  9:35 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: <linux-wireless@vger.kernel.org>, Sergey Senozhatsky,
	<netdev@vger.kernel.org>, Herbert Xu
In-Reply-To: <CAKv+Gu_ZH9hO_xbVfOfk90CxFJ6ZTz3PKWB1v23LRVzpBrb=oQ@mail.gmail.com>

On Mon, 2016-10-17 at 10:30 +0100, Ard Biesheuvel wrote:

> Yes. But as I replied, setting the req size is not supported atm,
> although it is reasonable to demand a way to allocate additional data
> in the request specifically for this issue. So let's proceed with the
> aead_request_alloc/free patch, but I would like to propose something
> on the API side to address this particular issue

Well, if your other patch to make it OK to be on-stack would be applied
instead, this wouldn't make much sense either :)

In this particular patch, we could reduce the size of the struct, but I
don't actually think it'll make a difference to go from 48 to 36 bytes,
given alignment etc., so I think I'll just leave it as is.

johannes

^ permalink raw reply

* Re: [PATCH v4] mac80211: move extra crypto data off the stack
From: Ard Biesheuvel @ 2016-10-17  9:30 UTC (permalink / raw)
  To: Johannes Berg
  Cc: <linux-wireless@vger.kernel.org>, Sergey Senozhatsky,
	<netdev@vger.kernel.org>, Herbert Xu
In-Reply-To: <1476696226.19992.13.camel@sipsolutions.net>

On 17 October 2016 at 10:23, Johannes Berg <johannes@sipsolutions.net> wrote:
>
>> Apologies for going back and forth on this, but it appears there may
>> be another way to deal with this.
>>
>> First of all, we only need this handling for the authenticated data,
>
> Are you sure b_0/j_0 aren't needed? We pass those
> to aead_request_set_crypt(), and I wasn't sure what that really did
> internally, perhaps like the internal data.
>

They are the IV[], which is a fixed length parameter of the algorithm.
In contrast, the AAD[] could be of arbitrary length (from the POV of
the crypto API) so it uses scatterlists.

> Testing with that on the stack does seem to work, in fact.
>
> Surely we need zero for GMAC though, since we also put that into the sg
> list. Thus for GMAC we definitely need 20+16 bytes, and since I round
> up to a cacheline (at least on SMP) it doesn't really matter that we
> could get 36 instead of the 48 I have now.
>
>> and only for CCM and GCM, not CMAC (which does not use scatterlists
>> at all, it simply calls the AES cipher directly)
>
> I didn't modify CMAC, I think, only GMAC, which also uses scatterlists.
>

Ah ok, I misread the patch.

>> So that leaves a fixed 20 bytes for GCM and fixed 32 bytes for CCM,
>
> and 36 for GMAC :)

Yes. But as I replied, setting the req size is not supported atm,
although it is reasonable to demand a way to allocate additional data
in the request specifically for this issue. So let's proceed with the
aead_request_alloc/free patch, but I would like to propose something
on the API side to address this particular issue

^ permalink raw reply

* Re: [PATCH v4] mac80211: move extra crypto data off the stack
From: Johannes Berg @ 2016-10-17  9:23 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: <linux-wireless@vger.kernel.org>, Sergey Senozhatsky,
	<netdev@vger.kernel.org>, Herbert Xu
In-Reply-To: <CAKv+Gu-aZhCBvnEQoZUZLDPXCrvgxO1pSd=6EHz+tMB+dFz5hg@mail.gmail.com>


> Apologies for going back and forth on this, but it appears there may
> be another way to deal with this.
> 
> First of all, we only need this handling for the authenticated data,

Are you sure b_0/j_0 aren't needed? We pass those
to aead_request_set_crypt(), and I wasn't sure what that really did
internally, perhaps like the internal data.

Testing with that on the stack does seem to work, in fact.

Surely we need zero for GMAC though, since we also put that into the sg
list. Thus for GMAC we definitely need 20+16 bytes, and since I round
up to a cacheline (at least on SMP) it doesn't really matter that we
could get 36 instead of the 48 I have now.

> and only for CCM and GCM, not CMAC (which does not use scatterlists
> at all, it simply calls the AES cipher directly)

I didn't modify CMAC, I think, only GMAC, which also uses scatterlists.

> So that leaves a fixed 20 bytes for GCM and fixed 32 bytes for CCM,

and 36 for GMAC :)

johannes

^ permalink raw reply

* Re: [PATCH v4] mac80211: move extra crypto data off the stack
From: Ard Biesheuvel @ 2016-10-17  9:17 UTC (permalink / raw)
  To: Johannes Berg
  Cc: <linux-wireless@vger.kernel.org>, Sergey Senozhatsky,
	<netdev@vger.kernel.org>, Herbert Xu, Johannes Berg
In-Reply-To: <CAKv+Gu-aZhCBvnEQoZUZLDPXCrvgxO1pSd=6EHz+tMB+dFz5hg@mail.gmail.com>

On 17 October 2016 at 10:14, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 17 October 2016 at 09:33, Johannes Berg <johannes@sipsolutions.net> wrote:
>> From: Johannes Berg <johannes.berg@intel.com>
>>
>> As the stack can (on x86-64) now be virtually mapped rather than
>> using "normal" kernel memory, Sergey noticed mac80211 isn't using
>> the SG APIs correctly by putting on-stack buffers into SG tables.
>> This leads to kernel crashes.
>>
>> Fix this by allocating the extra fields dynamically on the fly as
>> needed, using a kmem cache.
>>
>> I used per-CPU memory in a previous iteration of this patch, but
>> Ard Biesheuvel pointed out that was also vmalloc'ed on some
>> architectures.
>>
>> Reported-by: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
>> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
>
> Apologies for going back and forth on this, but it appears there may
> be another way to deal with this.
>
> First of all, we only need this handling for the authenticated data,
> and only for CCM and GCM, not CMAC (which does not use scatterlists at
> all, it simply calls the AES cipher directly)
>
> So that leaves a fixed 20 bytes for GCM and fixed 32 bytes for CCM,
> which we could allocate along with the AEAD request, e..g.,
>
> """
> diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
> index 8e898a6e8de8..c0c33e6ad94e 100644
> --- a/net/mac80211/aes_ccm.c
> +++ b/net/mac80211/aes_ccm.c
> @@ -24,13 +24,17 @@ int ieee80211_aes_ccm_encrypt(struct crypto_aead
> *tfm, u8 *b_0, u8 *aad,
>  {
>         struct scatterlist sg[3];
>         struct aead_request *aead_req;
> +       u8 *__aad;
>
>         aead_req = aead_request_alloc(tfm, GFP_ATOMIC);
>         if (!aead_req)
>                 return -ENOMEM;
>
> +       __aad = (u8 *)aead_req + crypto_aead_reqsize(tfm);
> +       memcpy(__aad, aad, 2 * AES_BLOCK_SIZE);
> +
>         sg_init_table(sg, 3);
> -       sg_set_buf(&sg[0], &aad[2], be16_to_cpup((__be16 *)aad));
> +       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);
>
> @@ -49,6 +53,7 @@ int ieee80211_aes_ccm_decrypt(struct crypto_aead
> *tfm, u8 *b_0, u8 *aad,
>  {
>         struct scatterlist sg[3];
>         struct aead_request *aead_req;
> +       u8 *__aad;
>         int err;
>
>         if (data_len == 0)
> @@ -58,8 +63,11 @@ int ieee80211_aes_ccm_decrypt(struct crypto_aead
> *tfm, u8 *b_0, u8 *aad,
>         if (!aead_req)
>                 return -ENOMEM;
>
> +       __aad = (u8 *)aead_req + crypto_aead_reqsize(tfm);
> +       memcpy(__aad, aad, 2 * AES_BLOCK_SIZE);
> +
>         sg_init_table(sg, 3);
> -       sg_set_buf(&sg[0], &aad[2], be16_to_cpup((__be16 *)aad));
> +       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);
>
> @@ -90,6 +98,8 @@ struct crypto_aead
> *ieee80211_aes_key_setup_encrypt(const u8 key[],
>         if (err)
>                 goto free_aead;
>
> +       crypto_aead_set_reqsize(tfm,
> +                               crypto_aead_reqsize(tfm) + 2 * AES_BLOCK_SIZE));
>         return tfm;
>

Darn, it seems crypto_aead_set_reqsize() is internal to the crypto API ... :-(

^ permalink raw reply

* Re: [PATCH v4] mac80211: move extra crypto data off the stack
From: Ard Biesheuvel @ 2016-10-17  9:14 UTC (permalink / raw)
  To: Johannes Berg
  Cc: <linux-wireless@vger.kernel.org>, Sergey Senozhatsky,
	<netdev@vger.kernel.org>, Herbert Xu, Johannes Berg
In-Reply-To: <1476693195-14071-1-git-send-email-johannes@sipsolutions.net>

On 17 October 2016 at 09:33, Johannes Berg <johannes@sipsolutions.net> wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> As the stack can (on x86-64) now be virtually mapped rather than
> using "normal" kernel memory, Sergey noticed mac80211 isn't using
> the SG APIs correctly by putting on-stack buffers into SG tables.
> This leads to kernel crashes.
>
> Fix this by allocating the extra fields dynamically on the fly as
> needed, using a kmem cache.
>
> I used per-CPU memory in a previous iteration of this patch, but
> Ard Biesheuvel pointed out that was also vmalloc'ed on some
> architectures.
>
> Reported-by: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>

Apologies for going back and forth on this, but it appears there may
be another way to deal with this.

First of all, we only need this handling for the authenticated data,
and only for CCM and GCM, not CMAC (which does not use scatterlists at
all, it simply calls the AES cipher directly)

So that leaves a fixed 20 bytes for GCM and fixed 32 bytes for CCM,
which we could allocate along with the AEAD request, e..g.,

"""
diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
index 8e898a6e8de8..c0c33e6ad94e 100644
--- a/net/mac80211/aes_ccm.c
+++ b/net/mac80211/aes_ccm.c
@@ -24,13 +24,17 @@ int ieee80211_aes_ccm_encrypt(struct crypto_aead
*tfm, u8 *b_0, u8 *aad,
 {
        struct scatterlist sg[3];
        struct aead_request *aead_req;
+       u8 *__aad;

        aead_req = aead_request_alloc(tfm, GFP_ATOMIC);
        if (!aead_req)
                return -ENOMEM;

+       __aad = (u8 *)aead_req + crypto_aead_reqsize(tfm);
+       memcpy(__aad, aad, 2 * AES_BLOCK_SIZE);
+
        sg_init_table(sg, 3);
-       sg_set_buf(&sg[0], &aad[2], be16_to_cpup((__be16 *)aad));
+       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);

@@ -49,6 +53,7 @@ int ieee80211_aes_ccm_decrypt(struct crypto_aead
*tfm, u8 *b_0, u8 *aad,
 {
        struct scatterlist sg[3];
        struct aead_request *aead_req;
+       u8 *__aad;
        int err;

        if (data_len == 0)
@@ -58,8 +63,11 @@ int ieee80211_aes_ccm_decrypt(struct crypto_aead
*tfm, u8 *b_0, u8 *aad,
        if (!aead_req)
                return -ENOMEM;

+       __aad = (u8 *)aead_req + crypto_aead_reqsize(tfm);
+       memcpy(__aad, aad, 2 * AES_BLOCK_SIZE);
+
        sg_init_table(sg, 3);
-       sg_set_buf(&sg[0], &aad[2], be16_to_cpup((__be16 *)aad));
+       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);

@@ -90,6 +98,8 @@ struct crypto_aead
*ieee80211_aes_key_setup_encrypt(const u8 key[],
        if (err)
                goto free_aead;

+       crypto_aead_set_reqsize(tfm,
+                               crypto_aead_reqsize(tfm) + 2 * AES_BLOCK_SIZE));
        return tfm;

 free_aead:
"""

^ permalink raw reply related

* [PATCH v4] mac80211: move extra crypto data off the stack
From: Johannes Berg @ 2016-10-17  8:33 UTC (permalink / raw)
  To: linux-wireless
  Cc: Ard Biesheuvel, Sergey Senozhatsky, netdev, herbert,
	Johannes Berg

From: Johannes Berg <johannes.berg@intel.com>

As the stack can (on x86-64) now be virtually mapped rather than
using "normal" kernel memory, Sergey noticed mac80211 isn't using
the SG APIs correctly by putting on-stack buffers into SG tables.
This leads to kernel crashes.

Fix this by allocating the extra fields dynamically on the fly as
needed, using a kmem cache.

I used per-CPU memory in a previous iteration of this patch, but
Ard Biesheuvel pointed out that was also vmalloc'ed on some
architectures.

Reported-by: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/aes_cmac.c    |   5 +-
 net/mac80211/aes_cmac.h    |   2 +
 net/mac80211/aes_gmac.c    |   9 ++-
 net/mac80211/aes_gmac.h    |   5 +-
 net/mac80211/ieee80211_i.h |   7 ++
 net/mac80211/main.c        |   8 +++
 net/mac80211/wpa.c         | 173 ++++++++++++++++++++++++++++++++++++---------
 7 files changed, 166 insertions(+), 43 deletions(-)

diff --git a/net/mac80211/aes_cmac.c b/net/mac80211/aes_cmac.c
index bdf0790d89cc..ebb8c2dc9928 100644
--- a/net/mac80211/aes_cmac.c
+++ b/net/mac80211/aes_cmac.c
@@ -20,7 +20,6 @@
 
 #define CMAC_TLEN 8 /* CMAC TLen = 64 bits (8 octets) */
 #define CMAC_TLEN_256 16 /* CMAC TLen = 128 bits (16 octets) */
-#define AAD_LEN 20
 
 
 static void gf_mulx(u8 *pad)
@@ -101,7 +100,7 @@ void ieee80211_aes_cmac(struct crypto_cipher *tfm, const u8 *aad,
 
 	memset(zero, 0, CMAC_TLEN);
 	addr[0] = aad;
-	len[0] = AAD_LEN;
+	len[0] = CMAC_AAD_LEN;
 	addr[1] = data;
 	len[1] = data_len - CMAC_TLEN;
 	addr[2] = zero;
@@ -119,7 +118,7 @@ void ieee80211_aes_cmac_256(struct crypto_cipher *tfm, const u8 *aad,
 
 	memset(zero, 0, CMAC_TLEN_256);
 	addr[0] = aad;
-	len[0] = AAD_LEN;
+	len[0] = CMAC_AAD_LEN;
 	addr[1] = data;
 	len[1] = data_len - CMAC_TLEN_256;
 	addr[2] = zero;
diff --git a/net/mac80211/aes_cmac.h b/net/mac80211/aes_cmac.h
index 3702041f44fd..6645f8963278 100644
--- a/net/mac80211/aes_cmac.h
+++ b/net/mac80211/aes_cmac.h
@@ -11,6 +11,8 @@
 
 #include <linux/crypto.h>
 
+#define CMAC_AAD_LEN 20
+
 struct crypto_cipher *ieee80211_aes_cmac_key_setup(const u8 key[],
 						   size_t key_len);
 void ieee80211_aes_cmac(struct crypto_cipher *tfm, const u8 *aad,
diff --git a/net/mac80211/aes_gmac.c b/net/mac80211/aes_gmac.c
index 6951af9715c0..86892e2e3c8c 100644
--- a/net/mac80211/aes_gmac.c
+++ b/net/mac80211/aes_gmac.c
@@ -19,13 +19,12 @@
 
 #define GMAC_MIC_LEN 16
 #define GMAC_NONCE_LEN 12
-#define AAD_LEN 20
 
 int ieee80211_aes_gmac(struct crypto_aead *tfm, const u8 *aad, u8 *nonce,
-		       const u8 *data, size_t data_len, u8 *mic)
+		       const u8 *data, size_t data_len, u8 *mic, u8 *zero)
 {
 	struct scatterlist sg[4];
-	u8 zero[GMAC_MIC_LEN], iv[AES_BLOCK_SIZE];
+	u8 iv[AES_BLOCK_SIZE];
 	struct aead_request *aead_req;
 
 	if (data_len < GMAC_MIC_LEN)
@@ -37,7 +36,7 @@ int ieee80211_aes_gmac(struct crypto_aead *tfm, const u8 *aad, u8 *nonce,
 
 	memset(zero, 0, GMAC_MIC_LEN);
 	sg_init_table(sg, 4);
-	sg_set_buf(&sg[0], aad, AAD_LEN);
+	sg_set_buf(&sg[0], aad, GMAC_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[3], mic, GMAC_MIC_LEN);
@@ -47,7 +46,7 @@ int ieee80211_aes_gmac(struct crypto_aead *tfm, const u8 *aad, u8 *nonce,
 	iv[AES_BLOCK_SIZE - 1] = 0x01;
 
 	aead_request_set_crypt(aead_req, sg, sg, 0, iv);
-	aead_request_set_ad(aead_req, AAD_LEN + data_len);
+	aead_request_set_ad(aead_req, GMAC_AAD_LEN + data_len);
 
 	crypto_aead_encrypt(aead_req);
 	aead_request_free(aead_req);
diff --git a/net/mac80211/aes_gmac.h b/net/mac80211/aes_gmac.h
index d328204d73a8..f06833c9095f 100644
--- a/net/mac80211/aes_gmac.h
+++ b/net/mac80211/aes_gmac.h
@@ -11,10 +11,13 @@
 
 #include <linux/crypto.h>
 
+#define GMAC_MIC_LEN 16
+#define GMAC_AAD_LEN 20
+
 struct crypto_aead *ieee80211_aes_gmac_key_setup(const u8 key[],
 						 size_t key_len);
 int ieee80211_aes_gmac(struct crypto_aead *tfm, const u8 *aad, u8 *nonce,
-		       const u8 *data, size_t data_len, u8 *mic);
+		       const u8 *data, size_t data_len, u8 *mic, u8 *zero);
 void ieee80211_aes_gmac_key_free(struct crypto_aead *tfm);
 
 #endif /* AES_GMAC_H */
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 34c2add2c455..a63593f6b645 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1128,6 +1128,13 @@ enum mac80211_scan_state {
 	SCAN_ABORT,
 };
 
+struct ieee80211_crypto_bufs {
+	u8 buf1[32];
+	u8 buf2[16];
+} ____cacheline_aligned_in_smp;
+
+extern struct kmem_cache *ieee80211_crypto_bufs_cache;
+
 struct ieee80211_local {
 	/* embed the driver visible part.
 	 * don't cast (use the static inlines below), but we keep
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 1075ac24c8c5..c6303a8a12d2 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -33,6 +33,8 @@
 #include "led.h"
 #include "debugfs.h"
 
+struct kmem_cache *ieee80211_crypto_bufs_cache;
+
 void ieee80211_configure_filter(struct ieee80211_local *local)
 {
 	u64 mc;
@@ -1234,6 +1236,10 @@ static int __init ieee80211_init(void)
 	BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, driver_data) +
 		     IEEE80211_TX_INFO_DRIVER_DATA_SIZE > sizeof(skb->cb));
 
+	ieee80211_crypto_bufs_cache = KMEM_CACHE(ieee80211_crypto_bufs, 0);
+	if (!ieee80211_crypto_bufs_cache)
+		return -ENOMEM;
+
 	ret = rc80211_minstrel_init();
 	if (ret)
 		return ret;
@@ -1264,6 +1270,8 @@ static void __exit ieee80211_exit(void)
 
 	ieee80211_iface_exit();
 
+	kmem_cache_destroy(ieee80211_crypto_bufs_cache);
+
 	rcu_barrier();
 }
 
diff --git a/net/mac80211/wpa.c b/net/mac80211/wpa.c
index 2e366438f8ef..ee9105d57545 100644
--- a/net/mac80211/wpa.c
+++ b/net/mac80211/wpa.c
@@ -405,8 +405,6 @@ static int ccmp_encrypt_skb(struct ieee80211_tx_data *tx, struct sk_buff *skb,
 	u8 *pos;
 	u8 pn[6];
 	u64 pn64;
-	u8 aad[2 * AES_BLOCK_SIZE];
-	u8 b_0[AES_BLOCK_SIZE];
 
 	if (info->control.hw_key &&
 	    !(info->control.hw_key->flags & IEEE80211_KEY_FLAG_GENERATE_IV) &&
@@ -456,13 +454,34 @@ static int ccmp_encrypt_skb(struct ieee80211_tx_data *tx, struct sk_buff *skb,
 	ccmp_pn2hdr(pos, pn, key->conf.keyidx);
 
 	/* hwaccel - with software CCMP header */
-	if (info->control.hw_key)
-		return 0;
+	if (unlikely(!info->control.hw_key)) {
+		struct ieee80211_crypto_bufs *bufs;
+		int err;
+		u8 *aad;
+		u8 *b_0;
+
+		bufs = kmem_cache_alloc(ieee80211_crypto_bufs_cache,
+					GFP_ATOMIC);
+		if (!bufs)
+			return -ENOMEM;
+
+		BUILD_BUG_ON(sizeof(bufs->buf1) < 2 * AES_BLOCK_SIZE);
+		BUILD_BUG_ON(sizeof(bufs->buf2) < AES_BLOCK_SIZE);
+
+		aad = bufs->buf1;
+		b_0 = bufs->buf2;
+
+		pos += IEEE80211_CCMP_HDR_LEN;
+		ccmp_special_blocks(skb, pn, b_0, aad);
+		err = ieee80211_aes_ccm_encrypt(key->u.ccmp.tfm, b_0, aad,
+						pos, len,
+						skb_put(skb, mic_len),
+						mic_len);
+		kmem_cache_free(ieee80211_crypto_bufs_cache, bufs);
+		return err;
+	}
 
-	pos += IEEE80211_CCMP_HDR_LEN;
-	ccmp_special_blocks(skb, pn, b_0, aad);
-	return ieee80211_aes_ccm_encrypt(key->u.ccmp.tfm, b_0, aad, pos, len,
-					 skb_put(skb, mic_len), mic_len);
+	return 0;
 }
 
 
@@ -532,16 +551,33 @@ ieee80211_crypto_ccmp_decrypt(struct ieee80211_rx_data *rx,
 		}
 
 		if (!(status->flag & RX_FLAG_DECRYPTED)) {
-			u8 aad[2 * AES_BLOCK_SIZE];
-			u8 b_0[AES_BLOCK_SIZE];
+			struct ieee80211_crypto_bufs *bufs;
+			int err;
+			u8 *aad;
+			u8 *b_0;
+
+			bufs = kmem_cache_alloc(ieee80211_crypto_bufs_cache,
+						GFP_ATOMIC);
+			if (!bufs)
+				return -ENOMEM;
+
+			BUILD_BUG_ON(sizeof(bufs->buf1) < 2 * AES_BLOCK_SIZE);
+			BUILD_BUG_ON(sizeof(bufs->buf2) < AES_BLOCK_SIZE);
+
+			aad = bufs->buf1;
+			b_0 = bufs->buf2;
+
 			/* hardware didn't decrypt/verify MIC */
 			ccmp_special_blocks(skb, pn, b_0, aad);
 
-			if (ieee80211_aes_ccm_decrypt(
+			err = ieee80211_aes_ccm_decrypt(
 				    key->u.ccmp.tfm, b_0, aad,
 				    skb->data + hdrlen + IEEE80211_CCMP_HDR_LEN,
 				    data_len,
-				    skb->data + skb->len - mic_len, mic_len))
+				    skb->data + skb->len - mic_len, mic_len);
+			kmem_cache_free(ieee80211_crypto_bufs_cache, bufs);
+
+			if (err)
 				return RX_DROP_UNUSABLE;
 		}
 
@@ -637,8 +673,6 @@ static int gcmp_encrypt_skb(struct ieee80211_tx_data *tx, struct sk_buff *skb)
 	u8 *pos;
 	u8 pn[6];
 	u64 pn64;
-	u8 aad[2 * AES_BLOCK_SIZE];
-	u8 j_0[AES_BLOCK_SIZE];
 
 	if (info->control.hw_key &&
 	    !(info->control.hw_key->flags & IEEE80211_KEY_FLAG_GENERATE_IV) &&
@@ -689,13 +723,34 @@ static int gcmp_encrypt_skb(struct ieee80211_tx_data *tx, struct sk_buff *skb)
 	gcmp_pn2hdr(pos, pn, key->conf.keyidx);
 
 	/* hwaccel - with software GCMP header */
-	if (info->control.hw_key)
-		return 0;
+	if (unlikely(!info->control.hw_key)) {
+		struct ieee80211_crypto_bufs *bufs;
+		int err;
+		u8 *aad;
+		u8 *j_0;
+		u8 *mic = skb_put(skb, IEEE80211_GCMP_MIC_LEN);
+
+		bufs = kmem_cache_alloc(ieee80211_crypto_bufs_cache,
+					GFP_ATOMIC);
+		if (!bufs)
+			return -ENOMEM;
+
+		BUILD_BUG_ON(sizeof(bufs->buf1) < 2 * AES_BLOCK_SIZE);
+		BUILD_BUG_ON(sizeof(bufs->buf2) < AES_BLOCK_SIZE);
+
+		aad = bufs->buf1;
+		j_0 = bufs->buf2;
+
+		pos += IEEE80211_GCMP_HDR_LEN;
+		gcmp_special_blocks(skb, pn, j_0, aad);
+		err = ieee80211_aes_gcm_encrypt(key->u.gcmp.tfm, j_0, aad,
+						pos, len, mic);
+		kmem_cache_free(ieee80211_crypto_bufs_cache, bufs);
+
+		return err;
+	}
 
-	pos += IEEE80211_GCMP_HDR_LEN;
-	gcmp_special_blocks(skb, pn, j_0, aad);
-	return ieee80211_aes_gcm_encrypt(key->u.gcmp.tfm, j_0, aad, pos, len,
-					 skb_put(skb, IEEE80211_GCMP_MIC_LEN));
+	return 0;
 }
 
 ieee80211_tx_result
@@ -760,17 +815,34 @@ ieee80211_crypto_gcmp_decrypt(struct ieee80211_rx_data *rx)
 		}
 
 		if (!(status->flag & RX_FLAG_DECRYPTED)) {
-			u8 aad[2 * AES_BLOCK_SIZE];
-			u8 j_0[AES_BLOCK_SIZE];
+			struct ieee80211_crypto_bufs *bufs;
+			int err;
+			u8 *aad;
+			u8 *j_0;
+
+			bufs = kmem_cache_alloc(ieee80211_crypto_bufs_cache,
+						GFP_ATOMIC);
+			if (!bufs)
+				return -ENOMEM;
+
+			BUILD_BUG_ON(sizeof(bufs->buf1) < 2 * AES_BLOCK_SIZE);
+			BUILD_BUG_ON(sizeof(bufs->buf2) < AES_BLOCK_SIZE);
+
+			aad = bufs->buf1;
+			j_0 = bufs->buf2;
+
 			/* hardware didn't decrypt/verify MIC */
 			gcmp_special_blocks(skb, pn, j_0, aad);
 
-			if (ieee80211_aes_gcm_decrypt(
+			err = ieee80211_aes_gcm_decrypt(
 				    key->u.gcmp.tfm, j_0, aad,
 				    skb->data + hdrlen + IEEE80211_GCMP_HDR_LEN,
 				    data_len,
 				    skb->data + skb->len -
-				    IEEE80211_GCMP_MIC_LEN))
+				    IEEE80211_GCMP_MIC_LEN);
+			kmem_cache_free(ieee80211_crypto_bufs_cache, bufs);
+
+			if (err)
 				return RX_DROP_UNUSABLE;
 		}
 
@@ -1119,9 +1191,12 @@ ieee80211_crypto_aes_gmac_encrypt(struct ieee80211_tx_data *tx)
 	struct ieee80211_key *key = tx->key;
 	struct ieee80211_mmie_16 *mmie;
 	struct ieee80211_hdr *hdr;
-	u8 aad[20];
 	u64 pn64;
 	u8 nonce[12];
+	struct ieee80211_crypto_bufs *bufs;
+	int err;
+	u8 *aad;
+	u8 *zero;
 
 	if (WARN_ON(skb_queue_len(&tx->skbs) != 1))
 		return TX_DROP;
@@ -1136,6 +1211,16 @@ ieee80211_crypto_aes_gmac_encrypt(struct ieee80211_tx_data *tx)
 	if (WARN_ON(skb_tailroom(skb) < sizeof(*mmie)))
 		return TX_DROP;
 
+	bufs = kmem_cache_alloc(ieee80211_crypto_bufs_cache, GFP_ATOMIC);
+	if (!bufs)
+		return TX_DROP;
+
+	BUILD_BUG_ON(sizeof(bufs->buf1) < GMAC_AAD_LEN);
+	BUILD_BUG_ON(sizeof(bufs->buf2) < GMAC_MIC_LEN);
+
+	aad = bufs->buf1;
+	zero = bufs->buf2;
+
 	mmie = (struct ieee80211_mmie_16 *)skb_put(skb, sizeof(*mmie));
 	mmie->element_id = WLAN_EID_MMIE;
 	mmie->length = sizeof(*mmie) - 2;
@@ -1153,11 +1238,13 @@ ieee80211_crypto_aes_gmac_encrypt(struct ieee80211_tx_data *tx)
 	bip_ipn_swap(nonce + ETH_ALEN, mmie->sequence_number);
 
 	/* MIC = AES-GMAC(IGTK, AAD || Management Frame Body || MMIE, 128) */
-	if (ieee80211_aes_gmac(key->u.aes_gmac.tfm, aad, nonce,
-			       skb->data + 24, skb->len - 24, mmie->mic) < 0)
-		return TX_DROP;
+	err = ieee80211_aes_gmac(key->u.aes_gmac.tfm, aad, nonce,
+				 skb->data + 24, skb->len - 24, mmie->mic,
+				 zero);
 
-	return TX_CONTINUE;
+	kmem_cache_free(ieee80211_crypto_bufs_cache, bufs);
+
+	return err < 0 ? TX_DROP : TX_CONTINUE;
 }
 
 ieee80211_rx_result
@@ -1167,7 +1254,7 @@ ieee80211_crypto_aes_gmac_decrypt(struct ieee80211_rx_data *rx)
 	struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
 	struct ieee80211_key *key = rx->key;
 	struct ieee80211_mmie_16 *mmie;
-	u8 aad[20], mic[16], ipn[6], nonce[12];
+	u8 mic[16], ipn[6], nonce[12];
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
 
 	if (!ieee80211_is_mgmt(hdr->frame_control))
@@ -1192,16 +1279,34 @@ ieee80211_crypto_aes_gmac_decrypt(struct ieee80211_rx_data *rx)
 	}
 
 	if (!(status->flag & RX_FLAG_DECRYPTED)) {
+		struct ieee80211_crypto_bufs *bufs;
+		int err;
+		u8 *aad;
+		u8 *zero;
+
+		bufs = kmem_cache_alloc(ieee80211_crypto_bufs_cache,
+					GFP_ATOMIC);
+		if (!bufs)
+			return RX_DROP_UNUSABLE;
+
+		BUILD_BUG_ON(sizeof(bufs->buf1) < GMAC_AAD_LEN);
+		BUILD_BUG_ON(sizeof(bufs->buf2) < GMAC_MIC_LEN);
+
+		aad = bufs->buf1;
+		zero = bufs->buf2;
+
 		/* hardware didn't decrypt/verify MIC */
 		bip_aad(skb, aad);
 
 		memcpy(nonce, hdr->addr2, ETH_ALEN);
 		memcpy(nonce + ETH_ALEN, ipn, 6);
 
-		if (ieee80211_aes_gmac(key->u.aes_gmac.tfm, aad, nonce,
-				       skb->data + 24, skb->len - 24,
-				       mic) < 0 ||
-		    memcmp(mic, mmie->mic, sizeof(mmie->mic)) != 0) {
+		err = ieee80211_aes_gmac(key->u.aes_gmac.tfm, aad, nonce,
+					 skb->data + 24, skb->len - 24,
+					 mic, zero);
+		kmem_cache_free(ieee80211_crypto_bufs_cache, bufs);
+
+		if (err < 0 || memcmp(mic, mmie->mic, sizeof(mmie->mic)) != 0) {
 			key->u.aes_gmac.icverrors++;
 			return RX_DROP_UNUSABLE;
 		}
-- 
2.8.1

^ permalink raw reply related

* Re: [PATCH v3] mac80211: move struct aead_req off the stack
From: Ard Biesheuvel @ 2016-10-17  8:10 UTC (permalink / raw)
  To: Johannes Berg
  Cc: <linux-wireless@vger.kernel.org>, Sergey Senozhatsky,
	<netdev@vger.kernel.org>, Herbert Xu, Johannes Berg
In-Reply-To: <1476691431-27693-1-git-send-email-johannes@sipsolutions.net>

On 17 October 2016 at 09:03, Johannes Berg <johannes@sipsolutions.net> wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> Some crypto implementations (such as the generic CCM wrapper in crypto/)
> use scatterlists to map fields of private data in their struct aead_req.
> This means these data structures cannot live in the vmalloc area, which
> means that they cannot live on the stack (with CONFIG_VMAP_STACK.)
>
> This currently occurs only with the generic software implementation, but
> the private data and usage is implementation specific, so move the whole
> data structures off the stack into heap by allocating every time we need
> to use them.
>
> This pattern already exists in the IPsec ESP driver, but in the future,
> we may want/need to improve upon this, e.g. by using a slab cache.
>
> (Based on Ard's patch, but that was missing error handling and GCM/GMAC)
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
> v3: remove superfluous aead_request_set_tfm() calls

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  net/mac80211/aes_ccm.c  | 36 +++++++++++++++++++-----------------
>  net/mac80211/aes_ccm.h  |  6 +++---
>  net/mac80211/aes_gcm.c  | 33 +++++++++++++++++----------------
>  net/mac80211/aes_gcm.h  |  4 ++--
>  net/mac80211/aes_gmac.c | 11 +++++------
>  net/mac80211/wpa.c      | 12 ++++--------
>  6 files changed, 50 insertions(+), 52 deletions(-)
>
> diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
> index 7663c28ba353..8e898a6e8de8 100644
> --- a/net/mac80211/aes_ccm.c
> +++ b/net/mac80211/aes_ccm.c
> @@ -18,29 +18,29 @@
>  #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)
> +int 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 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 = aead_request_alloc(tfm, GFP_ATOMIC);
> +       if (!aead_req)
> +               return -ENOMEM;
>
>         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_crypt(aead_req, sg, sg, data_len, b_0);
>         aead_request_set_ad(aead_req, sg[0].length);
>
>         crypto_aead_encrypt(aead_req);
> +       aead_request_free(aead_req);
> +
> +       return 0;
>  }
>
>  int ieee80211_aes_ccm_decrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
> @@ -48,26 +48,28 @@ 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 = aead_request_alloc(tfm, GFP_ATOMIC);
> +       if (!aead_req)
> +               return -ENOMEM;
>
>         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_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);
> +       aead_request_free(aead_req);
> +
> +       return err;
>  }
>
>  struct crypto_aead *ieee80211_aes_key_setup_encrypt(const u8 key[],
> diff --git a/net/mac80211/aes_ccm.h b/net/mac80211/aes_ccm.h
> index 6a73d1e4d186..03e21b0995e3 100644
> --- a/net/mac80211/aes_ccm.h
> +++ b/net/mac80211/aes_ccm.h
> @@ -15,9 +15,9 @@
>  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_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);
> diff --git a/net/mac80211/aes_gcm.c b/net/mac80211/aes_gcm.c
> index 3afe361fd27c..831054c6c756 100644
> --- a/net/mac80211/aes_gcm.c
> +++ b/net/mac80211/aes_gcm.c
> @@ -15,55 +15,56 @@
>  #include "key.h"
>  #include "aes_gcm.h"
>
> -void ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
> -                              u8 *data, size_t data_len, u8 *mic)
> +int ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
> +                             u8 *data, size_t data_len, u8 *mic)
>  {
>         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 = aead_request_alloc(tfm, GFP_ATOMIC);
> +       if (!aead_req)
> +               return -ENOMEM;
>
>         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_crypt(aead_req, sg, sg, data_len, j_0);
>         aead_request_set_ad(aead_req, sg[0].length);
>
>         crypto_aead_encrypt(aead_req);
> +       aead_request_free(aead_req);
> +       return 0;
>  }
>
>  int ieee80211_aes_gcm_decrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
>                               u8 *data, size_t data_len, u8 *mic)
>  {
>         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 = aead_request_alloc(tfm, GFP_ATOMIC);
> +       if (!aead_req)
> +               return -ENOMEM;
>
>         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_crypt(aead_req, sg, sg,
>                                data_len + IEEE80211_GCMP_MIC_LEN, j_0);
>         aead_request_set_ad(aead_req, sg[0].length);
>
> -       return crypto_aead_decrypt(aead_req);
> +       err = crypto_aead_decrypt(aead_req);
> +       aead_request_free(aead_req);
> +
> +       return err;
>  }
>
>  struct crypto_aead *ieee80211_aes_gcm_key_setup_encrypt(const u8 key[],
> diff --git a/net/mac80211/aes_gcm.h b/net/mac80211/aes_gcm.h
> index 1347fda6b76a..b36503cd7716 100644
> --- a/net/mac80211/aes_gcm.h
> +++ b/net/mac80211/aes_gcm.h
> @@ -11,8 +11,8 @@
>
>  #include <linux/crypto.h>
>
> -void ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
> -                              u8 *data, size_t data_len, u8 *mic);
> +int ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
> +                             u8 *data, size_t data_len, u8 *mic);
>  int ieee80211_aes_gcm_decrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
>                               u8 *data, size_t data_len, u8 *mic);
>  struct crypto_aead *ieee80211_aes_gcm_key_setup_encrypt(const u8 key[],
> diff --git a/net/mac80211/aes_gmac.c b/net/mac80211/aes_gmac.c
> index 3ddd927aaf30..6951af9715c0 100644
> --- a/net/mac80211/aes_gmac.c
> +++ b/net/mac80211/aes_gmac.c
> @@ -25,16 +25,15 @@ 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[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 zero[GMAC_MIC_LEN], iv[AES_BLOCK_SIZE];
> +       struct aead_request *aead_req;
>
>         if (data_len < GMAC_MIC_LEN)
>                 return -EINVAL;
>
> -       memset(aead_req, 0, sizeof(aead_req_data));
> +       aead_req = aead_request_alloc(tfm, GFP_ATOMIC);
> +       if (!aead_req)
> +               return -ENOMEM;
>
>         memset(zero, 0, GMAC_MIC_LEN);
>         sg_init_table(sg, 4);
> @@ -47,11 +46,11 @@ int ieee80211_aes_gmac(struct crypto_aead *tfm, const u8 *aad, u8 *nonce,
>         memset(iv + GMAC_NONCE_LEN, 0, sizeof(iv) - GMAC_NONCE_LEN);
>         iv[AES_BLOCK_SIZE - 1] = 0x01;
>
> -       aead_request_set_tfm(aead_req, tfm);
>         aead_request_set_crypt(aead_req, sg, sg, 0, iv);
>         aead_request_set_ad(aead_req, AAD_LEN + data_len);
>
>         crypto_aead_encrypt(aead_req);
> +       aead_request_free(aead_req);
>
>         return 0;
>  }
> diff --git a/net/mac80211/wpa.c b/net/mac80211/wpa.c
> index b48c1e13e281..2e366438f8ef 100644
> --- a/net/mac80211/wpa.c
> +++ b/net/mac80211/wpa.c
> @@ -461,10 +461,8 @@ static int ccmp_encrypt_skb(struct ieee80211_tx_data *tx, struct sk_buff *skb,
>
>         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,
> -                                 skb_put(skb, mic_len), mic_len);
> -
> -       return 0;
> +       return ieee80211_aes_ccm_encrypt(key->u.ccmp.tfm, b_0, aad, pos, len,
> +                                        skb_put(skb, mic_len), mic_len);
>  }
>
>
> @@ -696,10 +694,8 @@ static int gcmp_encrypt_skb(struct ieee80211_tx_data *tx, struct sk_buff *skb)
>
>         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,
> -                                 skb_put(skb, IEEE80211_GCMP_MIC_LEN));
> -
> -       return 0;
> +       return ieee80211_aes_gcm_encrypt(key->u.gcmp.tfm, j_0, aad, pos, len,
> +                                        skb_put(skb, IEEE80211_GCMP_MIC_LEN));
>  }
>
>  ieee80211_tx_result
> --
> 2.8.1
>

^ permalink raw reply

* Re: [PATCH v3] mac80211: move extra crypto data off the stack
From: Johannes Berg @ 2016-10-17  8:04 UTC (permalink / raw)
  To: linux-wireless; +Cc: Ard Biesheuvel, Sergey Senozhatsky, netdev, herbert
In-Reply-To: <1476691342-22347-1-git-send-email-johannes@sipsolutions.net>

[snip]

Sorry, this went out by mistake.

johannes

^ permalink raw reply

* [PATCH v3] mac80211: move struct aead_req off the stack
From: Johannes Berg @ 2016-10-17  8:03 UTC (permalink / raw)
  To: linux-wireless
  Cc: Ard Biesheuvel, Sergey Senozhatsky, netdev, herbert,
	Johannes Berg

From: Johannes Berg <johannes.berg@intel.com>

Some crypto implementations (such as the generic CCM wrapper in crypto/)
use scatterlists to map fields of private data in their struct aead_req.
This means these data structures cannot live in the vmalloc area, which
means that they cannot live on the stack (with CONFIG_VMAP_STACK.)

This currently occurs only with the generic software implementation, but
the private data and usage is implementation specific, so move the whole
data structures off the stack into heap by allocating every time we need
to use them.

This pattern already exists in the IPsec ESP driver, but in the future,
we may want/need to improve upon this, e.g. by using a slab cache.

(Based on Ard's patch, but that was missing error handling and GCM/GMAC)

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
v3: remove superfluous aead_request_set_tfm() calls
---
 net/mac80211/aes_ccm.c  | 36 +++++++++++++++++++-----------------
 net/mac80211/aes_ccm.h  |  6 +++---
 net/mac80211/aes_gcm.c  | 33 +++++++++++++++++----------------
 net/mac80211/aes_gcm.h  |  4 ++--
 net/mac80211/aes_gmac.c | 11 +++++------
 net/mac80211/wpa.c      | 12 ++++--------
 6 files changed, 50 insertions(+), 52 deletions(-)

diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
index 7663c28ba353..8e898a6e8de8 100644
--- a/net/mac80211/aes_ccm.c
+++ b/net/mac80211/aes_ccm.c
@@ -18,29 +18,29 @@
 #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)
+int 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 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 = aead_request_alloc(tfm, GFP_ATOMIC);
+	if (!aead_req)
+		return -ENOMEM;
 
 	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_crypt(aead_req, sg, sg, data_len, b_0);
 	aead_request_set_ad(aead_req, sg[0].length);
 
 	crypto_aead_encrypt(aead_req);
+	aead_request_free(aead_req);
+
+	return 0;
 }
 
 int ieee80211_aes_ccm_decrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
@@ -48,26 +48,28 @@ 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 = aead_request_alloc(tfm, GFP_ATOMIC);
+	if (!aead_req)
+		return -ENOMEM;
 
 	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_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);
+	aead_request_free(aead_req);
+
+	return err;
 }
 
 struct crypto_aead *ieee80211_aes_key_setup_encrypt(const u8 key[],
diff --git a/net/mac80211/aes_ccm.h b/net/mac80211/aes_ccm.h
index 6a73d1e4d186..03e21b0995e3 100644
--- a/net/mac80211/aes_ccm.h
+++ b/net/mac80211/aes_ccm.h
@@ -15,9 +15,9 @@
 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_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);
diff --git a/net/mac80211/aes_gcm.c b/net/mac80211/aes_gcm.c
index 3afe361fd27c..831054c6c756 100644
--- a/net/mac80211/aes_gcm.c
+++ b/net/mac80211/aes_gcm.c
@@ -15,55 +15,56 @@
 #include "key.h"
 #include "aes_gcm.h"
 
-void ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
-			       u8 *data, size_t data_len, u8 *mic)
+int ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
+			      u8 *data, size_t data_len, u8 *mic)
 {
 	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 = aead_request_alloc(tfm, GFP_ATOMIC);
+	if (!aead_req)
+		return -ENOMEM;
 
 	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_crypt(aead_req, sg, sg, data_len, j_0);
 	aead_request_set_ad(aead_req, sg[0].length);
 
 	crypto_aead_encrypt(aead_req);
+	aead_request_free(aead_req);
+	return 0;
 }
 
 int ieee80211_aes_gcm_decrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
 			      u8 *data, size_t data_len, u8 *mic)
 {
 	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 = aead_request_alloc(tfm, GFP_ATOMIC);
+	if (!aead_req)
+		return -ENOMEM;
 
 	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_crypt(aead_req, sg, sg,
 			       data_len + IEEE80211_GCMP_MIC_LEN, j_0);
 	aead_request_set_ad(aead_req, sg[0].length);
 
-	return crypto_aead_decrypt(aead_req);
+	err = crypto_aead_decrypt(aead_req);
+	aead_request_free(aead_req);
+
+	return err;
 }
 
 struct crypto_aead *ieee80211_aes_gcm_key_setup_encrypt(const u8 key[],
diff --git a/net/mac80211/aes_gcm.h b/net/mac80211/aes_gcm.h
index 1347fda6b76a..b36503cd7716 100644
--- a/net/mac80211/aes_gcm.h
+++ b/net/mac80211/aes_gcm.h
@@ -11,8 +11,8 @@
 
 #include <linux/crypto.h>
 
-void ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
-			       u8 *data, size_t data_len, u8 *mic);
+int ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
+			      u8 *data, size_t data_len, u8 *mic);
 int ieee80211_aes_gcm_decrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
 			      u8 *data, size_t data_len, u8 *mic);
 struct crypto_aead *ieee80211_aes_gcm_key_setup_encrypt(const u8 key[],
diff --git a/net/mac80211/aes_gmac.c b/net/mac80211/aes_gmac.c
index 3ddd927aaf30..6951af9715c0 100644
--- a/net/mac80211/aes_gmac.c
+++ b/net/mac80211/aes_gmac.c
@@ -25,16 +25,15 @@ 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[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 zero[GMAC_MIC_LEN], iv[AES_BLOCK_SIZE];
+	struct aead_request *aead_req;
 
 	if (data_len < GMAC_MIC_LEN)
 		return -EINVAL;
 
-	memset(aead_req, 0, sizeof(aead_req_data));
+	aead_req = aead_request_alloc(tfm, GFP_ATOMIC);
+	if (!aead_req)
+		return -ENOMEM;
 
 	memset(zero, 0, GMAC_MIC_LEN);
 	sg_init_table(sg, 4);
@@ -47,11 +46,11 @@ int ieee80211_aes_gmac(struct crypto_aead *tfm, const u8 *aad, u8 *nonce,
 	memset(iv + GMAC_NONCE_LEN, 0, sizeof(iv) - GMAC_NONCE_LEN);
 	iv[AES_BLOCK_SIZE - 1] = 0x01;
 
-	aead_request_set_tfm(aead_req, tfm);
 	aead_request_set_crypt(aead_req, sg, sg, 0, iv);
 	aead_request_set_ad(aead_req, AAD_LEN + data_len);
 
 	crypto_aead_encrypt(aead_req);
+	aead_request_free(aead_req);
 
 	return 0;
 }
diff --git a/net/mac80211/wpa.c b/net/mac80211/wpa.c
index b48c1e13e281..2e366438f8ef 100644
--- a/net/mac80211/wpa.c
+++ b/net/mac80211/wpa.c
@@ -461,10 +461,8 @@ static int ccmp_encrypt_skb(struct ieee80211_tx_data *tx, struct sk_buff *skb,
 
 	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,
-				  skb_put(skb, mic_len), mic_len);
-
-	return 0;
+	return ieee80211_aes_ccm_encrypt(key->u.ccmp.tfm, b_0, aad, pos, len,
+					 skb_put(skb, mic_len), mic_len);
 }
 
 
@@ -696,10 +694,8 @@ static int gcmp_encrypt_skb(struct ieee80211_tx_data *tx, struct sk_buff *skb)
 
 	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,
-				  skb_put(skb, IEEE80211_GCMP_MIC_LEN));
-
-	return 0;
+	return ieee80211_aes_gcm_encrypt(key->u.gcmp.tfm, j_0, aad, pos, len,
+					 skb_put(skb, IEEE80211_GCMP_MIC_LEN));
 }
 
 ieee80211_tx_result
-- 
2.8.1

^ permalink raw reply related

* [PATCH v3] mac80211: move extra crypto data off the stack
From: Johannes Berg @ 2016-10-17  8:02 UTC (permalink / raw)
  To: linux-wireless
  Cc: Ard Biesheuvel, Sergey Senozhatsky, netdev, herbert,
	Johannes Berg

From: Johannes Berg <johannes.berg@intel.com>

As the stack can (on x86-64) now be virtually mapped rather than
using "normal" kernel memory, Sergey noticed mac80211 isn't using
the SG APIs correctly by putting on-stack buffers into SG tables.
This leads to kernel crashes.

Fix this by allocating a bit of per-CPU memory for the extra data
that encryption/decryption/verification needs, instead of having
it stored on the stack.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/aes_ccm.c     |  2 --
 net/mac80211/aes_cmac.c    |  5 ++-
 net/mac80211/aes_cmac.h    |  2 ++
 net/mac80211/aes_gcm.c     |  2 --
 net/mac80211/aes_gmac.c    | 10 +++---
 net/mac80211/aes_gmac.h    |  5 ++-
 net/mac80211/ieee80211_i.h |  7 ++++
 net/mac80211/main.c        |  8 +++++
 net/mac80211/wpa.c         | 86 +++++++++++++++++++++++++++++++++++++---------
 9 files changed, 97 insertions(+), 30 deletions(-)

diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
index 5c9fe00be6cc..8e898a6e8de8 100644
--- a/net/mac80211/aes_ccm.c
+++ b/net/mac80211/aes_ccm.c
@@ -34,7 +34,6 @@ int ieee80211_aes_ccm_encrypt(struct crypto_aead *tfm, u8 *b_0, u8 *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_crypt(aead_req, sg, sg, data_len, b_0);
 	aead_request_set_ad(aead_req, sg[0].length);
 
@@ -64,7 +63,6 @@ int ieee80211_aes_ccm_decrypt(struct crypto_aead *tfm, u8 *b_0, u8 *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_crypt(aead_req, sg, sg, data_len + mic_len, b_0);
 	aead_request_set_ad(aead_req, sg[0].length);
 
diff --git a/net/mac80211/aes_cmac.c b/net/mac80211/aes_cmac.c
index bdf0790d89cc..ebb8c2dc9928 100644
--- a/net/mac80211/aes_cmac.c
+++ b/net/mac80211/aes_cmac.c
@@ -20,7 +20,6 @@
 
 #define CMAC_TLEN 8 /* CMAC TLen = 64 bits (8 octets) */
 #define CMAC_TLEN_256 16 /* CMAC TLen = 128 bits (16 octets) */
-#define AAD_LEN 20
 
 
 static void gf_mulx(u8 *pad)
@@ -101,7 +100,7 @@ void ieee80211_aes_cmac(struct crypto_cipher *tfm, const u8 *aad,
 
 	memset(zero, 0, CMAC_TLEN);
 	addr[0] = aad;
-	len[0] = AAD_LEN;
+	len[0] = CMAC_AAD_LEN;
 	addr[1] = data;
 	len[1] = data_len - CMAC_TLEN;
 	addr[2] = zero;
@@ -119,7 +118,7 @@ void ieee80211_aes_cmac_256(struct crypto_cipher *tfm, const u8 *aad,
 
 	memset(zero, 0, CMAC_TLEN_256);
 	addr[0] = aad;
-	len[0] = AAD_LEN;
+	len[0] = CMAC_AAD_LEN;
 	addr[1] = data;
 	len[1] = data_len - CMAC_TLEN_256;
 	addr[2] = zero;
diff --git a/net/mac80211/aes_cmac.h b/net/mac80211/aes_cmac.h
index 3702041f44fd..6645f8963278 100644
--- a/net/mac80211/aes_cmac.h
+++ b/net/mac80211/aes_cmac.h
@@ -11,6 +11,8 @@
 
 #include <linux/crypto.h>
 
+#define CMAC_AAD_LEN 20
+
 struct crypto_cipher *ieee80211_aes_cmac_key_setup(const u8 key[],
 						   size_t key_len);
 void ieee80211_aes_cmac(struct crypto_cipher *tfm, const u8 *aad,
diff --git a/net/mac80211/aes_gcm.c b/net/mac80211/aes_gcm.c
index aa8eb2718bc1..831054c6c756 100644
--- a/net/mac80211/aes_gcm.c
+++ b/net/mac80211/aes_gcm.c
@@ -30,7 +30,6 @@ int ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *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_crypt(aead_req, sg, sg, data_len, j_0);
 	aead_request_set_ad(aead_req, sg[0].length);
 
@@ -58,7 +57,6 @@ int ieee80211_aes_gcm_decrypt(struct crypto_aead *tfm, u8 *j_0, u8 *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_crypt(aead_req, sg, sg,
 			       data_len + IEEE80211_GCMP_MIC_LEN, j_0);
 	aead_request_set_ad(aead_req, sg[0].length);
diff --git a/net/mac80211/aes_gmac.c b/net/mac80211/aes_gmac.c
index 15cfcebf0884..86892e2e3c8c 100644
--- a/net/mac80211/aes_gmac.c
+++ b/net/mac80211/aes_gmac.c
@@ -19,13 +19,12 @@
 
 #define GMAC_MIC_LEN 16
 #define GMAC_NONCE_LEN 12
-#define AAD_LEN 20
 
 int ieee80211_aes_gmac(struct crypto_aead *tfm, const u8 *aad, u8 *nonce,
-		       const u8 *data, size_t data_len, u8 *mic)
+		       const u8 *data, size_t data_len, u8 *mic, u8 *zero)
 {
 	struct scatterlist sg[4];
-	u8 zero[GMAC_MIC_LEN], iv[AES_BLOCK_SIZE];
+	u8 iv[AES_BLOCK_SIZE];
 	struct aead_request *aead_req;
 
 	if (data_len < GMAC_MIC_LEN)
@@ -37,7 +36,7 @@ int ieee80211_aes_gmac(struct crypto_aead *tfm, const u8 *aad, u8 *nonce,
 
 	memset(zero, 0, GMAC_MIC_LEN);
 	sg_init_table(sg, 4);
-	sg_set_buf(&sg[0], aad, AAD_LEN);
+	sg_set_buf(&sg[0], aad, GMAC_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[3], mic, GMAC_MIC_LEN);
@@ -46,9 +45,8 @@ int ieee80211_aes_gmac(struct crypto_aead *tfm, const u8 *aad, u8 *nonce,
 	memset(iv + GMAC_NONCE_LEN, 0, sizeof(iv) - GMAC_NONCE_LEN);
 	iv[AES_BLOCK_SIZE - 1] = 0x01;
 
-	aead_request_set_tfm(aead_req, tfm);
 	aead_request_set_crypt(aead_req, sg, sg, 0, iv);
-	aead_request_set_ad(aead_req, AAD_LEN + data_len);
+	aead_request_set_ad(aead_req, GMAC_AAD_LEN + data_len);
 
 	crypto_aead_encrypt(aead_req);
 	aead_request_free(aead_req);
diff --git a/net/mac80211/aes_gmac.h b/net/mac80211/aes_gmac.h
index d328204d73a8..f06833c9095f 100644
--- a/net/mac80211/aes_gmac.h
+++ b/net/mac80211/aes_gmac.h
@@ -11,10 +11,13 @@
 
 #include <linux/crypto.h>
 
+#define GMAC_MIC_LEN 16
+#define GMAC_AAD_LEN 20
+
 struct crypto_aead *ieee80211_aes_gmac_key_setup(const u8 key[],
 						 size_t key_len);
 int ieee80211_aes_gmac(struct crypto_aead *tfm, const u8 *aad, u8 *nonce,
-		       const u8 *data, size_t data_len, u8 *mic);
+		       const u8 *data, size_t data_len, u8 *mic, u8 *zero);
 void ieee80211_aes_gmac_key_free(struct crypto_aead *tfm);
 
 #endif /* AES_GMAC_H */
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 34c2add2c455..d68aae40b8d5 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1128,6 +1128,13 @@ enum mac80211_scan_state {
 	SCAN_ABORT,
 };
 
+struct ieee80211_crypto_bufs {
+	u8 buf1[32];
+	u8 buf2[16];
+};
+
+extern struct ieee80211_crypto_bufs __percpu *ieee80211_crypto_bufs;
+
 struct ieee80211_local {
 	/* embed the driver visible part.
 	 * don't cast (use the static inlines below), but we keep
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 1075ac24c8c5..6175cde94c53 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -33,6 +33,8 @@
 #include "led.h"
 #include "debugfs.h"
 
+struct ieee80211_crypto_bufs __percpu *ieee80211_crypto_bufs;
+
 void ieee80211_configure_filter(struct ieee80211_local *local)
 {
 	u64 mc;
@@ -1234,6 +1236,10 @@ static int __init ieee80211_init(void)
 	BUILD_BUG_ON(offsetof(struct ieee80211_tx_info, driver_data) +
 		     IEEE80211_TX_INFO_DRIVER_DATA_SIZE > sizeof(skb->cb));
 
+	ieee80211_crypto_bufs = alloc_percpu(struct ieee80211_crypto_bufs);
+	if (!ieee80211_crypto_bufs)
+		return -ENOMEM;
+
 	ret = rc80211_minstrel_init();
 	if (ret)
 		return ret;
@@ -1264,6 +1270,8 @@ static void __exit ieee80211_exit(void)
 
 	ieee80211_iface_exit();
 
+	free_percpu(ieee80211_crypto_bufs);
+
 	rcu_barrier();
 }
 
diff --git a/net/mac80211/wpa.c b/net/mac80211/wpa.c
index 2e366438f8ef..3a83a6763664 100644
--- a/net/mac80211/wpa.c
+++ b/net/mac80211/wpa.c
@@ -405,8 +405,13 @@ static int ccmp_encrypt_skb(struct ieee80211_tx_data *tx, struct sk_buff *skb,
 	u8 *pos;
 	u8 pn[6];
 	u64 pn64;
-	u8 aad[2 * AES_BLOCK_SIZE];
-	u8 b_0[AES_BLOCK_SIZE];
+	struct ieee80211_crypto_bufs *bufs =
+		this_cpu_ptr(ieee80211_crypto_bufs);
+	u8 *aad = bufs->buf1;
+	u8 *b_0 = bufs->buf2;
+
+	BUILD_BUG_ON(sizeof(bufs->buf1) < 2 * AES_BLOCK_SIZE);
+	BUILD_BUG_ON(sizeof(bufs->buf2) < AES_BLOCK_SIZE);
 
 	if (info->control.hw_key &&
 	    !(info->control.hw_key->flags & IEEE80211_KEY_FLAG_GENERATE_IV) &&
@@ -532,8 +537,14 @@ ieee80211_crypto_ccmp_decrypt(struct ieee80211_rx_data *rx,
 		}
 
 		if (!(status->flag & RX_FLAG_DECRYPTED)) {
-			u8 aad[2 * AES_BLOCK_SIZE];
-			u8 b_0[AES_BLOCK_SIZE];
+			struct ieee80211_crypto_bufs *bufs =
+				this_cpu_ptr(ieee80211_crypto_bufs);
+			u8 *aad = bufs->buf1;
+			u8 *b_0 = bufs->buf2;
+
+			BUILD_BUG_ON(sizeof(bufs->buf1) < 2 * AES_BLOCK_SIZE);
+			BUILD_BUG_ON(sizeof(bufs->buf2) < AES_BLOCK_SIZE);
+
 			/* hardware didn't decrypt/verify MIC */
 			ccmp_special_blocks(skb, pn, b_0, aad);
 
@@ -637,8 +648,13 @@ static int gcmp_encrypt_skb(struct ieee80211_tx_data *tx, struct sk_buff *skb)
 	u8 *pos;
 	u8 pn[6];
 	u64 pn64;
-	u8 aad[2 * AES_BLOCK_SIZE];
-	u8 j_0[AES_BLOCK_SIZE];
+	struct ieee80211_crypto_bufs *bufs =
+		this_cpu_ptr(ieee80211_crypto_bufs);
+	u8 *aad = bufs->buf1;
+	u8 *j_0 = bufs->buf2;
+
+	BUILD_BUG_ON(sizeof(bufs->buf1) < 2 * AES_BLOCK_SIZE);
+	BUILD_BUG_ON(sizeof(bufs->buf2) < AES_BLOCK_SIZE);
 
 	if (info->control.hw_key &&
 	    !(info->control.hw_key->flags & IEEE80211_KEY_FLAG_GENERATE_IV) &&
@@ -760,8 +776,14 @@ ieee80211_crypto_gcmp_decrypt(struct ieee80211_rx_data *rx)
 		}
 
 		if (!(status->flag & RX_FLAG_DECRYPTED)) {
-			u8 aad[2 * AES_BLOCK_SIZE];
-			u8 j_0[AES_BLOCK_SIZE];
+			struct ieee80211_crypto_bufs *bufs =
+				this_cpu_ptr(ieee80211_crypto_bufs);
+			u8 *aad = bufs->buf1;
+			u8 *j_0 = bufs->buf2;
+
+			BUILD_BUG_ON(sizeof(bufs->buf1) < 2 * AES_BLOCK_SIZE);
+			BUILD_BUG_ON(sizeof(bufs->buf2) < AES_BLOCK_SIZE);
+
 			/* hardware didn't decrypt/verify MIC */
 			gcmp_special_blocks(skb, pn, j_0, aad);
 
@@ -931,8 +953,12 @@ ieee80211_crypto_aes_cmac_encrypt(struct ieee80211_tx_data *tx)
 	struct ieee80211_tx_info *info;
 	struct ieee80211_key *key = tx->key;
 	struct ieee80211_mmie *mmie;
-	u8 aad[20];
 	u64 pn64;
+	struct ieee80211_crypto_bufs *bufs =
+		this_cpu_ptr(ieee80211_crypto_bufs);
+	u8 *aad = bufs->buf1;
+
+	BUILD_BUG_ON(sizeof(bufs->buf1) < CMAC_AAD_LEN);
 
 	if (WARN_ON(skb_queue_len(&tx->skbs) != 1))
 		return TX_DROP;
@@ -975,8 +1001,12 @@ ieee80211_crypto_aes_cmac_256_encrypt(struct ieee80211_tx_data *tx)
 	struct ieee80211_tx_info *info;
 	struct ieee80211_key *key = tx->key;
 	struct ieee80211_mmie_16 *mmie;
-	u8 aad[20];
 	u64 pn64;
+	struct ieee80211_crypto_bufs *bufs =
+		this_cpu_ptr(ieee80211_crypto_bufs);
+	u8 *aad = bufs->buf1;
+
+	BUILD_BUG_ON(sizeof(bufs->buf1) < CMAC_AAD_LEN);
 
 	if (WARN_ON(skb_queue_len(&tx->skbs) != 1))
 		return TX_DROP;
@@ -1018,8 +1048,13 @@ ieee80211_crypto_aes_cmac_decrypt(struct ieee80211_rx_data *rx)
 	struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
 	struct ieee80211_key *key = rx->key;
 	struct ieee80211_mmie *mmie;
-	u8 aad[20], mic[8], ipn[6];
+	u8 mic[8], ipn[6];
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
+	struct ieee80211_crypto_bufs *bufs =
+		this_cpu_ptr(ieee80211_crypto_bufs);
+	u8 *aad = bufs->buf1;
+
+	BUILD_BUG_ON(sizeof(bufs->buf1) < CMAC_AAD_LEN);
 
 	if (!ieee80211_is_mgmt(hdr->frame_control))
 		return RX_CONTINUE;
@@ -1068,8 +1103,13 @@ ieee80211_crypto_aes_cmac_256_decrypt(struct ieee80211_rx_data *rx)
 	struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
 	struct ieee80211_key *key = rx->key;
 	struct ieee80211_mmie_16 *mmie;
-	u8 aad[20], mic[16], ipn[6];
+	u8 mic[16], ipn[6];
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
+	struct ieee80211_crypto_bufs *bufs =
+		this_cpu_ptr(ieee80211_crypto_bufs);
+	u8 *aad = bufs->buf1;
+
+	BUILD_BUG_ON(sizeof(bufs->buf1) < CMAC_AAD_LEN);
 
 	if (!ieee80211_is_mgmt(hdr->frame_control))
 		return RX_CONTINUE;
@@ -1119,9 +1159,15 @@ ieee80211_crypto_aes_gmac_encrypt(struct ieee80211_tx_data *tx)
 	struct ieee80211_key *key = tx->key;
 	struct ieee80211_mmie_16 *mmie;
 	struct ieee80211_hdr *hdr;
-	u8 aad[20];
 	u64 pn64;
 	u8 nonce[12];
+	struct ieee80211_crypto_bufs *bufs =
+		this_cpu_ptr(ieee80211_crypto_bufs);
+	u8 *aad = bufs->buf1;
+	u8 *zero = bufs->buf2;
+
+	BUILD_BUG_ON(sizeof(bufs->buf1) < GMAC_AAD_LEN);
+	BUILD_BUG_ON(sizeof(bufs->buf2) < GMAC_MIC_LEN);
 
 	if (WARN_ON(skb_queue_len(&tx->skbs) != 1))
 		return TX_DROP;
@@ -1154,7 +1200,8 @@ ieee80211_crypto_aes_gmac_encrypt(struct ieee80211_tx_data *tx)
 
 	/* MIC = AES-GMAC(IGTK, AAD || Management Frame Body || MMIE, 128) */
 	if (ieee80211_aes_gmac(key->u.aes_gmac.tfm, aad, nonce,
-			       skb->data + 24, skb->len - 24, mmie->mic) < 0)
+			       skb->data + 24, skb->len - 24, mmie->mic,
+			       zero) < 0)
 		return TX_DROP;
 
 	return TX_CONTINUE;
@@ -1167,8 +1214,15 @@ ieee80211_crypto_aes_gmac_decrypt(struct ieee80211_rx_data *rx)
 	struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
 	struct ieee80211_key *key = rx->key;
 	struct ieee80211_mmie_16 *mmie;
-	u8 aad[20], mic[16], ipn[6], nonce[12];
+	u8 mic[16], ipn[6], nonce[12];
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
+	struct ieee80211_crypto_bufs *bufs =
+		this_cpu_ptr(ieee80211_crypto_bufs);
+	u8 *aad = bufs->buf1;
+	u8 *zero = bufs->buf2;
+
+	BUILD_BUG_ON(sizeof(bufs->buf1) < GMAC_AAD_LEN);
+	BUILD_BUG_ON(sizeof(bufs->buf2) < GMAC_MIC_LEN);
 
 	if (!ieee80211_is_mgmt(hdr->frame_control))
 		return RX_CONTINUE;
@@ -1200,7 +1254,7 @@ ieee80211_crypto_aes_gmac_decrypt(struct ieee80211_rx_data *rx)
 
 		if (ieee80211_aes_gmac(key->u.aes_gmac.tfm, aad, nonce,
 				       skb->data + 24, skb->len - 24,
-				       mic) < 0 ||
+				       mic, zero) < 0 ||
 		    memcmp(mic, mmie->mic, sizeof(mmie->mic)) != 0) {
 			key->u.aes_gmac.icverrors++;
 			return RX_DROP_UNUSABLE;
-- 
2.8.1

^ permalink raw reply related

* [PATCH v2] mac80211: move struct aead_req off the stack
From: Johannes Berg @ 2016-10-17  8:01 UTC (permalink / raw)
  To: linux-wireless
  Cc: Ard Biesheuvel, Sergey Senozhatsky, netdev, herbert,
	Johannes Berg

From: Johannes Berg <johannes.berg@intel.com>

Some crypto implementations (such as the generic CCM wrapper in crypto/)
use scatterlists to map fields of private data in their struct aead_req.
This means these data structures cannot live in the vmalloc area, which
means that they cannot live on the stack (with CONFIG_VMAP_STACK.)

This currently occurs only with the generic software implementation, but
the private data and usage is implementation specific, so move the whole
data structures off the stack into heap by allocating every time we need
to use them.

This pattern already exists in the IPsec ESP driver, but in the future,
we may want/need to improve upon this, e.g. by using a slab cache.

(Based on Ard's patch, but that was missing error handling and GCM/GMAC)

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/aes_ccm.c  | 34 +++++++++++++++++++---------------
 net/mac80211/aes_ccm.h  |  6 +++---
 net/mac80211/aes_gcm.c  | 31 +++++++++++++++++--------------
 net/mac80211/aes_gcm.h  |  4 ++--
 net/mac80211/aes_gmac.c | 10 +++++-----
 net/mac80211/wpa.c      | 12 ++++--------
 6 files changed, 50 insertions(+), 47 deletions(-)

diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
index 7663c28ba353..5c9fe00be6cc 100644
--- a/net/mac80211/aes_ccm.c
+++ b/net/mac80211/aes_ccm.c
@@ -18,18 +18,16 @@
 #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)
+int 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 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 = aead_request_alloc(tfm, GFP_ATOMIC);
+	if (!aead_req)
+		return -ENOMEM;
 
 	sg_init_table(sg, 3);
 	sg_set_buf(&sg[0], &aad[2], be16_to_cpup((__be16 *)aad));
@@ -41,6 +39,9 @@ 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);
+	aead_request_free(aead_req);
+
+	return 0;
 }
 
 int ieee80211_aes_ccm_decrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
@@ -48,15 +49,15 @@ 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 = aead_request_alloc(tfm, GFP_ATOMIC);
+	if (!aead_req)
+		return -ENOMEM;
 
 	sg_init_table(sg, 3);
 	sg_set_buf(&sg[0], &aad[2], be16_to_cpup((__be16 *)aad));
@@ -67,7 +68,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);
+	aead_request_free(aead_req);
+
+	return err;
 }
 
 struct crypto_aead *ieee80211_aes_key_setup_encrypt(const u8 key[],
diff --git a/net/mac80211/aes_ccm.h b/net/mac80211/aes_ccm.h
index 6a73d1e4d186..03e21b0995e3 100644
--- a/net/mac80211/aes_ccm.h
+++ b/net/mac80211/aes_ccm.h
@@ -15,9 +15,9 @@
 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_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);
diff --git a/net/mac80211/aes_gcm.c b/net/mac80211/aes_gcm.c
index 3afe361fd27c..aa8eb2718bc1 100644
--- a/net/mac80211/aes_gcm.c
+++ b/net/mac80211/aes_gcm.c
@@ -15,17 +15,15 @@
 #include "key.h"
 #include "aes_gcm.h"
 
-void ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
-			       u8 *data, size_t data_len, u8 *mic)
+int ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
+			      u8 *data, size_t data_len, u8 *mic)
 {
 	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 = aead_request_alloc(tfm, GFP_ATOMIC);
+	if (!aead_req)
+		return -ENOMEM;
 
 	sg_init_table(sg, 3);
 	sg_set_buf(&sg[0], &aad[2], be16_to_cpup((__be16 *)aad));
@@ -37,21 +35,23 @@ void ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
 	aead_request_set_ad(aead_req, sg[0].length);
 
 	crypto_aead_encrypt(aead_req);
+	aead_request_free(aead_req);
+	return 0;
 }
 
 int ieee80211_aes_gcm_decrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
 			      u8 *data, size_t data_len, u8 *mic)
 {
 	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 = aead_request_alloc(tfm, GFP_ATOMIC);
+	if (!aead_req)
+		return -ENOMEM;
 
 	sg_init_table(sg, 3);
 	sg_set_buf(&sg[0], &aad[2], be16_to_cpup((__be16 *)aad));
@@ -63,7 +63,10 @@ int ieee80211_aes_gcm_decrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
 			       data_len + IEEE80211_GCMP_MIC_LEN, j_0);
 	aead_request_set_ad(aead_req, sg[0].length);
 
-	return crypto_aead_decrypt(aead_req);
+	err = crypto_aead_decrypt(aead_req);
+	aead_request_free(aead_req);
+
+	return err;
 }
 
 struct crypto_aead *ieee80211_aes_gcm_key_setup_encrypt(const u8 key[],
diff --git a/net/mac80211/aes_gcm.h b/net/mac80211/aes_gcm.h
index 1347fda6b76a..b36503cd7716 100644
--- a/net/mac80211/aes_gcm.h
+++ b/net/mac80211/aes_gcm.h
@@ -11,8 +11,8 @@
 
 #include <linux/crypto.h>
 
-void ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
-			       u8 *data, size_t data_len, u8 *mic);
+int ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
+			      u8 *data, size_t data_len, u8 *mic);
 int ieee80211_aes_gcm_decrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
 			      u8 *data, size_t data_len, u8 *mic);
 struct crypto_aead *ieee80211_aes_gcm_key_setup_encrypt(const u8 key[],
diff --git a/net/mac80211/aes_gmac.c b/net/mac80211/aes_gmac.c
index 3ddd927aaf30..15cfcebf0884 100644
--- a/net/mac80211/aes_gmac.c
+++ b/net/mac80211/aes_gmac.c
@@ -25,16 +25,15 @@ 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[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 zero[GMAC_MIC_LEN], iv[AES_BLOCK_SIZE];
+	struct aead_request *aead_req;
 
 	if (data_len < GMAC_MIC_LEN)
 		return -EINVAL;
 
-	memset(aead_req, 0, sizeof(aead_req_data));
+	aead_req = aead_request_alloc(tfm, GFP_ATOMIC);
+	if (!aead_req)
+		return -ENOMEM;
 
 	memset(zero, 0, GMAC_MIC_LEN);
 	sg_init_table(sg, 4);
@@ -52,6 +51,7 @@ int ieee80211_aes_gmac(struct crypto_aead *tfm, const u8 *aad, u8 *nonce,
 	aead_request_set_ad(aead_req, AAD_LEN + data_len);
 
 	crypto_aead_encrypt(aead_req);
+	aead_request_free(aead_req);
 
 	return 0;
 }
diff --git a/net/mac80211/wpa.c b/net/mac80211/wpa.c
index b48c1e13e281..2e366438f8ef 100644
--- a/net/mac80211/wpa.c
+++ b/net/mac80211/wpa.c
@@ -461,10 +461,8 @@ static int ccmp_encrypt_skb(struct ieee80211_tx_data *tx, struct sk_buff *skb,
 
 	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,
-				  skb_put(skb, mic_len), mic_len);
-
-	return 0;
+	return ieee80211_aes_ccm_encrypt(key->u.ccmp.tfm, b_0, aad, pos, len,
+					 skb_put(skb, mic_len), mic_len);
 }
 
 
@@ -696,10 +694,8 @@ static int gcmp_encrypt_skb(struct ieee80211_tx_data *tx, struct sk_buff *skb)
 
 	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,
-				  skb_put(skb, IEEE80211_GCMP_MIC_LEN));
-
-	return 0;
+	return ieee80211_aes_gcm_encrypt(key->u.gcmp.tfm, j_0, aad, pos, len,
+					 skb_put(skb, IEEE80211_GCMP_MIC_LEN));
 }
 
 ieee80211_tx_result
-- 
2.8.1

^ permalink raw reply related

* Re: linux-4.9-rc1/drivers/net/wireless/intel/iwlwifi/mvm/debugfs.c:1561: poor error checking ?
From: Luca Coelho @ 2016-10-17  7:58 UTC (permalink / raw)
  To: David Binderman, johannes.berg@intel.com,
	emmanuel.grumbach@intel.com, linuxwifi@intel.com,
	kvalo@codeaurora.org, linux-wireless@vger.kernel.org,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <VI1PR08MB1022786893DC435E1A71F3069CD00@VI1PR08MB1022.eurprd08.prod.outlook.com>

Hi David,
On Mon, 2016-10-17 at 07:40 +0000, David Binderman wrote:
> Hello there,
> 
> linux-4.9-rc1/drivers/net/wireless/intel/iwlwifi/mvm/debugfs.c:1561]: (style) Checking if unsigned variable 'len' is less than zero.
> 
> Source code is
> 
>     len = min((size_t)le32_to_cpu(rsp->len) << 2,
>           iwl_rx_packet_payload_len(hcmd.resp_pkt) - sizeof(*rsp));
>     len = min(len - delta, count);
>     if (len < 0) {
>         ret = -EFAULT;
>         goto out;
>     }
> 
> Suggest improve error checking.

Thanks for reporting! A fix for this is already queued in our internal
tree and will be sent upstream soon.

--
Cheers,
Luca.

^ permalink raw reply

* Re: [PATCH] crypto: ccm - avoid scatterlist for MAC encryption
From: Johannes Berg @ 2016-10-17  7:57 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Andy Lutomirski, Sergey Senozhatsky,
	<netdev@vger.kernel.org>, Herbert Xu, David S. Miller,
	<linux-wireless@vger.kernel.org>,
	linux-kernel@vger.kernel.org, Jouni Malinen
In-Reply-To: <CAKv+Gu9EPAF+Oihz0tXCqWURGom-wgSPCMp0dEHHnxqw-pYYJw@mail.gmail.com>

On Mon, 2016-10-17 at 08:50 +0100, Ard Biesheuvel wrote:

> I just realised that patch should probably use
> aead_request_alloc/aead_request_free [and drop the memset]. That also
> fixes the latent bug where the alignment of the req ctx is not take
> into account.

Good point, I'll fix that up.

> > 
> > > 
> > > Also, regarding your __percpu patch: those are located in the
> > > vmalloc
> > > area as well, at least on arm64, and likely other architectures
> > > too.
> > 
> > Crap. Any other bright ideas?
> > 
> 
> kmem_cache_create() and kmem_cache_alloc()

for the aad/b0/j0/etc? Hmm. Yeah I guess we should do those dynamically
as well then.

johannes

^ permalink raw reply

* Re: [PATCH] crypto: ccm - avoid scatterlist for MAC encryption
From: Ard Biesheuvel @ 2016-10-17  7:50 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Andy Lutomirski, Sergey Senozhatsky,
	<netdev@vger.kernel.org>, Herbert Xu, David S. Miller,
	<linux-wireless@vger.kernel.org>,
	linux-kernel@vger.kernel.org, Jouni Malinen
In-Reply-To: <1476690478.19992.4.camel@sipsolutions.net>

On 17 October 2016 at 08:47, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Mon, 2016-10-17 at 08:37 +0100, Ard Biesheuvel wrote:
>>
>> Could we get a statement first whether it is supported to allocate
>> aead_req (and other crypto req structures) on the stack?
>
> Well, we haven't heard from Herbert :)
>
>> If not, then
>> we have our work cut out for us. But if it is, I'd rather we didn't
>> apply the kzalloc/kfree patch, since it is just a workaround for the
>> broken generic CCM driver, for which a fix is already available.
>
> Yeah but I can't apply it. I just fixed up your kzalloc patch to also
> handle GCM and GMAC, and to have error checking. Will send it in a
> minute.
>

I just realised that patch should probably use
aead_request_alloc/aead_request_free [and drop the memset]. That also
fixes the latent bug where the alignment of the req ctx is not take
into account.

>> Also, regarding your __percpu patch: those are located in the vmalloc
>> area as well, at least on arm64, and likely other architectures too.
>
> Crap. Any other bright ideas?
>

kmem_cache_create() and kmem_cache_alloc()

^ permalink raw reply

* [PATCH] mac80211: move struct aead_req off the stack
From: Johannes Berg @ 2016-10-17  7:49 UTC (permalink / raw)
  To: linux-wireless
  Cc: Ard Biesheuvel, Sergey Senozhatsky, netdev, herbert,
	Johannes Berg

From: Johannes Berg <johannes.berg@intel.com>

Some crypto implementations (such as the generic CCM wrapper in crypto/)
use scatterlists to map fields of private data in their struct aead_req.
This means these data structures cannot live in the vmalloc area, which
means that they cannot live on the stack (with CONFIG_VMAP_STACK.)

This currently occurs only with the generic software implementation, but
the private data and usage is implementation specific, so move the whole
data structures off the stack into heap by allocating every time we need
to use them.

This pattern already exists in the IPsec ESP driver, but in the future,
we may want/need to improve upon this, e.g. by using a slab cache.

(Based on Ard's patch, but that was missing error handling and GCM/GMAC)

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/aes_ccm.c  | 34 +++++++++++++++++++---------------
 net/mac80211/aes_ccm.h  |  6 +++---
 net/mac80211/aes_gcm.c  | 33 +++++++++++++++++++--------------
 net/mac80211/aes_gcm.h  |  4 ++--
 net/mac80211/aes_gmac.c | 12 +++++++-----
 net/mac80211/wpa.c      | 12 ++++--------
 6 files changed, 54 insertions(+), 47 deletions(-)

diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
index 7663c28ba353..cd2f10744238 100644
--- a/net/mac80211/aes_ccm.c
+++ b/net/mac80211/aes_ccm.c
@@ -18,18 +18,15 @@
 #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)
+int 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 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(&sg[0], &aad[2], be16_to_cpup((__be16 *)aad));
@@ -41,6 +38,8 @@ 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);
+	return 0;
 }
 
 int ieee80211_aes_ccm_decrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
@@ -48,15 +47,16 @@ 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);
+	if (!aead_req)
+		return -ENOMEM;
 
 	sg_init_table(sg, 3);
 	sg_set_buf(&sg[0], &aad[2], be16_to_cpup((__be16 *)aad));
@@ -67,7 +67,11 @@ 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[],
diff --git a/net/mac80211/aes_ccm.h b/net/mac80211/aes_ccm.h
index 6a73d1e4d186..03e21b0995e3 100644
--- a/net/mac80211/aes_ccm.h
+++ b/net/mac80211/aes_ccm.h
@@ -15,9 +15,9 @@
 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_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);
diff --git a/net/mac80211/aes_gcm.c b/net/mac80211/aes_gcm.c
index 3afe361fd27c..2bd715df512f 100644
--- a/net/mac80211/aes_gcm.c
+++ b/net/mac80211/aes_gcm.c
@@ -15,17 +15,16 @@
 #include "key.h"
 #include "aes_gcm.h"
 
-void ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
-			       u8 *data, size_t data_len, u8 *mic)
+int ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
+			      u8 *data, size_t data_len, u8 *mic)
 {
 	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);
+	if (!aead_req)
+		return -ENOMEM;
 
 	sg_init_table(sg, 3);
 	sg_set_buf(&sg[0], &aad[2], be16_to_cpup((__be16 *)aad));
@@ -37,21 +36,23 @@ void ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
 	aead_request_set_ad(aead_req, sg[0].length);
 
 	crypto_aead_encrypt(aead_req);
+	return 0;
 }
 
 int ieee80211_aes_gcm_decrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
 			      u8 *data, size_t data_len, u8 *mic)
 {
 	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);
+	if (!aead_req)
+		return -ENOMEM;
 
 	sg_init_table(sg, 3);
 	sg_set_buf(&sg[0], &aad[2], be16_to_cpup((__be16 *)aad));
@@ -63,7 +64,11 @@ int ieee80211_aes_gcm_decrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
 			       data_len + IEEE80211_GCMP_MIC_LEN, j_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_gcm_key_setup_encrypt(const u8 key[],
diff --git a/net/mac80211/aes_gcm.h b/net/mac80211/aes_gcm.h
index 1347fda6b76a..b36503cd7716 100644
--- a/net/mac80211/aes_gcm.h
+++ b/net/mac80211/aes_gcm.h
@@ -11,8 +11,8 @@
 
 #include <linux/crypto.h>
 
-void ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
-			       u8 *data, size_t data_len, u8 *mic);
+int ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
+			      u8 *data, size_t data_len, u8 *mic);
 int ieee80211_aes_gcm_decrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
 			      u8 *data, size_t data_len, u8 *mic);
 struct crypto_aead *ieee80211_aes_gcm_key_setup_encrypt(const u8 key[],
diff --git a/net/mac80211/aes_gmac.c b/net/mac80211/aes_gmac.c
index 43ba5ecaaaa3..bb7f7449d3e9 100644
--- a/net/mac80211/aes_gmac.c
+++ b/net/mac80211/aes_gmac.c
@@ -24,16 +24,16 @@ int ieee80211_aes_gmac(struct crypto_aead *tfm, const u8 *aad, u8 *nonce,
 		       const u8 *data, size_t data_len, u8 *mic, u8 *zero)
 {
 	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];
+	struct aead_request *aead_req;
 
 	if (data_len < GMAC_MIC_LEN)
 		return -EINVAL;
 
-	memset(aead_req, 0, sizeof(aead_req_data));
+	aead_req = kzalloc(sizeof(struct aead_request) +
+			   crypto_aead_reqsize(tfm), GFP_ATOMIC);
+	if (!aead_req)
+		return -ENOMEM;
 
 	memset(zero, 0, GMAC_MIC_LEN);
 	sg_init_table(sg, 4);
@@ -52,6 +52,8 @@ int ieee80211_aes_gmac(struct crypto_aead *tfm, const u8 *aad, u8 *nonce,
 
 	crypto_aead_encrypt(aead_req);
 
+	kfree(aead_req);
+
 	return 0;
 }
 
diff --git a/net/mac80211/wpa.c b/net/mac80211/wpa.c
index c02634c4210c..3a83a6763664 100644
--- a/net/mac80211/wpa.c
+++ b/net/mac80211/wpa.c
@@ -466,10 +466,8 @@ static int ccmp_encrypt_skb(struct ieee80211_tx_data *tx, struct sk_buff *skb,
 
 	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,
-				  skb_put(skb, mic_len), mic_len);
-
-	return 0;
+	return ieee80211_aes_ccm_encrypt(key->u.ccmp.tfm, b_0, aad, pos, len,
+					 skb_put(skb, mic_len), mic_len);
 }
 
 
@@ -712,10 +710,8 @@ static int gcmp_encrypt_skb(struct ieee80211_tx_data *tx, struct sk_buff *skb)
 
 	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,
-				  skb_put(skb, IEEE80211_GCMP_MIC_LEN));
-
-	return 0;
+	return ieee80211_aes_gcm_encrypt(key->u.gcmp.tfm, j_0, aad, pos, len,
+					 skb_put(skb, IEEE80211_GCMP_MIC_LEN));
 }
 
 ieee80211_tx_result
-- 
2.8.1

^ permalink raw reply related

* Re: [PATCH] crypto: ccm - avoid scatterlist for MAC encryption
From: Johannes Berg @ 2016-10-17  7:47 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Andy Lutomirski, Sergey Senozhatsky,
	<netdev@vger.kernel.org>, Herbert Xu, David S. Miller,
	<linux-wireless@vger.kernel.org>,
	linux-kernel@vger.kernel.org, Jouni Malinen
In-Reply-To: <CAKv+Gu_P-6sHW3t7tp1N+3v9ZGqWcQrzMZkASY5g=QR76KN4wg@mail.gmail.com>

On Mon, 2016-10-17 at 08:37 +0100, Ard Biesheuvel wrote:
> 
> Could we get a statement first whether it is supported to allocate
> aead_req (and other crypto req structures) on the stack? 

Well, we haven't heard from Herbert :)

> If not, then
> we have our work cut out for us. But if it is, I'd rather we didn't
> apply the kzalloc/kfree patch, since it is just a workaround for the
> broken generic CCM driver, for which a fix is already available.

Yeah but I can't apply it. I just fixed up your kzalloc patch to also
handle GCM and GMAC, and to have error checking. Will send it in a
minute.

> Also, regarding your __percpu patch: those are located in the vmalloc
> area as well, at least on arm64, and likely other architectures too.

Crap. Any other bright ideas?

johannes

^ permalink raw reply

* Re: [PATCH] crypto: ccm - avoid scatterlist for MAC encryption
From: Ard Biesheuvel @ 2016-10-17  7:37 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Andy Lutomirski, Sergey Senozhatsky,
	<netdev@vger.kernel.org>, Herbert Xu, David S. Miller,
	<linux-wireless@vger.kernel.org>,
	linux-kernel@vger.kernel.org, Jouni Malinen
In-Reply-To: <1476689310.19992.1.camel@sipsolutions.net>

On 17 October 2016 at 08:28, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Sat, 2016-10-15 at 18:16 +0100, Ard Biesheuvel wrote:
>> The CCM code goes out of its way to perform the CTR encryption of the
>> MAC using the subordinate CTR driver. To this end, it tweaks the
>> input and output scatterlists so the aead_req 'odata' and/or
>> 'auth_tag' fields [which may live on the stack] are prepended to the
>> CTR payload. This involves calling sg_set_buf() on addresses which
>> are not direct mapped, which is not supported.
>
>> Since the calculation of the MAC keystream involves a single call
>> into the cipher, to which we have a handle already given that the
>> CBC-MAC calculation uses it as well, just calculate the MAC keystream
>> directly, and record it in the aead_req private context so we can
>> apply it to the MAC in cypto_ccm_auth_mac(). This greatly simplifies
>> the scatterlist manipulation, and no longer requires scatterlists to
>> refer to buffers that may live on the stack.
>
> No objection from me, Herbert?
>
> I'm getting a bit nervous though - I'd rather have any fix first so
> people get things working again - so maybe I'll apply your other patch
> and mine first, and then we can replace yours by this later.
>

Could we get a statement first whether it is supported to allocate
aead_req (and other crypto req structures) on the stack? If not, then
we have our work cut out for us. But if it is, I'd rather we didn't
apply the kzalloc/kfree patch, since it is just a workaround for the
broken generic CCM driver, for which a fix is already available.

Also, regarding your __percpu patch: those are located in the vmalloc
area as well, at least on arm64, and likely other architectures too.

^ permalink raw reply

* Re: [PATCH] crypto: ccm - avoid scatterlist for MAC encryption
From: Johannes Berg @ 2016-10-17  7:28 UTC (permalink / raw)
  To: Ard Biesheuvel, luto, sergey.senozhatsky.work, netdev, herbert,
	davem, linux-wireless, linux-kernel, j
In-Reply-To: <1476551776-8099-1-git-send-email-ard.biesheuvel@linaro.org>

On Sat, 2016-10-15 at 18:16 +0100, Ard Biesheuvel wrote:
> The CCM code goes out of its way to perform the CTR encryption of the
> MAC using the subordinate CTR driver. To this end, it tweaks the
> input and output scatterlists so the aead_req 'odata' and/or
> 'auth_tag' fields [which may live on the stack] are prepended to the
> CTR payload. This involves calling sg_set_buf() on addresses which
> are not direct mapped, which is not supported.

> Since the calculation of the MAC keystream involves a single call
> into the cipher, to which we have a handle already given that the
> CBC-MAC calculation uses it as well, just calculate the MAC keystream
> directly, and record it in the aead_req private context so we can
> apply it to the MAC in cypto_ccm_auth_mac(). This greatly simplifies
> the scatterlist manipulation, and no longer requires scatterlists to
> refer to buffers that may live on the stack.

No objection from me, Herbert?

I'm getting a bit nervous though - I'd rather have any fix first so
people get things working again - so maybe I'll apply your other patch
and mine first, and then we can replace yours by this later.

johannes

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox