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
