| Message ID | cover.1702925869.git.code@siddh.me (mailing list archive) |
|---|---|
| Headers | show |
| Series | nfc: Fix UAF during datagram sending caused by missing refcounting | expand |
Hello: This series was applied to netdev/net.git (main) by David S. Miller <davem@davemloft.net>: On Tue, 19 Dec 2023 23:19:42 +0530 you wrote: > 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. > > [...] Here is the summary with links: - [net-next,v7,1/2] nfc: llcp_core: Hold a ref to llcp_local->dev when holding a ref to llcp_local https://git.kernel.org/netdev/net/c/c95f919567d6 - [net-next,v7,2/2] nfc: Do not send datagram if socket state isn't LLCP_BOUND https://git.kernel.org/netdev/net/c/6ec0d7527c42 You are awesome, thank you!
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(-)