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

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

[Or Cohen]

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;
 		ret = -ENOMEM;
 		goto put_dev;
 	}
 	llcp_sock->ssap = nfc_llcp_get_sdp_ssap(local, llcp_sock);
 	if (llcp_sock->ssap == LLCP_SAP_MAX) {
 		nfc_llcp_local_put(llcp_sock->local);
+		llcp_sock->local = NULL;
 		kfree(llcp_sock->service_name);
 		llcp_sock->service_name = NULL;
 		ret = -EADDRINUSE;
@@ -709,6 +711,7 @@ static int llcp_sock_connect(struct socket *sock, struct sockaddr *_addr,
 	llcp_sock->ssap = nfc_llcp_get_local_ssap(local);
 	if (llcp_sock->ssap == LLCP_SAP_MAX) {
 		nfc_llcp_local_put(llcp_sock->local);
+		llcp_sock->local = NULL;
 		ret = -ENOMEM;
 		goto put_dev;
 	}
@@ -756,6 +759,7 @@ static int llcp_sock_connect(struct socket *sock, struct sockaddr *_addr,
 sock_llcp_release:
 	nfc_llcp_put_ssap(local, llcp_sock->ssap);
 	nfc_llcp_local_put(llcp_sock->local);
+	llcp_sock->local = NULL;
 
 put_dev:
 	nfc_put_device(dev);
-- 
2.7.4

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Tue,  4 May 2021 10:16:46 +0300 you 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);
> 
> [...]

Here is the summary with links:
  - net/nfc: fix use-after-free llcp_sock_bind/connect
    https://git.kernel.org/netdev/net/c/c61760e6940d

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[Leon Romanovsky]

On Tue, May 04, 2021 at 07:10:10PM +0000, patchwork-bot+netdevbpf@kernel.org wrote:
> Hello:
> 
> This patch was applied to netdev/net.git (refs/heads/master):
> 
> On Tue,  4 May 2021 10:16:46 +0300 you 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);
> > 
> > [...]
> 
> Here is the summary with links:
>   - net/nfc: fix use-after-free llcp_sock_bind/connect
>     https://git.kernel.org/netdev/net/c/c61760e6940d

Dave,

Can you please share your thoughts how this patch can be correct?
https://lore.kernel.org/netdev/YJIjN6MTRdQ7Bvcp@unreal/T/#m1e67ae6c2658312a134f65819c5ad92511f207c1

It is also under review, so unclear why it was merged.

Thanks

> 
> You are awesome, thank you!
> --
> Deet-doot-dot, I am a bot.
> https://korg.docs.kernel.org/patchwork/pwbot.html
> 
> 

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

[Or Cohen]

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;
 		ret = -ENOMEM;
 		goto put_dev;
 	}
 	llcp_sock->ssap = nfc_llcp_get_sdp_ssap(local, llcp_sock);
 	if (llcp_sock->ssap == LLCP_SAP_MAX) {
 		nfc_llcp_local_put(llcp_sock->local);
+		llcp_sock->local = NULL;
 		kfree(llcp_sock->service_name);
 		llcp_sock->service_name = NULL;
 		ret = -EADDRINUSE;
@@ -709,6 +711,7 @@ static int llcp_sock_connect(struct socket *sock, struct sockaddr *_addr,
 	llcp_sock->ssap = nfc_llcp_get_local_ssap(local);
 	if (llcp_sock->ssap == LLCP_SAP_MAX) {
 		nfc_llcp_local_put(llcp_sock->local);
+		llcp_sock->local = NULL;
 		ret = -ENOMEM;
 		goto put_dev;
 	}
@@ -756,6 +759,7 @@ static int llcp_sock_connect(struct socket *sock, struct sockaddr *_addr,
 sock_llcp_release:
 	nfc_llcp_put_ssap(local, llcp_sock->ssap);
 	nfc_llcp_local_put(llcp_sock->local);
+	llcp_sock->local = NULL;
 
 put_dev:
 	nfc_put_device(dev);
-- 
2.7.4

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

[Leon Romanovsky]

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

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

[Or Cohen]

Hi, can you please elaborate?

We don't understand why using kref_get_unless_zero will solve the problem.

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

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

[Leon Romanovsky]

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

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

[Nadav Markus]

On Wed, May 5, 2021 at 12:35 PM Nadav Markus
<nmarkus@paloaltonetworks.com> wrote:
>
>
>
> On Wed, May 5, 2021 at 7:46 AM Leon Romanovsky <leon@kernel.org> wrote:
>>
>> 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.
>
>
> Hi Leon - as far as we understand, the underlying issue is not incrementing the kref counter without checking if the function nfc_llcp_local_get succeeded or not. The function itself increments the reference count.
>
> The issue is that the nfc_llcp_local_put might be called twice on the llcp_sock->local field, however only one reference (the one that was gotten via nfc_llcp_local_get) is incremented. llcp_local_put will be called in two locations. The first one is just inside the bind function, if nfc_llcp_get_local_ssap fails. The second one is called unconditionally, at the socket destruction, at the function nfc_llcp_sock_free.
>
> Hence, our proposed solution is to prevent the second nfc_llcp_local_put from attempting to decrement the kref count, by setting local to NULL. This makes sense, as we immediately do so after decrementing the single ref count we took when calling nfc_llcp_local_get. Since we are under the sock lock, this also should be race safe, as no one should access the llcp_sock->local field without this lock's protection.
>>
>>
>> For example, what protection do you have from races between llcp_sock_bind(),
>> nfc_llcp_sock_free() and llcp_sock_connect()?
>
>
> As we replied, the llcp_sock->local field is protected under the lock sock, as far as we understand.
>>
>>
>> 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.
>
>
> We don't quite get what race are we talking about here - our trigger program doesn't even utilize threads. All it has to do is to cause nfc_llcp_local_get to fail - this can be seen clearly in our original trigger program. To clarify, the two sockets that are created there point to the same nfc_llcp_local struct (via their local field). The destruction of the first socket causes the reference count of the pointed object to drop to zero (since the code increments the ref count of the object from 1 to 2, but dercements it twice). The second socket later attempts to decrement the ref count of the same (already freed) nfc_llcp_local object, causing a kernel crash.
>>
>>
>> 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

I am resending this as the original contained HTML by mistake.

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

[Leon Romanovsky]

On Wed, May 05, 2021 at 12:35:48PM +0300, Nadav Markus wrote:
> On Wed, May 5, 2021 at 7:46 AM Leon Romanovsky <leon@kernel.org> wrote:
> 
> > 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.
> >
> 
> Hi Leon - as far as we understand, the underlying issue is not incrementing
> the kref counter without checking if the function nfc_llcp_local_get
> succeeded or not. The function itself increments the reference count.
> 
> The issue is that the nfc_llcp_local_put might be called twice on the
> llcp_sock->local field, however only one reference (the one that was gotten
> via nfc_llcp_local_get) is incremented. llcp_local_put will be called in
> two locations. The first one is just inside the bind function, if
> nfc_llcp_get_local_ssap fails. The second one is called unconditionally, at
> the socket destruction, at the function nfc_llcp_sock_free.
> 
> Hence, our proposed solution is to prevent the second nfc_llcp_local_put
> from attempting to decrement the kref count, by setting local to NULL. This
> makes sense, as we immediately do so after decrementing the single ref
> count we took when calling nfc_llcp_local_get. Since we are under the sock
> lock, this also should be race safe, as no one should access the
> llcp_sock->local field without this lock's protection.
> 
> >
> > For example, what protection do you have from races between
> > llcp_sock_bind(),
> > nfc_llcp_sock_free() and llcp_sock_connect()?
> >
> 
> As we replied, the llcp_sock->local field is protected under the lock sock,
> as far as we understand.
> 
> >
> > 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.
> >
> 
> We don't quite get what race are we talking about here - our trigger
> program doesn't even utilize threads. All it has to do is to
> cause nfc_llcp_local_get to fail - this can be seen clearly in our original
> trigger program. To clarify, the two sockets that are created there point
> to the same nfc_llcp_local struct (via their local field). The destruction
> of the first socket causes the reference count of the pointed object to
> drop to zero (since the code increments the ref count of the object from 1
> to 2, but dercements it twice). The second socket later attempts to
> decrement the ref count of the same (already freed) nfc_llcp_local object,
> causing a kernel crash.


So at the end, we are talking about situation where _get()/_put() are
protected by the lock and local can't disappear. Can you please help me to find
this socket lock? Did I miss it in bind path?

net/socket.c:
  int __sys_bind(int fd, struct sockaddr __user *umyaddr, int addrlen)
    sock->ops->bind(..)
     llcp_sock_bind(..)

And if we put lock issue aside, all your change can be squeezed to the following:

diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c
index a3b46f888803..cc9ee634269d 100644
--- a/net/nfc/llcp_sock.c
+++ b/net/nfc/llcp_sock.c
@@ -99,7 +99,6 @@ static int llcp_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
        }

        llcp_sock->dev = dev;
-       llcp_sock->local = nfc_llcp_local_get(local);
        llcp_sock->nfc_protocol = llcp_addr.nfc_protocol;
        llcp_sock->service_name_len = min_t(unsigned int,
                                            llcp_addr.service_name_len,
@@ -108,13 +107,11 @@ static int llcp_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
                                          llcp_sock->service_name_len,
                                          GFP_KERNEL);
        if (!llcp_sock->service_name) {
-               nfc_llcp_local_put(llcp_sock->local);
                ret = -ENOMEM;
                goto put_dev;
        }
        llcp_sock->ssap = nfc_llcp_get_sdp_ssap(local, llcp_sock);
        if (llcp_sock->ssap == LLCP_SAP_MAX) {
-               nfc_llcp_local_put(llcp_sock->local);
                kfree(llcp_sock->service_name);
                llcp_sock->service_name = NULL;
                ret = -EADDRINUSE;
@@ -122,6 +119,7 @@ static int llcp_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
        }

        llcp_sock->reserved_ssap = llcp_sock->ssap;
+       llcp_sock->local = nfc_llcp_local_get(local);

        nfc_llcp_sock_link(&local->sockets, sk);


Thanks

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

[Leon Romanovsky]

On Wed, May 05, 2021 at 04:36:05PM +0300, Nadav Markus wrote:
> On Wed, May 5, 2021 at 2:40 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Wed, May 05, 2021 at 12:35:48PM +0300, Nadav Markus wrote:
> > > On Wed, May 5, 2021 at 7:46 AM Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > > 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.
> > > >
> > >
> > > Hi Leon - as far as we understand, the underlying issue is not
> incrementing
> > > the kref counter without checking if the function nfc_llcp_local_get
> > > succeeded or not. The function itself increments the reference count.
> > >
> > > The issue is that the nfc_llcp_local_put might be called twice on the
> > > llcp_sock->local field, however only one reference (the one that was
> gotten
> > > via nfc_llcp_local_get) is incremented. llcp_local_put will be called in
> > > two locations. The first one is just inside the bind function, if
> > > nfc_llcp_get_local_ssap fails. The second one is called
> unconditionally, at
> > > the socket destruction, at the function nfc_llcp_sock_free.
> > >
> > > Hence, our proposed solution is to prevent the second nfc_llcp_local_put
> > > from attempting to decrement the kref count, by setting local to NULL.
> This
> > > makes sense, as we immediately do so after decrementing the single ref
> > > count we took when calling nfc_llcp_local_get. Since we are under the
> sock
> > > lock, this also should be race safe, as no one should access the
> > > llcp_sock->local field without this lock's protection.
> > >
> > > >
> > > > For example, what protection do you have from races between
> > > > llcp_sock_bind(),
> > > > nfc_llcp_sock_free() and llcp_sock_connect()?
> > > >
> > >
> > > As we replied, the llcp_sock->local field is protected under the lock
> sock,
> > > as far as we understand.
> > >
> > > >
> > > > 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.
> > > >
> > >
> > > We don't quite get what race are we talking about here - our trigger
> > > program doesn't even utilize threads. All it has to do is to
> > > cause nfc_llcp_local_get to fail - this can be seen clearly in our
> original
> > > trigger program. To clarify, the two sockets that are created there
> point
> > > to the same nfc_llcp_local struct (via their local field). The
> destruction
> > > of the first socket causes the reference count of the pointed object to
> > > drop to zero (since the code increments the ref count of the object
> from 1
> > > to 2, but dercements it twice). The second socket later attempts to
> > > decrement the ref count of the same (already freed) nfc_llcp_local
> object,
> > > causing a kernel crash.
> >
> >
> > So at the end, we are talking about situation where _get()/_put() are
> > protected by the lock and local can't disappear. Can you please help me
> to find
> > this socket lock? Did I miss it in bind path?
> 
> The lock we are talking about is the lock_sock - lock_sock(sk). It appears
> near the start of the function.
> 
> >
> >
> > net/socket.c:
> >   int __sys_bind(int fd, struct sockaddr __user *umyaddr, int addrlen)
> >     sock->ops->bind(..)
> >      llcp_sock_bind(..)
> >
> > And if we put lock issue aside, all your change can be squeezed to the
> following:
> >
> > diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c
> > index a3b46f888803..cc9ee634269d 100644
> > --- a/net/nfc/llcp_sock.c
> > +++ b/net/nfc/llcp_sock.c
> > @@ -99,7 +99,6 @@ static int llcp_sock_bind(struct socket *sock, struct
> sockaddr *addr, int alen)
> >         }
> >
> >         llcp_sock->dev = dev;
> > -       llcp_sock->local = nfc_llcp_local_get(local);
> >         llcp_sock->nfc_protocol = llcp_addr.nfc_protocol;
> >         llcp_sock->service_name_len = min_t(unsigned int,
> >                                             llcp_addr.service_name_len,
> > @@ -108,13 +107,11 @@ static int llcp_sock_bind(struct socket *sock,
> struct sockaddr *addr, int alen)
> >                                           llcp_sock->service_name_len,
> >                                           GFP_KERNEL);
> >         if (!llcp_sock->service_name) {
> > -               nfc_llcp_local_put(llcp_sock->local);
> >                 ret = -ENOMEM;
> >                 goto put_dev;
> >         }
> >         llcp_sock->ssap = nfc_llcp_get_sdp_ssap(local, llcp_sock);
> >         if (llcp_sock->ssap == LLCP_SAP_MAX) {
> > -               nfc_llcp_local_put(llcp_sock->local);
> >                 kfree(llcp_sock->service_name);
> >                 llcp_sock->service_name = NULL;
> >                 ret = -EADDRINUSE;
> > @@ -122,6 +119,7 @@ static int llcp_sock_bind(struct socket *sock, struct
> sockaddr *addr, int alen)
> >         }
> >
> >         llcp_sock->reserved_ssap = llcp_sock->ssap;
> > +       llcp_sock->local = nfc_llcp_local_get(local);
> >
> >         nfc_llcp_sock_link(&local->sockets, sk);
> >
> >
> > Thanks
> 
> While your suggested fix will work for the bind path (it implicitly makes
> sure that the 'local' field is always NULL, up until the point that
> everything is initialized properly), we have a problem in the 'connect'
> path.

It is a mess.

> 
> If we try to take the same approach inside the function llcp_sock_connect,
> there is a call to nfc_llcp_send_connect, which requires the local field to
> be set inside the socket. We thought about changing its interface to just
> accept the 'local' as an argument, but this function is exported in the .h
> file, and we are afraid of breaking external interfaces. Therefore, we
> think that we should take a consistent approach of explicitly setting the
> local field to NULL in all paths. Note that this explicit assignment should
> achieve the same result as your approach, of implicitly letting it stay
> NULL up until we can safely assign to it.
> 
> Please let us know WDYT.

It is in-kernel API with only one user, I would personally use that
opportunity and cleaned nfc_llcp_send_connect() from ridiculous checks.

This is always false:
  401         local = sock->local;
  402         if (local == NULL)

Always true:
  405         if (sock->service_name != NULL)

However the more I look on that code the more I come to the conclusion that it
is not worth to change it.

Thanks

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

[Or Cohen]

On Wed, May 5, 2021 at 5:17 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Wed, May 05, 2021 at 04:36:05PM +0300, Nadav Markus wrote:
> > On Wed, May 5, 2021 at 2:40 PM Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > On Wed, May 05, 2021 at 12:35:48PM +0300, Nadav Markus wrote:
> > > > On Wed, May 5, 2021 at 7:46 AM Leon Romanovsky <leon@kernel.org> wrote:
> > > >
> > > > > 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.
> > > > >
> > > >
> > > > Hi Leon - as far as we understand, the underlying issue is not
> > incrementing
> > > > the kref counter without checking if the function nfc_llcp_local_get
> > > > succeeded or not. The function itself increments the reference count.
> > > >
> > > > The issue is that the nfc_llcp_local_put might be called twice on the
> > > > llcp_sock->local field, however only one reference (the one that was
> > gotten
> > > > via nfc_llcp_local_get) is incremented. llcp_local_put will be called in
> > > > two locations. The first one is just inside the bind function, if
> > > > nfc_llcp_get_local_ssap fails. The second one is called
> > unconditionally, at
> > > > the socket destruction, at the function nfc_llcp_sock_free.
> > > >
> > > > Hence, our proposed solution is to prevent the second nfc_llcp_local_put
> > > > from attempting to decrement the kref count, by setting local to NULL.
> > This
> > > > makes sense, as we immediately do so after decrementing the single ref
> > > > count we took when calling nfc_llcp_local_get. Since we are under the
> > sock
> > > > lock, this also should be race safe, as no one should access the
> > > > llcp_sock->local field without this lock's protection.
> > > >
> > > > >
> > > > > For example, what protection do you have from races between
> > > > > llcp_sock_bind(),
> > > > > nfc_llcp_sock_free() and llcp_sock_connect()?
> > > > >
> > > >
> > > > As we replied, the llcp_sock->local field is protected under the lock
> > sock,
> > > > as far as we understand.
> > > >
> > > > >
> > > > > 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.
> > > > >
> > > >
> > > > We don't quite get what race are we talking about here - our trigger
> > > > program doesn't even utilize threads. All it has to do is to
> > > > cause nfc_llcp_local_get to fail - this can be seen clearly in our
> > original
> > > > trigger program. To clarify, the two sockets that are created there
> > point
> > > > to the same nfc_llcp_local struct (via their local field). The
> > destruction
> > > > of the first socket causes the reference count of the pointed object to
> > > > drop to zero (since the code increments the ref count of the object
> > from 1
> > > > to 2, but dercements it twice). The second socket later attempts to
> > > > decrement the ref count of the same (already freed) nfc_llcp_local
> > object,
> > > > causing a kernel crash.
> > >
> > >
> > > So at the end, we are talking about situation where _get()/_put() are
> > > protected by the lock and local can't disappear. Can you please help me
> > to find
> > > this socket lock? Did I miss it in bind path?
> >
> > The lock we are talking about is the lock_sock - lock_sock(sk). It appears
> > near the start of the function.
> >
> > >
> > >
> > > net/socket.c:
> > >   int __sys_bind(int fd, struct sockaddr __user *umyaddr, int addrlen)
> > >     sock->ops->bind(..)
> > >      llcp_sock_bind(..)
> > >
> > > And if we put lock issue aside, all your change can be squeezed to the
> > following:
> > >
> > > diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c
> > > index a3b46f888803..cc9ee634269d 100644
> > > --- a/net/nfc/llcp_sock.c
> > > +++ b/net/nfc/llcp_sock.c
> > > @@ -99,7 +99,6 @@ static int llcp_sock_bind(struct socket *sock, struct
> > sockaddr *addr, int alen)
> > >         }
> > >
> > >         llcp_sock->dev = dev;
> > > -       llcp_sock->local = nfc_llcp_local_get(local);
> > >         llcp_sock->nfc_protocol = llcp_addr.nfc_protocol;
> > >         llcp_sock->service_name_len = min_t(unsigned int,
> > >                                             llcp_addr.service_name_len,
> > > @@ -108,13 +107,11 @@ static int llcp_sock_bind(struct socket *sock,
> > struct sockaddr *addr, int alen)
> > >                                           llcp_sock->service_name_len,
> > >                                           GFP_KERNEL);
> > >         if (!llcp_sock->service_name) {
> > > -               nfc_llcp_local_put(llcp_sock->local);
> > >                 ret = -ENOMEM;
> > >                 goto put_dev;
> > >         }
> > >         llcp_sock->ssap = nfc_llcp_get_sdp_ssap(local, llcp_sock);
> > >         if (llcp_sock->ssap == LLCP_SAP_MAX) {
> > > -               nfc_llcp_local_put(llcp_sock->local);
> > >                 kfree(llcp_sock->service_name);
> > >                 llcp_sock->service_name = NULL;
> > >                 ret = -EADDRINUSE;
> > > @@ -122,6 +119,7 @@ static int llcp_sock_bind(struct socket *sock, struct
> > sockaddr *addr, int alen)
> > >         }
> > >
> > >         llcp_sock->reserved_ssap = llcp_sock->ssap;
> > > +       llcp_sock->local = nfc_llcp_local_get(local);
> > >
> > >         nfc_llcp_sock_link(&local->sockets, sk);
> > >
> > >
> > > Thanks
> >
> > While your suggested fix will work for the bind path (it implicitly makes
> > sure that the 'local' field is always NULL, up until the point that
> > everything is initialized properly), we have a problem in the 'connect'
> > path.
>
> It is a mess.
>
> >
> > If we try to take the same approach inside the function llcp_sock_connect,
> > there is a call to nfc_llcp_send_connect, which requires the local field to
> > be set inside the socket. We thought about changing its interface to just
> > accept the 'local' as an argument, but this function is exported in the .h
> > file, and we are afraid of breaking external interfaces. Therefore, we
> > think that we should take a consistent approach of explicitly setting the
> > local field to NULL in all paths. Note that this explicit assignment should
> > achieve the same result as your approach, of implicitly letting it stay
> > NULL up until we can safely assign to it.
> >
> > Please let us know WDYT.
>
> It is in-kernel API with only one user, I would personally use that
> opportunity and cleaned nfc_llcp_send_connect() from ridiculous checks.
>
> This is always false:
>   401         local = sock->local;
>   402         if (local == NULL)
>
> Always true:
>   405         if (sock->service_name != NULL)
>
> However the more I look on that code the more I come to the conclusion that it
> is not worth to change it.
>
> Thanks

Yes, seems like.
So I guess the path is correct.