lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250326162104.20801-1-kuniyu@amazon.com>
Date: Wed, 26 Mar 2025 09:19:56 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <kuba@...nel.org>
CC: <davem@...emloft.net>, <john.johansen@...onical.com>, <kuniyu@...zon.com>,
	<linux-kernel@...r.kernel.org>, <linux-next@...r.kernel.org>,
	<netdev@...r.kernel.org>, <pabeni@...hat.com>, <sfr@...b.auug.org.au>
Subject: Re: linux-next: build failure after merge of the apparmor tree

From: Jakub Kicinski <kuba@...nel.org>
Date: Wed, 26 Mar 2025 04:26:55 -0700
> On Wed, 26 Mar 2025 15:01:48 +1100 Stephen Rothwell wrote:
> > After merging the apparmor tree, today's linux-next build (x86_64
> > allmodconfig) failed like this:
> > 
> > security/apparmor/af_unix.c: In function 'unix_state_double_lock':
> > security/apparmor/af_unix.c:627:17: error: implicit declaration of function 'unix_state_lock'; did you mean 'unix_state_double_lock'? [-Wimplicit-function-declaration]
> >   627 |                 unix_state_lock(sk1);
> >       |                 ^~~~~~~~~~~~~~~
> >       |                 unix_state_double_lock
> > security/apparmor/af_unix.c: In function 'unix_state_double_unlock':
> > security/apparmor/af_unix.c:642:17: error: implicit declaration of function 'unix_state_unlock'; did you mean 'unix_state_double_lock'? [-Wimplicit-function-declaration]
> >   642 |                 unix_state_unlock(sk1);
> >       |                 ^~~~~~~~~~~~~~~~~
> >       |                 unix_state_double_lock
> 
> Thanks Stephen! I'll pop this into the tree in a few hours,
> just giving Kuniyuki a bit more time to ack.

Thanks for catching this, Stephen !

The patch itself looks good, for the patch:

Reviewed-by: Kuniyuki Iwashima <kuniyu@...zon.com>


John:

I had a cursory look at this commit and the exact user of
unix_state_lock() is broken for SOCK_DGRAM.

https://web.git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor.git/commit/?h=apparmor-next&id=c05e705812d179f4b85aeacc34a555a42bc4f9ac

---8<---
+
+	/* TODO: update sock label with new task label */
+	unix_state_lock(sock->sk);
+	peer_sk = unix_peer(sock->sk);
+	if (peer_sk)
+		sock_hold(peer_sk);
+
+	is_sk_fs = is_unix_fs(sock->sk);
+	if (is_sk_fs && peer_sk)
+		sk_req = request;
+	if (sk_req)
+		error = unix_label_sock_perm(subj_cred, label, op, sk_req,
+					     sock);
+	unix_state_unlock(sock->sk);
+	if (!peer_sk)
+		return error;
+
+	unix_state_double_lock(sock->sk, peer_sk);

Here, unix_peer(sock->sk) could have been changed and must be
double checked.  See unix_dgram_sendmsg().

The patch seems to be written in 2022 and recently merged.
I'm not sure if it's reviewed by netdev folks at that time,
but please cc me and netdev next time for patches regarding
AF_UNIX.

Thanks!


+	if (!is_sk_fs && is_unix_fs(peer_sk)) {
+		last_error(error,
+			   unix_fs_perm(op, request, subj_cred, label,
+					unix_sk(peer_sk)));
+	} else if (!is_sk_fs) {
+		struct aa_sk_ctx *pctx = aa_sock(peer_sk);
+
+		last_error(error,
+			xcheck(aa_unix_peer_perm(subj_cred, label, op,
+						 MAY_READ | MAY_WRITE,
+						 sock->sk, peer_sk, NULL),
+			       aa_unix_peer_perm(file->f_cred, pctx->label, op,
+						 MAY_READ | MAY_WRITE,
+						 peer_sk, sock->sk, label)));
+	}
+	unix_state_double_unlock(sock->sk, peer_sk);
---8<---

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ