Messages in this thread | | | Subject | Re: [PATCH net-next v4 1/2] nfc: llcp_core: Hold a ref to llcp_local->dev when holding a ref to llcp_local | From | Paolo Abeni <> | Date | Tue, 12 Dec 2023 14:01:57 +0100 |
| |
On Sat, 2023-12-09 at 16:34 +0530, Siddh Raman Pant wrote: > 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: c7aa12252f51 ("NFC: Take a reference on the LLCP local pointer when creating a socket") > Signed-off-by: Siddh Raman Pant <code@siddh.me> > Reviewed-by: Suman Ghosh <sumang@marvell.com> > --- > net/nfc/llcp_core.c | 55 ++++++++++++++++++++++++++++++++++----------- > 1 file changed, 42 insertions(+), 13 deletions(-) > > diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c > index 1dac28136e6a..0ae89ab42aaa 100644 > --- a/net/nfc/llcp_core.c > +++ b/net/nfc/llcp_core.c > @@ -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, > @@ -930,9 +945,7 @@ static void nfc_llcp_recv_connect(struct nfc_llcp_local *local, > > if (sk_acceptq_is_full(parent)) { > reason = LLCP_DM_REJ;
'reason' is set to 'LLCP_DM_REJ' every time you jump to the 'fail_put_sock' or 'fail_free_new_sock' labels, you can as well move the assignment after 'fail_put_sock:'
Cheers,
Paolo
| |