mirror of
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
synced 2024-12-29 09:13:38 +00:00
nfc: llcp_core: Hold a ref to llcp_local->dev when holding a ref to llcp_local
llcp_sock_sendmsg() calls nfc_llcp_send_ui_frame() which in turn calls
nfc_alloc_send_skb(), which accesses the nfc_dev from the llcp_sock for
getting the headroom and tailroom needed for skb allocation.
Parallelly the nfc_dev can be freed, as the refcount is decreased via
nfc_free_device(), leading to a UAF reported by Syzkaller, which can
be summarized as follows:
(1) llcp_sock_sendmsg() -> nfc_llcp_send_ui_frame()
-> nfc_alloc_send_skb() -> Dereference *nfc_dev
(2) virtual_ncidev_close() -> nci_free_device() -> nfc_free_device()
-> put_device() -> nfc_release() -> Free *nfc_dev
When a reference to llcp_local is acquired, we do not acquire the same
for the nfc_dev. This leads to freeing even when the llcp_local is in
use, and this is the case with the UAF described above too.
Thus, when we acquire a reference to llcp_local, we should acquire a
reference to nfc_dev, and release the references appropriately later.
References for llcp_local is initialized in nfc_llcp_register_device()
(which is called by nfc_register_device()). Thus, we should acquire a
reference to nfc_dev there.
nfc_unregister_device() calls nfc_llcp_unregister_device() which in
turn calls nfc_llcp_local_put(). Thus, the reference to nfc_dev is
appropriately released later.
Reported-and-tested-by: syzbot+bbe84a4010eeea00982d@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=bbe84a4010eeea00982d
Fixes: c7aa12252f
("NFC: Take a reference on the LLCP local pointer when creating a socket")
Reviewed-by: Suman Ghosh <sumang@marvell.com>
Signed-off-by: Siddh Raman Pant <code@siddh.me>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: David S. Miller <davem@davemloft.net>
This commit is contained in:
parent
7c5e046bdc
commit
c95f919567
@ -145,6 +145,13 @@ static void nfc_llcp_socket_release(struct nfc_llcp_local *local, bool device,
|
||||
|
||||
static struct nfc_llcp_local *nfc_llcp_local_get(struct nfc_llcp_local *local)
|
||||
{
|
||||
/* Since using nfc_llcp_local may result in usage of nfc_dev, whenever
|
||||
* we hold a reference to local, we also need to hold a reference to
|
||||
* the device to avoid UAF.
|
||||
*/
|
||||
if (!nfc_get_device(local->dev->idx))
|
||||
return NULL;
|
||||
|
||||
kref_get(&local->ref);
|
||||
|
||||
return local;
|
||||
@ -177,10 +184,18 @@ static void local_release(struct kref *ref)
|
||||
|
||||
int nfc_llcp_local_put(struct nfc_llcp_local *local)
|
||||
{
|
||||
struct nfc_dev *dev;
|
||||
int ret;
|
||||
|
||||
if (local == NULL)
|
||||
return 0;
|
||||
|
||||
return kref_put(&local->ref, local_release);
|
||||
dev = local->dev;
|
||||
|
||||
ret = kref_put(&local->ref, local_release);
|
||||
nfc_put_device(dev);
|
||||
|
||||
return ret;
|
||||
}
|
||||
|
||||
static struct nfc_llcp_sock *nfc_llcp_sock_get(struct nfc_llcp_local *local,
|
||||
@ -959,8 +974,17 @@ static void nfc_llcp_recv_connect(struct nfc_llcp_local *local,
|
||||
}
|
||||
|
||||
new_sock = nfc_llcp_sock(new_sk);
|
||||
new_sock->dev = local->dev;
|
||||
|
||||
new_sock->local = nfc_llcp_local_get(local);
|
||||
if (!new_sock->local) {
|
||||
reason = LLCP_DM_REJ;
|
||||
sock_put(&new_sock->sk);
|
||||
release_sock(&sock->sk);
|
||||
sock_put(&sock->sk);
|
||||
goto fail;
|
||||
}
|
||||
|
||||
new_sock->dev = local->dev;
|
||||
new_sock->rw = sock->rw;
|
||||
new_sock->miux = sock->miux;
|
||||
new_sock->nfc_protocol = sock->nfc_protocol;
|
||||
@ -1597,7 +1621,16 @@ int nfc_llcp_register_device(struct nfc_dev *ndev)
|
||||
if (local == NULL)
|
||||
return -ENOMEM;
|
||||
|
||||
local->dev = ndev;
|
||||
/* As we are going to initialize local's refcount, we need to get the
|
||||
* nfc_dev to avoid UAF, otherwise there is no point in continuing.
|
||||
* See nfc_llcp_local_get().
|
||||
*/
|
||||
local->dev = nfc_get_device(ndev->idx);
|
||||
if (!local->dev) {
|
||||
kfree(local);
|
||||
return -ENODEV;
|
||||
}
|
||||
|
||||
INIT_LIST_HEAD(&local->list);
|
||||
kref_init(&local->ref);
|
||||
mutex_init(&local->sdp_lock);
|
||||
|
Loading…
Reference in New Issue
Block a user