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]

Message-ID: <2e1fb95f613b6991b173d7947334927b22e49242.camel@redhat.com>
Date: Tue, 09 May 2023 16:22:01 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Chuck Lever III <chuck.lever@...cle.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 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.

Cheers,

Paolo