Re: [PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c
On 09/08/14 08:43, Mimi Zohar wrote: Behan, thank you for the explanation. No worries. I should have explained better. My apologies. The same snippet of code used here, and elsewhere in the kernel, is taken from the crypto subsystem. Once it is resolved in the crypto subsystem, the same solution should be propogated. Mimi Indeed that is my intention. I have tglx's suggested solution coded already. Just doing a bunch of allyesconfig builds to confirm all is compiling correctly. I will post all patches as a single patch set this time (posted to all concerned). I will repeat the explanation as well with the new patch set so everyone else in other subsystems sees those reasons as well. If this works for everyone I'll also go back and update the crypto patches for the subsystems that have already accepted my previous patches. Thanks, Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c
On Mon, 2014-09-08 at 07:25 -0500, Behan Webster wrote:
> On 09/08/14 04:15, Dmitry Kasatkin wrote:
> > On 07/09/14 05:06, Behan Webster wrote:
> >> 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
> >> Signed-off-by: Mark Charlebois
> >> Signed-off-by: Jan-Simon Möller
> > 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
> >>
> > Hi,
> >
> > Despite if it is crap or not, it was said already in this thread,
> > following "design pattern" is heavily used through out the kernel - by
> > crypto core itself and by many widely used clients.
> >
> > struct {
> > struct shash_desc shash;
> > char ctx[crypto_shash_descsize(tfm)];
> > } desc;
> >
> >
> > My question why do you want to change this particular piece of code?
> Because it employs Variable Length Arrays in Structs. A construct which
> is explicitly forbidden by the C standard (C89, C99, C11). Because the
> vast majority of kernel developers I've talked to about this have been
> unaware of the use of VLAIS in the kernel and most find its use
> objectionable (there is a similar objection to the use of nested
> functions). Because implementing VLAIS in a compiler can severely impact
> the generated instructions surrounding its use, which is why most
> compilers don't implement VLAIS as a feature. Because using such a
> construct precludes standards based compilers from competing with the
> incumbent (my interest is enabling the use of clang and LLVM based
> technologies as a toolchain choice to compile and develop the kernel).
>
> > What about rest of the kernel?
> The LLVMLinux project is systematically working to remove the use of
> VLAIS from the kernel (already removed from ext4, USB Gadget, netfilter,
>
Re: [PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c
On 09/08/14 04:15, Dmitry Kasatkin wrote:
On 07/09/14 05:06, Behan Webster wrote:
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
Signed-off-by: Mark Charlebois
Signed-off-by: Jan-Simon Möller
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
Hi,
Despite if it is crap or not, it was said already in this thread,
following "design pattern" is heavily used through out the kernel - by
crypto core itself and by many widely used clients.
struct {
struct shash_desc shash;
char ctx[crypto_shash_descsize(tfm)];
} desc;
My question why do you want to change this particular piece of code?
Because it employs Variable Length Arrays in Structs. A construct which
is explicitly forbidden by the C standard (C89, C99, C11). Because the
vast majority of kernel developers I've talked to about this have been
unaware of the use of VLAIS in the kernel and most find its use
objectionable (there is a similar objection to the use of nested
functions). Because implementing VLAIS in a compiler can severely impact
the generated instructions surrounding its use, which is why most
compilers don't implement VLAIS as a feature. Because using such a
construct precludes standards based compilers from competing with the
incumbent (my interest is enabling the use of clang and LLVM based
technologies as a toolchain choice to compile and develop the kernel).
What about rest of the kernel?
The LLVMLinux project is systematically working to remove the use of
VLAIS from the kernel (already removed from ext4, USB Gadget, netfilter,
mac802.11, apparmor, bluetooth, etc). Users of the crpyto subsystem are
one of the last and heaviest users of VLAIS.
To solve your problem you probably need to change everything.
Essentially yes. Though I like to think of it as finding alternatives to
where ever it is still used. "Changing everything" implies much larger
changes which aren't necessary in most cases. Sometimes the alternative
is merely using a flexible member (zero length array at the end of the
struct, instead of a VLA in the struct). In several places several VLAs
are used in the same struct. And recently we found that exofs is using a
VLAIS inside VLAIS (second order VLAIS) in
Re: [PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c
On 07/09/14 05:06, Behan Webster wrote:
> 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
> Signed-off-by: Mark Charlebois
> Signed-off-by: Jan-Simon Möller
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
>
Hi,
Despite if it is crap or not, it was said already in this thread,
following "design pattern" is heavily used through out the kernel - by
crypto core itself and by many widely used clients.
struct {
struct shash_desc shash;
char ctx[crypto_shash_descsize(tfm)];
} desc;
My question why do you want to change this particular piece of code?
What about rest of the kernel?
To solve your problem you probably need to change everything.
If we are going to change it and introduce any macros, it is better to
do with the guidance from crypto folks.
I added CC:linux-cry...@vger.kernel.org mailing list and Herbert Xu,
crypto maintainer.
- Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c
On 07/09/14 05:06, Behan Webster wrote:
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 beh...@converseincode.com
Signed-off-by: Mark Charlebois charl...@gmail.com
Signed-off-by: Jan-Simon Möller dl...@gmx.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
Hi,
Despite if it is crap or not, it was said already in this thread,
following design pattern is heavily used through out the kernel - by
crypto core itself and by many widely used clients.
struct {
struct shash_desc shash;
char ctx[crypto_shash_descsize(tfm)];
} desc;
My question why do you want to change this particular piece of code?
What about rest of the kernel?
To solve your problem you probably need to change everything.
If we are going to change it and introduce any macros, it is better to
do with the guidance from crypto folks.
I added CC:linux-cry...@vger.kernel.org mailing list and Herbert Xu,
crypto maintainer.
- Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c
On 09/08/14 04:15, Dmitry Kasatkin wrote:
On 07/09/14 05:06, Behan Webster wrote:
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 beh...@converseincode.com
Signed-off-by: Mark Charlebois charl...@gmail.com
Signed-off-by: Jan-Simon Möller dl...@gmx.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
Hi,
Despite if it is crap or not, it was said already in this thread,
following design pattern is heavily used through out the kernel - by
crypto core itself and by many widely used clients.
struct {
struct shash_desc shash;
char ctx[crypto_shash_descsize(tfm)];
} desc;
My question why do you want to change this particular piece of code?
Because it employs Variable Length Arrays in Structs. A construct which
is explicitly forbidden by the C standard (C89, C99, C11). Because the
vast majority of kernel developers I've talked to about this have been
unaware of the use of VLAIS in the kernel and most find its use
objectionable (there is a similar objection to the use of nested
functions). Because implementing VLAIS in a compiler can severely impact
the generated instructions surrounding its use, which is why most
compilers don't implement VLAIS as a feature. Because using such a
construct precludes standards based compilers from competing with the
incumbent (my interest is enabling the use of clang and LLVM based
technologies as a toolchain choice to compile and develop the kernel).
What about rest of the kernel?
The LLVMLinux project is systematically working to remove the use of
VLAIS from the kernel (already removed from ext4, USB Gadget, netfilter,
mac802.11, apparmor, bluetooth, etc). Users of the crpyto subsystem are
one of the last and heaviest users of VLAIS.
To solve your problem you probably need to change everything.
Essentially yes. Though I like to think of it as finding alternatives to
where ever it is still used. Changing everything implies much larger
changes which aren't necessary in most cases. Sometimes the alternative
is merely using a flexible member (zero length array at the end of the
struct, instead of a VLA in the struct). In several places several VLAs
are used in the same struct. And recently we found that exofs is using
Re: [PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c
On Mon, 2014-09-08 at 07:25 -0500, Behan Webster wrote:
On 09/08/14 04:15, Dmitry Kasatkin wrote:
On 07/09/14 05:06, Behan Webster wrote:
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 beh...@converseincode.com
Signed-off-by: Mark Charlebois charl...@gmail.com
Signed-off-by: Jan-Simon Möller dl...@gmx.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
Hi,
Despite if it is crap or not, it was said already in this thread,
following design pattern is heavily used through out the kernel - by
crypto core itself and by many widely used clients.
struct {
struct shash_desc shash;
char ctx[crypto_shash_descsize(tfm)];
} desc;
My question why do you want to change this particular piece of code?
Because it employs Variable Length Arrays in Structs. A construct which
is explicitly forbidden by the C standard (C89, C99, C11). Because the
vast majority of kernel developers I've talked to about this have been
unaware of the use of VLAIS in the kernel and most find its use
objectionable (there is a similar objection to the use of nested
functions). Because implementing VLAIS in a compiler can severely impact
the generated instructions surrounding its use, which is why most
compilers don't implement VLAIS as a feature. Because using such a
construct precludes standards based compilers from competing with the
incumbent (my interest is enabling the use of clang and LLVM based
technologies as a toolchain choice to compile and develop the kernel).
What about rest of the kernel?
The LLVMLinux project is systematically working to remove the use of
VLAIS from the kernel (already removed from ext4, USB Gadget, netfilter,
mac802.11, apparmor, bluetooth, etc). Users of the crpyto subsystem are
one of the last and heaviest users of VLAIS.
To solve your problem you probably need to change everything.
Essentially yes. Though I like to think of it as finding alternatives to
where ever it is still used. Changing everything implies much larger
changes which aren't necessary in most cases. Sometimes
Re: [PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c
On 09/08/14 08:43, Mimi Zohar wrote: Behan, thank you for the explanation. No worries. I should have explained better. My apologies. The same snippet of code used here, and elsewhere in the kernel, is taken from the crypto subsystem. Once it is resolved in the crypto subsystem, the same solution should be propogated. Mimi Indeed that is my intention. I have tglx's suggested solution coded already. Just doing a bunch of allyesconfig builds to confirm all is compiling correctly. I will post all patches as a single patch set this time (posted to all concerned). I will repeat the explanation as well with the new patch set so everyone else in other subsystems sees those reasons as well. If this works for everyone I'll also go back and update the crypto patches for the subsystems that have already accepted my previous patches. Thanks, Behan -- Behan Webster beh...@converseincode.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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
Signed-off-by: Mark Charlebois
Signed-off-by: Jan-Simon Möller
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
beh...@converseincode.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c
On Fri, 5 Sep 2014, Behan Webster wrote:
> On 09/05/14 17:18, Thomas Gleixner wrote:
> > > Signed-off-by: Behan Webster
> > > Signed-off-by: Mark Charlebois
> > > Signed-off-by: Jan-Simon Möller
> > 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.
> > > - 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'm not sure making fundamental changes to the crypto code in order to make
> "my favourite compiler happy" is really a better plan. I prefer smaller more
> measured steps.
What's fundamental about a change which produces the same code but
hides all the easy to get wrong mess in a single place?
> > or something along that line and hide all the stack allocation, type
> > conversion crap and make my favourite compiler happy in a single
> > place?
> At this point it is less about making a particular compiler happy, and more
> about making the code at least C99 compliant which enables a different
> compiler so that more people can use it to make more fundamental changes like
> you are suggesting.
Well, just blindly making it compliant is one thing. But if we do that
we really should make it better and using a macro for this is
definitely an improvement which is worthwhile to do.
Thanks,
tglx
Re: [PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c
On Fri, 5 Sep 2014, Behan Webster wrote:
On 09/05/14 17:18, Thomas Gleixner wrote:
Signed-off-by: Behan Webster beh...@converseincode.com
Signed-off-by: Mark Charlebois charl...@gmail.com
Signed-off-by: Jan-Simon Möller dl...@gmx.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.
- 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'm not sure making fundamental changes to the crypto code in order to make
my favourite compiler happy is really a better plan. I prefer smaller more
measured steps.
What's fundamental about a change which produces the same code but
hides all the easy to get wrong mess in a single place?
or something along that line and hide all the stack allocation, type
conversion crap and make my favourite compiler happy in a single
place?
At this point it is less about making a particular compiler happy, and more
about making the code at least C99 compliant which enables a different
compiler so that more people can use it to make more fundamental changes like
you are suggesting.
Well, just blindly making it compliant is one thing. But if we do that
we really should make it better and using a macro for this is
definitely an improvement which is worthwhile to do.
Thanks,
tglx
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 beh...@converseincode.com
Signed-off-by: Mark Charlebois charl...@gmail.com
Signed-off-by: Jan-Simon Möller dl...@gmx.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
beh...@converseincode.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c
On 09/05/14 17:18, Thomas Gleixner wrote:
On Fri, 5 Sep 2014, beh...@converseincode.com wrote:
From: Behan Webster
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
Signed-off-by: Mark Charlebois
Signed-off-by: Jan-Simon Möller
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?
---
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.
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.
I'm not sure making fundamental changes to the crypto code in order to
make "my favourite compiler happy" is really a better plan. I prefer
smaller more measured steps.
or something along that line and hide all the stack allocation, type
conversion crap and make my favourite compiler happy in a single
place?
At this point it is less about making a particular compiler happy, and
more about making the code at least C99 compliant which enables a
different compiler so that more people can use it to make more
fundamental changes like you are suggesting.
I appreciate your feedback,
Behan
--
Behan Webster
beh...@converseincode.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c
On Fri, 5 Sep 2014, beh...@converseincode.com wrote:
> From: Behan Webster
>
> 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
> Signed-off-by: Mark Charlebois
> Signed-off-by: Jan-Simon Möller
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
[PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c
From: Behan Webster
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
Signed-off-by: Mark Charlebois
Signed-off-by: Jan-Simon Möller
---
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;
- desc.shash.tfm = tfm;
- desc.shash.flags = 0;
+ shash->tfm = tfm;
+ shash->flags = 0;
hash->length = crypto_shash_digestsize(tfm);
- rc = crypto_shash_init();
+ rc = crypto_shash_init(shash);
if (rc != 0)
return rc;
@@ -420,7 +419,7 @@ static int ima_calc_file_hash_tfm(struct file *file,
break;
offset += rbuf_len;
- rc = crypto_shash_update(, rbuf, rbuf_len);
+ rc = crypto_shash_update(shash, rbuf, rbuf_len);
if (rc)
break;
}
@@ -429,7 +428,7 @@ static int ima_calc_file_hash_tfm(struct file *file,
kfree(rbuf);
out:
if (!rc)
- rc = crypto_shash_final(, hash->digest);
+ rc = crypto_shash_final(shash, hash->digest);
return rc;
}
@@ -487,18 +486,17 @@ static int ima_calc_field_array_hash_tfm(struct
ima_field_data *field_data,
struct ima_digest_data *hash,
struct crypto_shash *tfm)
{
- 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;
int rc, i;
- desc.shash.tfm = tfm;
- desc.shash.flags = 0;
+ shash->tfm = tfm;
+ shash->flags = 0;
hash->length = crypto_shash_digestsize(tfm);
- rc = crypto_shash_init();
+ rc = crypto_shash_init(shash);
if (rc != 0)
return rc;
@@ -508,7 +506,7 @@ static int ima_calc_field_array_hash_tfm(struct
ima_field_data *field_data,
u32 datalen = field_data[i].len;
if (strcmp(td->name, IMA_TEMPLATE_IMA_NAME) != 0) {
- rc = crypto_shash_update(,
+ rc = crypto_shash_update(shash,
(const u8 *) _data[i].len,
sizeof(field_data[i].len));
if (rc)
@@ -518,13 +516,13 @@ static int ima_calc_field_array_hash_tfm(struct
ima_field_data *field_data,
data_to_hash = buffer;
datalen = IMA_EVENT_NAME_LEN_MAX + 1;
}
- rc = crypto_shash_update(, data_to_hash, datalen);
+ rc = crypto_shash_update(shash, data_to_hash, datalen);
if (rc)
break;
}
if (!rc)
- rc = crypto_shash_final(, hash->digest);
+ rc = crypto_shash_final(shash, hash->digest);
return rc;
}
@@ -565,15 +563,14 @@ static int __init ima_calc_boot_aggregate_tfm(char
*digest,
{
u8 pcr_i[TPM_DIGEST_SIZE];
int rc, i;
- 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;
- desc.shash.tfm = tfm;
- desc.shash.flags = 0;
+ shash->tfm = tfm;
+ shash->flags = 0;
[PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c
From: Behan Webster beh...@converseincode.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 beh...@converseincode.com
Signed-off-by: Mark Charlebois charl...@gmail.com
Signed-off-by: Jan-Simon Möller dl...@gmx.de
---
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;
- desc.shash.tfm = tfm;
- desc.shash.flags = 0;
+ shash-tfm = tfm;
+ shash-flags = 0;
hash-length = crypto_shash_digestsize(tfm);
- rc = crypto_shash_init(desc.shash);
+ rc = crypto_shash_init(shash);
if (rc != 0)
return rc;
@@ -420,7 +419,7 @@ static int ima_calc_file_hash_tfm(struct file *file,
break;
offset += rbuf_len;
- rc = crypto_shash_update(desc.shash, rbuf, rbuf_len);
+ rc = crypto_shash_update(shash, rbuf, rbuf_len);
if (rc)
break;
}
@@ -429,7 +428,7 @@ static int ima_calc_file_hash_tfm(struct file *file,
kfree(rbuf);
out:
if (!rc)
- rc = crypto_shash_final(desc.shash, hash-digest);
+ rc = crypto_shash_final(shash, hash-digest);
return rc;
}
@@ -487,18 +486,17 @@ static int ima_calc_field_array_hash_tfm(struct
ima_field_data *field_data,
struct ima_digest_data *hash,
struct crypto_shash *tfm)
{
- 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;
int rc, i;
- desc.shash.tfm = tfm;
- desc.shash.flags = 0;
+ shash-tfm = tfm;
+ shash-flags = 0;
hash-length = crypto_shash_digestsize(tfm);
- rc = crypto_shash_init(desc.shash);
+ rc = crypto_shash_init(shash);
if (rc != 0)
return rc;
@@ -508,7 +506,7 @@ static int ima_calc_field_array_hash_tfm(struct
ima_field_data *field_data,
u32 datalen = field_data[i].len;
if (strcmp(td-name, IMA_TEMPLATE_IMA_NAME) != 0) {
- rc = crypto_shash_update(desc.shash,
+ rc = crypto_shash_update(shash,
(const u8 *) field_data[i].len,
sizeof(field_data[i].len));
if (rc)
@@ -518,13 +516,13 @@ static int ima_calc_field_array_hash_tfm(struct
ima_field_data *field_data,
data_to_hash = buffer;
datalen = IMA_EVENT_NAME_LEN_MAX + 1;
}
- rc = crypto_shash_update(desc.shash, data_to_hash, datalen);
+ rc = crypto_shash_update(shash, data_to_hash, datalen);
if (rc)
break;
}
if (!rc)
- rc = crypto_shash_final(desc.shash, hash-digest);
+ rc = crypto_shash_final(shash, hash-digest);
return rc;
}
@@ -565,15 +563,14 @@ static int __init ima_calc_boot_aggregate_tfm(char
*digest,
{
u8 pcr_i[TPM_DIGEST_SIZE];
int rc, i;
- 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
Re: [PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c
On Fri, 5 Sep 2014, beh...@converseincode.com wrote:
From: Behan Webster beh...@converseincode.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 beh...@converseincode.com
Signed-off-by: Mark Charlebois charl...@gmail.com
Signed-off-by: Jan-Simon Möller dl...@gmx.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
Re: [PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c
On 09/05/14 17:18, Thomas Gleixner wrote:
On Fri, 5 Sep 2014, beh...@converseincode.com wrote:
From: Behan Webster beh...@converseincode.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 beh...@converseincode.com
Signed-off-by: Mark Charlebois charl...@gmail.com
Signed-off-by: Jan-Simon Möller dl...@gmx.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?
---
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.
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.
I'm not sure making fundamental changes to the crypto code in order to
make my favourite compiler happy is really a better plan. I prefer
smaller more measured steps.
or something along that line and hide all the stack allocation, type
conversion crap and make my favourite compiler happy in a single
place?
At this point it is less about making a particular compiler happy, and
more about making the code at least C99 compliant which enables a
different compiler so that more people can use it to make more
fundamental changes like you are suggesting.
I appreciate your feedback,
Behan
--
Behan Webster
beh...@converseincode.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
