RE: [PATCH] ima: Calculate digest in ima_inode_hash() if not available

From: Mimi Zohar [mailto:zohar@linux.ibm.com]
Sent: Monday, February 14, 2022 11:33 PM
On Mon, 2022-02-14 at 17:05 +0000, Roberto Sassu wrote:

quoted

quoted

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:

quoted

__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

quoted

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

quoted

quoted

quoted

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.

I just wanted the ability to get a fresh digest after a file opened
for writing is closed. Since in my use case I would not use an IMA
policy, that would not be a problem.

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Zhong Ronghua

quoted

quoted

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.

quoted

quoted

Please split the IMA from the eBPF changes.

Ok.

--
thanks,

Mimi