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:   Tue, 9 Feb 2021 08:29:03 +0200
From:   Leon Romanovsky <leon@...nel.org>
To:     David Ahern <dsahern@...il.com>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        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 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.