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

[Mikhail Ivanov]

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).

> 
> 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 at 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
>>
>>