* [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper
@ 2024-01-24 16:19 Kevin Locke
2024-01-24 16:35 ` Kees Cook
0 siblings, 1 reply; 19+ messages in thread
From: Kevin Locke @ 2024-01-24 16:19 UTC (permalink / raw)
To: Linus Torvalds, Josh Triplett, Kees Cook, Mateusz Guzik, Al Viro
Cc: linux-mm, linux-fsdevel, linux-kernel
Hello Linux developers,
Using AppArmor 3.0.12 and libvirt 10.0.0 (from Debian packages) with
Linux 6.8-rc1 (unpatched), I'm unable to start KVM domains due to
AppArmor errors. Everything works fine on Linux 6.7. After attempting
to start a domain, syslog contains:
libvirtd[38705]: internal error: Child process (LIBVIRT_LOG_OUTPUTS=3:stderr /usr/lib/libvirt/virt-aa-helper -c -u libvirt-4fad83ef-4285-4cf5-953c-5c13d943c1fb) unexpected exit status 1: virt-aa-helper: error: apparmor_parser exited with error
libvirtd[38705]: internal error: cannot load AppArmor profile 'libvirt-4fad83ef-4285-4cf5-953c-5c13d943c1fb'
dmesg contains the additional message:
audit: type=1400 audit(1706112657.438:74): apparmor="DENIED" operation="open" class="file" profile="virt-aa-helper" name="/usr/sbin/apparmor_parser" pid=6333 comm="virt-aa-helper" requested_mask="r" denied_mask="r" fsuid=0 ouid=0
The libvirt-$GUID file is not created in /etc/apparmor.d/libvirt and
apparmor_parser is not executed as far as I can tell.
I've bisected the regression to 978ffcbf00d82b03b79e64b5c8249589b50e7463.
Perhaps the change in this commit causes AppArmor to deny opening
/usr/sbin/apparmor_parser in preparation for exec? For reference,
/etc/apparmor.d/usr.lib.libvirt.virt-aa-helper contains:
/{usr/,}sbin/apparmor_parser Ux,
I'd appreciate any help debugging the issue further. Let me know if I
should take it up with the AppArmor or libvirt developers to better
understand the issue.
Thanks,
Kevin
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper 2024-01-24 16:19 [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper Kevin Locke @ 2024-01-24 16:35 ` Kees Cook 2024-01-24 16:46 ` Linus Torvalds 2024-01-24 17:15 ` Kees Cook 0 siblings, 2 replies; 19+ messages in thread From: Kees Cook @ 2024-01-24 16:35 UTC (permalink / raw) To: Kevin Locke, John Johansen Cc: Linus Torvalds, Josh Triplett, Mateusz Guzik, Al Viro, linux-mm, linux-fsdevel, linux-kernel, linux-security-module On Wed, Jan 24, 2024 at 09:19:54AM -0700, Kevin Locke wrote: > Hello Linux developers, > > Using AppArmor 3.0.12 and libvirt 10.0.0 (from Debian packages) with > Linux 6.8-rc1 (unpatched), I'm unable to start KVM domains due to > AppArmor errors. Everything works fine on Linux 6.7. After attempting > to start a domain, syslog contains: > > libvirtd[38705]: internal error: Child process (LIBVIRT_LOG_OUTPUTS=3:stderr /usr/lib/libvirt/virt-aa-helper -c -u libvirt-4fad83ef-4285-4cf5-953c-5c13d943c1fb) unexpected exit status 1: virt-aa-helper: error: apparmor_parser exited with error > libvirtd[38705]: internal error: cannot load AppArmor profile 'libvirt-4fad83ef-4285-4cf5-953c-5c13d943c1fb' > > dmesg contains the additional message: > > audit: type=1400 audit(1706112657.438:74): apparmor="DENIED" operation="open" class="file" profile="virt-aa-helper" name="/usr/sbin/apparmor_parser" pid=6333 comm="virt-aa-helper" requested_mask="r" denied_mask="r" fsuid=0 ouid=0 Oh, yikes. This means the LSM lost the knowledge that this open is an _exec_, not a _read_. I will starting looking at this. John might be able to point me in the right direction more quickly, though. Thanks for the report! -Kees > > The libvirt-$GUID file is not created in /etc/apparmor.d/libvirt and > apparmor_parser is not executed as far as I can tell. > > I've bisected the regression to 978ffcbf00d82b03b79e64b5c8249589b50e7463. > Perhaps the change in this commit causes AppArmor to deny opening > /usr/sbin/apparmor_parser in preparation for exec? For reference, > /etc/apparmor.d/usr.lib.libvirt.virt-aa-helper contains: > > /{usr/,}sbin/apparmor_parser Ux, > > I'd appreciate any help debugging the issue further. Let me know if I > should take it up with the AppArmor or libvirt developers to better > understand the issue. > > Thanks, > Kevin -- Kees Cook ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper 2024-01-24 16:35 ` Kees Cook @ 2024-01-24 16:46 ` Linus Torvalds 2024-01-24 16:54 ` Linus Torvalds 2024-01-24 17:15 ` Kees Cook 1 sibling, 1 reply; 19+ messages in thread From: Linus Torvalds @ 2024-01-24 16:46 UTC (permalink / raw) To: Kees Cook Cc: Kevin Locke, John Johansen, Josh Triplett, Mateusz Guzik, Al Viro, linux-mm, linux-fsdevel, linux-kernel, linux-security-module On Wed, 24 Jan 2024 at 08:35, Kees Cook <keescook@chromium.org> wrote: > > Oh, yikes. This means the LSM lost the knowledge that this open is an > _exec_, not a _read_. > > I will starting looking at this. John might be able to point me in the > right direction more quickly, though. One obvious change in -rc1 is that the exec open was moved much earlier: commit 978ffcbf00d8 ("execve: open the executable file before doing anything else"). If the code ends up deciding "is this an exec" based on some state flag that hasn't been set, that would explain it. Something like "current->in_execve", perhaps? Linus ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper 2024-01-24 16:46 ` Linus Torvalds @ 2024-01-24 16:54 ` Linus Torvalds 2024-01-24 17:10 ` Linus Torvalds 0 siblings, 1 reply; 19+ messages in thread From: Linus Torvalds @ 2024-01-24 16:54 UTC (permalink / raw) To: Kees Cook Cc: Kevin Locke, John Johansen, Josh Triplett, Mateusz Guzik, Al Viro, linux-mm, linux-fsdevel, linux-kernel, linux-security-module On Wed, 24 Jan 2024 at 08:46, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > If the code ends up deciding "is this an exec" based on some state > flag that hasn't been set, that would explain it. > > Something like "current->in_execve", perhaps? Yeah, that looks like exactly what some of the security layer is testing. Hmm. That whole thing is disgusting. I think it should have checked FMODE_EXEC, and I have no idea why it doesn't. Linus ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper 2024-01-24 16:54 ` Linus Torvalds @ 2024-01-24 17:10 ` Linus Torvalds 2024-01-24 17:21 ` Kees Cook 2024-01-24 18:57 ` Kees Cook 0 siblings, 2 replies; 19+ messages in thread From: Linus Torvalds @ 2024-01-24 17:10 UTC (permalink / raw) To: Kees Cook Cc: Kevin Locke, John Johansen, Josh Triplett, Mateusz Guzik, Al Viro, linux-mm, linux-fsdevel, linux-kernel, linux-security-module [-- Attachment #1: Type: text/plain, Size: 848 bytes --] On Wed, 24 Jan 2024 at 08:54, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Hmm. That whole thing is disgusting. I think it should have checked > FMODE_EXEC, and I have no idea why it doesn't. Maybe because FMODE_EXEC gets set for uselib() calls too? I dunno. I think it would be even better if we had the 'intent' flags from 'struct open_flags' available, but they aren't there in the file_open() security chain. Anyway, moving current->in_execve earlier looks fairly trivial, but I worry about the randomness. I'd be *so*( much happier if this crazy flag went away, and it got changed to look at the open intent instead. Attached patch is ENTIRELY UNTESTED. And disgusting. I went back and looked. This whole disgusting thing goes back to 2009 and commit f9ce1f1cda8b ("Add in_execve flag into task_struct"). Linus [-- Attachment #2: patch.diff --] [-- Type: text/x-patch, Size: 1795 bytes --] fs/exec.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/fs/exec.c b/fs/exec.c index 8cdd5b2dd09c..fc1d6befe830 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1843,7 +1843,6 @@ static int bprm_execve(struct linux_binprm *bprm) * where setuid-ness is evaluated. */ check_unsafe_exec(bprm); - current->in_execve = 1; sched_mm_cid_before_execve(current); sched_exec(); @@ -1860,7 +1859,6 @@ static int bprm_execve(struct linux_binprm *bprm) sched_mm_cid_after_execve(current); /* execve succeeded */ current->fs->in_exec = 0; - current->in_execve = 0; rseq_execve(current); user_events_execve(current); acct_update_integrals(current); @@ -1879,7 +1877,6 @@ static int bprm_execve(struct linux_binprm *bprm) sched_mm_cid_after_execve(current); current->fs->in_exec = 0; - current->in_execve = 0; return retval; } @@ -1910,6 +1907,7 @@ static int do_execveat_common(int fd, struct filename *filename, /* We're below the limit (still or again), so we don't want to make * further execve() calls fail. */ current->flags &= ~PF_NPROC_EXCEEDED; + current->in_execve = 1; bprm = alloc_bprm(fd, filename, flags); if (IS_ERR(bprm)) { @@ -1965,6 +1963,7 @@ static int do_execveat_common(int fd, struct filename *filename, free_bprm(bprm); out_ret: + current->in_execve = 0; putname(filename); return retval; } @@ -1985,6 +1984,7 @@ int kernel_execve(const char *kernel_filename, if (IS_ERR(filename)) return PTR_ERR(filename); + current->in_execve = 1; bprm = alloc_bprm(fd, filename, 0); if (IS_ERR(bprm)) { retval = PTR_ERR(bprm); @@ -2024,6 +2024,7 @@ int kernel_execve(const char *kernel_filename, out_free: free_bprm(bprm); out_ret: + current->in_execve = 0; putname(filename); return retval; } ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper 2024-01-24 17:10 ` Linus Torvalds @ 2024-01-24 17:21 ` Kees Cook 2024-01-24 17:27 ` Linus Torvalds 2024-01-24 18:57 ` Kees Cook 1 sibling, 1 reply; 19+ messages in thread From: Kees Cook @ 2024-01-24 17:21 UTC (permalink / raw) To: Linus Torvalds Cc: Kevin Locke, John Johansen, Josh Triplett, Mateusz Guzik, Al Viro, linux-mm, linux-fsdevel, linux-kernel, linux-security-module On Wed, Jan 24, 2024 at 09:10:58AM -0800, Linus Torvalds wrote: > On Wed, 24 Jan 2024 at 08:54, Linus Torvalds > <torvalds@linux-foundation.org> wrote: > > > > Hmm. That whole thing is disgusting. I think it should have checked > > FMODE_EXEC, and I have no idea why it doesn't. > > Maybe because FMODE_EXEC gets set for uselib() calls too? I dunno. I > think it would be even better if we had the 'intent' flags from > 'struct open_flags' available, but they aren't there in the > file_open() security chain. I think there were other problems that I might have already fixed when I reorganized things in commit 0fd338b2d2cd ("exec: move path_noexec() check earlier") to more correctly map to LSM checks. > Anyway, moving current->in_execve earlier looks fairly trivial, but I > worry about the randomness. I'd be *so*( much happier if this crazy > flag went away, and it got changed to look at the open intent instead. > > Attached patch is ENTIRELY UNTESTED. And disgusting. I opted to tie "current->in_execve" lifetime to bprm lifetime just to have a clean boundary (i.e. strictly in alloc/free_bprm()). -Kees -- Kees Cook ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper 2024-01-24 17:21 ` Kees Cook @ 2024-01-24 17:27 ` Linus Torvalds 2024-01-24 18:27 ` Linus Torvalds 0 siblings, 1 reply; 19+ messages in thread From: Linus Torvalds @ 2024-01-24 17:27 UTC (permalink / raw) To: Kees Cook Cc: Kevin Locke, John Johansen, Josh Triplett, Mateusz Guzik, Al Viro, linux-mm, linux-fsdevel, linux-kernel, linux-security-module On Wed, 24 Jan 2024 at 09:21, Kees Cook <keescook@chromium.org> wrote: > > I opted to tie "current->in_execve" lifetime to bprm lifetime just to > have a clean boundary (i.e. strictly in alloc/free_bprm()). Honestly, the less uinnecessary churn that horrible flag causes, the better. IOW, I think the goal here should be "minimal fix" followed by "remove that horrendous thing". Linus ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper 2024-01-24 17:27 ` Linus Torvalds @ 2024-01-24 18:27 ` Linus Torvalds 2024-01-24 18:29 ` Linus Torvalds ` (2 more replies) 0 siblings, 3 replies; 19+ messages in thread From: Linus Torvalds @ 2024-01-24 18:27 UTC (permalink / raw) To: Kees Cook, Kentaro Takeda, Tetsuo Handa, John Johansen, Paul Moore Cc: Kevin Locke, Josh Triplett, Mateusz Guzik, Al Viro, linux-mm, linux-fsdevel, linux-kernel, linux-security-module [-- Attachment #1: Type: text/plain, Size: 1450 bytes --] On Wed, 24 Jan 2024 at 09:27, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > IOW, I think the goal here should be "minimal fix" followed by "remove > that horrendous thing". Ugh. The tomoyo use is even *more* disgusting, in how it uses it for "tomoyo_domain()" entirely independently of even the ->file_open() callback. So for tomoyo, it's not about the file open, it's about tomoyo_cred_prepare() and friends. So the patch I posted probably fixes apparmor, but only breaks tomoyo instead, because tomoyo really does seem to use it around the whole security_bprm_creds_for_exec() thing. Now, tomoyo *also* uses it for the file_open() callback, just to confuse things. IOW, I think the right thing to do is to split this in two: - leave the existing ->in_execve for the bprm_creds dance in boprm_execve(). Horrendous and disgusing. - the ->file_open() thing is changed to check file->f_flags (with a comment about how FMODE_EXEC is in f_flags, not f_mode like it should be). IOW, I think the patch I posted earlier - and Kees' version of the same thing - is just broken. This attached patch might work. And as noted, since it checks __FMODE_EXEC, it now allows the uselib() case too. I think that's ok. UNTESTED. But I think this is at least a movement in the right direction. The whole cred use of current->in_execve in tomoyo should *also* be fixed, but I didn't even try to follow what it actually wanted. Linus [-- Attachment #2: patch.diff --] [-- Type: text/x-patch, Size: 1366 bytes --] security/apparmor/lsm.c | 4 +++- security/tomoyo/tomoyo.c | 5 +++-- 2 files changed, 6 insertions(+), 3 deletions(-) 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..e8fb02b716aa 100644 --- a/security/tomoyo/tomoyo.c +++ b/security/tomoyo/tomoyo.c @@ -327,8 +327,9 @@ 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) + /* Don't check read permission here if execve(). */ + /* Illogically, FMODE_EXEC is in f_flags, not f_mode. */ + if (file->f_flags & __FMODE_EXEC) return 0; return tomoyo_check_open_permission(tomoyo_domain(), &f->f_path, f->f_flags); ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper 2024-01-24 18:27 ` Linus Torvalds @ 2024-01-24 18:29 ` Linus Torvalds 2024-01-24 19:02 ` Kees Cook 2024-01-25 14:16 ` Tetsuo Handa 2 siblings, 0 replies; 19+ messages in thread From: Linus Torvalds @ 2024-01-24 18:29 UTC (permalink / raw) To: Kees Cook, Kentaro Takeda, Tetsuo Handa, John Johansen, Paul Moore Cc: Kevin Locke, Josh Triplett, Mateusz Guzik, Al Viro, linux-mm, linux-fsdevel, linux-kernel, linux-security-module On Wed, 24 Jan 2024 at 10:27, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > UNTESTED .. and just to check who is awake, I used 'file->f_flags & __FMODE_EXEC' in tomoyo when 'file' doesn't exist as a variable. It should be 'f->f_flags & __FMODE_EXEC'. That way it at least compiles. Linus ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper 2024-01-24 18:27 ` Linus Torvalds 2024-01-24 18:29 ` Linus Torvalds @ 2024-01-24 19:02 ` Kees Cook 2024-01-24 19:41 ` Linus Torvalds 2024-01-25 14:16 ` Tetsuo Handa 2 siblings, 1 reply; 19+ messages in thread From: Kees Cook @ 2024-01-24 19:02 UTC (permalink / raw) To: Linus Torvalds Cc: Kentaro Takeda, Tetsuo Handa, John Johansen, Paul Moore, Kevin Locke, Josh Triplett, Mateusz Guzik, Al Viro, linux-mm, linux-fsdevel, linux-kernel, linux-security-module On Wed, Jan 24, 2024 at 10:27:03AM -0800, Linus Torvalds wrote: > On Wed, 24 Jan 2024 at 09:27, Linus Torvalds > <torvalds@linux-foundation.org> wrote: > > > > IOW, I think the goal here should be "minimal fix" followed by "remove > > that horrendous thing". > > Ugh. The tomoyo use is even *more* disgusting, in how it uses it for > "tomoyo_domain()" entirely independently of even the ->file_open() > callback. Yeah, I just sent a similar email. > So for tomoyo, it's not about the file open, it's about > tomoyo_cred_prepare() and friends. Yeah, it looks like it should happily follow cred lifetime, but I haven't fully convinced myself. > So the patch I posted probably fixes apparmor, but only breaks tomoyo > instead, because tomoyo really does seem to use it around the whole > security_bprm_creds_for_exec() thing. > > Now, tomoyo *also* uses it for the file_open() callback, just to confuse things. > > IOW, I think the right thing to do is to split this in two: > > - leave the existing ->in_execve for the bprm_creds dance in > boprm_execve(). Horrendous and disgusing. Agreed. > - the ->file_open() thing is changed to check file->f_flags Agreed. (And I've tested this for AppArmor now. I can confirm the failure case -- it's only for profile transitions, which is why I didn't see it originally in testing. > IOW, I think the patch I posted earlier - and Kees' version of the > same thing - is just broken. This attached patch might work. Yup. Should I post a formal patch, or do you want to commit what you've got (with the "file" -> "f" fix)? -Kees -- Kees Cook ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper 2024-01-24 19:02 ` Kees Cook @ 2024-01-24 19:41 ` Linus Torvalds 0 siblings, 0 replies; 19+ messages in thread From: Linus Torvalds @ 2024-01-24 19:41 UTC (permalink / raw) To: Kees Cook Cc: Kentaro Takeda, Tetsuo Handa, John Johansen, Paul Moore, Kevin Locke, Josh Triplett, Mateusz Guzik, Al Viro, linux-mm, linux-fsdevel, linux-kernel, linux-security-module On Wed, 24 Jan 2024 at 11:02, Kees Cook <keescook@chromium.org> wrote: > > Yup. Should I post a formal patch, or do you want to commit what you've > got (with the "file" -> "f" fix)? I took your formal patch. Thanks, Linus ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper 2024-01-24 18:27 ` Linus Torvalds 2024-01-24 18:29 ` Linus Torvalds 2024-01-24 19:02 ` Kees Cook @ 2024-01-25 14:16 ` Tetsuo Handa 2024-01-25 17:17 ` Linus Torvalds 2 siblings, 1 reply; 19+ messages in thread From: Tetsuo Handa @ 2024-01-25 14:16 UTC (permalink / raw) To: Linus Torvalds, Kees Cook, John Johansen, Paul Moore Cc: Kevin Locke, Josh Triplett, Mateusz Guzik, Al Viro, linux-mm, linux-fsdevel, linux-kernel, linux-security-module, Kentaro Takeda On 2024/01/25 3:27, Linus Torvalds wrote: > The whole cred use of current->in_execve in tomoyo should > *also* be fixed, but I didn't even try to follow what it actually > wanted. Due to TOMOYO's unique domain transition (transits to new domain before execve() succeeds and returns to old domain if execve() failed), TOMOYO depends on a tricky ordering shown below. ---------- // a caller tries execve(). sys_execve() { do_execve() { do_execveat_common() { bprm_execve() { prepare_bprm_creds() { prepare_exec_creds() { prepare_creds() { security_prepare_creds() { tomoyo_cred_prepare() { if (s->old_domain_info && !current->in_execve) { // false because s->old_domain_info == NULL. s->domain_info = s->old_domain_info; s->old_domain_info = NULL; } } } } } } current->in_execve = 1; do_open_execat() { (...snipped...) security_file_open() { tomoyo_file_open() // Not checked because current->in_execve == 1. } (...snipped...) } exec_binprm() { search_binary_handler() { security_bprm_check() { tomoyo_bprm_check_security() { if (!s->old_domain_info) { // true because s->old_domain_info == NULL. tomoyo_find_next_domain() { // Checks execute permission here. s->old_domain_info = s->domain_info; // Remember old domain. s->domain_info = domain; // Transit to new domain. } } } } fmt->load_binary() { // e.g. load_script() in fs/binfmt_script.c open_exec() { // Not checked because current->in_execve == 1. } } } search_binary_handler() { security_bprm_check() { tomoyo_bprm_check_security() { if (!s->old_domain_info) { // false because s->old_domain_info != NULL. } else { // Checks read permission here. } } } } // An error happens after s->domain_info was updated. } current->in_execve = 0; // No chance to restore s->domain_info. } } } // returning an error code to the caller. } // the caller retries execve(). sys_execve() { do_execve() { do_execveat_common() { bprm_execve() { prepare_bprm_creds() { prepare_exec_creds() { prepare_creds() { security_prepare_creds() { tomoyo_cred_prepare() { if (s->old_domain_info && !current->in_execve) { // true because s->old_domain_info != NULL && current->in_execve == 0. s->domain_info = s->old_domain_info; // Transit to old domain. s->old_domain_info = NULL; } } } } } } current->in_execve = 1; do_open_execat() { (...snipped...) security_file_open() { tomoyo_file_open() // Not checked because current->in_execve == 1. } (...snipped...) } exec_binprm() { search_binary_handler() { security_bprm_check() { tomoyo_bprm_check_security() { if (!s->old_domain_info) { // true because s->old_domain_info == NULL. tomoyo_find_next_domain() { // Checks execute permission here. s->old_domain_info = s->domain_info; // Remember old domain. s->domain_info = domain; // Transit to new domain. } } } } fmt->load_binary() { // e.g. load_script() in fs/binfmt_script.c open_exec() { // Not checked because current->in_execve == 1. } } } search_binary_handler() { security_bprm_check() { tomoyo_bprm_check_security() { if (!s->old_domain_info) { // false because s->old_domain_info != NULL. } else { // Checks read permission here. } } } } fmt->load_binary() { // e.g. load_elf_binary() in fs/binfmt_elf.c begin_new_exec() { security_bprm_committed_creds() { tomoyo_bprm_committed_creds() { s->old_domain_info = NULL; // Forget old domain. } } } } } current->in_execve = 0; } } } } ---------- Commit 978ffcbf00d8 ("execve: open the executable file before doing anything else") broke the ordering and commit 4759ff71f23e ("exec: Check __FMODE_EXEC instead of in_execve for LSMs") and commit 3eab830189d9 ("uselib: remove use of __FMODE_EXEC") fixed the regression. But current->in_execve remains required unless an LSM callback that is called when an execve() request failed which existed as security_bprm_free() until Linux 2.6.28 revives... ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper 2024-01-25 14:16 ` Tetsuo Handa @ 2024-01-25 17:17 ` Linus Torvalds 2024-01-27 7:04 ` Tetsuo Handa 0 siblings, 1 reply; 19+ messages in thread From: Linus Torvalds @ 2024-01-25 17:17 UTC (permalink / raw) To: Tetsuo Handa Cc: Kees Cook, John Johansen, Paul Moore, Kevin Locke, Josh Triplett, Mateusz Guzik, Al Viro, linux-mm, linux-fsdevel, linux-kernel, linux-security-module, Kentaro Takeda On Thu, 25 Jan 2024 at 06:17, Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > > On 2024/01/25 3:27, Linus Torvalds wrote: > > The whole cred use of current->in_execve in tomoyo should > > *also* be fixed, but I didn't even try to follow what it actually > > wanted. > > Due to TOMOYO's unique domain transition (transits to new domain before > execve() succeeds and returns to old domain if execve() failed), TOMOYO > depends on a tricky ordering shown below. Ok, that doesn't really clarify anything for me. I'm less interested in what the call paths are, and more like "_Why_ is all this needed for tomoyo?" Why doesn't tomoyo just install the new cred at "commit_creds()" time? (The security hooks that surround that are "->bprm_committing_creds()" and "->bprm_committed_creds()") IOW, the whole "save things across two *independent* execve() calls" seems crazy. Very strange and confusing. Linus ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper 2024-01-25 17:17 ` Linus Torvalds @ 2024-01-27 7:04 ` Tetsuo Handa 2024-01-27 11:00 ` Tetsuo Handa 0 siblings, 1 reply; 19+ messages in thread From: Tetsuo Handa @ 2024-01-27 7:04 UTC (permalink / raw) To: Linus Torvalds Cc: Kees Cook, John Johansen, Paul Moore, Kevin Locke, Josh Triplett, Mateusz Guzik, Al Viro, linux-mm, linux-fsdevel, linux-kernel, linux-security-module, Kentaro Takeda On 2024/01/26 2:17, Linus Torvalds wrote: > On Thu, 25 Jan 2024 at 06:17, Tetsuo Handa > <penguin-kernel@i-love.sakura.ne.jp> wrote: >> >> On 2024/01/25 3:27, Linus Torvalds wrote: >>> The whole cred use of current->in_execve in tomoyo should >>> *also* be fixed, but I didn't even try to follow what it actually >>> wanted. >> >> Due to TOMOYO's unique domain transition (transits to new domain before >> execve() succeeds and returns to old domain if execve() failed), TOMOYO >> depends on a tricky ordering shown below. > > Ok, that doesn't really clarify anything for me. > > I'm less interested in what the call paths are, and more like "_Why_ > is all this needed for tomoyo?" > > Why doesn't tomoyo just install the new cred at "commit_creds()" time? > > (The security hooks that surround that are > "->bprm_committing_creds()" and "->bprm_committed_creds()") DAC checks permission for any files accessed by a new program passed to execve() until the point of no return of execve() using the credentials of current program. But TOMOYO checks permission for any files accessed by a new program passed to execve() using a domain for that new program than a domain for current program. This is because TOMOYO considers that if a new program passed to execve() requires some file, permissions for accessing that file should be checked using the security context for that new program. Let's consider executing a shell script named /tmp/foo.sh from /bin/bash . [user@host ~]$ cat /tmp/foo.sh #!/bin/sh echo hello [user@host ~]$ chmod 755 /tmp/foo.sh [user@host ~]$ exec /tmp/foo.sh DAC checks permissions for /tmp/foo.sh and /bin/sh using the credentials of /bin/bash process, and checks permissions for shared libraries needed by /bin/sh using the new credentials of /tmp/foo.sh process. TOMOYO checks permissions for /tmp/foo.sh using the domain for /bin/bash process, and checks permissions for /bin/sh and permissions for shared libraries needed by /bin/sh using the domain for /tmp/foo.sh process. TOMOYO treats "/tmp/foo.sh needs to load /bin/sh" and "/tmp/foo.sh needs to load shared libraries needed by /bin/sh" in the same manner, by checking "open for read" permission. Since the COW cred mechanism introduced in Linux 2.6.29 cannot support such model, TOMOYO uses "struct task_struct"->security and does not use "struct cred"->security. > > IOW, the whole "save things across two *independent* execve() calls" > seems crazy. > > Very strange and confusing. > > Linus Since curity_bprm_free() callback was removed in Linux 2.6.29 because COW cred mechanism does not need it, currently I have to use such a crazy hack. Revival of security_task_alloc()/security_task_free()/security_bprm_free() was proposed in 2011 at https://lkml.kernel.org/r/201104202119.FAI21341.HtOJFSOVLFMOFQ@I-love.SAKURA.ne.jp and https://lkml.kernel.org/r/201104202120.FEJ57865.MFSOFFHVOOJLQt@I-love.SAKURA.ne.jp . security_task_alloc()/security_task_free() has been revived, but security_bprm_free() is not revived yet. If we can accept revival of security_bprm_free(), we can "get rid of current->in_execve flag" and "stop saving things across two *independent* execve() calls". ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper 2024-01-27 7:04 ` Tetsuo Handa @ 2024-01-27 11:00 ` Tetsuo Handa 2024-01-27 11:23 ` Tetsuo Handa 0 siblings, 1 reply; 19+ messages in thread From: Tetsuo Handa @ 2024-01-27 11:00 UTC (permalink / raw) To: Linus Torvalds, John Johansen Cc: Kees Cook, Paul Moore, Kevin Locke, Josh Triplett, Mateusz Guzik, Al Viro, linux-mm, linux-fsdevel, linux-kernel, linux-security-module, Kentaro Takeda On 2024/01/27 16:04, Tetsuo Handa wrote: > If we can accept revival of security_bprm_free(), we can "get rid of current->in_execve flag" > and "stop saving things across two *independent* execve() calls". Oops, I found a bug in TOMOYO (and possibly in AppArmor as well). TOMOYO has to continue depending on current->in_execve flag even if security_bprm_free() is revived. v6.7 and current linux.git check the following permissions. ---------------------------------------- <kernel> /usr/lib/systemd/systemd /etc/mail/make /usr/bin/grep 0: file getattr /etc/ld.so.cache 1: file getattr /etc/mail/sendmail.cf 2: file getattr /usr/lib/locale/locale-archive 3: file getattr /usr/lib64/gconv/gconv-modules.cache 4: file getattr /usr/lib64/libc-2.17.so 5: file getattr /usr/lib64/libpcre.so.1.2.0 6: file getattr /usr/lib64/libpthread-2.17.so 7: file getattr pipe:[24810] 8: file ioctl /etc/mail/sendmail.cf 0x5401 9: file ioctl pipe:[24810] 0x5401 10: file read /etc/ld.so.cache 11: file read /etc/mail/sendmail.cf 12: file read /usr/lib/locale/locale-archive 13: file read /usr/lib64/gconv/gconv-modules.cache 14: file read /usr/lib64/libc-2.17.so 15: file read /usr/lib64/libpcre.so.1.2.0 16: file read /usr/lib64/libpthread-2.17.so 17: misc env LANG 18: misc env OLDPWD 19: misc env PATH 20: misc env PWD 21: misc env SENDMAIL_OPTS 22: misc env SHLVL 23: misc env _ 24: use_group 0 ---------------------------------------- But due to "if (f->f_flags & __FMODE_EXEC)" test in current linux.git (or "if (current->in_execve)" test until v6.7), currently permission for the ELF loader file (i.e. /lib64/ld-linux-x86-64.so.2 shown below) is not checked. $ ldd /usr/bin/grep linux-vdso.so.1 => (0x00007ffc079ac000) libpcre.so.1 => /lib64/libpcre.so.1 (0x00007fdcfb000000) libc.so.6 => /lib64/libc.so.6 (0x00007fdcfac00000) libpthread.so.0 => /lib64/libpthread.so.0 (0x00007fdcfa800000) /lib64/ld-linux-x86-64.so.2 (0x00007fdcfb400000) If I make below change ---------------------------------------- diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c index 04a92c3d65d4..34739e4ba5a4 100644 --- a/security/tomoyo/tomoyo.c +++ b/security/tomoyo/tomoyo.c @@ -329,8 +329,8 @@ static int tomoyo_file_open(struct file *f) { /* Don't check read permission here if called from execve(). */ /* Illogically, FMODE_EXEC is in f_flags, not f_mode. */ - if (f->f_flags & __FMODE_EXEC) - return 0; + //if (f->f_flags & __FMODE_EXEC) + // return 0; return tomoyo_check_open_permission(tomoyo_domain(), &f->f_path, f->f_flags); } ---------------------------------------- permission for the ELF loader file is checked, but causes the caller to require read permission for /usr/bin/grep in addition to execute permission. ---------------------------------------- <kernel> /usr/lib/systemd/systemd /etc/mail/make /usr/bin/grep 0: file getattr /etc/ld.so.cache 1: file getattr /etc/mail/sendmail.cf 2: file getattr /usr/lib/locale/locale-archive 3: file getattr /usr/lib64/gconv/gconv-modules.cache 4: file getattr /usr/lib64/libc-2.17.so 5: file getattr /usr/lib64/libpcre.so.1.2.0 6: file getattr /usr/lib64/libpthread-2.17.so 7: file getattr pipe:[22370] 8: file ioctl /etc/mail/sendmail.cf 0x5401 9: file ioctl pipe:[22370] 0x5401 10: file read /etc/ld.so.cache 11: file read /etc/mail/sendmail.cf 12: file read /usr/lib/locale/locale-archive 13: file read /usr/lib64/gconv/gconv-modules.cache 14: file read /usr/lib64/ld-2.17.so 15: file read /usr/lib64/libc-2.17.so 16: file read /usr/lib64/libpcre.so.1.2.0 17: file read /usr/lib64/libpthread-2.17.so 18: misc env LANG 19: misc env OLDPWD 20: misc env PATH 21: misc env PWD 22: misc env SENDMAIL_OPTS 23: misc env SHLVL 24: misc env _ 25: use_group 0 ---------------------------------------- To make it possible to check permission for the ELF loader file without requiring the caller of execve() read permission of a program specified in execve() request, I need to ---------------------------------------- diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c index 04a92c3d65d4..942c08a36027 100644 --- a/security/tomoyo/tomoyo.c +++ b/security/tomoyo/tomoyo.c @@ -104,11 +104,6 @@ static int tomoyo_bprm_check_security(struct linux_binprm *bprm) tomoyo_read_unlock(idx); return err; } - /* - * Read permission is checked against interpreters using next domain. - */ - return tomoyo_check_open_permission(s->domain_info, - &bprm->file->f_path, O_RDONLY); } /** @@ -327,9 +322,13 @@ 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(). */ - /* Illogically, FMODE_EXEC is in f_flags, not f_mode. */ - if (f->f_flags & __FMODE_EXEC) + /* + * Don't check read permission here if called from execve() for + * the first time of that execve() request, for execute permission + * will be checked at tomoyo_bprm_check_security() with argv/envp + * taken into account. + */ + if (current->in_execve && !tomoyo_task(current)->old_domain_info) return 0; return tomoyo_check_open_permission(tomoyo_domain(), &f->f_path, f->f_flags); ---------------------------------------- in addition to moving around in_execve and reviving security_bprm_free(). Apparmor uses similar approach, which is based on an assumption that permission check is done by bprm hooks. ---------------------------------------- static int apparmor_file_open(struct file *file) { struct aa_file_ctx *fctx = file_ctx(file); struct aa_label *label; int error = 0; if (!path_mediated_fs(file->f_path.dentry)) return 0; /* If in exec, permission is handled by bprm hooks. * 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 (file->f_flags & __FMODE_EXEC) { fctx->allow = MAY_EXEC | MAY_READ | AA_EXEC_MMAP; return 0; } --------------------------------------- https://lkml.kernel.org/r/4bb5dd09-9e09-477b-9ea8-d7b9d2fb4760@canonical.com : > 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. But I can't find apparmor_bprm_check_security() callback. AppArmor uses apparmor_bprm_creds_for_exec() callback, but security_bprm_creds_for_exec() is called for only once for each execve() request. That is, apparmor_bprm_creds_for_exec() is not suitable for checking permission for interpreter or ELF loader, is it? https://lkml.kernel.org/r/ff9a525e-8c39-4590-9ace-57f4426cbe74@canonical.com : > that this even tripped a regression is a bug that I am going to > have to chase down. The file check at this point should just be > redundant. Then, how does AppArmor check permissions for files opened for interpreter or ELF loader? AppArmor does not check permissions for files opened for interpreter and ELF loader (i.e. accepting any malicious binary file being specified)? ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper 2024-01-27 11:00 ` Tetsuo Handa @ 2024-01-27 11:23 ` Tetsuo Handa 0 siblings, 0 replies; 19+ messages in thread From: Tetsuo Handa @ 2024-01-27 11:23 UTC (permalink / raw) To: Linus Torvalds Cc: Kees Cook, Paul Moore, Kevin Locke, Josh Triplett, Mateusz Guzik, Al Viro, linux-mm, linux-fsdevel, linux-kernel, linux-security-module, Kentaro Takeda, John Johansen On 2024/01/27 20:00, Tetsuo Handa wrote: > On 2024/01/27 16:04, Tetsuo Handa wrote: >> If we can accept revival of security_bprm_free(), we can "get rid of current->in_execve flag" >> and "stop saving things across two *independent* execve() calls". > > Oops, I found a bug in TOMOYO (and possibly in AppArmor as well). > TOMOYO has to continue depending on current->in_execve flag even if > security_bprm_free() is revived. No. We can "get rid of current->in_execve flag" and "stop saving things across two *independent* execve() calls". > @@ -327,9 +322,13 @@ 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(). */ > - /* Illogically, FMODE_EXEC is in f_flags, not f_mode. */ > - if (f->f_flags & __FMODE_EXEC) > + /* > + * Don't check read permission here if called from execve() for > + * the first time of that execve() request, for execute permission > + * will be checked at tomoyo_bprm_check_security() with argv/envp > + * taken into account. > + */ > + if (current->in_execve && !tomoyo_task(current)->old_domain_info) Since "f->f_flags & __FMODE_EXEC" == "current->in_execve", TOMOYO can continue using "f->f_flags & __FMODE_EXEC", provided that tomoyo_task(current)->old_domain_info is reset to NULL via security_bprm_free() callback when previous execve() request failed. That is, if security_bprm_free() is revived, we can also get rid of current->in_execve. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper 2024-01-24 17:10 ` Linus Torvalds 2024-01-24 17:21 ` Kees Cook @ 2024-01-24 18:57 ` Kees Cook 2024-01-27 5:17 ` John Johansen 1 sibling, 1 reply; 19+ messages in thread From: Kees Cook @ 2024-01-24 18:57 UTC (permalink / raw) To: Linus Torvalds Cc: Kevin Locke, John Johansen, Kentaro Takeda, Tetsuo Handa, Josh Triplett, Mateusz Guzik, Al Viro, linux-mm, linux-fsdevel, linux-kernel, linux-security-module On Wed, Jan 24, 2024 at 09:10:58AM -0800, Linus Torvalds wrote: > On Wed, 24 Jan 2024 at 08:54, Linus Torvalds > <torvalds@linux-foundation.org> wrote: > > > > Hmm. That whole thing is disgusting. I think it should have checked > > FMODE_EXEC, and I have no idea why it doesn't. > > Maybe because FMODE_EXEC gets set for uselib() calls too? I dunno. I > think it would be even better if we had the 'intent' flags from > 'struct open_flags' available, but they aren't there in the > file_open() security chain. I've tested AppArmor, and this works fine: diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 7717354ce095..ab104ce05f96 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -470,7 +470,7 @@ static int apparmor_file_open(struct file *file) * implicit read and executable mmap which are required to * actually execute the image. */ - if (current->in_execve) { + if (file->f_flags & __FMODE_EXEC) { fctx->allow = MAY_EXEC | MAY_READ | AA_EXEC_MMAP; return 0; } Converting TOMOYO is less obvious to me, though, as it has a helper that isn't strictly always called during open(). I haven't finished figuring out the call graphs for it... -- Kees Cook ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper 2024-01-24 18:57 ` Kees Cook @ 2024-01-27 5:17 ` John Johansen 0 siblings, 0 replies; 19+ messages in thread From: John Johansen @ 2024-01-27 5:17 UTC (permalink / raw) To: Kees Cook, Linus Torvalds Cc: Kevin Locke, Kentaro Takeda, Tetsuo Handa, Josh Triplett, Mateusz Guzik, Al Viro, linux-mm, linux-fsdevel, linux-kernel, linux-security-module On 1/24/24 10:57, Kees Cook wrote: > On Wed, Jan 24, 2024 at 09:10:58AM -0800, Linus Torvalds wrote: >> On Wed, 24 Jan 2024 at 08:54, Linus Torvalds >> <torvalds@linux-foundation.org> wrote: >>> >>> Hmm. That whole thing is disgusting. I think it should have checked >>> FMODE_EXEC, and I have no idea why it doesn't. >> >> Maybe because FMODE_EXEC gets set for uselib() calls too? I dunno. I >> think it would be even better if we had the 'intent' flags from >> 'struct open_flags' available, but they aren't there in the >> file_open() security chain. > > I've tested AppArmor, and this works fine: > thanks. I also ran it through the regression test suit, to double check so that Murphy doesn't bite. that this even tripped a regression is a bug that I am going to have to chase down. The file check at this point should just be redundant. thanks for the quick fix > diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c > index 7717354ce095..ab104ce05f96 100644 > --- a/security/apparmor/lsm.c > +++ b/security/apparmor/lsm.c > @@ -470,7 +470,7 @@ static int apparmor_file_open(struct file *file) > * implicit read and executable mmap which are required to > * actually execute the image. > */ > - if (current->in_execve) { > + if (file->f_flags & __FMODE_EXEC) { > fctx->allow = MAY_EXEC | MAY_READ | AA_EXEC_MMAP; > return 0; > } > > Converting TOMOYO is less obvious to me, though, as it has a helper that > isn't strictly always called during open(). I haven't finished figuring > out the call graphs for it... > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper 2024-01-24 16:35 ` Kees Cook 2024-01-24 16:46 ` Linus Torvalds @ 2024-01-24 17:15 ` Kees Cook 1 sibling, 0 replies; 19+ messages in thread From: Kees Cook @ 2024-01-24 17:15 UTC (permalink / raw) To: Kevin Locke, John Johansen Cc: Linus Torvalds, Josh Triplett, Mateusz Guzik, Al Viro, linux-mm, linux-fsdevel, linux-kernel, linux-security-module On Wed, Jan 24, 2024 at 08:35:29AM -0800, Kees Cook wrote: > On Wed, Jan 24, 2024 at 09:19:54AM -0700, Kevin Locke wrote: > > Hello Linux developers, > > > > Using AppArmor 3.0.12 and libvirt 10.0.0 (from Debian packages) with > > Linux 6.8-rc1 (unpatched), I'm unable to start KVM domains due to > > AppArmor errors. Everything works fine on Linux 6.7. After attempting > > to start a domain, syslog contains: > > > > libvirtd[38705]: internal error: Child process (LIBVIRT_LOG_OUTPUTS=3:stderr /usr/lib/libvirt/virt-aa-helper -c -u libvirt-4fad83ef-4285-4cf5-953c-5c13d943c1fb) unexpected exit status 1: virt-aa-helper: error: apparmor_parser exited with error > > libvirtd[38705]: internal error: cannot load AppArmor profile 'libvirt-4fad83ef-4285-4cf5-953c-5c13d943c1fb' > > > > dmesg contains the additional message: > > > > audit: type=1400 audit(1706112657.438:74): apparmor="DENIED" operation="open" class="file" profile="virt-aa-helper" name="/usr/sbin/apparmor_parser" pid=6333 comm="virt-aa-helper" requested_mask="r" denied_mask="r" fsuid=0 ouid=0 > > Oh, yikes. This means the LSM lost the knowledge that this open is an > _exec_, not a _read_. > > I will starting looking at this. John might be able to point me in the > right direction more quickly, though. Here's a possible patch for in_execve. Can you test this? I'm going to also examine switching to FMODE_EXEC ... I think I know why this wasn't done in the past, but I have to check the history... diff --git a/fs/exec.c b/fs/exec.c index 39d773021fff..ddd0fa2e84a7 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1505,7 +1505,7 @@ static int prepare_bprm_creds(struct linux_binprm *bprm) /* Matches do_open_execat() */ static void do_close_execat(struct file *file) { - if (!file) + if (IS_ERR_OR_NULL(file)) return; allow_write_access(file); fput(file); @@ -1530,23 +1530,30 @@ static void free_bprm(struct linux_binprm *bprm) kfree(bprm->interp); kfree(bprm->fdpath); kfree(bprm); + current->in_execve = 0; } static struct linux_binprm *alloc_bprm(int fd, struct filename *filename, int flags) { - struct linux_binprm *bprm; - struct file *file; + struct linux_binprm *bprm = NULL; + struct file *file = NULL; int retval = -ENOMEM; + /* + * Mark this "open" as an exec attempt for the LSMs. We reset + * it in bprm_free() (and our common error path below). + */ + current->in_execve = 1; + file = do_open_execat(fd, filename, flags); - if (IS_ERR(file)) - return ERR_CAST(file); + if (IS_ERR(file)) { + retval = PTR_ERR(file); + goto out_cleanup; + } bprm = kzalloc(sizeof(*bprm), GFP_KERNEL); - if (!bprm) { - do_close_execat(file); - return ERR_PTR(-ENOMEM); - } + if (!bprm) + goto out_cleanup; bprm->file = file; @@ -1559,7 +1566,7 @@ static struct linux_binprm *alloc_bprm(int fd, struct filename *filename, int fl bprm->fdpath = kasprintf(GFP_KERNEL, "/dev/fd/%d/%s", fd, filename->name); if (!bprm->fdpath) - goto out_free; + goto out_cleanup; /* * Record that a name derived from an O_CLOEXEC fd will be @@ -1581,8 +1588,11 @@ static struct linux_binprm *alloc_bprm(int fd, struct filename *filename, int fl if (!retval) return bprm; -out_free: - free_bprm(bprm); +out_cleanup: + if (bprm) + free_bprm(bprm); + do_close_execat(file); + current->in_execve = 0; return ERR_PTR(retval); } @@ -1633,6 +1643,7 @@ static void check_unsafe_exec(struct linux_binprm *bprm) } rcu_read_unlock(); + /* "users" and "in_exec" locked for copy_fs() */ if (p->fs->users > n_fs) bprm->unsafe |= LSM_UNSAFE_SHARE; else @@ -1863,7 +1874,6 @@ static int bprm_execve(struct linux_binprm *bprm) * where setuid-ness is evaluated. */ check_unsafe_exec(bprm); - current->in_execve = 1; sched_mm_cid_before_execve(current); sched_exec(); @@ -1880,7 +1890,6 @@ static int bprm_execve(struct linux_binprm *bprm) sched_mm_cid_after_execve(current); /* execve succeeded */ current->fs->in_exec = 0; - current->in_execve = 0; rseq_execve(current); user_events_execve(current); acct_update_integrals(current); @@ -1899,7 +1908,6 @@ static int bprm_execve(struct linux_binprm *bprm) sched_mm_cid_after_execve(current); current->fs->in_exec = 0; - current->in_execve = 0; return retval; } diff --git a/kernel/fork.c b/kernel/fork.c index 47ff3b35352e..0d944e92a43f 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -1748,6 +1748,7 @@ static int copy_fs(unsigned long clone_flags, struct task_struct *tsk) if (clone_flags & CLONE_FS) { /* tsk->fs is already what we want */ spin_lock(&fs->lock); + /* "users" and "in_exec" locked for check_unsafe_exec() */ if (fs->in_exec) { spin_unlock(&fs->lock); return -EAGAIN; -- Kees Cook ^ permalink raw reply related [flat|nested] 19+ messages in thread
end of thread, other threads:[~2024-01-27 11:23 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-01-24 16:19 [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper Kevin Locke 2024-01-24 16:35 ` Kees Cook 2024-01-24 16:46 ` Linus Torvalds 2024-01-24 16:54 ` Linus Torvalds 2024-01-24 17:10 ` Linus Torvalds 2024-01-24 17:21 ` Kees Cook 2024-01-24 17:27 ` Linus Torvalds 2024-01-24 18:27 ` Linus Torvalds 2024-01-24 18:29 ` Linus Torvalds 2024-01-24 19:02 ` Kees Cook 2024-01-24 19:41 ` Linus Torvalds 2024-01-25 14:16 ` Tetsuo Handa 2024-01-25 17:17 ` Linus Torvalds 2024-01-27 7:04 ` Tetsuo Handa 2024-01-27 11:00 ` Tetsuo Handa 2024-01-27 11:23 ` Tetsuo Handa 2024-01-24 18:57 ` Kees Cook 2024-01-27 5:17 ` John Johansen 2024-01-24 17:15 ` Kees Cook
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).