Netdev Archive on lore.kernel.org
 help / color / Atom feed
From: Leon Romanovsky <leon@kernel.org>
To: Or Cohen <orcohen@paloaltonetworks.com>, davem@davemloft.net
Cc: netdev@vger.kernel.org, kuba@kernel.org,
	Xiaoming Ni <nixiaoming@huawei.com>,
	matthieu.baerts@tessares.net, mkl@pengutronix.de,
	Nadav Markus <nmarkus@paloaltonetworks.com>
Subject: Re: [PATCH] net/nfc: fix use-after-free llcp_sock_bind/connect
Date: Wed, 5 May 2021 07:46:47 +0300
Message-ID: <YJIjN6MTRdQ7Bvcp@unreal> (raw)
In-Reply-To: <CAM6JnLe=ZoHrpX8_i=_s5P-Q4h=mZxU=RN5pQuHbxq8pdZhYRQ@mail.gmail.com>

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

  reply index

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-04  7:15 Or Cohen
2021-05-04  8:12 ` Leon Romanovsky
2021-05-04 16:01   ` Or Cohen
2021-05-05  4:46     ` Leon Romanovsky [this message]
     [not found]       ` <CABV_C9OJ6v1deEknc+V3cJaT+CPjmzg6Wb06_Rsey3AXqOBNYg@mail.gmail.com>
2021-05-05 11:30         ` Nadav Markus
2021-05-05 11:39         ` Leon Romanovsky
     [not found]           ` <CABV_C9PscjqNPTbK0JuNGsgCAX-xYg9=GG1KNyOh3hQU1TuzWQ@mail.gmail.com>
2021-05-05 14:17             ` Leon Romanovsky
2021-05-05 14:31               ` Or Cohen
2021-05-04  7:16 Or Cohen
2021-05-04 19:10 ` patchwork-bot+netdevbpf
2021-05-05  4:50   ` Leon Romanovsky

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YJIjN6MTRdQ7Bvcp@unreal \
    --to=leon@kernel.org \
    --cc=davem@davemloft.net \
    --cc=kuba@kernel.org \
    --cc=matthieu.baerts@tessares.net \
    --cc=mkl@pengutronix.de \
    --cc=netdev@vger.kernel.org \
    --cc=nixiaoming@huawei.com \
    --cc=nmarkus@paloaltonetworks.com \
    --cc=orcohen@paloaltonetworks.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git