[v2] mac80211: LLVMLinux: Remove VLAIS usage from mac80211

Message ID 1394241960-1764-1-git-send-email-behanw@converseincode.com
State New
Headers show

Commit Message

Behan Webster March 8, 2014, 1:26 a.m.
From: Jan-Simon Möller <dl9pf@gmx.de>

Replaced the use of a Variable Length Array In Struct (VLAIS) with a C99
compliant equivalent. This is the original VLAIS struct.

struct {
	struct aead_request     req;
	u8                      priv[crypto_aead_reqsize(tfm)];
} aead_req;

This patch instead allocates the appropriate amount of memory using an char
array.

The new code can be compiled with both gcc and clang.

Signed-off-by: Jan-Simon Möller <dl9pf@gmx.de>
Signed-off-by: Behan Webster <behanw@converseincode.com>
Signed-off-by: Vinícius Tinti <viniciustinti@gmail.com>
Signed-off-by: Mark Charlebois <charlebm@gmail.com>
---
 net/mac80211/aes_ccm.c | 40 ++++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

Comments

Joe Perches March 8, 2014, 1:56 a.m. | #1
On Fri, 2014-03-07 at 17:26 -0800, behanw@converseincode.com wrote:
> From: Jan-Simon Möller <dl9pf@gmx.de>
> 
> Replaced the use of a Variable Length Array In Struct (VLAIS) with a C99
> compliant equivalent. This is the original VLAIS struct.
[]
> diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
[]
> @@ -23,12 +23,14 @@ void ieee80211_aes_ccm_encrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
>  			       u8 *data, size_t data_len, u8 *mic)
>  {
>  	struct scatterlist assoc, pt, ct[2];
> -	struct {
> -		struct aead_request	req;
> -		u8			priv[crypto_aead_reqsize(tfm)];
> -	} aead_req;
>  
> -	memset(&aead_req, 0, sizeof(aead_req));
> +	char aead_req_data[sizeof(struct aead_request) +
> +				crypto_aead_reqsize(tfm) +
> +				CRYPTO_MINALIGN] CRYPTO_MINALIGN_ATTR;

Can this be a too large amount of stack?

Is crypto_aead_reqsize() limited to < ~1k?

Perhaps it'd be better to use kzalloc for this
or another reserved pool


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Behan Webster March 8, 2014, 2:15 a.m. | #2
On 03/07/14 17:56, Joe Perches wrote:
> On Fri, 2014-03-07 at 17:26 -0800, behanw@converseincode.com wrote:
>> From: Jan-Simon Möller <dl9pf@gmx.de>
>>
>> Replaced the use of a Variable Length Array In Struct (VLAIS) with a C99
>> compliant equivalent. This is the original VLAIS struct.
> []
>> diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
> []
>> @@ -23,12 +23,14 @@ void ieee80211_aes_ccm_encrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
>>   			       u8 *data, size_t data_len, u8 *mic)
>>   {
>>   	struct scatterlist assoc, pt, ct[2];
>> -	struct {
>> -		struct aead_request	req;
>> -		u8			priv[crypto_aead_reqsize(tfm)];
>> -	} aead_req;
>>   
>> -	memset(&aead_req, 0, sizeof(aead_req));
>> +	char aead_req_data[sizeof(struct aead_request) +
>> +				crypto_aead_reqsize(tfm) +
>> +				CRYPTO_MINALIGN] CRYPTO_MINALIGN_ATTR;
> Can this be a too large amount of stack?
>
> Is crypto_aead_reqsize() limited to < ~1k?
>
> Perhaps it'd be better to use kzalloc for this
> or another reserved pool
No more stack being used than with the the original code. The stack 
memory use is identical.

Behan
Joe Perches March 8, 2014, 2:27 a.m. | #3
On Fri, 2014-03-07 at 18:15 -0800, Behan Webster wrote:
> On 03/07/14 17:56, Joe Perches wrote:
> > On Fri, 2014-03-07 at 17:26 -0800, behanw@converseincode.com wrote:
> >> From: Jan-Simon Möller <dl9pf@gmx.de>
> >>
> >> Replaced the use of a Variable Length Array In Struct (VLAIS) with a C99
> >> compliant equivalent. This is the original VLAIS struct.
> > []
> >> diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
> > []
> >> @@ -23,12 +23,14 @@ void ieee80211_aes_ccm_encrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
> >>   			       u8 *data, size_t data_len, u8 *mic)
> >>   {
> >>   	struct scatterlist assoc, pt, ct[2];
> >> -	struct {
> >> -		struct aead_request	req;
> >> -		u8			priv[crypto_aead_reqsize(tfm)];
> >> -	} aead_req;
> >>   
> >> -	memset(&aead_req, 0, sizeof(aead_req));
> >> +	char aead_req_data[sizeof(struct aead_request) +
> >> +				crypto_aead_reqsize(tfm) +
> >> +				CRYPTO_MINALIGN] CRYPTO_MINALIGN_ATTR;
> > Can this be a too large amount of stack?
> >
> > Is crypto_aead_reqsize() limited to < ~1k?
> >
> > Perhaps it'd be better to use kzalloc for this
> > or another reserved pool
> No more stack being used than with the the original code. The stack 
> memory use is identical.

I do understand that, but that's not my question.

I appreciate you're getting this to compile for llvm.

Any idea of the max value of crypto_aead_reqsize?

include/linux/crypto.h:static inline unsigned int crypto_aead_reqsize(struct crypto_aead *tfm)
include/linux/crypto.h-{
include/linux/crypto.h- return crypto_aead_crt(tfm)->reqsize;
include/linux/crypto.h-}

$ grep-2.5.4 -rP --include=*.[ch] "(?:->|\.)\s*\breqsize\s*=[^=][^;]+;" *
arch/x86/crypto/aesni-intel_glue.c:	tfm->crt_aead.reqsize = sizeof(struct aead_request)
		+ crypto_aead_reqsize(&cryptd_tfm->base);
crypto/chainiv.c:	tfm->crt_ablkcipher.reqsize = sizeof(struct ablkcipher_request);
crypto/authenc.c:	tfm->crt_aead.reqsize = sizeof(struct authenc_request_ctx) +
				ctx->reqoff +
				max_t(unsigned int,
				crypto_ahash_reqsize(auth) +
				sizeof(struct ahash_request),
				sizeof(struct skcipher_givcrypt_request) +
				crypto_ablkcipher_reqsize(enc));
crypto/authencesn.c:	tfm->crt_aead.reqsize = sizeof(struct authenc_esn_request_ctx) +
				ctx->reqoff +
				max_t(unsigned int,
				crypto_ahash_reqsize(auth) +
				sizeof(struct ahash_request),
				sizeof(struct skcipher_givcrypt_request) +
				crypto_ablkcipher_reqsize(enc));
crypto/shash.c:	crt->reqsize = sizeof(struct shash_desc) + crypto_shash_descsize(shash);
crypto/cryptd.c:	tfm->crt_ablkcipher.reqsize =
		sizeof(struct cryptd_blkcipher_request_ctx);
crypto/cryptd.c:	tfm->crt_aead.reqsize = sizeof(struct cryptd_aead_request_ctx);
crypto/seqiv.c:	tfm->crt_ablkcipher.reqsize = sizeof(struct ablkcipher_request);
crypto/seqiv.c:	tfm->crt_aead.reqsize = sizeof(struct aead_request);
crypto/ablk_helper.c:	tfm->crt_ablkcipher.reqsize = sizeof(struct ablkcipher_request) +
		crypto_ablkcipher_reqsize(&cryptd_tfm->base);
crypto/ctr.c:	tfm->crt_ablkcipher.reqsize = align +
		sizeof(struct crypto_rfc3686_req_ctx) +
		crypto_ablkcipher_reqsize(cipher);
crypto/eseqiv.c:	tfm->crt_ablkcipher.reqsize = reqsize +
				      sizeof(struct ablkcipher_request);
crypto/ccm.c:	tfm->crt_aead.reqsize = align +
				sizeof(struct crypto_ccm_req_priv_ctx) +
				crypto_ablkcipher_reqsize(ctr);
crypto/ccm.c:	tfm->crt_aead.reqsize = sizeof(struct aead_request) +
				ALIGN(crypto_aead_reqsize(aead),
				      crypto_tfm_ctx_alignment()) +
				align + 16;
crypto/gcm.c:	tfm->crt_aead.reqsize = align +
		offsetof(struct crypto_gcm_req_priv_ctx, u) +
		max(sizeof(struct ablkcipher_request) +
		    crypto_ablkcipher_reqsize(ctr),
		    sizeof(struct ahash_request) +
		    crypto_ahash_reqsize(ghash));
crypto/gcm.c:	tfm->crt_aead.reqsize = sizeof(struct aead_request) +
				ALIGN(crypto_aead_reqsize(aead),
				      crypto_tfm_ctx_alignment()) +
				align + 16;
crypto/gcm.c:	tfm->crt_aead.reqsize = sizeof(struct crypto_rfc4543_req_ctx) +
				ALIGN(crypto_aead_reqsize(aead),
				      crypto_tfm_ctx_alignment()) +
				align + 16;
crypto/pcrypt.c:	tfm->crt_aead.reqsize = sizeof(struct pcrypt_request)
		+ sizeof(struct aead_givcrypt_request)
		+ crypto_aead_reqsize(cipher);
drivers/staging/sep/sep_crypto.c:	tfm->crt_ablkcipher.reqsize = sizeof(struct this_task_ctx);
drivers/crypto/n2_core.c:	tfm->crt_ablkcipher.reqsize = sizeof(struct n2_request_context);
drivers/crypto/mxs-dcp.c:	tfm->crt_ablkcipher.reqsize = sizeof(struct dcp_aes_req_ctx);
drivers/crypto/sahara.c:	tfm->crt_ablkcipher.reqsize = sizeof(struct sahara_aes_reqctx);
drivers/crypto/ccp/ccp-crypto-aes.c:	tfm->crt_ablkcipher.reqsize = sizeof(struct ccp_aes_req_ctx);
drivers/crypto/ccp/ccp-crypto-aes.c:	tfm->crt_ablkcipher.reqsize = sizeof(struct ccp_aes_req_ctx);
drivers/crypto/ccp/ccp-crypto-aes-xts.c:	tfm->crt_ablkcipher.reqsize = sizeof(struct ccp_aes_req_ctx) +
				      fallback_tfm->base.crt_ablkcipher.reqsize;
drivers/crypto/s5p-sss.c:	tfm->crt_ablkcipher.reqsize = sizeof(struct s5p_aes_reqctx);
drivers/crypto/atmel-aes.c:	tfm->crt_ablkcipher.reqsize = sizeof(struct atmel_aes_reqctx);
drivers/crypto/ixp4xx_crypto.c:	tfm->crt_ablkcipher.reqsize = sizeof(struct ablk_ctx);
drivers/crypto/ixp4xx_crypto.c:	tfm->crt_aead.reqsize = sizeof(struct aead_ctx);
drivers/crypto/omap-des.c:	tfm->crt_ablkcipher.reqsize = sizeof(struct omap_des_reqctx);
drivers/crypto/mv_cesa.c:	tfm->crt_ablkcipher.reqsize = sizeof(struct mv_req_ctx);
drivers/crypto/omap-aes.c:	tfm->crt_ablkcipher.reqsize = sizeof(struct omap_aes_reqctx);
drivers/crypto/hifn_795x.c:	tfm->crt_ablkcipher.reqsize = sizeof(struct hifn_request_context);
drivers/crypto/amcc/crypto4xx_core.c:		tfm->crt_ablkcipher.reqsize = sizeof(struct crypto4xx_ctx);
drivers/crypto/atmel-tdes.c:	tfm->crt_ablkcipher.reqsize = sizeof(struct atmel_tdes_reqctx);
drivers/crypto/picoxcell_crypto.c:	tfm->crt_aead.reqsize = sizeof(struct spacc_req);
drivers/crypto/picoxcell_crypto.c:	tfm->crt_ablkcipher.reqsize = sizeof(struct spacc_req);
include/crypto/internal/hash.h:	tfm->reqsize = reqsize;


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stanislaw Gruszka March 8, 2014, 2:53 p.m. | #4
On Fri, Mar 07, 2014 at 06:15:43PM -0800, Behan Webster wrote:
> On 03/07/14 17:56, Joe Perches wrote:
> >On Fri, 2014-03-07 at 17:26 -0800, behanw@converseincode.com wrote:
> >>From: Jan-Simon Möller <dl9pf@gmx.de>
> >>
> >>Replaced the use of a Variable Length Array In Struct (VLAIS) with a C99
> >>compliant equivalent. This is the original VLAIS struct.
> >[]
> >>diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
> >[]
> >>@@ -23,12 +23,14 @@ void ieee80211_aes_ccm_encrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
> >>  			       u8 *data, size_t data_len, u8 *mic)
> >>  {
> >>  	struct scatterlist assoc, pt, ct[2];
> >>-	struct {
> >>-		struct aead_request	req;
> >>-		u8			priv[crypto_aead_reqsize(tfm)];
> >>-	} aead_req;
> >>-	memset(&aead_req, 0, sizeof(aead_req));
> >>+	char aead_req_data[sizeof(struct aead_request) +
> >>+				crypto_aead_reqsize(tfm) +
> >>+				CRYPTO_MINALIGN] CRYPTO_MINALIGN_ATTR;
> >Can this be a too large amount of stack?
> >
> >Is crypto_aead_reqsize() limited to < ~1k?
> >
> >Perhaps it'd be better to use kzalloc for this
> >or another reserved pool
> No more stack being used than with the the original code. The stack
> memory use is identical.

Could you explain that? It looks like aead_req_data can be bigger than
original struct aead_req up to CRYPTO_MINALIGN bytes. IOW adding
CRYPTO_MINALIGN to size seems unneeded as aead_request->__ctx alignment
requirement should by assured by proper sizeof(struct aead_request).

Besides, why not add VLAIS feature to llvm instead of fixing all
programs that use it?

Stanislaw
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Behan Webster March 8, 2014, 8:17 p.m. | #5
On 03/07/14 18:27, Joe Perches wrote:
> On Fri, 2014-03-07 at 18:15 -0800, Behan Webster wrote:
>> On 03/07/14 17:56, Joe Perches wrote:
>>> On Fri, 2014-03-07 at 17:26 -0800, behanw@converseincode.com wrote:
>>>> From: Jan-Simon Möller <dl9pf@gmx.de>
>>>>
>>>> Replaced the use of a Variable Length Array In Struct (VLAIS) with a C99
>>>> compliant equivalent. This is the original VLAIS struct.
>>> []
>>>> diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
>>> []
>>>> @@ -23,12 +23,14 @@ void ieee80211_aes_ccm_encrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
>>>>    			       u8 *data, size_t data_len, u8 *mic)
>>>>    {
>>>>    	struct scatterlist assoc, pt, ct[2];
>>>> -	struct {
>>>> -		struct aead_request	req;
>>>> -		u8			priv[crypto_aead_reqsize(tfm)];
>>>> -	} aead_req;
>>>>    
>>>> -	memset(&aead_req, 0, sizeof(aead_req));
>>>> +	char aead_req_data[sizeof(struct aead_request) +
>>>> +				crypto_aead_reqsize(tfm) +
>>>> +				CRYPTO_MINALIGN] CRYPTO_MINALIGN_ATTR;
>>> Can this be a too large amount of stack?
>>>
>>> Is crypto_aead_reqsize() limited to < ~1k?
>>>
>>> Perhaps it'd be better to use kzalloc for this
>>> or another reserved pool
>> No more stack being used than with the the original code. The stack
>> memory use is identical.
> I do understand that, but that's not my question.
>
> I appreciate you're getting this to compile for llvm.
>
> Any idea of the max value of crypto_aead_reqsize?
And I understand your question, as well as why it is important.

Moving it from being stack based to alloacted memory may or may not be a 
good thing, but is orthogonal to the intention of this particular patch 
(which is just to remove the use of VLAIS from this code).

> $ grep-2.5.4 -rP --include=*.[ch] "(?:->|\.)\s*\breqsize\s*=[^=][^;]+;" *
Very clever. I'm going to use this. :)

Behan
Sergei Antonov March 8, 2014, 8:29 p.m. | #6
On 8 March 2014 02:26,  <behanw@converseincode.com> wrote:
> diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
> index 7c7df47..3317578 100644
> --- a/net/mac80211/aes_ccm.c
> +++ b/net/mac80211/aes_ccm.c
> @@ -23,12 +23,14 @@ void ieee80211_aes_ccm_encrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
>                                u8 *data, size_t data_len, u8 *mic)
>  {
>         struct scatterlist assoc, pt, ct[2];
> -       struct {
> -               struct aead_request     req;
> -               u8                      priv[crypto_aead_reqsize(tfm)];
> -       } aead_req;
>
> -       memset(&aead_req, 0, sizeof(aead_req));
> +       char aead_req_data[sizeof(struct aead_request) +
> +                               crypto_aead_reqsize(tfm) +
> +                               CRYPTO_MINALIGN] CRYPTO_MINALIGN_ATTR;
> +
> +       struct aead_request *aead_req = (void *) aead_req_data;

Bad trick with regards to alignment.
The alignment requirement for struct aead_request is stronger than
what an array of chars may have.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Behan Webster March 8, 2014, 8:42 p.m. | #7
On 03/08/14 12:29, Sergei Antonov wrote:
> On 8 March 2014 02:26,  <behanw@converseincode.com> wrote:
>> diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
>> index 7c7df47..3317578 100644
>> --- a/net/mac80211/aes_ccm.c
>> +++ b/net/mac80211/aes_ccm.c
>> @@ -23,12 +23,14 @@ void ieee80211_aes_ccm_encrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
>>                                 u8 *data, size_t data_len, u8 *mic)
>>   {
>>          struct scatterlist assoc, pt, ct[2];
>> -       struct {
>> -               struct aead_request     req;
>> -               u8                      priv[crypto_aead_reqsize(tfm)];
>> -       } aead_req;
>>
>> -       memset(&aead_req, 0, sizeof(aead_req));
>> +       char aead_req_data[sizeof(struct aead_request) +
>> +                               crypto_aead_reqsize(tfm) +
>> +                               CRYPTO_MINALIGN] CRYPTO_MINALIGN_ATTR;
>> +
>> +       struct aead_request *aead_req = (void *) aead_req_data;
> Bad trick with regards to alignment.
> The alignment requirement for struct aead_request is stronger than
> what an array of chars may have.
Damn it. I should have seen that too. We had to do that in our related 
netfilter patch.

Good catch. Will fix.

Thanks,

Behan
Behan Webster March 8, 2014, 9:36 p.m. | #8
On 03/08/14 06:53, Stanislaw Gruszka wrote:
> On Fri, Mar 07, 2014 at 06:15:43PM -0800, Behan Webster wrote:
>> On 03/07/14 17:56, Joe Perches wrote:
>>> On Fri, 2014-03-07 at 17:26 -0800, behanw@converseincode.com wrote:
>>>> From: Jan-Simon Möller <dl9pf@gmx.de>
>>>>
>>>> Replaced the use of a Variable Length Array In Struct (VLAIS) with a C99
>>>> compliant equivalent. This is the original VLAIS struct.
>>> []
>>>> diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
>>> []
>>>> @@ -23,12 +23,14 @@ void ieee80211_aes_ccm_encrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
>>>>   			       u8 *data, size_t data_len, u8 *mic)
>>>>   {
>>>>   	struct scatterlist assoc, pt, ct[2];
>>>> -	struct {
>>>> -		struct aead_request	req;
>>>> -		u8			priv[crypto_aead_reqsize(tfm)];
>>>> -	} aead_req;
>>>> -	memset(&aead_req, 0, sizeof(aead_req));
>>>> +	char aead_req_data[sizeof(struct aead_request) +
>>>> +				crypto_aead_reqsize(tfm) +
>>>> +				CRYPTO_MINALIGN] CRYPTO_MINALIGN_ATTR;
>>> Can this be a too large amount of stack?
>>>
>>> Is crypto_aead_reqsize() limited to < ~1k?
>>>
>>> Perhaps it'd be better to use kzalloc for this
>>> or another reserved pool
>> No more stack being used than with the the original code. The stack
>> memory use is identical.
> Could you explain that? It looks like aead_req_data can be bigger than
> original struct aead_req up to CRYPTO_MINALIGN bytes. IOW adding
> CRYPTO_MINALIGN to size seems unneeded as aead_request->__ctx alignment
> requirement should by assured by proper sizeof(struct aead_request).
True, possibly a few more bytes. Nothing significant. However, I will 
fix this too.

> Besides, why not add VLAIS feature to llvm instead of fixing all
> programs that use it?
You aren't the first to ask this (I certainly get asked this regularly). :)

VLAIS is specifically disallowed by the C89, C99, and later C standards. 
VLAIS is also not an officially documented feature of gcc (It is a side 
effect from the support of other languages supported by the gcc 
toolchain). The LLVM project won't add VLAIS support for these reasons, 
and because it would unnecessarily complicate their compiler 
architecture with almost no appreciable gain.

VLAIS is only used in a handful of places in the kernel (almost 
exclusively in crypto related code), so changing it in the kernel not 
only is easier, but also makes the kernel code C99 (and beyond) 
compliant. Being standards compliant is important not only for clang, 
but also for newer versions of gcc (though not yet in the case of VLAIS).

VLAIS should not be confused with Variable Length Arrays (VLA) and 
flexible members (undefined or zero length arrays at the end of a non 
embedded struct) which are both explicitly in the C standards and 
supported by both gcc and clang.

Behan
PaX Team March 8, 2014, 10:01 p.m. | #9
On 8 Mar 2014 at 21:29, Sergei Antonov wrote:

> > -       memset(&aead_req, 0, sizeof(aead_req));
> > +       char aead_req_data[sizeof(struct aead_request) +
> > +                               crypto_aead_reqsize(tfm) +
> > +                               CRYPTO_MINALIGN] CRYPTO_MINALIGN_ATTR;
                                                     ^^^^^^^^^^^^^^^^^^^^
wouldn't the underlined attribute ensure the required alignment?

> Bad trick with regards to alignment.
> The alignment requirement for struct aead_request is stronger than
> what an array of chars may have.



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Sergei Antonov March 8, 2014, 11 p.m. | #10
On 8 March 2014 23:01, PaX Team <pageexec@freemail.hu> wrote:
> On 8 Mar 2014 at 21:29, Sergei Antonov wrote:
>
>> > -       memset(&aead_req, 0, sizeof(aead_req));
>> > +       char aead_req_data[sizeof(struct aead_request) +
>> > +                               crypto_aead_reqsize(tfm) +
>> > +                               CRYPTO_MINALIGN] CRYPTO_MINALIGN_ATTR;
>                                                      ^^^^^^^^^^^^^^^^^^^^
> wouldn't the underlined attribute ensure the required alignment?
Yes. Sorry, I overlooked it.

I would remove unneeded CRYPTO_MINALIGN and get the alignment from the
target structure:
        char aead_req_data[sizeof(struct aead_request) +
                           crypto_aead_reqsize(tfm)]
                __attribute__((__aligned__(__alignof__(struct aead_request))));
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
dl9pf@gmx.de March 8, 2014, 11:41 p.m. | #11
On 9. März 2014 00:00:19 MEZ, Sergei Antonov <saproj@gmail.com> wrote:
>On 8 March 2014 23:01, PaX Team <pageexec@freemail.hu> wrote:
>> On 8 Mar 2014 at 21:29, Sergei Antonov wrote:
>>
>>> > -       memset(&aead_req, 0, sizeof(aead_req));
>>> > +       char aead_req_data[sizeof(struct aead_request) +
>>> > +                               crypto_aead_reqsize(tfm) +
>>> > +                               CRYPTO_MINALIGN]
>CRYPTO_MINALIGN_ATTR;
>>                                                     
>^^^^^^^^^^^^^^^^^^^^
>> wouldn't the underlined attribute ensure the required alignment?
>Yes. Sorry, I overlooked it.
>
>I would remove unneeded CRYPTO_MINALIGN and get the alignment from the
>target structure:
>        char aead_req_data[sizeof(struct aead_request) +
>                           crypto_aead_reqsize(tfm)]
>        __attribute__((__aligned__(__alignof__(struct aead_request))));



LGTM
David Miller March 9, 2014, 12:01 a.m. | #12
From: Sergei Antonov <saproj@gmail.com>
Date: Sat, 8 Mar 2014 21:29:57 +0100

> On 8 March 2014 02:26,  <behanw@converseincode.com> wrote:
>> diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
>> index 7c7df47..3317578 100644
>> --- a/net/mac80211/aes_ccm.c
>> +++ b/net/mac80211/aes_ccm.c
>> @@ -23,12 +23,14 @@ void ieee80211_aes_ccm_encrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
>>                                u8 *data, size_t data_len, u8 *mic)
>>  {
>>         struct scatterlist assoc, pt, ct[2];
>> -       struct {
>> -               struct aead_request     req;
>> -               u8                      priv[crypto_aead_reqsize(tfm)];
>> -       } aead_req;
>>
>> -       memset(&aead_req, 0, sizeof(aead_req));
>> +       char aead_req_data[sizeof(struct aead_request) +
>> +                               crypto_aead_reqsize(tfm) +
>> +                               CRYPTO_MINALIGN] CRYPTO_MINALIGN_ATTR;
>> +
>> +       struct aead_request *aead_req = (void *) aead_req_data;
> 
> Bad trick with regards to alignment.
> The alignment requirement for struct aead_request is stronger than
> what an array of chars may have.

Indeed, this will crash on platforms such as sparc64 that trap on
unaligned accesses.

You have to tell the compiler the alignment using proper types.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Behan Webster March 9, 2014, 1:58 a.m. | #13
On 03/08/14 15:00, Sergei Antonov wrote:
> On 8 March 2014 23:01, PaX Team <pageexec@freemail.hu> wrote:
>> On 8 Mar 2014 at 21:29, Sergei Antonov wrote:
>>
>>>> -       memset(&aead_req, 0, sizeof(aead_req));
>>>> +       char aead_req_data[sizeof(struct aead_request) +
>>>> +                               crypto_aead_reqsize(tfm) +
>>>> +                               CRYPTO_MINALIGN] CRYPTO_MINALIGN_ATTR;
>>                                                       ^^^^^^^^^^^^^^^^^^^^
>> wouldn't the underlined attribute ensure the required alignment?
> Yes. Sorry, I overlooked it.
>
> I would remove unneeded CRYPTO_MINALIGN and get the alignment from the
> target structure:
>          char aead_req_data[sizeof(struct aead_request) +
>                             crypto_aead_reqsize(tfm)]
>                  __attribute__((__aligned__(__alignof__(struct aead_request))));
Even better. Will resubmit.

Behan

Patch hide | download patch | download mbox

diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
index 7c7df47..3317578 100644
--- a/net/mac80211/aes_ccm.c
+++ b/net/mac80211/aes_ccm.c
@@ -23,12 +23,14 @@  void ieee80211_aes_ccm_encrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
 			       u8 *data, size_t data_len, u8 *mic)
 {
 	struct scatterlist assoc, pt, ct[2];
-	struct {
-		struct aead_request	req;
-		u8			priv[crypto_aead_reqsize(tfm)];
-	} aead_req;
 
-	memset(&aead_req, 0, sizeof(aead_req));
+	char aead_req_data[sizeof(struct aead_request) +
+				crypto_aead_reqsize(tfm) +
+				CRYPTO_MINALIGN] CRYPTO_MINALIGN_ATTR;
+
+	struct aead_request *aead_req = (void *) aead_req_data;
+
+	memset(aead_req, 0, sizeof(aead_req_data));
 
 	sg_init_one(&pt, data, data_len);
 	sg_init_one(&assoc, &aad[2], be16_to_cpup((__be16 *)aad));
@@ -36,23 +38,25 @@  void ieee80211_aes_ccm_encrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
 	sg_set_buf(&ct[0], data, data_len);
 	sg_set_buf(&ct[1], mic, IEEE80211_CCMP_MIC_LEN);
 
-	aead_request_set_tfm(&aead_req.req, tfm);
-	aead_request_set_assoc(&aead_req.req, &assoc, assoc.length);
-	aead_request_set_crypt(&aead_req.req, &pt, ct, data_len, b_0);
+	aead_request_set_tfm(aead_req, tfm);
+	aead_request_set_assoc(aead_req, &assoc, assoc.length);
+	aead_request_set_crypt(aead_req, &pt, ct, data_len, b_0);
 
-	crypto_aead_encrypt(&aead_req.req);
+	crypto_aead_encrypt(aead_req);
 }
 
 int ieee80211_aes_ccm_decrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
 			      u8 *data, size_t data_len, u8 *mic)
 {
 	struct scatterlist assoc, pt, ct[2];
-	struct {
-		struct aead_request	req;
-		u8			priv[crypto_aead_reqsize(tfm)];
-	} aead_req;
 
-	memset(&aead_req, 0, sizeof(aead_req));
+	char aead_req_data[sizeof(struct aead_request) +
+				crypto_aead_reqsize(tfm) +
+				CRYPTO_MINALIGN] CRYPTO_MINALIGN_ATTR;
+
+	struct aead_request *aead_req = (void *) aead_req_data;
+
+	memset(aead_req, 0, sizeof(aead_req_data));
 
 	sg_init_one(&pt, data, data_len);
 	sg_init_one(&assoc, &aad[2], be16_to_cpup((__be16 *)aad));
@@ -60,12 +64,12 @@  int ieee80211_aes_ccm_decrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
 	sg_set_buf(&ct[0], data, data_len);
 	sg_set_buf(&ct[1], mic, IEEE80211_CCMP_MIC_LEN);
 
-	aead_request_set_tfm(&aead_req.req, tfm);
-	aead_request_set_assoc(&aead_req.req, &assoc, assoc.length);
-	aead_request_set_crypt(&aead_req.req, ct, &pt,
+	aead_request_set_tfm(aead_req, tfm);
+	aead_request_set_assoc(aead_req, &assoc, assoc.length);
+	aead_request_set_crypt(aead_req, ct, &pt,
 			       data_len + IEEE80211_CCMP_MIC_LEN, b_0);
 
-	return crypto_aead_decrypt(&aead_req.req);
+	return crypto_aead_decrypt(aead_req);
 }
 
 struct crypto_aead *ieee80211_aes_key_setup_encrypt(const u8 key[])