Re: [PATCH net v2] nfc: llcp: prevent resource leak on repeated connect after DM

[Aldo Ariel Panzardo <qwe.aldo@gmail.com>]

Thanks for the detailed review.  v3 addresses the device reference
issues; the remaining points are either pre-existing or need a
follow-up.  Responding to each finding below.

> [Critical] Can this release resources that a blocking connect() on the
> same socket still owns?  (Thread A in sock_wait_state, Thread B wins
> lock_sock and runs cleanup, Thread A unwinds with stale local/dev)

This is a real concern but pre-existing: the socket lock is dropped
inside sock_wait_state() and a second connect() on the same fd from
another thread can race regardless of this patch.  The cleanup does
not make the window wider -- without it, Thread B's connect() would
overwrite the fields without releasing anything (the original leak).
A proper fix for concurrent connect() on the same socket would need
serialization beyond the socket lock, which is a separate change.

> [Critical] Does a CLOSED socket with non-NULL llcp_sock->dev always
> own a reference?  bind() keeps dev without a reference;
> socket_release() drops the connected ref without clearing dev.

This is what syzbot confirmed and v3 fixes.  v3 clears llcp_sock->dev
in nfc_llcp_socket_release() after the connected put, and for
bound/listening sockets that never owned a device reference.  It also
clears dev in nfc_llcp_recv_dm() for bound/listening sockets before
setting LLCP_CLOSED.  After v3, the cleanup's if (llcp_sock->dev)
guard only fires when the socket genuinely owns the reference (the
rejected async connect case).

The syzbot reproducer for the v2 double-put passes cleanly with v3
applied (tested with KASAN, 0 reports).

> [High] Is the socket always off local->sockets at this point?
> Cleanup sets local = NULL without unlinking; socket stays hashed.

Valid concern.  The cleanup should unlink the socket from whichever
list it is on before clearing ->local.  This is not addressed in v3
and needs a follow-up patch.  I will send one.

> [High] Can the same leak happen through bind()?

Yes.  bind() also accepts CLOSED sockets and overwrites the fields.
The cleanup helper should be called from bind() as well.  Not
addressed in v3; will include in the follow-up.

> [High, pre-existing] Stale sk_err = ENXIO from recv_dm not cleared
> on retry; if recv_cc() races in, connect() unwinds and leaves dev
> NULL, then destruct dereferences NULL.

Pre-existing and not introduced by this patch.  Clearing sk_err in
the LLCP_CLOSED cleanup is the right thing to do.  Will include in
the follow-up.

Summary of what is addressed and what remains:

  v3 fixes:
  - Device reference underflow (syzbot confirmed, KASAN verified)

  Follow-up needed:
  - Unlink socket from local->sockets/connecting_sockets in cleanup
  - Call cleanup from bind() as well
  - Clear stale sk_err on reconnect

I will send the follow-up as a separate patch once v3 is reviewed.

thanks,
Aldo