| Message ID | 20221103183904.103562-3-zohar@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | address deprecated warnings | expand |
Mimi, On Thu, Nov 03, 2022 at 02:38:49PM -0400, Mimi Zohar wrote: > Define a log_errno_reset macro to emit the errno string at or near the > time of error, similar to the existing log_errno macro, but also reset > errno to avoid dangling or duplicate errno messages on exit. > > The initial usage is for non-critical file open failures. > > Suggested-by: Vitaly Chikunov <vt@altlinux.org> > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> > --- > src/evmctl.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) Reviewed-by: Vitaly Chikunov <vt@altlinux.org> > > diff --git a/src/evmctl.c b/src/evmctl.c > index 0412bc0ac2b0..54123bf20f03 100644 > --- a/src/evmctl.c > +++ b/src/evmctl.c > @@ -166,6 +166,9 @@ struct tpm_bank_info { > static char *pcrfile[MAX_PCRFILE]; > static unsigned npcrfile; > > +#define log_errno_reset(level, fmt, args...) \ > + {do_log(level, fmt " (errno: %s)\n", ##args, strerror(errno)); errno = 0; } > + > static int bin2file(const char *file, const char *ext, const unsigned char *data, int len) > { > FILE *fp; > @@ -1911,8 +1914,10 @@ static int read_sysfs_pcrs(int num_banks, struct tpm_bank_info *tpm_banks) > fp = fopen(pcrs, "r"); > if (!fp) > fp = fopen(misc_pcrs, "r"); > - if (!fp) > + if (!fp) { > + log_errno_reset(LOG_DEBUG, "Failed to read TPM 1.2 PCRs"); > return -1; > + } > > result = read_one_bank(&tpm_banks[0], fp); > fclose(fp); > @@ -2055,7 +2060,6 @@ static int ima_measurement(const char *file) > int err_padded = -1; > int err = -1; > > - errno = 0; > memset(zero, 0, MAX_DIGEST_SIZE); > > pseudo_padded_banks = init_tpm_banks(&num_banks); > @@ -2072,6 +2076,8 @@ static int ima_measurement(const char *file) > init_public_keys(imaevm_params.keyfile); > else /* assume read pubkey from x509 cert */ > init_public_keys("/etc/keys/x509_evm.der"); > + if (errno) > + log_errno_reset(LOG_DEBUG, "Failed to initialize public keys"); Library prints appropriate error messages, so this is perhaps just to clear errno. But it's not necessarily completely failed, but maybe failure in one key. So I would say "Failure in initializing public keys" to be precise. ps. BTW, init_public_keys API call cannot return error except by errno, but it does not set it consistently so some errors may be missed. init_public_keys loops calling read_pub_pkey entry->key = read_pub_pkey(keyfile, 1); if (!entry->key) { free(entry); continue; } and read_pub_pkey have such code: if (!keyfile) return NULL; In that case some key is not read but we don't get any error notification. I think it's legal, by the right of being library, so set `errno = EINVAL` there somewhere. But, I'm not sure where - as we should not clobber existing errno values. Perhaps, errno setting should be added to libimaevm consistently to all functions, but this is huge task, so I would not suggest to do it now. Just suggestion for the future developments, maybe. Thanks, > > /* > * Reading the PCRs before walking the IMA measurement list > @@ -2746,6 +2752,8 @@ int main(int argc, char *argv[]) > unsigned long keyid; > char *eptr; > > + errno = 0; /* initialize global errno */ > + > #if !(OPENSSL_VERSION_NUMBER < 0x10100000) > OPENSSL_init_crypto( > #ifndef DISABLE_OPENSSL_CONF > -- > 2.31.1
On Fri, Nov 04, 2022 at 01:05:31AM +0300, Vitaly Chikunov wrote: > Mimi, > > On Thu, Nov 03, 2022 at 02:38:49PM -0400, Mimi Zohar wrote: > > Define a log_errno_reset macro to emit the errno string at or near the > > time of error, similar to the existing log_errno macro, but also reset > > errno to avoid dangling or duplicate errno messages on exit. > > > > The initial usage is for non-critical file open failures. > > > > Suggested-by: Vitaly Chikunov <vt@altlinux.org> > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> > > --- > > src/evmctl.c | 12 ++++++++++-- > > 1 file changed, 10 insertions(+), 2 deletions(-) > > Reviewed-by: Vitaly Chikunov <vt@altlinux.org> > > > > > diff --git a/src/evmctl.c b/src/evmctl.c > > index 0412bc0ac2b0..54123bf20f03 100644 > > --- a/src/evmctl.c > > +++ b/src/evmctl.c > > @@ -2055,7 +2060,6 @@ static int ima_measurement(const char *file) > > int err_padded = -1; > > int err = -1; > > > > - errno = 0; > > memset(zero, 0, MAX_DIGEST_SIZE); > > > > pseudo_padded_banks = init_tpm_banks(&num_banks); > > @@ -2072,6 +2076,8 @@ static int ima_measurement(const char *file) > > init_public_keys(imaevm_params.keyfile); > > else /* assume read pubkey from x509 cert */ > > init_public_keys("/etc/keys/x509_evm.der"); > > + if (errno) > > + log_errno_reset(LOG_DEBUG, "Failed to initialize public keys"); > > Library prints appropriate error messages, so this is perhaps just to > clear errno. But it's not necessarily completely failed, but maybe > failure in one key. So I would say "Failure in initializing public > keys" to be precise. > > ps. > > BTW, init_public_keys API call cannot return error except by errno, > but it does not set it consistently so some errors may be missed. > > init_public_keys loops calling read_pub_pkey > > entry->key = read_pub_pkey(keyfile, 1); > if (!entry->key) { > free(entry); > continue; > } > > and read_pub_pkey have such code: > > if (!keyfile) > return NULL; > > In that case some key is not read but we don't get any error notification. > > I think it's legal, by the right of being library, so set `errno = > EINVAL` there somewhere. But, I'm not sure where - as we should not > clobber existing errno values. Perhaps, errno setting should be added to > libimaevm consistently to all functions, but this is huge task, so I > would not suggest to do it now. Just suggestion for the future > developments, maybe. Just to compare with other library - libtracefs sets errno _sometimes_, for example, their API call tracefs_dynevent_get have: struct tracefs_dynevent * tracefs_dynevent_get(enum tracefs_dynevent_type type, const char *system, const char *event) { ... if (!event) { errno = -EINVAL; return NULL; } count = get_all_dynevents(type, system, &events); if (count <= 0) return NULL; ... So it sets errno sometimes, but not always, which I am not sure is correct approach. This needs to be discussed more with library experts. Thanks,
On Fri, Nov 04, 2022 at 01:24:21AM +0300, Vitaly Chikunov wrote: > On Fri, Nov 04, 2022 at 01:05:31AM +0300, Vitaly Chikunov wrote: > > Mimi, > > > > On Thu, Nov 03, 2022 at 02:38:49PM -0400, Mimi Zohar wrote: > > > Define a log_errno_reset macro to emit the errno string at or near the > > > time of error, similar to the existing log_errno macro, but also reset > > > errno to avoid dangling or duplicate errno messages on exit. > > > > > > The initial usage is for non-critical file open failures. > > > > > > Suggested-by: Vitaly Chikunov <vt@altlinux.org> > > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> > > > --- > > > src/evmctl.c | 12 ++++++++++-- > > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > Reviewed-by: Vitaly Chikunov <vt@altlinux.org> > > > > > > > > diff --git a/src/evmctl.c b/src/evmctl.c > > > index 0412bc0ac2b0..54123bf20f03 100644 > > > --- a/src/evmctl.c > > > +++ b/src/evmctl.c > > > @@ -2055,7 +2060,6 @@ static int ima_measurement(const char *file) > > > int err_padded = -1; > > > int err = -1; > > > > > > - errno = 0; > > > memset(zero, 0, MAX_DIGEST_SIZE); > > > > > > pseudo_padded_banks = init_tpm_banks(&num_banks); > > > @@ -2072,6 +2076,8 @@ static int ima_measurement(const char *file) > > > init_public_keys(imaevm_params.keyfile); > > > else /* assume read pubkey from x509 cert */ > > > init_public_keys("/etc/keys/x509_evm.der"); > > > + if (errno) > > > + log_errno_reset(LOG_DEBUG, "Failed to initialize public keys"); > > > > Library prints appropriate error messages, so this is perhaps just to > > clear errno. But it's not necessarily completely failed, but maybe > > failure in one key. So I would say "Failure in initializing public > > keys" to be precise. > > > > ps. > > > > BTW, init_public_keys API call cannot return error except by errno, > > but it does not set it consistently so some errors may be missed. > > > > init_public_keys loops calling read_pub_pkey > > > > entry->key = read_pub_pkey(keyfile, 1); > > if (!entry->key) { > > free(entry); > > continue; > > } > > > > and read_pub_pkey have such code: > > > > if (!keyfile) > > return NULL; > > > > In that case some key is not read but we don't get any error notification. > > > > I think it's legal, by the right of being library, so set `errno = > > EINVAL` there somewhere. But, I'm not sure where - as we should not > > clobber existing errno values. Perhaps, errno setting should be added to > > libimaevm consistently to all functions, but this is huge task, so I > > would not suggest to do it now. Just suggestion for the future > > developments, maybe. > > Just to compare with other library - libtracefs sets errno _sometimes_, It "is not consistent" in the sense that error in the API call does not have always errno set. And this is unlike we have for common libc API calls where errno is defined. (As a consequence we cannot just add strerror() to the printing errors from these APIs). But, it's not necessarily there is standard or common practice about this matter. Thanks, > for example, their API call tracefs_dynevent_get have: > > struct tracefs_dynevent * > tracefs_dynevent_get(enum tracefs_dynevent_type type, const char *system, > const char *event) > { > ... > if (!event) { > errno = -EINVAL; > return NULL; > } > > count = get_all_dynevents(type, system, &events); > if (count <= 0) > return NULL; > ... > > So it sets errno sometimes, but not always, which I am not sure is correct > approach. This needs to be discussed more with library experts. > > Thanks,
Hi Mimi,
Reviewed-by: Petr Vorel <pvorel@suse.cz>
Kind regards,
Petr
On Fri, 2022-11-04 at 01:35 +0300, Vitaly Chikunov wrote: > On Fri, Nov 04, 2022 at 01:24:21AM +0300, Vitaly Chikunov wrote: > > On Fri, Nov 04, 2022 at 01:05:31AM +0300, Vitaly Chikunov wrote: > > > Mimi, > > > > > > On Thu, Nov 03, 2022 at 02:38:49PM -0400, Mimi Zohar wrote: > > > > Define a log_errno_reset macro to emit the errno string at or near the > > > > time of error, similar to the existing log_errno macro, but also reset > > > > errno to avoid dangling or duplicate errno messages on exit. > > > > > > > > The initial usage is for non-critical file open failures. > > > > > > > > Suggested-by: Vitaly Chikunov <vt@altlinux.org> > > > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> > > > > --- > > > > src/evmctl.c | 12 ++++++++++-- > > > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > > > Reviewed-by: Vitaly Chikunov <vt@altlinux.org> Thanks! > > > > > > > > > > > diff --git a/src/evmctl.c b/src/evmctl.c > > > > index 0412bc0ac2b0..54123bf20f03 100644 > > > > --- a/src/evmctl.c > > > > +++ b/src/evmctl.c > > > > @@ -2055,7 +2060,6 @@ static int ima_measurement(const char *file) > > > > int err_padded = -1; > > > > int err = -1; > > > > > > > > - errno = 0; > > > > memset(zero, 0, MAX_DIGEST_SIZE); > > > > > > > > pseudo_padded_banks = init_tpm_banks(&num_banks); > > > > @@ -2072,6 +2076,8 @@ static int ima_measurement(const char *file) > > > > init_public_keys(imaevm_params.keyfile); > > > > else /* assume read pubkey from x509 cert */ > > > > init_public_keys("/etc/keys/x509_evm.der"); > > > > + if (errno) > > > > + log_errno_reset(LOG_DEBUG, "Failed to initialize public keys"); > > > > > > Library prints appropriate error messages, so this is perhaps just to > > > clear errno. But it's not necessarily completely failed, but maybe > > > failure in one key. So I would say "Failure in initializing public > > > keys" to be precise. > > > > > > ps. > > > > > > BTW, init_public_keys API call cannot return error except by errno, > > > but it does not set it consistently so some errors may be missed. > > > > > > init_public_keys loops calling read_pub_pkey > > > > > > entry->key = read_pub_pkey(keyfile, 1); > > > if (!entry->key) { > > > free(entry); > > > continue; > > > } > > > > > > and read_pub_pkey have such code: > > > > > > if (!keyfile) > > > return NULL; > > > > > > In that case some key is not read but we don't get any error notification. > > > > > > I think it's legal, by the right of being library, so set `errno = > > > EINVAL` there somewhere. But, I'm not sure where - as we should not > > > clobber existing errno values. Perhaps, errno setting should be added to > > > libimaevm consistently to all functions, but this is huge task, so I > > > would not suggest to do it now. Just suggestion for the future > > > developments, maybe. > > > > Just to compare with other library - libtracefs sets errno _sometimes_, > > It "is not consistent" in the sense that error in the API call does not > have always errno set. And this is unlike we have for common libc API > calls where errno is defined. (As a consequence we cannot just add > strerror() to the printing errors from these APIs). > > But, it's not necessarily there is standard or common practice about > this matter. The perror() man page seems to say that it is optional, but doesn't say anything about libraries being self-consistent: "When system call fails, it usually returns -1 and sets the variable errno to a value describing what went wrong. (These values can be found in <errno.h>.) Many library functions do likewise." > > > > for example, their API call tracefs_dynevent_get have: > > > > struct tracefs_dynevent * > > tracefs_dynevent_get(enum tracefs_dynevent_type type, const char *system, > > const char *event) > > { > > ... > > if (!event) { > > errno = -EINVAL; > > return NULL; > > } > > > > count = get_all_dynevents(type, system, &events); > > if (count <= 0) > > return NULL; > > ... > > > > So it sets errno sometimes, but not always, which I am not sure is correct > > approach. This needs to be discussed more with library experts. Perhaps this isn't a question of self-consistency, but of who is setting errno. In the tracefs_dynevent_get() case, nothing else is being called, so nothing could have set errno. In the get_all_dynevents() case, without looking at the code, something could have already set errno. As for the ima-evm-utils example, I agree read_pub_pkey() should return -EINVAL when keyfile is NULL. init_public_keys() verifies keyfile is not NULL before calling read_pub_pkey(), so read_pub_pkey() returning -EINVAL, shouldn't affect it.
diff --git a/src/evmctl.c b/src/evmctl.c index 0412bc0ac2b0..54123bf20f03 100644 --- a/src/evmctl.c +++ b/src/evmctl.c @@ -166,6 +166,9 @@ struct tpm_bank_info { static char *pcrfile[MAX_PCRFILE]; static unsigned npcrfile; +#define log_errno_reset(level, fmt, args...) \ + {do_log(level, fmt " (errno: %s)\n", ##args, strerror(errno)); errno = 0; } + static int bin2file(const char *file, const char *ext, const unsigned char *data, int len) { FILE *fp; @@ -1911,8 +1914,10 @@ static int read_sysfs_pcrs(int num_banks, struct tpm_bank_info *tpm_banks) fp = fopen(pcrs, "r"); if (!fp) fp = fopen(misc_pcrs, "r"); - if (!fp) + if (!fp) { + log_errno_reset(LOG_DEBUG, "Failed to read TPM 1.2 PCRs"); return -1; + } result = read_one_bank(&tpm_banks[0], fp); fclose(fp); @@ -2055,7 +2060,6 @@ static int ima_measurement(const char *file) int err_padded = -1; int err = -1; - errno = 0; memset(zero, 0, MAX_DIGEST_SIZE); pseudo_padded_banks = init_tpm_banks(&num_banks); @@ -2072,6 +2076,8 @@ static int ima_measurement(const char *file) init_public_keys(imaevm_params.keyfile); else /* assume read pubkey from x509 cert */ init_public_keys("/etc/keys/x509_evm.der"); + if (errno) + log_errno_reset(LOG_DEBUG, "Failed to initialize public keys"); /* * Reading the PCRs before walking the IMA measurement list @@ -2746,6 +2752,8 @@ int main(int argc, char *argv[]) unsigned long keyid; char *eptr; + errno = 0; /* initialize global errno */ + #if !(OPENSSL_VERSION_NUMBER < 0x10100000) OPENSSL_init_crypto( #ifndef DISABLE_OPENSSL_CONF
Define a log_errno_reset macro to emit the errno string at or near the time of error, similar to the existing log_errno macro, but also reset errno to avoid dangling or duplicate errno messages on exit. The initial usage is for non-critical file open failures. Suggested-by: Vitaly Chikunov <vt@altlinux.org> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> --- src/evmctl.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)