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

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

On 1/8/2025 12:00 AM, Paul Moore wrote:
> On Tue, Jan 7, 2025 at 3:16 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
>> On Fri, Dec 13, 2024 at 3:09 PM Paul Moore <paul@paul-moore.com> wrote:
>>> On Fri, Dec 13, 2024 at 11:40 AM Mikhail Ivanov
>>> <ivanov.mikhail1@huawei-partners.com> wrote:
>>>> 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.
>>>
>>> Yes, thankfully we don't really encode the IP address family in any of
>>> the SELinux object classes so that shouldn't be an issue.  I also
>>> don't think we have to worry about the per-packet labeling protocols
>>> as it's too late in the communication to change the socket's
>>> associated packet labeling, it's either working or it isn't; we should
>>> handle the mapped IPv4 address already.
>>>
>>> I am a little concerned about bind being the only place where we have
>>> to worry about accessing sk_family while the socket isn't locked.  As
>>> an example, I'm a little concerned about the netfilter code paths; I
>>> haven't chased them down, but my guess is that the associated
>>> socket/sock isn't locked in those cases (in the relevant output and
>>> postroute cases, forward should be a non-issue).
> 
> We still need an answer on this.

Sorry for the late reply,
I found out that security_sock_rcv_skb() can also be called without
locking the IPv6 socket (this can be easily verified by manual testing).

Netfilter hooks seems to be ok, family value is taken from the
nf_hook_state structure, so there is no access to sk->sk_family.

SCTP and MPTCP hooks should not be considered, because IPV6_ADDRFORM is
only available for TCP, UDP and UDPLITE protocols.

There are 2 more functions that access sk_family:
* security_sock_graft() - socket is locked by inet_accept(),
* security_inet_conn_established() - socket is locked by connect(2) or
   in BH context (Cf. tcp_v6_rcv).

> 
>>> How bad is the performance impact of READ_ONCE()?  In other words, how
>>> stupid would it be to simply do all of our sock->sk_family lookups
>>> using READ_ONCE()?
>>
>> I could be wrong, but I don't think there is any overhead except on Dec Alpha.
> 
> Then perhaps the right answer is to use it everywhere.
> 

Indeed, using READ_ONCE() in the considered hooks should not lead to
any overhead. I wonder if it would be better not to touch the SCTP
and MPTCP hooks anyway. Adding READ_ONCE() in selinux_sock_graft() is
fine if you think it's better this way.