netdev - Re: [PATCH] selinux: Read sk->sk_family once in selinux_socket

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEjxPJ737irXncrwoM3avg4L+U37QB2w+fjJZZYTjND5Z4_Nig@mail.gmail.com>
Date: Fri, 13 Dec 2024 10:46:25 -0500
From: Stephen Smalley <stephen.smalley.work@...il.com>
To: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
Cc: Mickaël Salaün <mic@...ikod.net>, 
	paul@...l-moore.com, selinux@...r.kernel.org, omosnace@...hat.com, 
	linux-security-module@...r.kernel.org, netdev@...r.kernel.org, 
	yusongping@...wei.com, artem.kuzin@...wei.com, 
	konstantin.meskhidze@...wei.com
Subject: Re: [PATCH] selinux: Read sk->sk_family once in selinux_socket_bind()

On Fri, Dec 13, 2024 at 5:57 AM Mikhail Ivanov
<ivanov.mikhail1@...wei-partners.com> wrote:
>
> On 12/12/2024 8:50 PM, Mickaël Salaün wrote:
> > This looks good be there are other places using sk->sk_family that
> > should also be fixed.
>
> Thanks for checking this!
>
> For selinux this should be enough, I haven't found any other places
> where sk->sk_family could be read from an IPv6 socket without locking.
>
> I also would like to prepare such fix for other LSMs (apparmor, smack,
> tomoyo) (in separate patches).

I'm wondering about the implications for SELinux beyond just
sk->sk_family access, e.g. SELinux maps the (family, type, protocol)
triple to a security class at socket creation time via
socket_type_to_security_class() and caches the security class in the
inode_security_struct and sk_security_struct for later use.

>
> >
> > On Thu, Dec 12, 2024 at 06:20:00PM +0800, Mikhail Ivanov wrote:
> >> selinux_socket_bind() is called without holding the socket lock.
> >>
> >> Use READ_ONCE() to safely read sk->sk_family for IPv6 socket in case
> >> of lockless transformation to IPv4 socket via IPV6_ADDRFORM [1].
> >>
> >> [1] https://lore.kernel.org/all/20240202095404.183274-1-edumazet@google.com/
> >>
> >> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> >> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
> >> ---
> >>   security/selinux/hooks.c | 4 +++-
> >>   1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >> index 5e5f3398f39d..b7adff2cf5f6 100644
> >> --- a/security/selinux/hooks.c
> >> +++ b/security/selinux/hooks.c
> >> @@ -4715,8 +4715,10 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
> >>      if (err)
> >>              goto out;
> >>
> >> +    /* IPV6_ADDRFORM can change sk->sk_family under us. */
> >> +    family = READ_ONCE(sk->sk_family);
> >> +
> >>      /* If PF_INET or PF_INET6, check name_bind permission for the port. */
> >> -    family = sk->sk_family;
> >>      if (family == PF_INET || family == PF_INET6) {
> >>              char *addrp;
> >>              struct common_audit_data ad;
> >>
> >> base-commit: 034294fbfdf0ded4f931f9503d2ca5bbf8b9aebd
> >> --
> >> 2.34.1
> >>
> >>