| Message ID | 20190724204204.25383-1-vt@altlinux.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima-evm-utils: Namespace some too generic function names | expand |
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 >
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)
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,
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
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)
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(-)