netdev - RE: [EXT] [PATCH net-next v2 1/2] nfc: llcp_core: Hold a ref to llcp_local->dev when holding a ref to llcp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Sun, 3 Dec 2023 16:59:39 +0000
From: Suman Ghosh <sumang@...vell.com>
To: Siddh Raman Pant <code@...dh.me>,
        Krzysztof Kozlowski
	<krzysztof.kozlowski@...aro.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
        Paolo
 Abeni <pabeni@...hat.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Samuel Ortiz
	<sameo@...ux.intel.com>,
        "syzbot+bbe84a4010eeea00982d@...kaller.appspotmail.com"
	<syzbot+bbe84a4010eeea00982d@...kaller.appspotmail.com>
Subject: RE: [EXT] [PATCH net-next v2 1/2] nfc: llcp_core: Hold a ref to
 llcp_local->dev when holding a ref to llcp_local

Hi Siddh,

>@@ -180,6 +183,7 @@ int nfc_llcp_local_put(struct nfc_llcp_local *local)
> 	if (local == NULL)
> 		return 0;
>
>+	nfc_put_device(local->dev);
[Suman] One question here, if we consider the path, nfc_llcp_mac_is_down() -> nfc_llcp_socket_release() -> nfc_llcp_local_put(), then inside nfc_llcp_socket_release()
we are already doing nfc_put_device() if "sk->sk_state == LLCP_CONNECTED", with this change we are doing it again. I guess you need to add some check to avoid that. Let me know if I am missing something.

> 	return kref_put(&local->ref, local_release);  }
>
>@@ -959,8 +963,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;
>+		release_sock(&sock->sk);
>+		sock_put(&sock->sk);
>+		sock_put(&new_sock->sk);
[Suman] don't we need to free new_sock? nfc_llcp_sock_free()?
>+		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 +1610,11 @@
>int nfc_llcp_register_device(struct nfc_dev *ndev)
> 	if (local == NULL)
> 		return -ENOMEM;
>
>-	local->dev = ndev;
>+	/* Hold a reference to the device. */
>+	local->dev = nfc_get_device(ndev->idx);
>+	if (!local->dev)
>+		return -ENODEV;
[Suman] Memory leak here. Need to call kfree(local).
>+
> 	INIT_LIST_HEAD(&local->list);
> 	kref_init(&local->ref);
> 	mutex_init(&local->sdp_lock);
>--
>2.42.0
>