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]

Date:	Sat, 06 Sep 2014 19:06:22 -0700
From:	Behan Webster <behanw@...verseincode.com>
To:	Thomas Gleixner <tglx@...utronix.de>
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 09/06/14 03:11, Thomas Gleixner wrote:
> On Fri, 5 Sep 2014, Behan Webster wrote:
>> On 09/05/14 17:18, Thomas Gleixner wrote:
>>>> 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/...
>> "The Signed-off-by: tag indicates that the signer was involved in the
>> development of the patch, or that he/she was in the patch's delivery path."
>>
>> All three of us were involved. Does that not satisfy this rule?
> No. Read #12
>
> The sign-off is a simple line at the end of the explanation for the
> patch, which certifies that you wrote it or otherwise have the right to
> pass it on as an open-source patch.
>
> So the above chain says:
>
>     Written-by:   Behan
>     Passed-on-by: Mark
>     Passed-on-by: Jan
>
> That would be correct if you sent the patch to Mark, Mark sent it to
> Jan and Jan finally submitted it to LKML.
I suppose "Reviewed-by" is probably more appropriate for the last 2 
then. Will fix.

>>>> -	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.
>> Sadly this is a design pattern used in many places through out the kernel, and
>> appears to be fundamental to the crypto system. I was advised *not* to change
>> it, so we haven't.
>>
>> I agree that it's not a good practice.
>>
>>>    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);
>> Nothing is wrong with that. I would have actually preferred that. But it would
>> have fundamentally changed a lot more code.
> Errm. Why is
>
> #define SHASH_DESC_ON_STACK(shash, tfm)				\
> 	char __shash[sizeof(.....)];				\
> 	struct shash_desc *shash = (struct shash_desc *) __shash
>
> requiring more fundamental than open coding the same thing a gazillion
> times. You still need to change ALL usage sides of the anon struct.
>
> So in fact you could avoid the whole code change by making it
>
>     SHASH_DESC_ON_STACK(desc, tfm);
>
> and do the anon struct or a proper struct magic in the macro.
I see. I thought you meant a more fundamental change to the crypto 
system API. My misunderstanding.

Ironically we tried to stay away from macros since the last time we 
tried to replace VLAIS using macros (we've attempted patches to remove 
VLAIS a few times) we were told *not* to hide the implementation with 
macro magic. Though, to be fair, we were using more pointer math in our 
other macro-based effort, and the non-crypto uses of VLAIS are a lot 
more complex to replace.

Like I said I'm actually a fan of hiding ugliness in macros. Will fix.

Again, thanks for the feedback,

Behan

-- 
Behan Webster
behanw@...verseincode.com

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/