Received-SPF: pass (google.com: domain of linux-kernel+bounces-37409-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 24 Jan 2024 09:15:38 -0800
From: Kees Cook <keescook@chromium.org>
To: Kevin Locke <kevin@kevinlocke.name>,
	John Johansen <john.johansen@canonical.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Josh Triplett <josh@joshtriplett.org>,
	Mateusz Guzik <mjguzik@gmail.com>,
	Al Viro <viro@zeniv.linux.org.uk>, linux-mm@kvack.org,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-security-module@vger.kernel.org
Subject: Re: [6.8-rc1 Regression] Unable to exec apparmor_parser from
 virt-aa-helper
Message-ID: <202401240910.E0449F0F@keescook>
References: <ZbE4qn9_h14OqADK@kevinlocke.name>
 <202401240832.02940B1A@keescook>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <202401240832.02940B1A@keescook>

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