lkml.org 
[lkml]   [2023]   [Apr]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH] overlayfs: Trigger file re-evaluation by IMA / EVM after writes
    From


    On 4/7/23 09:29, Jeff Layton wrote:
    >>>>>
    >>>>> I would ditch the original proposal in favor of this 2-line patch shown here:
    >>>>>
    >>>>> https://lore.kernel.org/linux-integrity/a95f62ed-8b8a-38e5-e468-ecbde3b221af@linux.ibm.com/T/#m3bd047c6e5c8200df1d273c0ad551c645dd43232
    >>
    >> We should cool it with the quick hacks to fix things. :)
    >>
    >
    > Yeah. It might fix this specific testcase, but I think the way it uses
    > the i_version is "gameable" in other situations. Then again, I don't
    > know a lot about IMA in this regard.
    >
    > When is it expected to remeasure? If it's only expected to remeasure on
    > a close(), then that's one thing. That would be a weird design though.

    IMA should remeasure the file when it has visibly changed for another thread or process.


    >>> -----------------------8<---------------------------
    >>>
    >>> [PATCH] IMA: use vfs_getattr_nosec to get the i_version
    >>>
    >>> IMA currently accesses the i_version out of the inode directly when it
    >>> does a measurement. This is fine for most simple filesystems, but can be
    >>> problematic with more complex setups (e.g. overlayfs).
    >>>
    >>> Make IMA instead call vfs_getattr_nosec to get this info. This allows
    >>> the filesystem to determine whether and how to report the i_version, and
    >>> should allow IMA to work properly with a broader class of filesystems in
    >>> the future.
    >>>
    >>> Reported-by: Stefan Berger <stefanb@linux.ibm.com>
    >>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
    >>> ---
    >>
    >> So, I think we want both; we want the ovl_copyattr() and the
    >> vfs_getattr_nosec() change:
    >>
    >> (1) overlayfs should copy up the inode version in ovl_copyattr(). That
    >> is in line what we do with all other inode attributes. IOW, the
    >> overlayfs inode's i_version counter should aim to mirror the
    >> relevant layer's i_version counter. I wouldn't know why that
    >> shouldn't be the case. Asking the other way around there doesn't
    >> seem to be any use for overlayfs inodes to have an i_version that
    >> isn't just mirroring the relevant layer's i_version.
    >
    > It's less than ideal to do this IMO, particularly with an IS_I_VERSION
    > inode.
    >
    > You can't just copy up the value from the upper. You'll need to call
    > inode_query_iversion(upper_inode), which will flag the upper inode for a
    > logged i_version update on the next write. IOW, this could create some
    > (probably minor) metadata write amplification in the upper layer inode
    > with IS_I_VERSION inodes.
    >
    >
    >> (2) Jeff's changes for ima to make it rely on vfs_getattr_nosec().
    >> Currently, ima assumes that it will get the correct i_version from
    >> an inode but that just doesn't hold for stacking filesystem.
    >>
    >> While (1) would likely just fix the immediate bug (2) is correct and
    >> _robust_. If we change how attributes are handled vfs_*() helpers will
    >> get updated and ima with it. Poking at raw inodes without using
    >> appropriate helpers is much more likely to get ima into trouble.
    >
    > This will fix it the right way, I think (assuming it actually works),
    > and should open the door for IMA to work properly with networked
    > filesystems that support i_version as well.
    >
    > Note that there Stephen is correct that calling getattr is probably
    > going to be less efficient here since we're going to end up calling
    > generic_fillattr unnecessarily, but I still think it's the right thing
    > to do.

    I was wondering whether to use the existing inode_eq_iversion() for all
    other filesystems than overlayfs, nfs, and possibly other ones (which ones?)
    where we would use the vfs_getattr_nosec() via a case on inode->i_sb->s_magic?
    If so, would this function be generic enough to be a public function for libfs.c?

    I'll hopefully be able to test the proposed patch tomorrow.

    >
    > If it turns out to cause measurable performance regressions though,
    > maybe we can look at adding a something that still calls ->getattr if it
    > exists but only returns the change_cookie value.

    \
     
     \ /
      Last update: 2023-04-17 03:58    [W:4.035 / U:0.088 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site