Re: IMA/EVM writing xattrs during remount filesystem

[Mimi Zohar <zohar@linux.vnet.ibm.com>]

On Mon, 2018-02-26 at 16:38 +0100, Sascha Hauer wrote:
> On Mon, Feb 26, 2018 at 10:12:05AM -0500, Mimi Zohar wrote:
> > Hi Sascha,
> > 
> > On Mon, 2018-02-26 at 15:23 +0100, Sascha Hauer wrote:
> > > Hi All,
> > > 
> > > When a filesystem is remounted from rw to ro then
> > > sb_prepare_remount_readonly() is called. After this call there shouldn't
> > > be any writers left on the filesystem. However, IMA/EVM is not aware of
> > > this as it never calls mnt_want_write[_file](), but only looks add the
> > > MS_RDONLY superblock flag before writing to its xattrs. This flag is
> > > only changed after sb->s_op->remount_fs() is called. As a consequence
> > > IMA/EVM still updates xattrs while the filesystem is going to readonly
> > > mode.
> > > 
> > > We observed that on a 4.0 Kernel in conjunction with UBIFS, but the
> > > relevant code in IMA/EVM still looks the same so I assume it's present
> > > in the current kernel aswell.
> > > 
> > > UBIFS calculates its free space before and after the remount_fs op and
> > > if there's a difference it prints a backtrace (dbg_check_space_info:
> > > free space changed from x to y). We see this backtrace sometimes when
> > > remounting the fs readonly. If I understand the situation correctly this
> > > is not UBIFS's fault, right? Any hint what we can do about it?
> > 
> > Not updating the file hashes could result in verification errors.  I
> > would classify updating the xattrs as working as designed.  Wouldn't
> > you?
> 
> If IMA updates the hash for a file that actually gets written then the
> file is opened for writing. Trying remount the filesystem readonly in
> this situation will return -EBUSY. Everything fine here, but look at
> evm_verify_hmac(). Here a file is not necessarily opened for writing,
> but if the file is signed the code replaces the signature with a HMAC by
> calling evm_update_evmxattr(). This function updates the xattr even when
> the VFS is in the process of remounting the filesystem readonly.
> 
> More specifically look at do_remount_sb():
> 
> >	/* If we are remounting RDONLY and current sb is read/write,
> >	   make sure there are no rw files opened */
> >	if (remount_ro) {
> >		if (force) {
> >			sb->s_readonly_remount = 1;
> >			smp_wmb();
> >		} else {
> >			retval = sb_prepare_remount_readonly(sb);
> >			if (retval)
> >				return retval;
> >		}
> >	}
> 
> When sb_prepare_remount_readonly() succeeds there are no writers left.
> 
> >
> >	if (sb->s_op->remount_fs) {
> >		retval = sb->s_op->remount_fs(sb, &sb_flags, data);
> >		if (retval) {
> >			if (!force)
> >				goto cancel_readonly;
> >			/* If forced remount, go ahead despite any errors */
> >			WARN(1, "forced remount of a %s fs returned %i\n",
> >			     sb->s_type->name, retval);
> >		}
> >	}
> 
> remount_fs assumes VFS has stopped writing. At least UBIFS expects this,
> maybe it's wrong:
> 
> /**
>  * ubifs_remount_ro - re-mount in read-only mode.
>  * @c: UBIFS file-system description object
>  *
>  * We assume VFS has stopped writing. Possibly the background thread could be
>  * running a commit, however kthread_stop will wait in that case.
>  */
> 
> >	sb->s_flags = (sb->s_flags & ~MS_RMT_MASK) | (sb_flags & MS_RMT_MASK);
> 
> Here, *after* remount_fs has returned the MS_RDONLY sb flag is set which
> EVM tests for before calling evm_update_evmxattr() and the race window
> closes.

So the cause of the problem is not IMA, per se, but EVM converting the
EVM signature to an HMAC.  There's no harm in not re-writing the xattr
signature as an HMAC.  Feel free to add the additional
"s_readonly_remount" test.

During this open window, we upstreamed support for EVM portable and
immutable file signatures.  Please make sure you base the change on
the linux-integrity #next-integrity branch.

thanks,

Mimi