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
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
On Wed, 24 Jan 2024 at 08:35, Kees Cook <[email protected]> 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
On Wed, 24 Jan 2024 at 08:46, Linus Torvalds
<[email protected]> 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
On Wed, 24 Jan 2024 at 08:54, Linus Torvalds
<[email protected]> 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
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
On Wed, Jan 24, 2024 at 09:10:58AM -0800, Linus Torvalds wrote:
> On Wed, 24 Jan 2024 at 08:54, Linus Torvalds
> <[email protected]> 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
On Wed, 24 Jan 2024 at 09:21, Kees Cook <[email protected]> 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
On Wed, 24 Jan 2024 at 09:27, Linus Torvalds
<[email protected]> 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
On Wed, 24 Jan 2024 at 10:27, Linus Torvalds
<[email protected]> 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
On Wed, Jan 24, 2024 at 09:10:58AM -0800, Linus Torvalds wrote:
> On Wed, 24 Jan 2024 at 08:54, Linus Torvalds
> <[email protected]> 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
On Wed, Jan 24, 2024 at 10:27:03AM -0800, Linus Torvalds wrote:
> On Wed, 24 Jan 2024 at 09:27, Linus Torvalds
> <[email protected]> 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
On Wed, 24 Jan 2024 at 11:02, Kees Cook <[email protected]> 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
On Thu, 25 Jan 2024 at 06:17, Tetsuo Handa
<[email protected]> 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
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
>> <[email protected]> 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...
>