netdev - Re: [PATCH net-next v7 1/3] sock: support copying cmsgs to the user space in sendmsg

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: <231e099e-ce57-442b-901f-2a9d11149a2f@bytedance.com>
Date: Thu, 25 Jul 2024 17:05:51 -0700
From: Zijian Zhang <zijianzhang@...edance.com>
To: Mina Almasry <almasrymina@...gle.com>
Cc: netdev@...r.kernel.org, edumazet@...gle.com,
 willemdebruijn.kernel@...il.com, cong.wang@...edance.com,
 xiaochun.lu@...edance.com
Subject: Re: [PATCH net-next v7 1/3] sock: support copying cmsgs to the user
 space in sendmsg

On 7/25/24 4:50 PM, Zijian Zhang wrote:
>>> diff --git a/net/core/sock.c b/net/core/sock.c
>>> index 9abc4fe25953..efb30668dac3 100644
>>> --- a/net/core/sock.c
>>> +++ b/net/core/sock.c
>>> @@ -2826,7 +2826,7 @@ struct sk_buff *sock_alloc_send_pskb(struct 
>>> sock *sk, unsigned long header_len,
>>>   }
>>>   EXPORT_SYMBOL(sock_alloc_send_pskb);
>>>
>>> -int __sock_cmsg_send(struct sock *sk, struct cmsghdr *cmsg,
>>> +int __sock_cmsg_send(struct sock *sk, struct msghdr *msg, struct 
>>> cmsghdr *cmsg,
>>>                struct sockcm_cookie *sockc)
>>>   {
>>>       u32 tsflags;
>>> @@ -2866,6 +2866,8 @@ int __sock_cmsg_send(struct sock *sk, struct 
>>> cmsghdr *cmsg,
>>>       default:
>>>           return -EINVAL;
>>>       }
>>> +    if (cmsg_copy_to_user(cmsg))
>>> +        msg->msg_control_copy_to_user = true;
>>>       return 0;
>>>   }

msg_control_copy_to_user is set to true here.

>>
>>
>> This may be a lack of knowledge on my part, but i'm very confused that
>> msg_control_copy_to_user is set to false here, and then checked below, 
>> and it's
>> not touched in between. How could it evaluate to true below? Is it 
>> because something
>> overwrites the value in msg_sys between this set and the check?
>>
>> If something is overwriting it, is the initialization to false necessary?
>> I don't see other fields of msg_sys initialized this way.
>>
> 
> ```
> msg_sys->msg_control_copy_to_user = false;
> ...
> err = __sock_sendmsg(sock, msg_sys); -> __sock_cmsg_send
> ...
> if (msg && msg_sys->msg_control_copy_to_user && err >= 0)
> ```
> 
> The msg_control_copy_to_user maybe updated by the cmsg handler in
> the function __sock_cmsg_send. In patch 2/3, we have
> msg_control_copy_to_user updated to true in SCM_ZC_NOTIFICATION
> handler.

Not in patch 2/3 In this patchset msg_control_copy_to_user is set in
this patch, in __sock_cmsg_send.

> 
> As for the initialization,
> 
> msg_sys is allocated from the kernel stack, if we don't initialize
> it to false, it might be randomly true, even though there is no
> cmsg wants to be copied back.
> 
> Why is there only one initialization here? The existing bit 
> msg_control_is_user only get initialized where the following code
> path will use it. msg_control_is_user is initialized in multiple
> locations in net/socket.c. However, In function hidp_send_frame,
> msg_control_is_user is not initialized, because the following path will
> not use this bit.
> 
> We only initialize msg_control_copy_to_user in function
> ____sys_sendmsg, because only in this function will we check this bit.
> 
> If the initialization here makes people confused, I will add some docs.
>