| Message ID | 20240124192228.work.788-kees@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | exec: Check __FMODE_EXEC instead of in_execve for LSMs | expand |
On Wed, 2024-01-24 at 11:22 -0800, Kees Cook wrote: > After commit 978ffcbf00d8 ("execve: open the executable file before > doing anything else"), current->in_execve was no longer in sync with the > open(). This broke AppArmor and TOMOYO which depend on this flag to > distinguish "open" operations from being "exec" operations. > > Instead of moving around in_execve, switch to using __FMODE_EXEC, which > is where the "is this an exec?" intent is stored. Note that TOMOYO still > uses in_execve around cred handling. It solves the AppArmor issue I was experiencing and I don't notice any other issues. Tested-by: Kevin Locke <kevin@kevinlocke.name> Thanks! Kevin
On Wed, Jan 24, 2024 at 12:39:38PM -0700, Kevin Locke wrote: > On Wed, 2024-01-24 at 11:22 -0800, Kees Cook wrote: > > After commit 978ffcbf00d8 ("execve: open the executable file before > > doing anything else"), current->in_execve was no longer in sync with the > > open(). This broke AppArmor and TOMOYO which depend on this flag to > > distinguish "open" operations from being "exec" operations. > > > > Instead of moving around in_execve, switch to using __FMODE_EXEC, which > > is where the "is this an exec?" intent is stored. Note that TOMOYO still > > uses in_execve around cred handling. > > It solves the AppArmor issue I was experiencing and I don't notice any > other issues. > > Tested-by: Kevin Locke <kevin@kevinlocke.name> Thanks! Sounds like Linus has taken the patch directly, and I'll send a follow-up PR with other clean-ups. -Kees
On Wed, Jan 24, 2024 at 8:22 PM Kees Cook <keescook@chromium.org> wrote: > After commit 978ffcbf00d8 ("execve: open the executable file before > doing anything else"), current->in_execve was no longer in sync with the > open(). This broke AppArmor and TOMOYO which depend on this flag to > distinguish "open" operations from being "exec" operations. > > Instead of moving around in_execve, switch to using __FMODE_EXEC, which > is where the "is this an exec?" intent is stored. Note that TOMOYO still > uses in_execve around cred handling. I think this is wrong. When CONFIG_USELIB is enabled, the uselib() syscall will open a file with __FMODE_EXEC but without going through execve(). From what I can tell, there are no bprm hooks on this path. I don't know if it _matters_ much, given that it'll only let you read/execute stuff from files with valid ELF headers, but still.
On Wed, Jan 24, 2024 at 08:58:55PM +0100, Jann Horn wrote: > On Wed, Jan 24, 2024 at 8:22 PM Kees Cook <keescook@chromium.org> wrote: > > After commit 978ffcbf00d8 ("execve: open the executable file before > > doing anything else"), current->in_execve was no longer in sync with the > > open(). This broke AppArmor and TOMOYO which depend on this flag to > > distinguish "open" operations from being "exec" operations. > > > > Instead of moving around in_execve, switch to using __FMODE_EXEC, which > > is where the "is this an exec?" intent is stored. Note that TOMOYO still > > uses in_execve around cred handling. > > I think this is wrong. When CONFIG_USELIB is enabled, the uselib() > syscall will open a file with __FMODE_EXEC but without going through > execve(). From what I can tell, there are no bprm hooks on this path. Hrm, that's true. We've been trying to remove uselib for at least 10 years[1]. :( > I don't know if it _matters_ much, given that it'll only let you > read/execute stuff from files with valid ELF headers, but still. Hmpf, and frustratingly Ubuntu (and Debian) still builds with CONFIG_USELIB, even though it was reported[2] to them almost 4 years ago. -Kees [1] https://lore.kernel.org/lkml/20140221181103.GA5773@jtriplet-mobl1/ [2] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1879454
On Wed, 24 Jan 2024 at 12:15, Kees Cook <keescook@chromium.org> wrote: > > Hmpf, and frustratingly Ubuntu (and Debian) still builds with > CONFIG_USELIB, even though it was reported[2] to them almost 4 years ago. Well, we could just remove the __FMODE_EXEC from uselib. It's kind of wrong anyway. Unlike a real execve(), where the target executable actually takes control and you can't actually control it (except with ptrace, of course), 'uselib()' really is just a wrapper around a special mmap. And you can see it in the "acc_mode" flags: uselib already requires MAY_READ for that reason. So you cannot uselib() a non-readable file, unlike execve(). So I think just removing __FMODE_EXEC would just do the RightThing(tm), and changes nothing for any sane situation. In fact, I don't think __FMODE_EXEC really ever did anything for the uselib() case, so removing it *really* shouldn't matter, and only fix the new AppArmor / Tomoyo use. Of course, as you say, not having CONFIG_USELIB enabled at all is the _truly_ sane thing, but the only thing that used the FMODE_EXEC bit were landlock and some special-case nfs stuff. And at least the nfs stuff was about "don't require read permissions for exec", which was already wrong for the uselib() case as per above. So I think the simple oneliner is literally just --- 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, but I obviously have nothing that uses uselib(). I don't see how it really *could* break anything, though, exactly because of that .acc_mode = MAY_READ | MAY_EXEC, that means that the *regular* permission checks already require the file to be readable. Never mind any LSM checks that might be confused. Linus
On Wed, Jan 24, 2024 at 9:47 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Wed, 24 Jan 2024 at 12:15, Kees Cook <keescook@chromium.org> wrote: > > > > Hmpf, and frustratingly Ubuntu (and Debian) still builds with > > CONFIG_USELIB, even though it was reported[2] to them almost 4 years ago. > > Well, we could just remove the __FMODE_EXEC from uselib. > > It's kind of wrong anyway. > > Unlike a real execve(), where the target executable actually takes > control and you can't actually control it (except with ptrace, of > course), 'uselib()' really is just a wrapper around a special mmap. > > And you can see it in the "acc_mode" flags: uselib already requires > MAY_READ for that reason. So you cannot uselib() a non-readable file, > unlike execve(). > > So I think just removing __FMODE_EXEC would just do the > RightThing(tm), and changes nothing for any sane situation. Sounds like a good idea. That makes this codepath behave more as if userspace had done the same steps manually...
On Wed, Jan 24, 2024 at 12:47:34PM -0800, Linus Torvalds wrote: > On Wed, 24 Jan 2024 at 12:15, Kees Cook <keescook@chromium.org> wrote: > > > > Hmpf, and frustratingly Ubuntu (and Debian) still builds with > > CONFIG_USELIB, even though it was reported[2] to them almost 4 years ago. For completeness, Fedora hasn't had CONFIG_USELIB for a while now. > Well, we could just remove the __FMODE_EXEC from uselib. > > It's kind of wrong anyway. Yeah. > So I think just removing __FMODE_EXEC would just do the > RightThing(tm), and changes nothing for any sane situation. Agreed about these: - fs/fcntl.c is just doing a bitfield sanity check. - nfs_open_permission_mask(), as you say, is only checking for unreadable case. - fsnotify would also see uselib() as a read, but afaict, that's what it would see for an mmap(), so this should be functionally safe. This one, though, I need some more time to examine: - AppArmor, TOMOYO, and LandLock will see uselib() as an open-for-read, so that might still be a problem? As you say, it's more of a mmap() call, but that would mean adding something a call like security_mmap_file() into uselib()... The issue isn't an insane "support uselib() under AppArmor" case, but rather "Can uselib() be used to bypass exec/mmap checks?" This totally untested patch might give appropriate coverage: diff --git a/fs/exec.c b/fs/exec.c index d179abb78a1c..0c9265312c8d 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -143,6 +143,10 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) if (IS_ERR(file)) goto out; + error = security_mmap_file(file, PROT_READ | PROT_EXEC, MAP_FIXED | MAP_SHARED); + if (error) + goto exit; + /* * may_open() has already checked for this, so it should be * impossible to trip now. But we need to be extra cautious > Of course, as you say, not having CONFIG_USELIB enabled at all is the > _truly_ sane thing, but the only thing that used the FMODE_EXEC bit > were landlock and some special-case nfs stuff. Do we want to attempt deprecation again? This was suggested last time: https://lore.kernel.org/lkml/20200518130251.zih2s32q2rxhxg6f@wittgenstein/ -Kees
On Wed, Jan 24, 2024 at 01:32:02PM -0800, Kees Cook wrote: > On Wed, Jan 24, 2024 at 12:47:34PM -0800, Linus Torvalds wrote: > > On Wed, 24 Jan 2024 at 12:15, Kees Cook <keescook@chromium.org> wrote: > > > > > > Hmpf, and frustratingly Ubuntu (and Debian) still builds with > > > CONFIG_USELIB, even though it was reported[2] to them almost 4 years ago. > > For completeness, Fedora hasn't had CONFIG_USELIB for a while now. > > > Well, we could just remove the __FMODE_EXEC from uselib. > > > > It's kind of wrong anyway. > > Yeah. > > > So I think just removing __FMODE_EXEC would just do the > > RightThing(tm), and changes nothing for any sane situation. > > Agreed about these: > > - fs/fcntl.c is just doing a bitfield sanity check. > > - nfs_open_permission_mask(), as you say, is only checking for > unreadable case. > > - fsnotify would also see uselib() as a read, but afaict, > that's what it would see for an mmap(), so this should > be functionally safe. > > This one, though, I need some more time to examine: > > - AppArmor, TOMOYO, and LandLock will see uselib() as an > open-for-read, so that might still be a problem? As you > say, it's more of a mmap() call, but that would mean > adding something a call like security_mmap_file() into > uselib()... > > The issue isn't an insane "support uselib() under AppArmor" case, but > rather "Can uselib() be used to bypass exec/mmap checks?" > > This totally untested patch might give appropriate coverage: > > diff --git a/fs/exec.c b/fs/exec.c > index d179abb78a1c..0c9265312c8d 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -143,6 +143,10 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) > if (IS_ERR(file)) > goto out; > > + error = security_mmap_file(file, PROT_READ | PROT_EXEC, MAP_FIXED | MAP_SHARED); Actually, this should probably match was load_shlib() uses: PROT_READ | PROT_WRITE | PROT_EXEC, MAP_FIXED_NOREPLACE | MAP_PRIVATE,
On Wed, Jan 24, 2024 at 10:32 PM Kees Cook <keescook@chromium.org> wrote: > > On Wed, Jan 24, 2024 at 12:47:34PM -0800, Linus Torvalds wrote: > > On Wed, 24 Jan 2024 at 12:15, Kees Cook <keescook@chromium.org> wrote: > > > > > > Hmpf, and frustratingly Ubuntu (and Debian) still builds with > > > CONFIG_USELIB, even though it was reported[2] to them almost 4 years ago. > > For completeness, Fedora hasn't had CONFIG_USELIB for a while now. > > > Well, we could just remove the __FMODE_EXEC from uselib. > > > > It's kind of wrong anyway. > > Yeah. > > > So I think just removing __FMODE_EXEC would just do the > > RightThing(tm), and changes nothing for any sane situation. > > Agreed about these: > > - fs/fcntl.c is just doing a bitfield sanity check. > > - nfs_open_permission_mask(), as you say, is only checking for > unreadable case. > > - fsnotify would also see uselib() as a read, but afaict, > that's what it would see for an mmap(), so this should > be functionally safe. > > This one, though, I need some more time to examine: > > - AppArmor, TOMOYO, and LandLock will see uselib() as an > open-for-read, so that might still be a problem? As you > say, it's more of a mmap() call, but that would mean > adding something a call like security_mmap_file() into > uselib()... > > The issue isn't an insane "support uselib() under AppArmor" case, but > rather "Can uselib() be used to bypass exec/mmap checks?" > > This totally untested patch might give appropriate coverage: > > diff --git a/fs/exec.c b/fs/exec.c > index d179abb78a1c..0c9265312c8d 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -143,6 +143,10 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) > if (IS_ERR(file)) > goto out; > > + error = security_mmap_file(file, PROT_READ | PROT_EXEC, MAP_FIXED | MAP_SHARED); > + if (error) > + goto exit; Call path from here is: sys_uselib -> load_elf_library -> elf_load -> elf_map -> vm_mmap -> vm_mmap_pgoff Call path for normal mmap is: sys_mmap_pgoff -> ksys_mmap_pgoff -> vm_mmap_pgoff So I think the call paths converge before any real security checks happen, and the check you're suggesting should be superfluous. (There is some weird audit call in ksys_mmap_pgoff() but that's just to record the FD number, so I guess that doesn't matter.)
On Wed, Jan 24, 2024 at 10:40:49PM +0100, Jann Horn wrote: > On Wed, Jan 24, 2024 at 10:32 PM Kees Cook <keescook@chromium.org> wrote: > > > > On Wed, Jan 24, 2024 at 12:47:34PM -0800, Linus Torvalds wrote: > > > On Wed, 24 Jan 2024 at 12:15, Kees Cook <keescook@chromium.org> wrote: > > > > > > > > Hmpf, and frustratingly Ubuntu (and Debian) still builds with > > > > CONFIG_USELIB, even though it was reported[2] to them almost 4 years ago. > > > > For completeness, Fedora hasn't had CONFIG_USELIB for a while now. > > > > > Well, we could just remove the __FMODE_EXEC from uselib. > > > > > > It's kind of wrong anyway. > > > > Yeah. > > > > > So I think just removing __FMODE_EXEC would just do the > > > RightThing(tm), and changes nothing for any sane situation. > > > > Agreed about these: > > > > - fs/fcntl.c is just doing a bitfield sanity check. > > > > - nfs_open_permission_mask(), as you say, is only checking for > > unreadable case. > > > > - fsnotify would also see uselib() as a read, but afaict, > > that's what it would see for an mmap(), so this should > > be functionally safe. > > > > This one, though, I need some more time to examine: > > > > - AppArmor, TOMOYO, and LandLock will see uselib() as an > > open-for-read, so that might still be a problem? As you > > say, it's more of a mmap() call, but that would mean > > adding something a call like security_mmap_file() into > > uselib()... > > > > The issue isn't an insane "support uselib() under AppArmor" case, but > > rather "Can uselib() be used to bypass exec/mmap checks?" > > > > This totally untested patch might give appropriate coverage: > > > > diff --git a/fs/exec.c b/fs/exec.c > > index d179abb78a1c..0c9265312c8d 100644 > > --- a/fs/exec.c > > +++ b/fs/exec.c > > @@ -143,6 +143,10 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) > > if (IS_ERR(file)) > > goto out; > > > > + error = security_mmap_file(file, PROT_READ | PROT_EXEC, MAP_FIXED | MAP_SHARED); > > + if (error) > > + goto exit; > > Call path from here is: > > sys_uselib -> load_elf_library -> elf_load -> elf_map -> vm_mmap -> > vm_mmap_pgoff > > Call path for normal mmap is: > > sys_mmap_pgoff -> ksys_mmap_pgoff -> vm_mmap_pgoff > > So I think the call paths converge before any real security checks > happen, and the check you're suggesting should be superfluous. (There > is some weird audit call in ksys_mmap_pgoff() but that's just to > record the FD number, so I guess that doesn't matter.) Yeah, I was just noticing this. I was over thinking. :) It does look like all that is needed is to remove __FMODE_EXEC.
On 2024/01/25 6:50, Kees Cook wrote: > Yeah, I was just noticing this. I was over thinking. :) It does look > like all that is needed is to remove __FMODE_EXEC. I worry that some out-of-tree kernel code continues using __FMODE_EXEC for opening for non-execve() purpose. If that happened, TOMOYO will be fooled... Can't we remove __FMODE_EXEC and FMODE_EXEC flag from f_flags instead of replacing current->in_execve with file->f_flags & __FMODE_EXEC ?
On Thu, Jan 25, 2024 at 3:35 PM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > > On 2024/01/25 6:50, Kees Cook wrote: > > Yeah, I was just noticing this. I was over thinking. :) It does look > > like all that is needed is to remove __FMODE_EXEC. > > I worry that some out-of-tree kernel code continues using __FMODE_EXEC for > opening for non-execve() purpose. If that happened, TOMOYO will be fooled... I just scrolled through the Github code search results for the query "__FMODE_EXEC -path:fs/exec.c -path:fs/fcntl.c -path:fs/nfs/ -path:security/tomoyo/ -path:security/apparmor/ -path:include/linux/fsnotify.h -path:nfs/dir.c -path:include/linux/fs.h -path:security/landlock/", and the only place I saw in there that sets __FMODE_EXEC, other than copies of core kernel code in weirdly named files, was this one hit in a patch for the 2.6.39 kernel to add plan9 syscalls: https://github.com/longlene/clx/blob/fdf996e0c2a7835d61ee827a82146723de76a364/sys-kernel/glendix-sources/files/glendix_2.6.39.patch#L2833 Debian codesearch also doesn't show anything relevant. So I don't think we have to be particularly worried about that.
On Wed, Jan 24, 2024 at 01:32:02PM -0800, Kees Cook wrote: > On Wed, Jan 24, 2024 at 12:47:34PM -0800, Linus Torvalds wrote: > > On Wed, 24 Jan 2024 at 12:15, Kees Cook <keescook@chromium.org> wrote: > > > > > > Hmpf, and frustratingly Ubuntu (and Debian) still builds with > > > CONFIG_USELIB, even though it was reported[2] to them almost 4 years ago. > > For completeness, Fedora hasn't had CONFIG_USELIB for a while now. > > > Well, we could just remove the __FMODE_EXEC from uselib. > > > > It's kind of wrong anyway. > > Yeah. > > > So I think just removing __FMODE_EXEC would just do the > > RightThing(tm), and changes nothing for any sane situation. > > Agreed about these: > > - fs/fcntl.c is just doing a bitfield sanity check. > > - nfs_open_permission_mask(), as you say, is only checking for > unreadable case. > > - fsnotify would also see uselib() as a read, but afaict, > that's what it would see for an mmap(), so this should > be functionally safe. > > This one, though, I need some more time to examine: > > - AppArmor, TOMOYO, and LandLock will see uselib() as an > open-for-read, so that might still be a problem? As you > say, it's more of a mmap() call, but that would mean > adding something a call like security_mmap_file() into > uselib()... If user space can emulate uselib() without opening a file with __FMODE_EXEC, then there is no security reason to keep __FMODE_EXEC for uselib(). Removing __FMODE_EXEC from uselib() looks OK for Landlock. We use __FMODE_EXEC to infer if a file is being open for execution i.e., by execve(2). If __FMODE_EXEC is removed from uselib(), I think it should also be backported to all stable kernels for consistency though. > > The issue isn't an insane "support uselib() under AppArmor" case, but > rather "Can uselib() be used to bypass exec/mmap checks?" > > This totally untested patch might give appropriate coverage: > > diff --git a/fs/exec.c b/fs/exec.c > index d179abb78a1c..0c9265312c8d 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -143,6 +143,10 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) > if (IS_ERR(file)) > goto out; > > + error = security_mmap_file(file, PROT_READ | PROT_EXEC, MAP_FIXED | MAP_SHARED); > + if (error) > + goto exit; > + > /* > * may_open() has already checked for this, so it should be > * impossible to trip now. But we need to be extra cautious > > > Of course, as you say, not having CONFIG_USELIB enabled at all is the > > _truly_ sane thing, but the only thing that used the FMODE_EXEC bit > > were landlock and some special-case nfs stuff. > > Do we want to attempt deprecation again? This was suggested last time: > https://lore.kernel.org/lkml/20200518130251.zih2s32q2rxhxg6f@wittgenstein/ > > -Kees > > -- > Kees Cook >
On 1/25/24 08:38, Mickaël Salaün wrote: > On Wed, Jan 24, 2024 at 01:32:02PM -0800, Kees Cook wrote: >> On Wed, Jan 24, 2024 at 12:47:34PM -0800, Linus Torvalds wrote: >>> On Wed, 24 Jan 2024 at 12:15, Kees Cook <keescook@chromium.org> wrote: >>>> >>>> Hmpf, and frustratingly Ubuntu (and Debian) still builds with >>>> CONFIG_USELIB, even though it was reported[2] to them almost 4 years ago. >> >> For completeness, Fedora hasn't had CONFIG_USELIB for a while now. >> >>> Well, we could just remove the __FMODE_EXEC from uselib. >>> >>> It's kind of wrong anyway. >> >> Yeah. >> >>> So I think just removing __FMODE_EXEC would just do the >>> RightThing(tm), and changes nothing for any sane situation. >> >> Agreed about these: >> >> - fs/fcntl.c is just doing a bitfield sanity check. >> >> - nfs_open_permission_mask(), as you say, is only checking for >> unreadable case. >> >> - fsnotify would also see uselib() as a read, but afaict, >> that's what it would see for an mmap(), so this should >> be functionally safe. >> >> This one, though, I need some more time to examine: >> >> - AppArmor, TOMOYO, and LandLock will see uselib() as an >> open-for-read, so that might still be a problem? As you >> say, it's more of a mmap() call, but that would mean >> adding something a call like security_mmap_file() into >> uselib()... > > If user space can emulate uselib() without opening a file with > __FMODE_EXEC, then there is no security reason to keep __FMODE_EXEC for > uselib(). > agreed > Removing __FMODE_EXEC from uselib() looks OK for Landlock. We use > __FMODE_EXEC to infer if a file is being open for execution i.e., by > execve(2). > apparmor the hint should be to avoid doing permission work again that we are doing in exec. That it regressed anything more than performance here is a bug, that will get fixed. > If __FMODE_EXEC is removed from uselib(), I think it should also be > backported to all stable kernels for consistency though. > hrmmm, I am not opposed to it being backported but I don't know that it should be backported. Consistency is good but its not a serious bug fix either > >> >> The issue isn't an insane "support uselib() under AppArmor" case, but >> rather "Can uselib() be used to bypass exec/mmap checks?" >> >> This totally untested patch might give appropriate coverage: >> >> diff --git a/fs/exec.c b/fs/exec.c >> index d179abb78a1c..0c9265312c8d 100644 >> --- a/fs/exec.c >> +++ b/fs/exec.c >> @@ -143,6 +143,10 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) >> if (IS_ERR(file)) >> goto out; >> >> + error = security_mmap_file(file, PROT_READ | PROT_EXEC, MAP_FIXED | MAP_SHARED); >> + if (error) >> + goto exit; >> + >> /* >> * may_open() has already checked for this, so it should be >> * impossible to trip now. But we need to be extra cautious >> >>> Of course, as you say, not having CONFIG_USELIB enabled at all is the >>> _truly_ sane thing, but the only thing that used the FMODE_EXEC bit >>> were landlock and some special-case nfs stuff. >> >> Do we want to attempt deprecation again? This was suggested last time: >> https://lore.kernel.org/lkml/20200518130251.zih2s32q2rxhxg6f@wittgenstein/ >> >> -Kees >> >> -- >> Kees Cook >>
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 7717354ce095..98e1150bee9d 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -469,8 +469,10 @@ static int apparmor_file_open(struct file *file) * Cache permissions granted by the previous exec check, with * implicit read and executable mmap which are required to * actually execute the image. + * + * Illogically, FMODE_EXEC is in f_flags, not f_mode. */ - if (current->in_execve) { + if (file->f_flags & __FMODE_EXEC) { fctx->allow = MAY_EXEC | MAY_READ | AA_EXEC_MMAP; return 0; } diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c index 3c3af149bf1c..04a92c3d65d4 100644 --- a/security/tomoyo/tomoyo.c +++ b/security/tomoyo/tomoyo.c @@ -328,7 +328,8 @@ static int tomoyo_file_fcntl(struct file *file, unsigned int cmd, static int tomoyo_file_open(struct file *f) { /* Don't check read permission here if called from execve(). */ - if (current->in_execve) + /* Illogically, FMODE_EXEC is in f_flags, not f_mode. */ + if (f->f_flags & __FMODE_EXEC) return 0; return tomoyo_check_open_permission(tomoyo_domain(), &f->f_path, f->f_flags);
After commit 978ffcbf00d8 ("execve: open the executable file before doing anything else"), current->in_execve was no longer in sync with the open(). This broke AppArmor and TOMOYO which depend on this flag to distinguish "open" operations from being "exec" operations. Instead of moving around in_execve, switch to using __FMODE_EXEC, which is where the "is this an exec?" intent is stored. Note that TOMOYO still uses in_execve around cred handling. Reported-by: Kevin Locke <kevin@kevinlocke.name> Closes: https://lore.kernel.org/all/ZbE4qn9_h14OqADK@kevinlocke.name Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Fixes: 978ffcbf00d8 ("execve: open the executable file before doing anything else") Cc: Josh Triplett <josh@joshtriplett.org> Cc: John Johansen <john.johansen@canonical.com> Cc: Paul Moore <paul@paul-moore.com> Cc: James Morris <jmorris@namei.org> Cc: "Serge E. Hallyn" <serge@hallyn.com> Cc: Kentaro Takeda <takedakn@nttdata.co.jp> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: Christian Brauner <brauner@kernel.org> Cc: Jan Kara <jack@suse.cz> Cc: Eric Biederman <ebiederm@xmission.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: linux-fsdevel@vger.kernel.org Cc: linux-mm@kvack.org Cc: apparmor@lists.ubuntu.com Cc: linux-security-module@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- security/apparmor/lsm.c | 4 +++- security/tomoyo/tomoyo.c | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-)