netdev - Re: [PATCH v2 2/6] net/handshake: Fix handshake

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 May 2023 14:32:39 +0000
From: Chuck Lever III <chuck.lever@...cle.com>
To: Paolo Abeni <pabeni@...hat.com>
CC: Leon Romanovsky <leon@...nel.org>, Chuck Lever <cel@...nel.org>,
        kernel-tls-handshake <kernel-tls-handshake@...ts.linux.dev>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "dan.carpenter@...aro.org"
	<dan.carpenter@...aro.org>
Subject: Re: [PATCH v2 2/6] net/handshake: Fix handshake_dup() ref counting



> On May 9, 2023, at 7:22 AM, Paolo Abeni <pabeni@...hat.com> wrote:
> 
> On Tue, 2023-05-09 at 13:59 +0000, Chuck Lever III wrote:
>> 
>>> On May 9, 2023, at 12:04 AM, Paolo Abeni <pabeni@...hat.com> wrote:
>>> 
>>> On Sun, 2023-05-07 at 11:25 +0300, Leon Romanovsky wrote:
>>>> On Fri, May 05, 2023 at 08:46:01PM -0400, Chuck Lever wrote:
>>>>> From: Chuck Lever <chuck.lever@...cle.com>
>>>>> 
>>>>> If get_unused_fd_flags() fails, we ended up calling fput(sock->file)
>>>>> twice.
>>>>> 
>>>>> Reported-by: Dan Carpenter <dan.carpenter@...aro.org>
>>>>> Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
>>>>> Signed-off-by: Chuck Lever <chuck.lever@...cle.com>
>>>>> ---
>>>>> net/handshake/netlink.c |    4 +---
>>>>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>>>> 
>>>>> diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
>>>>> index 7ec8a76c3c8a..032d96152e2f 100644
>>>>> --- a/net/handshake/netlink.c
>>>>> +++ b/net/handshake/netlink.c
>>>>> @@ -101,10 +101,8 @@ static int handshake_dup(struct socket *sock)
>>>>> 
>>>>> file = get_file(sock->file);
>>>>> newfd = get_unused_fd_flags(O_CLOEXEC);
>>>>> - if (newfd < 0) {
>>>>> - fput(file);
>>>>> + if (newfd < 0)
>>>>> return newfd;
>>>> 
>>>> IMHO, the better way to fix it is to change handshake_nl_accept_doit()
>>>> do not call to fput(sock->file) in error case. It is not right thing
>>>> to have a call to handshake_dup() and rely on elevated get_file()
>>>> for failure too as it will be problematic for future extension of
>>>> handshake_dup().
>>> 
>>> I agree with the above: I think a failing helper should leave the
>>> larger scope status unmodified. In this case a failing handshake_dup()
>>> should not touch file refcount, and handshake_nl_accept_doit() should
>>> be modified accordingly, something alike:
>>> 
>>> ---
>>> diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
>>> index e865fcf68433..8897a17189ad 100644
>>> --- a/net/handshake/netlink.c
>>> +++ b/net/handshake/netlink.c
>>> @@ -138,14 +138,15 @@ int handshake_nl_accept_doit(struct sk_buff *skb, struct genl_info *info)
>>> }
>>> err = req->hr_proto->hp_accept(req, info, fd);
>>> if (err)
>>> - goto out_complete;
>>> + goto out_put;
>>> 
>>> trace_handshake_cmd_accept(net, req, req->hr_sk, fd);
>>> return 0;
>>> 
>>> +out_put:
>>> + fput(sock->file);
>>> out_complete:
>>> handshake_complete(req, -EIO, NULL);
>>> - fput(sock->file);
>>> out_status:
>>> trace_handshake_cmd_accept_err(net, req, NULL, err);
>>> return err;
>> 
>> I'm happy to accommodate these changes, but it's not clear to me
>> whether you want this hunk applied /in addition to/ my fix or
>> /instead of/.
> 
> It's above (completely untested!) chunk is intended to be a replace for
> patch 2/6
> 
>>> ---
>>> 
>>> Somewhat related: handshake_nl_done_doit() releases the file refcount
>>> even if the req lookup fails.
>> 
>> That's because sockfd_lookup() increments the file ref count.
> 
> Ooops, I missed that.
> 
> Then in the successful path handshake_nl_done_doit() should call
> fput() twice ?!? 1 for the reference acquired by sockfd_lookup() and 1
> for the reference owned by  'req' ?!? Otherwise a ref will be leaked.
> 
>>> If that is caused by a concurrent
>>> req_cancel - not sure if possible at all, possibly syzkaller could
>>> guess if instructed about the API - such refcount will underflow, as it
>>> is rightfully decremented by req_cancel, too.
>> 
>> More likely, req_cancel might take the file ref count to zero
>> before sockfd_lookup can increment it, resulting in a UAF?
>> 
>> Let me think about this.
> 
> I now think this race is not possible, but I now fear the refcount leak
> mentioned above.

Not sure why I haven't seen evidence of a leak here. I'll have a closer look.


--
Chuck Lever