[PATCH] ima-evm-utils: Namespace some too generic function names

[PATCH] ima-evm-utils: Namespace some too generic function names

[Vitaly Chikunov]

Prefix `dump', `do_dump', and `params' with `ima_' to avoid colliding
with other global symbols.

`params' is prefixed with a #define trick to avoid change in half
hundred places.

Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
---

I think all other exported functions (good example is verify_hash) should be
prefixed too.

 src/evmctl.c    | 6 +++---
 src/imaevm.h    | 9 ++++++---
 src/libimaevm.c | 6 +++---
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index 3289061..b2e5af5 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -565,7 +565,7 @@ static int sign_evm(const char *file, const char *key)
 		sig[1] = 3; /* immutable signature version */
 
 	if (sigdump || params.verbose >= LOG_INFO)
-		dump(sig, len);
+		ima_dump(sig, len);
 
 	if (xattr) {
 		err = lsetxattr(file, xattr_evm, sig, len, 0);
@@ -604,7 +604,7 @@ static int hash_ima(const char *file)
 		log_info("hash: ");
 
 	if (sigdump || params.verbose >= LOG_INFO)
-		dump(hash, len);
+		ima_dump(hash, len);
 
 	if (xattr) {
 		err = lsetxattr(file, xattr_ima, hash, len, 0);
@@ -638,7 +638,7 @@ static int sign_ima(const char *file, const char *key)
 	sig[0] = EVM_IMA_XATTR_DIGSIG;
 
 	if (sigdump || params.verbose >= LOG_INFO)
-		dump(sig, len);
+		ima_dump(sig, len);
 
 	if (sigfile)
 		bin2file(file, "sig", sig, len);
diff --git a/src/imaevm.h b/src/imaevm.h
index 0414433..d00922c 100644
--- a/src/imaevm.h
+++ b/src/imaevm.h
@@ -49,9 +49,12 @@
 
 #include <openssl/rsa.h>
 
+/* Namespace some internal symbols */
+#define params		ima_params
+
 #ifdef USE_FPRINTF
 #define do_log(level, fmt, args...)	({ if (level <= params.verbose) fprintf(stderr, fmt, ##args); })
-#define do_log_dump(level, p, len, cr)	({ if (level <= params.verbose) do_dump(stderr, p, len, cr); })
+#define do_log_dump(level, p, len, cr)	({ if (level <= params.verbose) ima_do_dump(stderr, p, len, cr); })
 #else
 #define do_log(level, fmt, args...)	syslog(level, fmt, ##args)
 #define do_log_dump(level, p, len, cr)
@@ -206,8 +209,8 @@ struct RSA_ASN1_template {
 
 extern struct libevm_params params;
 
-void do_dump(FILE *fp, const void *ptr, int len, bool cr);
-void dump(const void *ptr, int len);
+void ima_do_dump(FILE *fp, const void *ptr, int len, bool cr);
+void ima_dump(const void *ptr, int len);
 int ima_calc_hash(const char *file, uint8_t *hash);
 int get_hash_algo(const char *algo);
 RSA *read_pub_key(const char *keyfile, int x509);
diff --git a/src/libimaevm.c b/src/libimaevm.c
index 2d99570..afa978f 100644
--- a/src/libimaevm.c
+++ b/src/libimaevm.c
@@ -89,7 +89,7 @@ struct libevm_params params = {
 
 static void __attribute__ ((constructor)) libinit(void);
 
-void do_dump(FILE *fp, const void *ptr, int len, bool cr)
+void ima_do_dump(FILE *fp, const void *ptr, int len, bool cr)
 {
 	int i;
 	uint8_t *data = (uint8_t *) ptr;
@@ -100,9 +100,9 @@ void do_dump(FILE *fp, const void *ptr, int len, bool cr)
 		fprintf(fp, "\n");
 }
 
-void dump(const void *ptr, int len)
+void ima_dump(const void *ptr, int len)
 {
-	do_dump(stdout, ptr, len, true);
+	ima_do_dump(stdout, ptr, len, true);
 }
 
 const char *get_hash_algo_by_id(int algo)
-- 
2.11.0

Re: [PATCH] ima-evm-utils: Namespace some too generic function names

[Bruno E. O. Meneguele]

[-- Attachment #1: Type: text/plain, Size: 3995 bytes --]

Hi Vitaly,

On Wed, Jul 24, 2019 at 11:42:04PM +0300, Vitaly Chikunov wrote:
> Prefix `dump', `do_dump', and `params' with `ima_' to avoid colliding
> with other global symbols.
> 
> `params' is prefixed with a #define trick to avoid change in half
> hundred places.
> 

I understand your point and as a short-term fix I'm fine with that, but
IMO it would be better to actually rename the object in near future,
mainly to avoid possible confusions.

Perhaps we can use Mimi's suggestions of a patch to standardize all
return codes and also rework the naming with some convention.

Just thoughts... :)

> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>

Anyway.. thanks Vitaly!

Reviewed-by: Bruno E. O. Meneguele <bmeneg@redhat.com>
> ---
> 
> I think all other exported functions (good example is verify_hash) should be
> prefixed too.
> 
>  src/evmctl.c    | 6 +++---
>  src/imaevm.h    | 9 ++++++---
>  src/libimaevm.c | 6 +++---
>  3 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/src/evmctl.c b/src/evmctl.c
> index 3289061..b2e5af5 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -565,7 +565,7 @@ static int sign_evm(const char *file, const char *key)
>  		sig[1] = 3; /* immutable signature version */
>  
>  	if (sigdump || params.verbose >= LOG_INFO)
> -		dump(sig, len);
> +		ima_dump(sig, len);
>  
>  	if (xattr) {
>  		err = lsetxattr(file, xattr_evm, sig, len, 0);
> @@ -604,7 +604,7 @@ static int hash_ima(const char *file)
>  		log_info("hash: ");
>  
>  	if (sigdump || params.verbose >= LOG_INFO)
> -		dump(hash, len);
> +		ima_dump(hash, len);
>  
>  	if (xattr) {
>  		err = lsetxattr(file, xattr_ima, hash, len, 0);
> @@ -638,7 +638,7 @@ static int sign_ima(const char *file, const char *key)
>  	sig[0] = EVM_IMA_XATTR_DIGSIG;
>  
>  	if (sigdump || params.verbose >= LOG_INFO)
> -		dump(sig, len);
> +		ima_dump(sig, len);
>  
>  	if (sigfile)
>  		bin2file(file, "sig", sig, len);
> diff --git a/src/imaevm.h b/src/imaevm.h
> index 0414433..d00922c 100644
> --- a/src/imaevm.h
> +++ b/src/imaevm.h
> @@ -49,9 +49,12 @@
>  
>  #include <openssl/rsa.h>
>  
> +/* Namespace some internal symbols */
> +#define params		ima_params
> +
>  #ifdef USE_FPRINTF
>  #define do_log(level, fmt, args...)	({ if (level <= params.verbose) fprintf(stderr, fmt, ##args); })
> -#define do_log_dump(level, p, len, cr)	({ if (level <= params.verbose) do_dump(stderr, p, len, cr); })
> +#define do_log_dump(level, p, len, cr)	({ if (level <= params.verbose) ima_do_dump(stderr, p, len, cr); })
>  #else
>  #define do_log(level, fmt, args...)	syslog(level, fmt, ##args)
>  #define do_log_dump(level, p, len, cr)
> @@ -206,8 +209,8 @@ struct RSA_ASN1_template {
>  
>  extern struct libevm_params params;
>  
> -void do_dump(FILE *fp, const void *ptr, int len, bool cr);
> -void dump(const void *ptr, int len);
> +void ima_do_dump(FILE *fp, const void *ptr, int len, bool cr);
> +void ima_dump(const void *ptr, int len);
>  int ima_calc_hash(const char *file, uint8_t *hash);
>  int get_hash_algo(const char *algo);
>  RSA *read_pub_key(const char *keyfile, int x509);
> diff --git a/src/libimaevm.c b/src/libimaevm.c
> index 2d99570..afa978f 100644
> --- a/src/libimaevm.c
> +++ b/src/libimaevm.c
> @@ -89,7 +89,7 @@ struct libevm_params params = {
>  
>  static void __attribute__ ((constructor)) libinit(void);
>  
> -void do_dump(FILE *fp, const void *ptr, int len, bool cr)
> +void ima_do_dump(FILE *fp, const void *ptr, int len, bool cr)
>  {
>  	int i;
>  	uint8_t *data = (uint8_t *) ptr;
> @@ -100,9 +100,9 @@ void do_dump(FILE *fp, const void *ptr, int len, bool cr)
>  		fprintf(fp, "\n");
>  }
>  
> -void dump(const void *ptr, int len)
> +void ima_dump(const void *ptr, int len)
>  {
> -	do_dump(stdout, ptr, len, true);
> +	ima_do_dump(stdout, ptr, len, true);
>  }
>  
>  const char *get_hash_algo_by_id(int algo)
> -- 
> 2.11.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ima-evm-utils: Namespace some too generic function names

[Mimi Zohar]

On Wed, 2019-07-24 at 23:42 +0300, Vitaly Chikunov wrote:
> Prefix `dump', `do_dump', and `params' with `ima_' to avoid colliding
> with other global symbols.

The package is named ima-evm-utils, the tool is named evmctl, and now
we're prefixing the global symbols with "ima".  Some of the functions,
like dump(), are used by both "ima" and "evm".  Aiming for some sort
of consistency, maybe it should be prefixed with "ima_evm", not just
"ima_"? 

dump() should never have been named just "dump".  It should have at
least been named "hexdump".
 
> 
> `params' is prefixed with a #define trick to avoid change in half
> hundred places.

Perhaps separate this change from the other change?

thanks,

Mimi
  
> 
> Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
> ---
> 
> I think all other exported functions (good example is verify_hash) should be
> prefixed too.
> 
>  src/evmctl.c    | 6 +++---
>  src/imaevm.h    | 9 ++++++---
>  src/libimaevm.c | 6 +++---
>  3 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/src/evmctl.c b/src/evmctl.c
> index 3289061..b2e5af5 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -565,7 +565,7 @@ static int sign_evm(const char *file, const char *key)
>  		sig[1] = 3; /* immutable signature version */
> 
>  	if (sigdump || params.verbose >= LOG_INFO)
> -		dump(sig, len);
> +		ima_dump(sig, len);
> 
>  	if (xattr) {
>  		err = lsetxattr(file, xattr_evm, sig, len, 0);
> @@ -604,7 +604,7 @@ static int hash_ima(const char *file)
>  		log_info("hash: ");
> 
>  	if (sigdump || params.verbose >= LOG_INFO)
> -		dump(hash, len);
> +		ima_dump(hash, len);
> 
>  	if (xattr) {
>  		err = lsetxattr(file, xattr_ima, hash, len, 0);
> @@ -638,7 +638,7 @@ static int sign_ima(const char *file, const char *key)
>  	sig[0] = EVM_IMA_XATTR_DIGSIG;
> 
>  	if (sigdump || params.verbose >= LOG_INFO)
> -		dump(sig, len);
> +		ima_dump(sig, len);
> 
>  	if (sigfile)
>  		bin2file(file, "sig", sig, len);
> diff --git a/src/imaevm.h b/src/imaevm.h
> index 0414433..d00922c 100644
> --- a/src/imaevm.h
> +++ b/src/imaevm.h
> @@ -49,9 +49,12 @@
> 
>  #include <openssl/rsa.h>
> 
> +/* Namespace some internal symbols */
> +#define params		ima_params
> +
>  #ifdef USE_FPRINTF
>  #define do_log(level, fmt, args...)	({ if (level <= params.verbose) fprintf(stderr, fmt, ##args); })
> -#define do_log_dump(level, p, len, cr)	({ if (level <= params.verbose) do_dump(stderr, p, len, cr); })
> +#define do_log_dump(level, p, len, cr)	({ if (level <= params.verbose) ima_do_dump(stderr, p, len, cr); })
>  #else
>  #define do_log(level, fmt, args...)	syslog(level, fmt, ##args)
>  #define do_log_dump(level, p, len, cr)
> @@ -206,8 +209,8 @@ struct RSA_ASN1_template {
> 
>  extern struct libevm_params params;
> 
> -void do_dump(FILE *fp, const void *ptr, int len, bool cr);
> -void dump(const void *ptr, int len);
> +void ima_do_dump(FILE *fp, const void *ptr, int len, bool cr);
> +void ima_dump(const void *ptr, int len);
>  int ima_calc_hash(const char *file, uint8_t *hash);
>  int get_hash_algo(const char *algo);
>  RSA *read_pub_key(const char *keyfile, int x509);
> diff --git a/src/libimaevm.c b/src/libimaevm.c
> index 2d99570..afa978f 100644
> --- a/src/libimaevm.c
> +++ b/src/libimaevm.c
> @@ -89,7 +89,7 @@ struct libevm_params params = {
> 
>  static void __attribute__ ((constructor)) libinit(void);
> 
> -void do_dump(FILE *fp, const void *ptr, int len, bool cr)
> +void ima_do_dump(FILE *fp, const void *ptr, int len, bool cr)
>  {
>  	int i;
>  	uint8_t *data = (uint8_t *) ptr;
> @@ -100,9 +100,9 @@ void do_dump(FILE *fp, const void *ptr, int len, bool cr)
>  		fprintf(fp, "\n");
>  }
> 
> -void dump(const void *ptr, int len)
> +void ima_dump(const void *ptr, int len)
>  {
> -	do_dump(stdout, ptr, len, true);
> +	ima_do_dump(stdout, ptr, len, true);
>  }
> 
>  const char *get_hash_algo_by_id(int algo)

Re: [PATCH] ima-evm-utils: Namespace some too generic function names

[Vitaly Chikunov]

Mimi,

On Wed, Jul 24, 2019 at 07:24:20PM -0400, Mimi Zohar wrote:
> On Wed, 2019-07-24 at 23:42 +0300, Vitaly Chikunov wrote:
> > Prefix `dump', `do_dump', and `params' with `ima_' to avoid colliding
> > with other global symbols.
> 
> The package is named ima-evm-utils, the tool is named evmctl, and now
> we're prefixing the global symbols with "ima".  Some of the functions,
> like dump(), are used by both "ima" and "evm".  Aiming for some sort
> of consistency, maybe it should be prefixed with "ima_evm", not just
> "ima_"? 

Just ima_ is OK with me. EVM could be thought as IMA extension.
Or we can use evm_ like in evmctl. Or imaevm_ (without underscore, like
in libimaevm or imaevm.h).

> dump() should never have been named just "dump".  It should have at
> least been named "hexdump".
>  
> > `params' is prefixed with a #define trick to avoid change in half
> > hundred places.
> 
> Perhaps separate this change from the other change?

I agree to Bruno E. O. Meneguele it's better to actually rename `params'
like other functions instead of redefining. Then all renames can go in
one commit?

Thanks,

Re: [PATCH] ima-evm-utils: Namespace some too generic function names

[Mimi Zohar]

On Thu, 2019-07-25 at 04:53 +0300, Vitaly Chikunov wrote:
> Mimi,
> 
> On Wed, Jul 24, 2019 at 07:24:20PM -0400, Mimi Zohar wrote:
> > On Wed, 2019-07-24 at 23:42 +0300, Vitaly Chikunov wrote:
> > > Prefix `dump', `do_dump', and `params' with `ima_' to avoid colliding
> > > with other global symbols.
> > 
> > The package is named ima-evm-utils, the tool is named evmctl, and now
> > we're prefixing the global symbols with "ima".  Some of the functions,
> > like dump(), are used by both "ima" and "evm".  Aiming for some sort
> > of consistency, maybe it should be prefixed with "ima_evm", not just
> > "ima_"? 
> 
> Just ima_ is OK with me. EVM could be thought as IMA extension.

At least in the kernel, I've tried really hard to keep them as two
independent subsystems.  Does it make sense to use EVM without IMA,
probably not.  The EVM design allows for other subsystems, not only
IMA, to verify the file metdata integrity.  In fact, I heard about
some plans, relatively recently, to do so.

> Or we can use evm_ like in evmctl. Or imaevm_ (without underscore, like
> in libimaevm or imaevm.h).

There's already a lot of confusion as to what is "IMA".  Not only can
IMA be configured to store measurements, but can also be configured to
verify file signatures/hashes and audit file hashes.  Not that anyone
is looking at the naming details in this code, but I don't think we
should add to the confusion.  Could we use "imaevm_" as you suggested?

struct libimaevm_params {
        int verbose;
        int x509;
        const char *hash_algo;
        const char *keyfile;
        const char *keypass;
};

extern struct libimaevm_params ima_params;

imaevm_params?

> 
> > dump() should never have been named just "dump".  It should have at
> > least been named "hexdump".
> >  
> > > `params' is prefixed with a #define trick to avoid change in half
> > > hundred places.
> > 
> > Perhaps separate this change from the other change?
> 
> I agree to Bruno E. O. Meneguele it's better to actually rename `params'
> like other functions instead of redefining. Then all renames can go in
> one commit?

Sure.

"get_hash_algo()" can't be made static as it is being called from
hash_ima().  Could you also include renaming "get_hash_algo()" as
well?

Thanks!

Mimi