| Message ID | 20240124220619.work.227-kees@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | exec: Remove __FMODE_EXEC from uselib() | expand |
On Wed, Jan 24, 2024 at 02:06:23PM -0800, Kees Cook wrote: > Path-based LSMs will bypass uselib() "open" checks since commit > 4759ff71f23e ("exec: Check __FMODE_EXEC instead of in_execve for LSMs"), > so don't set __FMODE_EXEC during uselib(). The LSM "open" and eventual > "mmap" hooks will be restored. (uselib() never set current->in_execve.) Ah, nevermind, I see Linux's commit has taken care of this already: https://git.kernel.org/linus/3eab830189d94f0f80f34cbff609b5bb54002679
On Wed 24-01-24 14:06:23, Kees Cook wrote: > Path-based LSMs will bypass uselib() "open" checks since commit > 4759ff71f23e ("exec: Check __FMODE_EXEC instead of in_execve for LSMs"), > so don't set __FMODE_EXEC during uselib(). The LSM "open" and eventual > "mmap" hooks will be restored. (uselib() never set current->in_execve.) > > Other things that checked __FMODE_EXEC: > > - fs/fcntl.c is just doing a bitfield sanity check. > > - nfs_open_permission_mask() is only checking for the > "unreadable exec" case, which is not an issue for uselib(), > which sets MAY_READ, unlike execve(). > > - fsnotify would no longer see uselib() as FS_OPEN_EXEC_PERM, but > rather as FS_OPEN_PERM, but this is likely a bug fix, as uselib() isn't > an exec: it's more like mmap(), which fsnotify doesn't intercept. OK, I went back to the original discussion with Steve Grubb and Matthew Bobrowski who asked for FS_OPEN_EXEC_PERM and AFAICT this change in uselib() should be fine wrt usescases we discussed. That doesn't mean there cannot be some userspace which will get broken by this (in which case we'd have to revert or find some other solution) but I'm willing to try. I'm also CCing Steve & Matthew for input but from my side feel free to add: Acked-by: Jan Kara <jack@suse.cz> Honza > > Reported-by: Jann Horn <jannh@google.com> > Closes: https://lore.kernel.org/lkml/CAG48ez017tTwxXbxdZ4joVDv5i8FLWEjk=K_z1Vf=pf0v1=cTg@mail.gmail.com/ > Fixes: 4759ff71f23e ("exec: Check __FMODE_EXEC instead of in_execve for LSMs") > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Kevin Locke <kevin@kevinlocke.name> > Cc: Eric Biederman <ebiederm@xmission.com> > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > Cc: Christian Brauner <brauner@kernel.org> > Cc: Jan Kara <jack@suse.cz> > Cc: linux-mm@kvack.org > Cc: linux-fsdevel@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > fs/exec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/exec.c b/fs/exec.c > index d179abb78a1c..af4fbb61cd53 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -128,7 +128,7 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) > struct filename *tmp = getname(library); > int error = PTR_ERR(tmp); > static const struct open_flags uselib_flags = { > - .open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC, > + .open_flag = O_LARGEFILE | O_RDONLY, > .acc_mode = MAY_READ | MAY_EXEC, > .intent = LOOKUP_OPEN, > .lookup_flags = LOOKUP_FOLLOW, > -- > 2.34.1 >
diff --git a/fs/exec.c b/fs/exec.c index d179abb78a1c..af4fbb61cd53 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -128,7 +128,7 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) struct filename *tmp = getname(library); int error = PTR_ERR(tmp); static const struct open_flags uselib_flags = { - .open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC, + .open_flag = O_LARGEFILE | O_RDONLY, .acc_mode = MAY_READ | MAY_EXEC, .intent = LOOKUP_OPEN, .lookup_flags = LOOKUP_FOLLOW,
Path-based LSMs will bypass uselib() "open" checks since commit 4759ff71f23e ("exec: Check __FMODE_EXEC instead of in_execve for LSMs"), so don't set __FMODE_EXEC during uselib(). The LSM "open" and eventual "mmap" hooks will be restored. (uselib() never set current->in_execve.) Other things that checked __FMODE_EXEC: - fs/fcntl.c is just doing a bitfield sanity check. - nfs_open_permission_mask() is only checking for the "unreadable exec" case, which is not an issue for uselib(), which sets MAY_READ, unlike execve(). - fsnotify would no longer see uselib() as FS_OPEN_EXEC_PERM, but rather as FS_OPEN_PERM, but this is likely a bug fix, as uselib() isn't an exec: it's more like mmap(), which fsnotify doesn't intercept. Reported-by: Jann Horn <jannh@google.com> Closes: https://lore.kernel.org/lkml/CAG48ez017tTwxXbxdZ4joVDv5i8FLWEjk=K_z1Vf=pf0v1=cTg@mail.gmail.com/ Fixes: 4759ff71f23e ("exec: Check __FMODE_EXEC instead of in_execve for LSMs") Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Cc: Kevin Locke <kevin@kevinlocke.name> Cc: Eric Biederman <ebiederm@xmission.com> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: Christian Brauner <brauner@kernel.org> Cc: Jan Kara <jack@suse.cz> Cc: linux-mm@kvack.org Cc: linux-fsdevel@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- fs/exec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)