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.
