[6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper

[6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper

[Kevin Locke]

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

Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper

[Kees Cook]

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

Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper

[Linus Torvalds]

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

Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper

[Linus Torvalds]

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

Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper

[Linus Torvalds]

[-- 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;
 }

Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper

[Kees Cook]

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

Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper

[Kees Cook]

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

Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper

[Linus Torvalds]

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

Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper

[Linus Torvalds]

[-- 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);

Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper

[Linus Torvalds]

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

Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper

[Kees Cook]

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

Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper

[Kees Cook]

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

Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper

[Linus Torvalds]

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

Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper

[Tetsuo Handa]

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...

Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper

[Linus Torvalds]

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

Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper

[John Johansen]

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...
> 

Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper

[Tetsuo Handa]

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".

Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper

[Tetsuo Handa]

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)?

Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from virt-aa-helper

[Tetsuo Handa]

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.