nfc: Fix UAF during datagram sending caused by missing refcounting
Status from this message: This series was applied to netdev/net.git (main)
Get series with b4
b4 am [email protected]Versions newest first · linked by author and title
- v7 Latest2 patches 1 replyApplied 2/2
- v62 patches 2 repliesSuperseded
- v52 patches 3 repliesSuperseded
Earlier versions · 3
| Version | Posted | Patches | Replies | Status |
|---|---|---|---|---|
| v4 | 2 patches | 2 replies | Superseded | |
| v3 | 2 patches | 8 replies | Superseded | |
| v2 | 2 patches | 3 replies | Superseded |
Changes in v7 from the cover letter
- Stupidly reverted ordering in recv() too, fix that.
- Remove redundant call to nfc_llcp_sock_free().
Patches in v7 All 2 applied
| # | Subject and files | Size | Replies | Applied |
|---|---|---|---|---|
| 1/2 | nfc: llcp_core: Hold a ref to llcp_local->dev when holding a ref to llcp_localnet/ | +36 | 1 reply | Applied |
| 2/2 | nfc: Do not send datagram if socket state isn't LLCP_BOUNDnet/ | +5 | 0 replies | Applied |
Reviews trailers per patch
All 2 patches have a Reviewed-by or Acked-by.
| Patch | Krzysztof Kozlowski | Suman Ghosh | [email protected] |
|---|---|---|---|
| 1/2 | R-b | R-b | Reported-and-tested-by |
| 2/2 | R-b | R-b |
Trailers given in replies are filled; those carried in the patches from earlier versions are outlined with dashes. Scroll the table sideways for all 3 reviewers.
Cover letter [PATCH net-next v7 0/2] nfc: Fix UAF during datagram sending caused by missing refcounting
Changes in v7:
- Stupidly reverted ordering in recv() too, fix that.
- Remove redundant call to nfc_llcp_sock_free().
Changes in v6:
- Revert label introduction from v4, and thus also v5 entirely.
Changes in v5:
- Move reason = LLCP_DM_REJ under the fail_put_sock label.
- Checkpatch now warns about == NULL check for new_sk, so fix that, and also at other similar places in the same function.
Changes in v4:
- Fix put ordering and comments.
- Separate freeing in recv() into end labels.
- Remove obvious comment and add reasoning.
- Picked up r-bs by Suman.
Changes in v3:
- Fix missing freeing statements.
Changes in v2:
- Add net-next in patch subject.
- Removed unnecessary extra lock and hold nfc_dev ref when holding llcp_sock.
- Remove last formatting patch.
- Picked up r-b from Krzysztof for LLCP_BOUND patch.
---
For connectionless transmission, llcp_sock_sendmsg() codepath will eventually call nfc_alloc_send_skb() which takes in an nfc_dev as an argument for calculating the total size for skb allocation.
virtual_ncidev_close() codepath eventually releases socket by calling nfc_llcp_socket_release() (which sets the sk->sk_state to LLCP_CLOSED) and afterwards the nfc_dev will be eventually freed.
When an ndev gets freed, llcp_sock_sendmsg() will result in an use-after-free as it
(1) doesn't have any checks in place for avoiding the datagram sending.
(2) calls nfc_llcp_send_ui_frame(), which also has a do-while loop which can race with freeing. This loop contains the call to nfc_alloc_send_skb() where we dereference the nfc_dev pointer.
nfc_dev is being freed because we do not hold a reference to it when we hold a reference to llcp_local. Thus, virtual_ncidev_close() eventually calls nfc_release() due to refcount going to 0.
Since state has to be LLCP_BOUND for datagram sending, we can bail out early in llcp_sock_sendmsg().
Please review and let me know if any errors are there, and hopefully this gets accepted.
Thanks,
Siddh
Siddh Raman Pant (2):
nfc: llcp_core: Hold a ref to llcp_local->dev when holding a ref to
llcp_local
nfc: Do not send datagram if socket state isn't LLCP_BOUND
net/nfc/llcp_core.c | 39 ++++++++++++++++++++++++++++++++++++--- net/nfc/llcp_sock.c | 5 +++++ 2 files changed, 41 insertions(+), 3 deletions(-)
Signature · 1 line
2.43.0