linux-ima-user — Q&A for users of IMA

[Linux-ima-user] [PATCH v3 02/12] btrfs: LLVMLinux: Remove VLAIS

[<be...@co...>]

From: Vinícius Tinti <vin...@gm...>

Replaced the use of a Variable Length Array In Struct (VLAIS) with a C99
compliant equivalent. This is the original VLAIS struct.

struct {
	struct shash_desc shash;
	char ctx[crypto_shash_descsize(tfm)];
} desc;

This patch instead allocates the appropriate amount of memory using a
char array using the SHASH_DESC_ON_STACK macro.

The new code can be compiled with both gcc and clang.

Signed-off-by: Vinícius Tinti <vin...@gm...>
Reviewed-by: Jan-Simon Möller <dl...@gm...>
Reviewed-by: Mark Charlebois <cha...@gm...>
Signed-off-by: Behan Webster <be...@co...>
Cc: "David S. Miller" <da...@da...>
Cc: Herbert Xu <he...@go...>
---
 fs/btrfs/hash.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/hash.c b/fs/btrfs/hash.c
index 85889aa..4bf4d3a 100644
--- a/fs/btrfs/hash.c
+++ b/fs/btrfs/hash.c
@@ -33,18 +33,16 @@ void btrfs_hash_exit(void)
 
 u32 btrfs_crc32c(u32 crc, const void *address, unsigned int length)
 {
-	struct {
-		struct shash_desc shash;
-		char ctx[crypto_shash_descsize(tfm)];
-	} desc;
+	SHASH_DESC_ON_STACK(shash, tfm);
+	u32 *ctx = (u32 *)shash_desc_ctx(shash);
 	int err;
 
-	desc.shash.tfm = tfm;
-	desc.shash.flags = 0;
-	*(u32 *)desc.ctx = crc;
+	shash->tfm = tfm;
+	shash->flags = 0;
+	*ctx = crc;
 
-	err = crypto_shash_update(&desc.shash, address, length);
+	err = crypto_shash_update(shash, address, length);
 	BUG_ON(err);
 
-	return *(u32 *)desc.ctx;
+	return *ctx;
 }
-- 
1.9.1

[Linux-ima-user] [PATCH v3 01/12] crypto: LLVMLinux: Add macro to remove use of VLAIS in crypto code

[<be...@co...>]

From: Behan Webster <be...@co...>

Add a macro which replaces the use of a Variable Length Array In Struct (VLAIS)
with a C99 compliant equivalent. This macro instead 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 <be...@co...>
---
 include/crypto/hash.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/crypto/hash.h b/include/crypto/hash.h
index a391955..541125b 100644
--- a/include/crypto/hash.h
+++ b/include/crypto/hash.h
@@ -58,6 +58,11 @@ struct shash_desc {
 	void *__ctx[] CRYPTO_MINALIGN_ATTR;
 };
 
+#define SHASH_DESC_ON_STACK(shash, tfm)				  \
+	char __desc[sizeof(struct shash_desc) +                   \
+		crypto_shash_descsize(tfm)] CRYPTO_MINALIGN_ATTR; \
+	struct shash_desc *shash = (struct shash_desc *)__desc
+
 struct shash_alg {
 	int (*init)(struct shash_desc *desc);
 	int (*update)(struct shash_desc *desc, const u8 *data,
-- 
1.9.1

[Linux-ima-user] [PATCH v3 00/12] LLVMLinux: Patches to enable the kernel to be compiled with clang/LLVM

[<be...@co...>]

From: Behan Webster <be...@co...>

These patches replace the use of Variable Length Arrays In Structs (VLAIS) in
crypto related code with C99 compliant equivalent code. A SHASH_DESC_ON_STACK()
macro is added to hash.h which is then used to replace the use of VLAIS in all
the other patches. The minimum size and alignment are maintained by the new code.

The new code can be compiled with both gcc and clang.

The LLVMLinux project aims to fully build the Linux kernel using both gcc and
clang (the C front end for the LLVM compiler infrastructure project). 

Behan Webster (6):
  crypto: LLVMLinux: Add macro to remove use of VLAIS in crypto code
  crypto: LLVMLinux: Remove VLAIS from crypto/mv_cesa.c
  crypto: LLVMLinux: Remove VLAIS from crypto/n2_core.c
  crypto: LLVMLinux: Remove VLAIS from crypto/omap_sham.c
  crypto: LLVMLinux: Remove VLAIS from crypto/.../qat_algs.c
  security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c

Jan-Simon Möller (5):
  crypto: LLVMLinux: Remove VLAIS from crypto/ccp/ccp-crypto-sha.c
  crypto, dm: LLVMLinux: Remove VLAIS usage from dm-crypt
  crypto: LLVMLinux: Remove VLAIS usage from crypto/hmac.c
  crypto: LLVMLinux: Remove VLAIS usage from libcrc32c.c
  crypto: LLVMLinux: Remove VLAIS usage from crypto/testmgr.c

Vinícius Tinti (1):
  btrfs: LLVMLinux: Remove VLAIS

 crypto/hmac.c                            | 25 +++++++---------
 crypto/testmgr.c                         | 14 ++++-----
 drivers/crypto/ccp/ccp-crypto-sha.c      | 13 ++++----
 drivers/crypto/mv_cesa.c                 | 41 +++++++++++--------------
 drivers/crypto/n2_core.c                 | 11 +++----
 drivers/crypto/omap-sham.c               | 28 +++++++-----------
 drivers/crypto/qat/qat_common/qat_algs.c | 31 +++++++++----------
 drivers/md/dm-crypt.c                    | 34 +++++++++------------
 fs/btrfs/hash.c                          | 16 +++++-----
 include/crypto/hash.h                    |  5 ++++
 lib/libcrc32c.c                          | 16 +++++-----
 security/integrity/ima/ima_crypto.c      | 51 ++++++++++++++------------------
 12 files changed, 126 insertions(+), 159 deletions(-)

-- 
1.9.1

Re: [Linux-ima-user] IMA/EVM

[Dmitry K. <d.k...@sa...>]

On 10/09/14 20:08, Petko Manolov wrote:
> On 14-09-10 08:52:28, Mimi Zohar wrote:
>> Enabling IMA-appraisal is anything but simple.  Keys need to be created and files need to be signed (eg. hash/signature).  To simplify the process will 
>> require packages to come with the file signatures included in the packages and installed with the package.  Fin Gunter posted the RPM patches.  I posted the 
>> debhelper patches.  That is as far as we've gotten.
> Yes, i already figured IMA-appraisal can be a tough nut to crack.  This is why i started over and tried to do it as simple as possible.  Created one RSA keypair 
> (as per the WIKI) and used it for both IMA and EVM signatures.
>
> The initramdisk i use is squashfs image.  When i run the kernel with 'ima_appraise_tcb' it just could not execute /sbin/init.  OK, luckily squashfs has got 
> extended attributes so i quickly hashed (find $RD -type f -exec evmctl ima_hash -a sha256 {} \;) its content and i finally got to the shell prompt.  I wonder 
> how this is handled with 'cpio' archives (or other FS images) without extended attribute support?
>
> The RSA public key is imported as follows:
>
> 	ima_id=`keyctl newring _ima @u`
> 	evmctl import --rsa /etc/keys/rsa_public.pem $ima_id
>
> 	evm_id=`keyctl newring _evm @u`
> 	evmctl import --rsa /etc/keys/rsa_public.pem $evm_id
>
> In attempt to keep my setup as simple as possible i used the private RSA key to sign another squashfs image, that is going to be my rootfs with:
>
> 	evmctl sign --imasig -r --rsa -k $RSA $ROOT
>
> Trying to switch_root into the new root filesystem results in "permission denied"...
>
> Another signing/hashing scheme:
>
> 	evmctl sign --imahash -u -a sha256 -r --rsa -k $RSA $ROOT

There 2 main things to understand..

1. Key & signature types.

Signature version 1: use '--rsa' for signing and import. Plain RSA key
is used.

Signature version 2: do not use '--rsa'. You need X509 certificate.

1. EVM version

You have to pay attention to the kernel configuration. It might be
configured to use HMAC version 2 which includes FS UUID.
If it does, you must not use '-u' parameter, because default is to use
FSUUID.




> however seems to do the trick.  Now i am able to switch_root and do things in the new root FS.  Why the first doesn't work while the former does is a bit of 
> mystery right now.  The key is the same, only 'ima' attribute is hash instead of signature.  I suspect i should enable IMA/EVM debug messages to get better idea 
> of what exactly is behind "permission denied".

Your kernel probably is using EVM HMAC v1.

- Dmitry

>
> Once i switch to the new root FS i did some tests to see if the system catches tampering attempts.  Since squashfs is read-only i could not alter its content so 
> i copied a few binaries to /tmp and /var, both of which are mounted RW.  /var is loop-mounted ext3 image, btw.
>
> I noticed that adding stuff at the end of a file (like: echo "1234" >> /var/kill) does not prevent the file from execution.  However:
>
> 	dd if=/dev/urandom of=/var/kill bs=1 seek=15000 count=1
>
> results in "permission denied", which i guess is the IMA-appraisal at work.  Modifying the same byte of the same binary, but in /tmp resulted in "Segmentation 
> fault", which seems to be a result of code corruption and not IMA-appraisal actions.  I assume both directories somehow have different policies, but i may be 
> wrong.
>
>
> I do apologize for the low quality of this post and the abundance of trivial information.  I'll try to summarize my questions:
>
> 	- how the kernel handles ima_appraise_tcb for the initramfs?
> 	- when exactly ima_appraise_tcb kicks in?  after loading the appropriate keys or...
> 	- why getting different results with --imasig and --imahash?
> 	- what policies should be put in place to prevent anything that has been changed from execution from any location?
> 	- what should i do to debug kernel's IMA/EVM code? (apart from the obvious ccflags-$(IMA_xxx) += -DDEBUG)
>
>
> thanks,
> Petko
>

Re: [Linux-ima-user] IMA/EVM

[Petko M. <pe...@nu...>]

On 14-09-10 08:52:28, Mimi Zohar wrote:
> 
> Enabling IMA-appraisal is anything but simple.  Keys need to be created and files need to be signed (eg. hash/signature).  To simplify the process will 
> require packages to come with the file signatures included in the packages and installed with the package.  Fin Gunter posted the RPM patches.  I posted the 
> debhelper patches.  That is as far as we've gotten.

Yes, i already figured IMA-appraisal can be a tough nut to crack.  This is why i started over and tried to do it as simple as possible.  Created one RSA keypair 
(as per the WIKI) and used it for both IMA and EVM signatures.

The initramdisk i use is squashfs image.  When i run the kernel with 'ima_appraise_tcb' it just could not execute /sbin/init.  OK, luckily squashfs has got 
extended attributes so i quickly hashed (find $RD -type f -exec evmctl ima_hash -a sha256 {} \;) its content and i finally got to the shell prompt.  I wonder 
how this is handled with 'cpio' archives (or other FS images) without extended attribute support?

The RSA public key is imported as follows:

	ima_id=`keyctl newring _ima @u`
	evmctl import --rsa /etc/keys/rsa_public.pem $ima_id

	evm_id=`keyctl newring _evm @u`
	evmctl import --rsa /etc/keys/rsa_public.pem $evm_id

In attempt to keep my setup as simple as possible i used the private RSA key to sign another squashfs image, that is going to be my rootfs with:

	evmctl sign --imasig -r --rsa -k $RSA $ROOT

Trying to switch_root into the new root filesystem results in "permission denied"...

Another signing/hashing scheme:

	evmctl sign --imahash -u -a sha256 -r --rsa -k $RSA $ROOT

however seems to do the trick.  Now i am able to switch_root and do things in the new root FS.  Why the first doesn't work while the former does is a bit of 
mystery right now.  The key is the same, only 'ima' attribute is hash instead of signature.  I suspect i should enable IMA/EVM debug messages to get better idea 
of what exactly is behind "permission denied".


Once i switch to the new root FS i did some tests to see if the system catches tampering attempts.  Since squashfs is read-only i could not alter its content so 
i copied a few binaries to /tmp and /var, both of which are mounted RW.  /var is loop-mounted ext3 image, btw.

I noticed that adding stuff at the end of a file (like: echo "1234" >> /var/kill) does not prevent the file from execution.  However:

	dd if=/dev/urandom of=/var/kill bs=1 seek=15000 count=1

results in "permission denied", which i guess is the IMA-appraisal at work.  Modifying the same byte of the same binary, but in /tmp resulted in "Segmentation 
fault", which seems to be a result of code corruption and not IMA-appraisal actions.  I assume both directories somehow have different policies, but i may be 
wrong.


I do apologize for the low quality of this post and the abundance of trivial information.  I'll try to summarize my questions:

	- how the kernel handles ima_appraise_tcb for the initramfs?
	- when exactly ima_appraise_tcb kicks in?  after loading the appropriate keys or...
	- why getting different results with --imasig and --imahash?
	- what policies should be put in place to prevent anything that has been changed from execution from any location?
	- what should i do to debug kernel's IMA/EVM code? (apart from the obvious ccflags-$(IMA_xxx) += -DDEBUG)


thanks,
Petko

Re: [Linux-ima-user] [PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c

[Behan W. <be...@co...>]

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
be...@co...

Re: [Linux-ima-user] [PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c

[Mimi Z. <zo...@li...>]

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 <be...@co...>
> >>>>>> Signed-off-by: Mark Charlebois <cha...@gm...>
> >>>>>> Signed-off-by: Jan-Simon Möller <dl...@gm...>
> >>>>> 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 one of its structures. So not 
> finished yet.
> 
> > If we are going to change it and introduce any macros, it is better to
> > do with the guidance from crypto folks.
> Absolutely. Most of the crypto related patches have been sent to them. I 
> am absolutely looking for their input.
> 
> > I added CC:lin...@vg... mailing list and Herbert Xu,
> > crypto maintainer.
> I suppose this specific patch may not have CC that list. However, most 
> of the other VLAIS removal patches were copied to linux-crypto, Herbert 
> 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 be
propogated.

Mimi

Re: [Linux-ima-user] [PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c

[Behan W. <be...@co...>]

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 <be...@co...>
>>>>>> Signed-off-by: Mark Charlebois <cha...@gm...>
>>>>>> Signed-off-by: Jan-Simon Möller <dl...@gm...>
>>>>> 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 one of its structures. So not 
finished yet.

> If we are going to change it and introduce any macros, it is better to
> do with the guidance from crypto folks.
Absolutely. Most of the crypto related patches have been sent to them. I 
am absolutely looking for their input.

> I added CC:lin...@vg... mailing list and Herbert Xu,
> crypto maintainer.
I suppose this specific patch may not have CC that list. However, most 
of the other VLAIS removal patches were copied to linux-crypto, Herbert 
Xu and David Miller.

Thanks,

Behan

-- 
Behan Webster
be...@co...

Re: [Linux-ima-user] [PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c

[Dmitry K. <d.k...@sa...>]

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 <be...@co...>
>>>>> Signed-off-by: Mark Charlebois <cha...@gm...>
>>>>> Signed-off-by: Jan-Simon Möller <dl...@gm...>
>>>> 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:lin...@vg... mailing list and Herbert Xu,
crypto maintainer.

- Dmitry

Re: [Linux-ima-user] [PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c

[Behan W. <be...@co...>]

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 <be...@co...>
>>>> Signed-off-by: Mark Charlebois <cha...@gm...>
>>>> Signed-off-by: Jan-Simon Möller <dl...@gm...>
>>> 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
be...@co...

Re: [Linux-ima-user] [PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c

[Thomas G. <tg...@li...>]

On Fri, 5 Sep 2014, Behan Webster wrote:
> On 09/05/14 17:18, Thomas Gleixner wrote:
> > > Signed-off-by: Behan Webster <be...@co...>
> > > Signed-off-by: Mark Charlebois <cha...@gm...>
> > > Signed-off-by: Jan-Simon Möller <dl...@gm...>
> > 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: [Linux-ima-user] [PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c

[Behan W. <be...@co...>]

On 09/05/14 17:18, Thomas Gleixner wrote:
> On Fri, 5 Sep 2014, be...@co... wrote:
>
>> From: Behan Webster <be...@co...>
>>
>> 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 <be...@co...>
>> Signed-off-by: Mark Charlebois <cha...@gm...>
>> Signed-off-by: Jan-Simon Möller <dl...@gm...>
> 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
be...@co...

Re: [Linux-ima-user] [PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c

[Thomas G. <tg...@li...>]

On Fri, 5 Sep 2014, be...@co... wrote:

> From: Behan Webster <be...@co...>
> 
> 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 <be...@co...>
> Signed-off-by: Mark Charlebois <cha...@gm...>
> Signed-off-by: Jan-Simon Möller <dl...@gm...>

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

[Linux-ima-user] [PATCH] security, crypto: LLVMLinux: Remove VLAIS from ima_crypto.c

[<be...@co...>]

From: Behan Webster <be...@co...>

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 <be...@co...>
Signed-off-by: Mark Charlebois <cha...@gm...>
Signed-off-by: Jan-Simon Möller <dl...@gm...>
---
 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 = (struct shash_desc *)desc;
 
-	desc.shash.tfm = tfm;
-	desc.shash.flags = 0;
+	shash->tfm = tfm;
+	shash->flags = 0;
 
-	rc = crypto_shash_init(&desc.shash);
+	rc = crypto_shash_init(shash);
 	if (rc != 0)
 		return rc;
 
@@ -581,10 +578,10 @@ static int __init ima_calc_boot_aggregate_tfm(char *digest,
 	for (i = TPM_PCR0; i < TPM_PCR8; i++) {
 		ima_pcrread(i, pcr_i);
 		/* now accumulate with current aggregate */
-		rc = crypto_shash_update(&desc.shash, pcr_i, TPM_DIGEST_SIZE);
+		rc = crypto_shash_update(shash, pcr_i, TPM_DIGEST_SIZE);
 	}
 	if (!rc)
-		crypto_shash_final(&desc.shash, digest);
+		crypto_shash_final(shash, digest);
 	return rc;
 }
 
-- 
1.9.1

Re: [Linux-ima-user] Possible to have 'fixed' PCR values

[Mimi Z. <zo...@li...>]

On Fri, 2014-08-01 at 04:36 -0700, TEH JIA YEW wrote:
> Good day. I had successfully extended (using TPM_Extend)  two entities
> in the following forms:
> 
> a) memory address in the form of hex i.e. 0x08049d49  and,
> b) contents of WORD at a memory address above i.e. 0xffffffff ,
> 
> into PCR-16  and 15. 
> 
> 
> I discovered that by running "$ cat /sys/class/misc/tpm0/device/pcrs
> " with both a) & b) as input, the PCR contents changes as expected,
> (based on how TPM_Extend works) , even if both a) and b) changes
> NOT . 

Before extending the TPM or adding the measurement to the measurement
list, IMA calls ima_lookup_digest_entry() to prevent duplicate
measurement values from being added.

> What I need is for the PCR-16 and 15 contents NOT to change if a) and
> b) remains constant when extended to PCR-16 and 15.

Either maintain a hash table of previous values (or just the current
value).  Check that the new value (or current value) doesn't already
exist before extending the PCR.

Mimi

> An example of what I require is similar to the contents of PCR 0-7,
> whereby the contents are fixed for a fixed system configuration, i.e.
> as per below.
> 
> 
> PCR-00: 20 57 44 36 7B C6 06 BE D4 F4 69 1A DE F7 0F D9 C7 21 B0 0D 
> PCR-01: A8 9F B8 F8 8C AA 95 90 E6 12 9B 63 3B 14 4A 68 51 44 90 D5 
> PCR-02: A8 9F B8 F8 8C AA 95 90 E6 12 9B 63 3B 14 4A 68 51 44 90 D5 
> PCR-03: A8 9F B8 F8 8C AA 95 90 E6 12 9B 63 3B 14 4A 68 51 44 90 D5 
> PCR-04: AF 83 CE A3 2B 1F 7F FB F7 FA BD FC 27 68 72 45 69 1F 31 CB 
> PCR-05: 0C 4C A3 E3 3A 8B 34 A2 39 9B AF 0C 96 69 56 39 80 80 02 1F 
> PCR-06: A8 9F B8 F8 8C AA 95 90 E6 12 9B 63 3B 14 4A 68 51 44 90 D5 
> PCR-07: A8 9F B8 F8 8C AA 95 90 E6 12 9B 63 3B 14 4A 68 51 44 90 D5 
> 
> 
> I had read the paper : "Design & Implemenation of TCG based IMA" and
> "http://sourceforge.net/p/linux-ima/wiki/Home/" and have looked into
> vanilla kernel sources under 'ima' but have no idea how to implement
> what I seek or where to start off.
> 
> 
> Aprreciate if someone could point out where I can start or point out
> the code segment for implementing a fixed PCR 0-7 contents?  Thanks in
> advance.    
> 
>   
> Rgds
> jyteh
> ------------------------------------------------------------------------------
> Want fast and easy access to all the code in your enterprise? Index and
> search up to 200,000 lines of code with a free copy of Black Duck
> Code Sight - the same software that powers the world's largest code
> search on Ohloh, the Black Duck Open Hub! Try it now.
> http://p.sf.net/sfu/bds
> _______________________________________________
> Linux-ima-user mailing list
> Lin...@li...
> https://lists.sourceforge.net/lists/listinfo/linux-ima-user

[Linux-ima-user] Possible to have 'fixed' PCR values

[TEH J. Y. <jy...@ya...>]

Good day. I had successfully extended (using TPM_Extend)  two entities in the following forms:

a) memory address in the form of hex i.e. 0x08049d49  and,
b) contents of WORD at a memory address above i.e. 0xffffffff ,

into PCR-16  and 15. 


I discovered that by running "$ cat /sys/class/misc/tpm0/device/pcrs " with both a) & b) as input, the PCR contents changes as expected, (based on how TPM_Extend works) , even if both a) and b) changes NOT . 


What I need is for the PCR-16 and 15 contents NOT to change if a) and b) remains constant when extended to PCR-16 and 15.

An example of what I require is similar to the contents of PCR 0-7, whereby the contents are fixed for a fixed system configuration, i.e. as per below.


PCR-00: 20 57 44 36 7B C6 06 BE D4 F4 69 1A DE F7 0F D9 C7 21 B0 0D 
PCR-01: A8 9F B8 F8 8C AA 95 90 E6 12 9B 63 3B 14 4A 68 51 44 90 D5 
PCR-02: A8 9F B8 F8 8C AA 95 90 E6 12 9B 63 3B 14 4A 68 51 44 90 D5 
PCR-03: A8 9F B8 F8 8C AA 95 90 E6 12 9B 63 3B 14 4A 68 51 44 90 D5 
PCR-04: AF 83 CE A3 2B 1F 7F FB F7 FA BD FC 27 68 72 45 69 1F 31 CB 
PCR-05: 0C 4C A3 E3 3A 8B 34 A2 39 9B AF 0C 96 69 56 39 80 80 02 1F 
PCR-06: A8 9F B8 F8 8C AA 95 90 E6 12 9B 63 3B 14 4A 68 51 44 90 D5 
PCR-07: A8 9F B8 F8 8C AA 95 90 E6 12 9B 63 3B 14 4A 68 51 44 90 D5 


I had read the paper : "Design & Implemenation of TCG based IMA" and "http://sourceforge.net/p/linux-ima/wiki/Home/" and have looked into vanilla kernel sources under 'ima' but have no idea how to implement what I seek or where to start off.


Aprreciate if someone could point out where I can start or point out the code segment for implementing a fixed PCR 0-7 contents?  Thanks in advance.    

 
Rgds
jyteh

Re: [Linux-ima-user] Measuring lib and executables only ....

[Andreas S. <and...@st...>]

Hi,

if you are using an initramfs then the following HOWTO also tells
you how to set up a specific IMA policy.

https://wiki.strongswan.org/projects/strongswan/wiki/IMA#Configure-the-IMA-Policy

Regards

Andreas

On 15.07.2014 05:49, M.AMJAD wrote:
>
> 1. Is there any option in configuration file to measure only binaries
> and executable permanently(name of that configuration file plz)?
> 2. How to get SMLs in the following format
> #000 :D6DC07881A7EFDS8EB8E9186CC72F452AS4D3DB boot_aggregate
> #001 :CDSS4B285123353BDA1794D9AB4BD69B2F74D73  linuxrc
> #002 :9F860256709F1CD35037563DCDF798054F978705 nash
> #003 :7DF33561E2A467AE7CDD4BBEF6880517D3CAECBlibc-2.3.2.so
> ETC
> M.AMJAD
> +92-345-9208119

======================================================================
Andreas Steffen                         and...@st...
strongSwan - the Open Source VPN Solution!          www.strongswan.org
Institute for Internet Technologies and Applications
University of Applied Sciences Rapperswil
CH-8640 Rapperswil (Switzerland)
===========================================================[ITA-HSR]==

Re: [Linux-ima-user] Measuring lib and executables only ....

[Mimi Z. <zo...@li...>]

On Mon, 2014-07-14 at 20:49 -0700, M.AMJAD wrote: 
> 
> 1. Is there any option in configuration file to measure only
> binaries and executable permanently(name of that configuration file plz)?
> 2. How to get SMLs in the following format 
>  #000 :   D6DC07881A7EFDS8EB8E9186CC72F452AS4D3DB
> boot_aggregate
>  #001 :   CDSS4B285123353BDA1794D9AB4BD69B2F74D73  linuxrc
>  #002 :   9F860256709F1CD35037563DCDF798054F978705  nash  
>  #003 :   7DF33561E2A467AE7CDD4BBEF6880517D3CAECB  libc-2.3.2.so
> ETC

There isn't a builtin policy, but you can load a custom policy.
Probably only the bprm and mmap hooks are needed.

measure func=BPRM_CHECK
measure func=MMAP_CHECK

Refer to Documentation/ABI/testing/ima_policy.

Mimi

Re: [Linux-ima-user] Measuring lib and executables only ....

[M.AMJAD <am...@ya...>]

1. Is there any option in configuration file to measure only
binaries and executable permanently(name of that configuration file plz)?
2. How to get SMLs in the following format 
 #000 :   D6DC07881A7EFDS8EB8E9186CC72F452AS4D3DB
boot_aggregate
 #001 :   CDSS4B285123353BDA1794D9AB4BD69B2F74D73  linuxrc
 #002 :   9F860256709F1CD35037563DCDF798054F978705  nash  
 #003 :   7DF33561E2A467AE7CDD4BBEF6880517D3CAECB  libc-2.3.2.so
ETC
 
M.AMJAD
+92-345-9208119




On Saturday, 12 July 2014, 3:40, Dmitry Kasatkin <dmi...@gm...> wrote:
 


You can use grep to search for your binaries from measurement list...


On 11 July 2014 08:57, M.AMJAD <am...@ya...> wrote:
> Hi,
> How can I take only lib and executables hashes in ascii_runtime_measurement
> (Don’t need other files or data),
> Thanks
>
> M.AMJAD
>
>
> ------------------------------------------------------------------------------
> Open source business process management suite built on Java and Eclipse
> Turn processes into business applications with Bonita BPM Community Edition
> Quickly connect people, data, and systems into organized workflows
> Winner of BOSSIE, CODIE, OW2 and Gartner awards
> http://p.sf.net/sfu/Bonitasoft
> _______________________________________________
> Linux-ima-user mailing list
> Lin...@li...
> https://lists.sourceforge.net/lists/listinfo/linux-ima-user

>



-- 
Thanks,
Dmitry

Re: [Linux-ima-user] Measuring lib and executables only ....

[Dmitry K. <dmi...@gm...>]

You can use grep to search for your binaries from measurement list...


On 11 July 2014 08:57, M.AMJAD <am...@ya...> wrote:
> Hi,
> How can I take only lib and executables hashes in ascii_runtime_measurement
> (Don’t need other files or data),
> Thanks
>
> M.AMJAD
>
>
> ------------------------------------------------------------------------------
> Open source business process management suite built on Java and Eclipse
> Turn processes into business applications with Bonita BPM Community Edition
> Quickly connect people, data, and systems into organized workflows
> Winner of BOSSIE, CODIE, OW2 and Gartner awards
> http://p.sf.net/sfu/Bonitasoft
> _______________________________________________
> Linux-ima-user mailing list
> Lin...@li...
> https://lists.sourceforge.net/lists/listinfo/linux-ima-user
>



-- 
Thanks,
Dmitry

[Linux-ima-user] Measuring lib and executables only ....

[M.AMJAD <am...@ya...>]

Hi,

How can I take only lib and executables hashes in
ascii_runtime_measurement (Don’t need other files or data),
Thanks
 
M.AMJAD

Re: [Linux-ima-user] storing SML (Hashes) of kernel

[Mimi Z. <zo...@li...>]

On Wed, 2014-06-25 at 21:30 -0700, M.AMJAD wrote:
>           i just want to measure the kernel (find PCR values) and then
> compare with other SML (already stored with me), to check the
> integrity of Operating System (Kernel).

The kernel itself should be measured as part of trusted boot.  This
mailing list is for discussion of the linux-integrity subsystem.   For
more information about the linux-integrity subsystem, please refer to
the linux-ima wiki http://sourceforge.net/p/linux-ima/wiki/Home/.

thanks,

Mimi

Re: [Linux-ima-user] storing SML (Hashes) of kernel

[M.AMJAD <am...@ya...>]

Hello, thanks for response.
          i just want to measure the kernel (find PCR values) and then compare with other SML (already stored with me), to check the integrity of Operating System (Kernel).

 
M.AMJAD
+92-345-9208119




On Wednesday, 25 June 2014, 18:32, Dmitry Kasatkin <d.k...@sa...> wrote:
 


Hi,

I have couple of questions.

1) what is SML in your context?
2) what does it mean to get hashes "via TPM"?
3) What kind of use of those hashes or use-case you have in mind?

- Dmitry



On 25/06/14 12:01, M.AMJAD wrote:
> Dear All,
>            how can i get SML or hashes of a centOS kernel via TPM and
> store in a location for future use ? please.
>  
> M.AMJAD
> Please consider the environment before printing
>
>
> ------------------------------------------------------------------------------
> Open source business process management suite built on Java and Eclipse
> Turn processes into business applications with Bonita BPM Community Edition
> Quickly connect people, data, and systems into organized workflows
> Winner of BOSSIE, CODIE, OW2 and Gartner awards
> http://p.sf.net/sfu/Bonitasoft
>
>
> _______________________________________________
> Linux-ima-user mailing list
> Lin...@li...
> https://lists.sourceforge.net/lists/listinfo/linux-ima-user

Re: [Linux-ima-user] storing SML (Hashes) of kernel

[Dmitry K. <d.k...@sa...>]

Hi,

I have couple of questions.

1) what is SML in your context?
2) what does it mean to get hashes "via TPM"?
3) What kind of use of those hashes or use-case you have in mind?

- Dmitry


On 25/06/14 12:01, M.AMJAD wrote:
> Dear All,
>            how can i get SML or hashes of a centOS kernel via TPM and
> store in a location for future use ? please.
>  
> M.AMJAD
> Please consider the environment before printing
>
>
> ------------------------------------------------------------------------------
> Open source business process management suite built on Java and Eclipse
> Turn processes into business applications with Bonita BPM Community Edition
> Quickly connect people, data, and systems into organized workflows
> Winner of BOSSIE, CODIE, OW2 and Gartner awards
> http://p.sf.net/sfu/Bonitasoft
>
>
> _______________________________________________
> Linux-ima-user mailing list
> Lin...@li...
> https://lists.sourceforge.net/lists/listinfo/linux-ima-user

[Linux-ima-user] storing SML (Hashes) of kernel

[M.AMJAD <am...@ya...>]

Dear All,
           how can i get SML or hashes of a centOS kernel via TPM and store in a location for future use ? please.


 
M.AMJAD