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]

Message-ID: <af35d535-8d58-3cf3-60e3-1764e409308b@gmail.com>
Date:   Mon, 8 Feb 2021 20:20:29 -0700
From:   David Ahern <dsahern@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Leon Romanovsky <leon@...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 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.