lkml.org 
[lkml]   [2023]   [Dec]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH net-next v7 1/2] nfc: llcp_core: Hold a ref to llcp_local->dev when holding a ref to llcp_local
    Date
    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")
    Reviewed-by: Suman Ghosh <sumang@marvell.com>
    Signed-off-by: Siddh Raman Pant <code@siddh.me>
    ---
    net/nfc/llcp_core.c | 39 ++++++++++++++++++++++++++++++++++++---
    1 file changed, 36 insertions(+), 3 deletions(-)

    diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
    index 1dac28136e6a..18be13fb9b75 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,
    @@ -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);
    --
    2.43.0

    \
     
     \ /
      Last update: 2023-12-19 18:55    [W:5.376 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site