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
