Re: [PATCH] kernel/fs: last check for exec credentials on NOEXEC mount

[Christian Brauner <brauner@kernel.org>]

On Sat, Aug 03, 2024 at 08:29:17AM GMT, Mateusz Guzik wrote:
> On Fri, Aug 2, 2024 at 5:59 PM Josef Bacik <josef@toxicpanda.com> wrote:
> >
> > On Thu, Aug 01, 2024 at 05:15:06PM +0200, Mateusz Guzik wrote:
> > > I'm not confident this is particularly valuable, but if it is, it
> > > probably should hide behind some debug flags.
> >
> > I'm still going to disagree here, putting it behind a debug flag means it'll
> > never get caught, and it obviously proved valuable because we're discussing this
> > particular case.
> >
> > Is it racy? Yup sure.  I think that your solution is the right way to fix it,
> > and then we can have a
> >
> > WARN_ON(!(file->f_mode & FMODE_NO_EXEC_CHECKED));
> >
> > or however we choose to flag the file, that way we are no longer racing with the
> > mount flags and only validating that a check that should have already occurred
> > has in fact occurred.  Thanks,
> >
> 
> To my understanding the submitter ran into the thing tripping over the
> racy check, so this check did not find a real bug elsewhere in this
> instance.

Mateusz is right. That check is mostly nonsensical. Nothing will protect
against mount properties being changed if the caller isn't claiming
write access to the mount. Which this code doesn't do (And can't do (or
anything like it) because it would cause spurious remount failures just
because someone execs something.).

So 0fd338b2d2cd ("exec: move path_noexec() check earlier") introduced
WARN_ON_ONCE(). I suspect that it simply wasn't clear that mount
properties can change while a is file held open if no write access was
claimed.

Stuff like noexec, nodev or whatever can change anytime if e.g., just
read access was requested. In other words, successful permission
checking during path lookup doesn't mean that permission checking
wouldn't fail if one redid the checks immediately after.

I think it probably never triggered because noexec -> exec remounts are
rarely done and the timing would have to be rather precise.

I think the immediate solution is to limit the scope of the
WARN_ON_ONCE() to the ->i_mode check.

diff --git a/fs/exec.c b/fs/exec.c
index a126e3d1cacb..12914e14132d 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -145,13 +145,14 @@ SYSCALL_DEFINE1(uselib, const char __user *, library)
                goto out;

        /*
-        * may_open() has already checked for this, so it should be
-        * impossible to trip now. But we need to be extra cautious
-        * and check again at the very end too.
+        * Safety paranoia: Redo the check whether the mount isn't
+        * noexec so it's as close the the actual open() as possible.
+        * may_open() has already check this but the mount properties
+        * may have already changed since then.
         */
-       error = -EACCES;
-       if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode) ||
-                        path_noexec(&file->f_path)))
+       err = -EACCES;
+       if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode)) ||
+           path_noexec(&file->f_path))
                goto exit;

        error = -ENOEXEC;
@@ -974,13 +975,14 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
                goto out;

        /*
-        * may_open() has already checked for this, so it should be
-        * impossible to trip now. But we need to be extra cautious
-        * and check again at the very end too.
+        * Safety paranoia: Redo the check whether the mount isn't
+        * noexec so it's as close the the actual open() as possible.
+        * may_open() has already check this but the mount properties
+        * may have already changed since then.
         */
        err = -EACCES;
-       if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode) ||
-                        path_noexec(&file->f_path)))
+       if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode)) ||
+           path_noexec(&file->f_path))
                goto exit;

 out: