nfc_llcp_sock_get() takes a reference on the LLCP socket via sock_hold().

In nfc_llcp_recv_disc(), when the socket is already in LLCP_CLOSED state, the
code used to perform release_sock() and nfc_llcp_sock_put() in the CLOSED branch
but then continued execution and later performed the same cleanup again on the
common exit path. This results in refcount imbalance (double put) and unbalanced
lock release.

Remove the redundant CLOSED-branch cleanup so that release_sock() and
nfc_llcp_sock_put() are performed exactly once via the common exit path, while
keeping the existing DM_DISC reply behavior.

Fixes: d646960f7986fefb460a2b062d5ccc8ccfeacc3a ("NFC: Initial LLCP support")
Cc: stable@vger.kernel.org
Signed-off-by: Qianchang Zhao <pioooooooooip@gmail.com>
---
 net/nfc/llcp_core.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
index beeb3b4d2..ed37604ed 100644
--- a/net/nfc/llcp_core.c
+++ b/net/nfc/llcp_core.c
@@ -1177,11 +1177,6 @@ static void nfc_llcp_recv_disc(struct nfc_llcp_local *local,
 
 	nfc_llcp_socket_purge(llcp_sock);
 
-	if (sk->sk_state == LLCP_CLOSED) {
-		release_sock(sk);
-		nfc_llcp_sock_put(llcp_sock);
-	}
-
 	if (sk->sk_state == LLCP_CONNECTED) {
 		nfc_put_device(local->dev);
 		sk->sk_state = LLCP_CLOSED;
-- 
2.34.1

On 17/12/2025 13:46, Qianchang Zhao wrote:
> nfc_llcp_sock_get() takes a reference on the LLCP socket via sock_hold().
> 
> In nfc_llcp_recv_disc(), when the socket is already in LLCP_CLOSED state, the
> code used to perform release_sock() and nfc_llcp_sock_put() in the CLOSED branch
> but then continued execution and later performed the same cleanup again on the
> common exit path. This results in refcount imbalance (double put) and unbalanced

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597

> lock release.
> 
> Remove the redundant CLOSED-branch cleanup so that release_sock() and
> nfc_llcp_sock_put() are performed exactly once via the common exit path, while
> keeping the existing DM_DISC reply behavior.
> 
> Fixes: d646960f7986fefb460a2b062d5ccc8ccfeacc3a ("NFC: Initial LLCP support")

12 char sha.

Please run scripts/checkpatch.pl on the patches and fix reported
warnings. After that, run also 'scripts/checkpatch.pl --strict' on the
patches and (probably) fix more warnings. Some warnings can be ignored,
especially from --strict run, but the code here looks like it needs a
fix. Feel free to get in touch if the warning is not clear.


> Cc: stable@vger.kernel.org
> Signed-off-by: Qianchang Zhao <pioooooooooip@gmail.com>
> ---
>  net/nfc/llcp_core.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
> index beeb3b4d2..ed37604ed 100644
> --- a/net/nfc/llcp_core.c
> +++ b/net/nfc/llcp_core.c
> @@ -1177,11 +1177,6 @@ static void nfc_llcp_recv_disc(struct nfc_llcp_local *local,
>  
>  	nfc_llcp_socket_purge(llcp_sock);
>  
> -	if (sk->sk_state == LLCP_CLOSED) {
> -		release_sock(sk);
> -		nfc_llcp_sock_put(llcp_sock);

You did not answer my previous review. You also did not answer my
concerns from earlier private report. Please respond before you send
again v3.

> -	}
> -
>  	if (sk->sk_state == LLCP_CONNECTED) {
>  		nfc_put_device(local->dev);
>  		sk->sk_state = LLCP_CLOSED;


Best regards,
Krzysztof

Hi Krzysztof,

Sorry about that — my previous response might not have made it to the
list/thread.
Replying here to address your concerns before sending v3.

1) DM_DISC reply after LLCP_CLOSED
This is not a new behavior introduced by my change. In the old code, the
LLCP_CLOSED branch did release_sock() and nfc_llcp_sock_put(), but it did not
return/goto, so execution continued and still reached nfc_llcp_send_dm(...,
LLCP_DM_DISC) afterwards. The disc patch only removes the redundant
CLOSED-branch
cleanup so release_sock()/nfc_llcp_sock_put() are performed exactly once via the
common exit path, while keeping the existing DM_DISC reply behavior.

2) Initial refcount / double free concern
nfc_llcp_recv_disc()/recv_hdlc() take an extra reference via nfc_llcp_sock_get()
(sock_hold()). The issue is the mismatched put/unlock: the CLOSED branch drops
the reference and releases the lock, and then the common exit path does the same
again. This is a refcount/locking imbalance regardless of whether it immediately
frees the object, and it may become a UAF depending on timing/refcounting.

Regarding your formatting notes: I will wrap commit messages per
submitting-patches,
use a 12-char sha in Fixes, and run scripts/checkpatch.pl (and --strict) and fix
reported warnings before sending v3.

Best regards,
Qianchang

On Wed, Dec 17, 2025 at 9:57 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 17/12/2025 13:46, Qianchang Zhao wrote:
> > nfc_llcp_sock_get() takes a reference on the LLCP socket via sock_hold().
> >
> > In nfc_llcp_recv_disc(), when the socket is already in LLCP_CLOSED state, the
> > code used to perform release_sock() and nfc_llcp_sock_put() in the CLOSED branch
> > but then continued execution and later performed the same cleanup again on the
> > common exit path. This results in refcount imbalance (double put) and unbalanced
>
> Please wrap commit message according to Linux coding style / submission
> process (neither too early nor over the limit):
> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
>
> > lock release.
> >
> > Remove the redundant CLOSED-branch cleanup so that release_sock() and
> > nfc_llcp_sock_put() are performed exactly once via the common exit path, while
> > keeping the existing DM_DISC reply behavior.
> >
> > Fixes: d646960f7986fefb460a2b062d5ccc8ccfeacc3a ("NFC: Initial LLCP support")
>
> 12 char sha.
>
> Please run scripts/checkpatch.pl on the patches and fix reported
> warnings. After that, run also 'scripts/checkpatch.pl --strict' on the
> patches and (probably) fix more warnings. Some warnings can be ignored,
> especially from --strict run, but the code here looks like it needs a
> fix. Feel free to get in touch if the warning is not clear.
>
>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Qianchang Zhao <pioooooooooip@gmail.com>
> > ---
> >  net/nfc/llcp_core.c | 5 -----
> >  1 file changed, 5 deletions(-)
> >
> > diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
> > index beeb3b4d2..ed37604ed 100644
> > --- a/net/nfc/llcp_core.c
> > +++ b/net/nfc/llcp_core.c
> > @@ -1177,11 +1177,6 @@ static void nfc_llcp_recv_disc(struct nfc_llcp_local *local,
> >
> >       nfc_llcp_socket_purge(llcp_sock);
> >
> > -     if (sk->sk_state == LLCP_CLOSED) {
> > -             release_sock(sk);
> > -             nfc_llcp_sock_put(llcp_sock);
>
> You did not answer my previous review. You also did not answer my
> concerns from earlier private report. Please respond before you send
> again v3.
>
> > -     }
> > -
> >       if (sk->sk_state == LLCP_CONNECTED) {
> >               nfc_put_device(local->dev);
> >               sk->sk_state = LLCP_CLOSED;
>
>
> Best regards,
> Krzysztof

On 17/12/2025 14:05, くさあさ wrote:
> Hi Krzysztof,
> 
> Sorry about that — my previous response might not have made it to the
> list/thread.
> Replying here to address your concerns before sending v3.
> 
> 1) DM_DISC reply after LLCP_CLOSED
> This is not a new behavior introduced by my change. In the old code, the
> LLCP_CLOSED branch did release_sock() and nfc_llcp_sock_put(), but it did not
> return/goto, so execution continued and still reached nfc_llcp_send_dm(...,
> LLCP_DM_DISC) afterwards. The disc patch only removes the redundant
> CLOSED-branch
> cleanup so release_sock()/nfc_llcp_sock_put() are performed exactly once via the
> common exit path, while keeping the existing DM_DISC reply behavior.

I understand that you did not change the flow. I did not claim you did.
I ask why do you think your code is correct.

Do not top post and do not send new versions while the discussion is
still going.
> 
> 2) Initial refcount / double free concern
> nfc_llcp_recv_disc()/recv_hdlc() take an extra reference via nfc_llcp_sock_get()
> (sock_hold()). The issue is the mismatched put/unlock: the CLOSED branch drops
> the reference and releases the lock, and then the common exit path does the same
> again. This is a refcount/locking imbalance regardless of whether it immediately
> frees the object, and it may become a UAF depending on timing/refcounting.

You did not really address the problem. The refcnt has imbalance only if
you assume initial refcnt was 0.



Best regards,
Krzysztof

> I ask why do you think your code is correct.

Because the current code can execute release_sock()/nfc_llcp_sock_put() twice
for the single ref acquired by nfc_llcp_sock_get(), and it can keep operating
on llcp_sock/sk after the first put/unlock. The fix ensures exactly one cleanup
(recv_disc) or exits immediately after the CLOSED cleanup (recv_hdlc), while
keeping the existing DM_DISC send behavior.

> The refcnt has imbalance only if you assume initial refcnt was 0.

No. Let baseline refcnt be N before nfc_llcp_sock_get(); sock_hold() makes N+1.
CLOSED path put -> N, common exit put -> N-1. So it drops one extra ref
regardless of N (whether it immediately frees depends on N). git blame shows
both cleanup sites trace back to d646960f7986.

I will not send a new revision until this discussion is resolved.

Best regards,
Qianchang

On Thu, Dec 18, 2025 at 7:22 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 17/12/2025 14:05, くさあさ wrote:
> > Hi Krzysztof,
> >
> > Sorry about that — my previous response might not have made it to the
> > list/thread.
> > Replying here to address your concerns before sending v3.
> >
> > 1) DM_DISC reply after LLCP_CLOSED
> > This is not a new behavior introduced by my change. In the old code, the
> > LLCP_CLOSED branch did release_sock() and nfc_llcp_sock_put(), but it did not
> > return/goto, so execution continued and still reached nfc_llcp_send_dm(...,
> > LLCP_DM_DISC) afterwards. The disc patch only removes the redundant
> > CLOSED-branch
> > cleanup so release_sock()/nfc_llcp_sock_put() are performed exactly once via the
> > common exit path, while keeping the existing DM_DISC reply behavior.
>
> I understand that you did not change the flow. I did not claim you did.
> I ask why do you think your code is correct.
>
> Do not top post and do not send new versions while the discussion is
> still going.
> >
> > 2) Initial refcount / double free concern
> > nfc_llcp_recv_disc()/recv_hdlc() take an extra reference via nfc_llcp_sock_get()
> > (sock_hold()). The issue is the mismatched put/unlock: the CLOSED branch drops
> > the reference and releases the lock, and then the common exit path does the same
> > again. This is a refcount/locking imbalance regardless of whether it immediately
> > frees the object, and it may become a UAF depending on timing/refcounting.
>
> You did not really address the problem. The refcnt has imbalance only if
> you assume initial refcnt was 0.
>
>
>
> Best regards,
> Krzysztof