From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id A4C70C43217 for ; Thu, 3 Nov 2022 22:35:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231539AbiKCWfS (ORCPT ); Thu, 3 Nov 2022 18:35:18 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51660 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231535AbiKCWfS (ORCPT ); Thu, 3 Nov 2022 18:35:18 -0400 Received: from vmicros1.altlinux.org (vmicros1.altlinux.org [194.107.17.57]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 6CB3A20185 for ; Thu, 3 Nov 2022 15:35:17 -0700 (PDT) Received: from imap.altlinux.org (imap.altlinux.org [194.107.17.38]) by vmicros1.altlinux.org (Postfix) with ESMTP id 9EF6872C983; Fri, 4 Nov 2022 01:35:16 +0300 (MSK) Received: from altlinux.org (sole.flsd.net [185.75.180.6]) by imap.altlinux.org (Postfix) with ESMTPSA id 893904A472A; Fri, 4 Nov 2022 01:35:16 +0300 (MSK) Date: Fri, 4 Nov 2022 01:35:16 +0300 From: Vitaly Chikunov To: Mimi Zohar Cc: linux-integrity@vger.kernel.org, Petr Vorel , Stefan Berger , "Dmitry V. Levin" Subject: Re: [PATCH ima-evm-utils v5 02/17] log and reset 'errno' after failure to open non-critical files Message-ID: <20221103223516.x5eyb65chj6th3e3@altlinux.org> References: <20221103183904.103562-1-zohar@linux.ibm.com> <20221103183904.103562-3-zohar@linux.ibm.com> <20221103220531.ok6bwddhxmq6oxky@altlinux.org> <20221103222421.fhtdymd2spiejvuh@altlinux.org> MIME-Version: 1.0 Content-Type: text/plain; charset=koi8-r Content-Disposition: inline In-Reply-To: <20221103222421.fhtdymd2spiejvuh@altlinux.org> Precedence: bulk List-ID: X-Mailing-List: linux-integrity@vger.kernel.org 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 > > > Signed-off-by: Mimi Zohar > > > --- > > > src/evmctl.c | 12 ++++++++++-- > > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > Reviewed-by: Vitaly Chikunov > > > > > > > > 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,