Re: [PATCH] net/nfc: fix use-after-free llcp_sock_bind/connect

[Leon Romanovsky <leon@kernel.org>]

On Tue, May 04, 2021 at 07:01:01PM +0300, Or Cohen wrote:
> Hi, can you please elaborate?
> 
> We don't understand why using kref_get_unless_zero will solve the problem.

Please don't reply in top-posting format.
------

The rationale behind _put()/_get() wrappers over kref is to allow
delayed release after all consumers are gone. 

In order to make it happen, the developer should ensure that consumers
don't have an access to the kref-ed struct. This is done with
kref_get_unless_zero().

In your case, you simply increment some counter without checking if
nfc_llcp_local_get() actually succeeded.

For example, what protection do you have from races between llcp_sock_bind(),
nfc_llcp_sock_free() and llcp_sock_connect()?

So in case you have some lock outside, it is unclear how use-after-free
is possible, because nfc_llcp_find_local() should return NULL.
In case, no lock exists, except reducing race window, you didn't fix anything
and didn't sanitize lcp_sock too.

Thanks

> 
> On Tue, May 4, 2021 at 11:12 AM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Tue, May 04, 2021 at 10:15:25AM +0300, Or Cohen wrote:
> > > Commits 8a4cd82d ("nfc: fix refcount leak in llcp_sock_connect()")
> > > and c33b1cc62 ("nfc: fix refcount leak in llcp_sock_bind()")
> > > fixed a refcount leak bug in bind/connect but introduced a
> > > use-after-free if the same local is assigned to 2 different sockets.
> > >
> > > This can be triggered by the following simple program:
> > >     int sock1 = socket( AF_NFC, SOCK_STREAM, NFC_SOCKPROTO_LLCP );
> > >     int sock2 = socket( AF_NFC, SOCK_STREAM, NFC_SOCKPROTO_LLCP );
> > >     memset( &addr, 0, sizeof(struct sockaddr_nfc_llcp) );
> > >     addr.sa_family = AF_NFC;
> > >     addr.nfc_protocol = NFC_PROTO_NFC_DEP;
> > >     bind( sock1, (struct sockaddr*) &addr, sizeof(struct sockaddr_nfc_llcp) )
> > >     bind( sock2, (struct sockaddr*) &addr, sizeof(struct sockaddr_nfc_llcp) )
> > >     close(sock1);
> > >     close(sock2);
> > >
> > > Fix this by assigning NULL to llcp_sock->local after calling
> > > nfc_llcp_local_put.
> > >
> > > This addresses CVE-2021-23134.
> > >
> > > Reported-by: Or Cohen <orcohen@paloaltonetworks.com>
> > > Reported-by: Nadav Markus <nmarkus@paloaltonetworks.com>
> > > Fixes: c33b1cc62 ("nfc: fix refcount leak in llcp_sock_bind()")
> > > Signed-off-by: Or Cohen <orcohen@paloaltonetworks.com>
> > > ---
> > >
> > >  net/nfc/llcp_sock.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c
> > > index a3b46f888803..53dbe733f998 100644
> > > --- a/net/nfc/llcp_sock.c
> > > +++ b/net/nfc/llcp_sock.c
> > > @@ -109,12 +109,14 @@ static int llcp_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
> > >                                         GFP_KERNEL);
> > >       if (!llcp_sock->service_name) {
> > >               nfc_llcp_local_put(llcp_sock->local);
> > > +             llcp_sock->local = NULL;
> >
> > This "_put() -> set to NULL" pattern can't be correct.
> >
> > You need to fix nfc_llcp_local_get() to use kref_get_unless_zero()
> > and prevent any direct use of llcp_sock->local without taking kref
> > first. The nfc_llcp_local_put() isn't right either.
> >
> > Thanks