[PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c

[Dmitry Kasatkin]

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

"The Signed-off-by: tag indicates that the signer was involved in t=

he

development of the patch, or that he/she was in the patch's deliver=

y

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.
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 =3D (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 op=

en

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 =3D (struct shash_desc *) __shash
requiring more fundamental than open coding the same thing a gazilli=

on

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 remov=

e

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=

=2E

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-***@vger.kernel.org mailing list and Herbert Xu,
crypto maintainer.

- Dmitry

--
To unsubscribe from this list: send the line "unsubscribe linux-securit=
y-module" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Behan Webster]

Post by Dmitry Kasatkin

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

"The Signed-off-by: tag indicates that the signer was involved in =

the

Post by Dmitry Kasatkin

development of the patch, or that he/she was in the patch's delive=

ry

Post by Dmitry Kasatkin

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 righ=

t to

Post by Dmitry Kasatkin

pass it on as an open-source patch.
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 t=

o

Post by Dmitry Kasatkin

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 =3D (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=

*

Post by Dmitry Kasatkin

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 o=

pen

Post by Dmitry Kasatkin

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 =3D (struct shash_desc *) __shash
requiring more fundamental than open coding the same thing a gazill=

ion

Post by Dmitry Kasatkin

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 remo=

ve

Post by Dmitry Kasatkin

VLAIS a few times) we were told *not* to hide the implementation wit=

h

Post by Dmitry Kasatkin

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 fi=

x.

Post by Dmitry Kasatkin

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 - b=

y

Post by Dmitry Kasatkin
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=
=20
is explicitly forbidden by the C standard (C89, C99, C11). Because the=20
vast majority of kernel developers I've talked to about this have been=20
unaware of the use of VLAIS in the kernel and most find its use=20
objectionable (there is a similar objection to the use of nested=20
functions). Because implementing VLAIS in a compiler can severely impac=
t=20
the generated instructions surrounding its use, which is why most=20
compilers don't implement VLAIS as a feature. Because using such a=20
construct precludes standards based compilers from competing with the=20
incumbent (my interest is enabling the use of clang and LLVM based=20
technologies as a toolchain choice to compile and develop the kernel).

Post by Dmitry Kasatkin
What about rest of the kernel?

The LLVMLinux project is systematically working to remove the use of=20
VLAIS from the kernel (already removed from ext4, USB Gadget, netfilter=
,=20
mac802.11, apparmor, bluetooth, etc). Users of the crpyto subsystem are=
=20
one of the last and heaviest users of VLAIS.

Post by Dmitry Kasatkin
To solve your problem you probably need to change everything.

Essentially yes. Though I like to think of it as finding alternatives t=
o=20
where ever it is still used. "Changing everything" implies much larger=20
changes which aren't necessary in most cases. Sometimes the alternative=
=20
is merely using a flexible member (zero length array at the end of the=20
struct, instead of a VLA in the struct). In several places several VLAs=
=20
are used in the same struct. And recently we found that exofs is using =
a=20
VLAIS inside VLAIS (second order VLAIS) in one of its structures. So no=
t=20
finished yet.

Post by Dmitry Kasatkin
If we are going to change it and introduce any macros, it is better t=

o

Post by Dmitry Kasatkin
do with the guidance from crypto folks.

Absolutely. Most of the crypto related patches have been sent to them. =
I=20
am absolutely looking for their input.

Post by Dmitry Kasatkin
crypto maintainer.

I suppose this specific patch may not have CC that list. However, most=20
of the other VLAIS removal patches were copied to linux-crypto, Herbert=
=20
Xu and David Miller.

Thanks,

Behan

--=20
Behan Webster
***@converseincode.com

[Mimi Zohar]

On Mon, 2014-09-08 at 07:25 -0500, Behan Webster wrote:=20

Post by Dmitry Kasatkin

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

=2E.

Post by Dmitry Kasatkin

"The Signed-off-by: tag indicates that the signer was involved i=

n the

Post by Dmitry Kasatkin

development of the patch, or that he/she was in the patch's deli=

very

Post by Dmitry Kasatkin

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 t=

he

Post by Dmitry Kasatkin

patch, which certifies that you wrote it or otherwise have the ri=

ght to

Post by Dmitry Kasatkin

pass it on as an open-source patch.
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

Post by Dmitry Kasatkin

Jan and Jan finally submitted it to LKML.

I suppose "Reviewed-by" is probably more appropriate for the last =

2

Post by Dmitry Kasatkin

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 =3D (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 t=

he

Post by Dmitry Kasatkin

kernel, and
appears to be fundamental to the crypto system. I was advised *n=

ot*

Post by Dmitry Kasatkin

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

Post by Dmitry Kasatkin

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=

=2E

Post by Dmitry Kasatkin

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 =3D (struct shash_desc *) __shash
requiring more fundamental than open coding the same thing a gazi=

llion

Post by Dmitry Kasatkin

times. You still need to change ALL usage sides of the anon struc=

t.

Post by Dmitry Kasatkin

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 w=

e

Post by Dmitry Kasatkin

tried to replace VLAIS using macros (we've attempted patches to re=

move

Post by Dmitry Kasatkin

VLAIS a few times) we were told *not* to hide the implementation w=

ith

Post by Dmitry Kasatkin

macro magic. Though, to be fair, we were using more pointer math i=

n

Post by Dmitry Kasatkin

our other macro-based effort, and the non-crypto uses of VLAIS are=

a

Post by Dmitry Kasatkin

lot more complex to replace.
Like I said I'm actually a fan of hiding ugliness in macros. Will =

fix.

Post by Dmitry Kasatkin

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

Post by Dmitry Kasatkin
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 whi=

ch=20

is explicitly forbidden by the C standard (C89, C99, C11). Because th=

e=20

vast majority of kernel developers I've talked to about this have bee=

n=20

unaware of the use of VLAIS in the kernel and most find its use=20
objectionable (there is a similar objection to the use of nested=20
functions). Because implementing VLAIS in a compiler can severely imp=

act=20

the generated instructions surrounding its use, which is why most=20
compilers don't implement VLAIS as a feature. Because using such a=20
construct precludes standards based compilers from competing with the=

=20

incumbent (my interest is enabling the use of clang and LLVM based=20
technologies as a toolchain choice to compile and develop the kernel)=

=2E

=20

Post by Dmitry Kasatkin
What about rest of the kernel?

The LLVMLinux project is systematically working to remove the use of=20
VLAIS from the kernel (already removed from ext4, USB Gadget, netfilt=

er,=20

mac802.11, apparmor, bluetooth, etc). Users of the crpyto subsystem a=

re=20

one of the last and heaviest users of VLAIS.
=20

Post by Dmitry Kasatkin
To solve your problem you probably need to change everything.

Essentially yes. Though I like to think of it as finding alternatives=

to=20

where ever it is still used. "Changing everything" implies much large=

r=20

changes which aren't necessary in most cases. Sometimes the alternati=

ve=20

is merely using a flexible member (zero length array at the end of th=

e=20

struct, instead of a VLA in the struct). In several places several VL=

As=20

are used in the same struct. And recently we found that exofs is usin=

g a=20

VLAIS inside VLAIS (second order VLAIS) in one of its structures. So =

not=20

finished yet.
=20

Post by Dmitry Kasatkin
If we are going to change it and introduce any macros, it is better=

to

Post by Dmitry Kasatkin
do with the guidance from crypto folks.

Absolutely. Most of the crypto related patches have been sent to them=

=2E I=20

am absolutely looking for their input.
=20

,

Post by Dmitry Kasatkin
crypto maintainer.

I suppose this specific patch may not have CC that list. However, mos=

t=20

of the other VLAIS removal patches were copied to linux-crypto, Herbe=

rt=20

Xu and David Miller.

Behan, thank you for the explanation. 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 b=
e
propogated.

Mimi


--
To unsubscribe from this list: send the line "unsubscribe linux-securit=
y-module" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Behan Webster]

Post by Mimi Zohar
Behan, thank you for the explanation.

No worries. I should have explained better. My apologies.

Post by Mimi Zohar
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