netdev - Re: [PATCH v4] mac80211: move extra crypto data off the stack

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 17 Oct 2016 10:17:52 +0100
From:   Ard Biesheuvel <ard.biesheuvel@...aro.org>
To:     Johannes Berg <johannes@...solutions.net>
Cc:     "<linux-wireless@...r.kernel.org>" <linux-wireless@...r.kernel.org>,
        Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        "<netdev@...r.kernel.org>" <netdev@...r.kernel.org>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Johannes Berg <johannes.berg@...el.com>
Subject: Re: [PATCH v4] mac80211: move extra crypto data off the stack

On 17 October 2016 at 10:14, Ard Biesheuvel <ard.biesheuvel@...aro.org> wrote:
> On 17 October 2016 at 09:33, Johannes Berg <johannes@...solutions.net> wrote:
>> From: Johannes Berg <johannes.berg@...el.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@...il.com>
>> Signed-off-by: Johannes Berg <johannes.berg@...el.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 ... :-(