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
| ||
|
Message-ID: <d41ea6ff-3c29-4a76-833d-19e6a6649d3c@linaro.org> Date: Tue, 5 Dec 2023 18:27:28 +0100 From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org> To: Siddh Raman Pant <code@...dh.me> Cc: "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Suman Ghosh <sumang@...vell.com>, netdev <netdev@...r.kernel.org>, linux-kernel <linux-kernel@...r.kernel.org>, syzbot+bbe84a4010eeea00982d <syzbot+bbe84a4010eeea00982d@...kaller.appspotmail.com> Subject: Re: [PATCH net-next v3 1/2] nfc: llcp_core: Hold a ref to llcp_local->dev when holding a ref to llcp_local On 05/12/2023 18:21, Siddh Raman Pant wrote: > On Tue, 05 Dec 2023 22:10:00 +0530, Krzysztof Kozlowski wrote: >>> @@ -180,6 +183,7 @@ int nfc_llcp_local_put(struct nfc_llcp_local *local) >>> if (local == NULL) >>> return 0; >>> >>> + nfc_put_device(local->dev); >> >> Mismatched order with get. Unwinding is always in reversed order. Or >> maybe other order is here on purpose? Then it needs to be explained. > > Yes, local_release() will free local, so local->dev cannot be accessed. > Will add a comment. So the problem is just storing the pointer? That's not really the valid reason. > >>> @@ -959,8 +963,18 @@ 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) { >> >> There is already an cleanup path/label, so extend it. Existing code >> needs some improvements in that matter as well. > > Sure. > >>> + reason = LLCP_DM_REJ; >>> + release_sock(&sock->sk); >>> + sock_put(&sock->sk); >>> + sock_put(&new_sock->sk); >>> + nfc_llcp_sock_free(new_sock); >>> + 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 +1611,13 @@ int nfc_llcp_register_device(struct nfc_dev *ndev) >>> if (local == NULL) >>> return -ENOMEM; >>> >>> - local->dev = ndev; >>> + /* Hold a reference to the device. */ >> >> That's obvious. Instead write something not obvious - why you call >> nfc_get_device() while not incrementing reference to llcp_local. > > Should I move it after kref_init()? Here, I'm bailing out early so we > don't have to do unnecessary init first, and the rest of the function > will never fail. I meant, comment is obvious. > >>> + local->dev = nfc_get_device(ndev->idx); >> >> This looks confusing. If you can access ndev->idx, then ndev reference >> was already increased. In such case iterating through all devices to >> find it, is unnecessary and confusing. > > I agree, it was something I thought about as well. There should be a > new function for refcount increment. Maybe the existing one could be > renamed to nfc_get_device_from_idx() and a new nfc_get_device() be > defined. > > I didn't want to introduce improvement patches in this UAF series, as > that would be an independent unit of change. Best regards, Krzysztof
Powered by blists - more mailing lists