[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CD7ADFAB-137C-407C-93D4-4AF314FE0E52@oracle.com>
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
Powered by blists - more mailing lists