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

[David Ahern <dsahern@gmail.com>]

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.

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