netdev - Re: [net-next v2] tcp: Explicitly mark reserved field in tcp_zerocopy

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]

Date:   Mon, 8 Feb 2021 19:24:05 -0700
From:   David Ahern <dsahern@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>, Leon Romanovsky <leon@...nel.org>
Cc:     Arjun Roy <arjunroy.kdev@...il.com>, davem@...emloft.net,
        netdev@...r.kernel.org, arjunroy@...gle.com, edumazet@...gle.com,
        soheil@...gle.com
Subject: Re: [net-next v2] tcp: Explicitly mark reserved field in
 tcp_zerocopy_receive args.

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@...gle.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.