Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <36f85113f181f78eda3576823bd92be3f9cd1802.camel@linux.ibm.com>
Subject: Re: [PATCH] ima: Calculate digest in ima_inode_hash() if not
 available
From:   Mimi Zohar <zohar@linux.ibm.com>
To:     Roberto Sassu <roberto.sassu@huawei.com>,
        "shuah@kernel.org" <shuah@kernel.org>,
        "ast@kernel.org" <ast@kernel.org>,
        "daniel@iogearbox.net" <daniel@iogearbox.net>,
        "andrii@kernel.org" <andrii@kernel.org>,
        "kpsingh@kernel.org" <kpsingh@kernel.org>,
        Florent Revest <revest@chromium.org>
Cc:     "linux-integrity@vger.kernel.org" <linux-integrity@vger.kernel.org>,
        "linux-security-module@vger.kernel.org" 
        <linux-security-module@vger.kernel.org>,
        "linux-kselftest@vger.kernel.org" <linux-kselftest@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "bpf@vger.kernel.org" <bpf@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Date:   Mon, 14 Feb 2022 17:32:49 -0500
In-Reply-To: <cc6bcb7742dc432ba990ee38b5909496@huawei.com>
References: <20220211104828.4061334-1-roberto.sassu@huawei.com>
         <537635732d9cbcc42bcf7be5ed932d284b03d39f.camel@linux.ibm.com>
         <cc6bcb7742dc432ba990ee38b5909496@huawei.com>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
Precedence: bulk

On Mon, 2022-02-14 at 17:05 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Sunday, February 13, 2022 2:06 PM
> > Hi Roberto,
> > 
> > On Fri, 2022-02-11 at 11:48 +0100, Roberto Sassu wrote:
> > > __ima_inode_hash() checks if a digest has been already calculated by
> > > looking for the integrity_iint_cache structure associated to the passed
> > > inode.
> > >
> > > Users of ima_file_hash() and ima_inode_hash() (e.g. eBPF) might be
> > > interested in obtaining the information without having to setup an IMA
> > > policy so that the digest is always available at the time they call one of
> > > those functions.
> > >
> > > Open a new file descriptor in __ima_inode_hash(), so that this function
> > > could invoke ima_collect_measurement() to calculate the digest if it is not
> > > available. Still return -EOPNOTSUPP if the calculation failed.
> > >
> > > Instead of opening a new file descriptor, the one from ima_file_hash()
> > > could have been used. However, since ima_inode_hash() was created to
> > obtain
> > > the digest when the file descriptor is not available, it could benefit from
> > > this change too. Also, the opened file descriptor might be not suitable for
> > > use (file descriptor opened not for reading).
> > >
> > > This change does not cause memory usage increase, due to using a temporary
> > > integrity_iint_cache structure for the digest calculation, and due to
> > > freeing the ima_digest_data structure inside integrity_iint_cache before
> > > exiting from __ima_inode_hash().
> > >
> > > Finally, update the test by removing ima_setup.sh (it is not necessary
> > > anymore to set an IMA policy) and by directly executing /bin/true.
> > >
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > Although this patch doesn't directly modify either ima_file_hash() or
> > ima_inode_hash(),  this change affects both functions.  ima_file_hash()
> > was introduced to be used with eBPF.  Based on Florent's post, changing
> > the ima_file_hash() behavor seems fine.  Since I have no idea whether
> > anyone is still using ima_inode_hash(), perhaps it would be safer to
> > limit this behavior change to just ima_file_hash().
> 
> Hi Mimi
> 
> ok.
> 
> I found that just checking that iint->ima_hash is not NULL is not enough
> (ima_inode_hash() might still return the old digest after a file write).
> Should I replace that check with !(iint->flags & IMA_COLLECTED)?
> Or should I do only for ima_file_hash() and recalculate the digest
> if necessary?

Updating the file hash after each write would really impact IMA
performance.  If you really want to detect any file change, no matter
how frequently it occurs, your best bet would be to track i_generation
and i_version.  Stefan is already adding "i_generation" for IMA
namespacing.

> 
> > Please update the ima_file_hash() doc.  While touching this area, I'd
> > appreciate your fixing the first doc line in both ima_file_hash() and
> > ima_inode_hash() cases, which wraps spanning two lines.
> 
> Did you mean to make the description shorter or to have everything
> in one line? According to the kernel documentation (kernel-doc.rst),
> having the brief description in multiple lines should be fine.

Thanks for checking kernel-doc.   The "brief description"  not wrapping
across multiple lines did in fact change.

> > Please split the IMA from the eBPF changes.
> 
> Ok.

-- 
thanks,

Mimi