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:

quoted

On Sat, Feb 06, 2021 at 03:28:28PM -0800, Jakub Kicinski wrote:

quoted

On Sat,  6 Feb 2021 12:36:48 -0800 Arjun Roy wrote:  

quoted

From: Arjun Roy <redacted>

Explicitly define reserved field and require it to be 0-valued.  

quoted

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.