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

[Leon Romanovsky <leon@kernel.org>]

On Mon, Feb 08, 2021 at 08:20:29PM -0700, David Ahern wrote:
> On 2/8/21 7:53 PM, Jakub Kicinski wrote:
> > On Mon, 8 Feb 2021 19:24:05 -0700 David Ahern wrote:
> >> On 2/8/21 11:41 AM, Jakub Kicinski wrote:
> >>> On Sun, 7 Feb 2021 10:26:54 +0200 Leon Romanovsky wrote:
> >>>> 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.
> >
> > Indeed, so we're agreeing that check_zeroed_user() is needed before
> > original optlen from user space gets truncated?
> >
>
> I thought so, but maybe not. To think through this ...
>
> If current kernel understands a struct of size N, it can only copy that
> amount from user to kernel. Anything beyond is ignored in these
> multiplexed uAPIs, and that is where the new userspace on old kernel falls.
>
> Known value checks can only be done up to size N. In this case, the
> reserved field is at the end of the known struct size, so checking just
> the field is fine. Going beyond the reserved field has implications for
> extensions to the API which should be handled when those extensions are
> added.

It is handled.
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/net/ipv4/tcp.c#n4155
		if (len > sizeof(zc)) {
			len = sizeof(zc);
			if (put_user(len, optlen))
				return -EFAULT;
		}

Thanks

>
> So, in short I think the "if (zc.reserved)" is correct as Leon noted.