Re: [net-next v2] tcp: Explicitly mark reserved field in tcp_zerocopy_receive args.

[David Ahern <dsahern@gmail.com>]

On 2/8/21 11:41 AM, Jakub Kicinski wrote:
> On Sun, 7 Feb 2021 10:26:54 +0200 Leon Romanovsky wrote:
>> On Sat, Feb 06, 2021 at 03:28:28PM -0800, Jakub Kicinski wrote:
>>> On Sat,  6 Feb 2021 12:36:48 -0800 Arjun Roy wrote:  
>>>> From: Arjun Roy <arjunroy@google.com>
>>>>
>>>> Explicitly define reserved field and require it to be 0-valued.  
>>>  
>>>> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
>>>> index e1a17c6b473c..c8469c579ed8 100644
>>>> --- a/net/ipv4/tcp.c
>>>> +++ b/net/ipv4/tcp.c
>>>> @@ -4159,6 +4159,8 @@ static int do_tcp_getsockopt(struct sock *sk, int level,
>>>>  		}
>>>>  		if (copy_from_user(&zc, optval, len))
>>>>  			return -EFAULT;
>>>> +		if (zc.reserved)
>>>> +			return -EINVAL;
>>>>  		lock_sock(sk);
>>>>  		err = tcp_zerocopy_receive(sk, &zc, &tss);
>>>>  		release_sock(sk);  
>>>
>>> I was expecting we'd also throw in a check_zeroed_user().
>>> Either we can check if the buffer is zeroed all the way,
>>> or we can't and we shouldn't validate reserved either
>>>
>>> 	check_zeroed_user(optval + offsetof(reserved),
>>> 			  len - offsetof(reserved))
>>> ?  
>>
>> There is a check that len is not larger than zs and users can't give
>> large buffer.
>>
>> I would say that is pretty safe to write "if (zc.reserved)".
> 
> Which check? There's a check which truncates (writes back to user space
> len = min(len, sizeof(zc)). Application can still pass garbage beyond
> sizeof(zc) and syscall may start failing in the future if sizeof(zc)
> changes.
> 

That would be the case for new userspace on old kernel. Extending the
check to the end of the struct would guarantee new userspace can not ask
for something that the running kernel does not understand.