On Fri, Dec 13, 2024 at 5:57 AM Mikhail Ivanov <ivanov.mikhail1@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx/ > >> > >> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > >> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@xxxxxxxxxxxxxxxxxxx> > >> --- > >> 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 > >> > >>
