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 13:59: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 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/.


> ---
> 
> 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.


> 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 think it should be safer adding a chunk like:
> 
> ---
> diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
> index e865fcf68433..3e3e849f302a 100644
> --- a/net/handshake/netlink.c
> +++ b/net/handshake/netlink.c
> @@ -172,7 +173,6 @@ int handshake_nl_done_doit(struct sk_buff *skb, struct genl_info *info)
> req = handshake_req_hash_lookup(sock->sk);
> if (!req) {
> err = -EBUSY;
> - fput(sock->file);
> goto out_status;
> }
> ---
> 
> Possibly explicitly documenting the used ownership rules for the file
> refcount in the relevant functions could help with future maintenance.
> 
> Finally it's not clear to me if we agreed to a target tree or not ;) I
> see no replies so my suggestion.

Since we expect other subsystems to use net/handshake besides
SunRPC, I agree to going with netdev, even for this series once
it is acceptable.


--
Chuck Lever