linux-kernel - Re: [PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima

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]

Message-ID: <alpine.DEB.2.10.1409060131260.5472@nanos>
Date:	Sat, 6 Sep 2014 02:18:27 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Behan Webster <behanw@...verseincode.com>
cc:	zohar@...ux.vnet.ibm.com, d.kasatkin@...sung.com,
	james.l.morris@...cle.com, linux-ima-devel@...ts.sourceforge.net,
	linux-ima-user@...ts.sourceforge.net, linux-kernel@...r.kernel.org,
	linux-security-module@...r.kernel.org, serge@...lyn.com,
	torvalds@...ux-foundation.org,
	Mark Charlebois <charlebm@...il.com>,
	Jan-Simon Möller <dl9pf@....de>
Subject: Re: [PATCH] security, crypto: LLVMLinux: Remove VLAIS from
 ima_crypto.c

On Fri, 5 Sep 2014, behanw@...verseincode.com wrote:

> From: Behan Webster <behanw@...verseincode.com>
> 
> Replaced the use of a Variable Length Array In Struct (VLAIS) with a C99
> compliant equivalent. This patch allocates the appropriate amount of memory
> using an char array.
> 
> The new code can be compiled with both gcc and clang.
> 
> 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: Behan Webster <behanw@...verseincode.com>
> Signed-off-by: Mark Charlebois <charlebm@...il.com>
> Signed-off-by: Jan-Simon Möller <dl9pf@....de>

This SOB chain is completely ass backwards. See Documentation/...

> ---
>  security/integrity/ima/ima_crypto.c | 53 +++++++++++++++++--------------------
>  1 file changed, 25 insertions(+), 28 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index 0bd7328..a6aa2b0 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -380,17 +380,16 @@ static int ima_calc_file_hash_tfm(struct file *file,
>  	loff_t i_size, offset = 0;
>  	char *rbuf;
>  	int rc, read = 0;
> -	struct {
> -		struct shash_desc shash;
> -		char ctx[crypto_shash_descsize(tfm)];
> -	} desc;
> +	char desc[sizeof(struct shash_desc) +
> +		crypto_shash_descsize(tfm)] CRYPTO_MINALIGN_ATTR;
> +	struct shash_desc *shash = (struct shash_desc *)desc;

That anon struct should have never happened in the first place. Not
your problem, but you are not making it any better. You replace open
coded crap with even more unreadable crap.

Whats wrong with 

      SHASH_DESC_ON_STACK(shash, tfm);

or something along that line and hide all the stack allocation, type
conversion crap and make my favourite compiler happy in a single
place?

Nothing wrong as far as I can tell, it just would add the benefit that
you can halfways ensure that nobody fatfingers the magic allocation
and conversion conventions.

Brainlessly replacing crap by more crap just to make it compile with
your favourite compiler is obviously convers to the goals of proper
written code, but considering your mail domain ....

Thanks,

	tglx