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

[Nadav Markus <nmarkus@paloaltonetworks.com>]

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.