[PATCH v2] crypto, dm: LLVMLinux: Remove VLAIS usage from dm-crypt

[PATCH v2] crypto, dm: LLVMLinux: Remove VLAIS usage from dm-crypt

[behanw]

From: Jan-Simon Möller <dl9pf@gmx.de>

The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
precludes the use of compilers which don't implement VLAIS (for instance the
Clang compiler). This patch instead allocates the appropriate amount of memory
using an char array.

struct shash_desc contains a flexible array member member ctx declared with
CRYPTO_MINALIGN_ATTR, so sizeof(struct shash_desc) aligns the beginning
of the array declared after struct shash_desc with long long.

No trailing padding is required because it is not a struct type that can
be used in an array.

The CRYPTO_MINALIGN_ATTR is required so that desc is aligned with long long
as would be the case for a struct containing a member with
CRYPTO_MINALIGN_ATTR.

Signed-off-by: Jan-Simon Möller <dl9pf@gmx.de>
Signed-off-by: Behan Webster <behanw@converseincode.com>
Cc: pageexec@freemail.hu
---
 drivers/md/dm-crypt.c | 38 ++++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index cd15e08..ad696b8 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -526,29 +526,28 @@ static int crypt_iv_lmk_one(struct crypt_config *cc, u8 *iv,
 			    u8 *data)
 {
 	struct iv_lmk_private *lmk = &cc->iv_gen_private.lmk;
-	struct {
-		struct shash_desc desc;
-		char ctx[crypto_shash_descsize(lmk->hash_tfm)];
-	} sdesc;
+	char sdesc[sizeof(struct shash_desc) +
+		crypto_shash_descsize(lmk->hash_tfm)] CRYPTO_MINALIGN_ATTR;
+	struct shash_desc *desc = (struct shash_desc *)sdesc;
 	struct md5_state md5state;
 	__le32 buf[4];
 	int i, r;
 
-	sdesc.desc.tfm = lmk->hash_tfm;
-	sdesc.desc.flags = CRYPTO_TFM_REQ_MAY_SLEEP;
+	desc->tfm = lmk->hash_tfm;
+	desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
 
-	r = crypto_shash_init(&sdesc.desc);
+	r = crypto_shash_init(desc);
 	if (r)
 		return r;
 
 	if (lmk->seed) {
-		r = crypto_shash_update(&sdesc.desc, lmk->seed, LMK_SEED_SIZE);
+		r = crypto_shash_update(desc, lmk->seed, LMK_SEED_SIZE);
 		if (r)
 			return r;
 	}
 
 	/* Sector is always 512B, block size 16, add data of blocks 1-31 */
-	r = crypto_shash_update(&sdesc.desc, data + 16, 16 * 31);
+	r = crypto_shash_update(desc, data + 16, 16 * 31);
 	if (r)
 		return r;
 
@@ -557,12 +556,12 @@ static int crypt_iv_lmk_one(struct crypt_config *cc, u8 *iv,
 	buf[1] = cpu_to_le32((((u64)dmreq->iv_sector >> 32) & 0x00FFFFFF) | 0x80000000);
 	buf[2] = cpu_to_le32(4024);
 	buf[3] = 0;
-	r = crypto_shash_update(&sdesc.desc, (u8 *)buf, sizeof(buf));
+	r = crypto_shash_update(desc, (u8 *)buf, sizeof(buf));
 	if (r)
 		return r;
 
 	/* No MD5 padding here */
-	r = crypto_shash_export(&sdesc.desc, &md5state);
+	r = crypto_shash_export(desc, &md5state);
 	if (r)
 		return r;
 
@@ -679,10 +678,9 @@ static int crypt_iv_tcw_whitening(struct crypt_config *cc,
 	struct iv_tcw_private *tcw = &cc->iv_gen_private.tcw;
 	u64 sector = cpu_to_le64((u64)dmreq->iv_sector);
 	u8 buf[TCW_WHITENING_SIZE];
-	struct {
-		struct shash_desc desc;
-		char ctx[crypto_shash_descsize(tcw->crc32_tfm)];
-	} sdesc;
+	char sdesc[sizeof(struct shash_desc)
+		+ crypto_shash_descsize(tcw->crc32_tfm)] CRYPTO_MINALIGN_ATTR;
+	struct shash_desc *desc = (struct shash_desc *)sdesc;
 	int i, r;
 
 	/* xor whitening with sector number */
@@ -691,16 +689,16 @@ static int crypt_iv_tcw_whitening(struct crypt_config *cc,
 	crypto_xor(&buf[8], (u8 *)&sector, 8);
 
 	/* calculate crc32 for every 32bit part and xor it */
-	sdesc.desc.tfm = tcw->crc32_tfm;
-	sdesc.desc.flags = CRYPTO_TFM_REQ_MAY_SLEEP;
+	desc->tfm = tcw->crc32_tfm;
+	desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
 	for (i = 0; i < 4; i++) {
-		r = crypto_shash_init(&sdesc.desc);
+		r = crypto_shash_init(desc);
 		if (r)
 			goto out;
-		r = crypto_shash_update(&sdesc.desc, &buf[i * 4], 4);
+		r = crypto_shash_update(desc, &buf[i * 4], 4);
 		if (r)
 			goto out;
-		r = crypto_shash_final(&sdesc.desc, &buf[i * 4]);
+		r = crypto_shash_final(desc, &buf[i * 4]);
 		if (r)
 			goto out;
 	}
-- 
1.9.1

Re: [PATCH v2] crypto, dm: LLVMLinux: Remove VLAIS usage from dm-crypt

[Milan Broz]

On 09/06/2014 01:02 AM, behanw@converseincode.com wrote:
> From: Jan-Simon Möller <dl9pf@gmx.de>
> 
> The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
> precludes the use of compilers which don't implement VLAIS (for instance the
> Clang compiler). This patch instead allocates the appropriate amount of memory
> using an char array.

Well, if clang (or C99 code) is now preferred for kernel, why not.

Just please commit the patch series en bloc (together with the patches
removing VLAIS from crypto code you posted to cryptoapi list).

Dmcrypt should always use the same coding practices as crypto part of the kernel
so we should not divert here.

> -	struct {
> -		struct shash_desc desc;
> -		char ctx[crypto_shash_descsize(lmk->hash_tfm)];
> -	} sdesc;
> +	char sdesc[sizeof(struct shash_desc) +
> +		crypto_shash_descsize(lmk->hash_tfm)] CRYPTO_MINALIGN_ATTR;
> +	struct shash_desc *desc = (struct shash_desc *)sdesc;

TBH, this looks even more uglier that the previous code :-)

(But tglx already complained on different patch and I fully agree that crypto code
should not use this kind of construction in the first place...
It would be very nice to introduce at least some macro hiding these crazy
stack allocations later.)

Thanks,
Milan

Re: [PATCH v2] crypto, dm: LLVMLinux: Remove VLAIS usage from dm-crypt

[Behan Webster]

On 09/06/14 01:46, Milan Broz wrote:
> On 09/06/2014 01:02 AM, behanw@converseincode.com wrote:
>> From: Jan-Simon Möller <dl9pf@gmx.de>
>>
>> The use of variable length arrays in structs (VLAIS) in the Linux Kernel code
>> precludes the use of compilers which don't implement VLAIS (for instance the
>> Clang compiler). This patch instead allocates the appropriate amount of memory
>> using an char array.
> Well, if clang (or C99 code) is now preferred for kernel, why not.
>
> Just please commit the patch series en bloc (together with the patches
> removing VLAIS from crypto code you posted to cryptoapi list).
They seemed more separate than that. However, happy to post them as a 
patch set.

>> -	struct {
>> -		struct shash_desc desc;
>> -		char ctx[crypto_shash_descsize(lmk->hash_tfm)];
>> -	} sdesc;
>> +	char sdesc[sizeof(struct shash_desc) +
>> +		crypto_shash_descsize(lmk->hash_tfm)] CRYPTO_MINALIGN_ATTR;
>> +	struct shash_desc *desc = (struct shash_desc *)sdesc;
> TBH, this looks even more uglier that the previous code :-)
I'm not claiming it's prettier. Merely C99. :)

> (But tglx already complained on different patch and I fully agree that crypto code
> should not use this kind of construction in the first place...
> It would be very nice to introduce at least some macro hiding these crazy
> stack allocations later.)
I actually agree with you and tglx. Will fix.

As I said to tglx, we were asked not to hide things with macro magic in 
some of our non-crypto VLAIS removal patches. Happy to use macros in 
this case.

Thanks,

Behan

-- 
Behan Webster
behanw@converseincode.com