Re: [PATCH] selinux: Read sk->sk_family once in selinux_socket_bind()

[Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>]

On 12/13/2024 6:46 PM, Stephen Smalley wrote:
> On Fri, Dec 13, 2024 at 5:57 AM Mikhail Ivanov
> <ivanov.mikhail1@huawei-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.

IPv6 and IPv4 TCP sockets are mapped to the same SECCLASS_TCP_SOCKET
security class. AFAICS there is no other places that can be affected by
the IPV6_ADDFORM transformation.

> 
>>
>>>
>>> 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@huawei-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
>>>>
>>>>