| Message ID | 4143dc4398aa4940a76d3f375ec7984e98891a11.1701627492.git.code@siddh.me (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | nfc: Fix UAF during datagram sending caused by missing refcounting | expand |
>Reported-and-tested-by: >syzbot+bbe84a4010eeea00982d@syzkaller.appspotmail.com >Closes: https://urldefense.proofpoint.com/v2/url?u=https- >3A__syzkaller.appspot.com_bug-3Fextid- >3Dbbe84a4010eeea00982d&d=DwIDAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=7si3Xn9Ly- >Se1a655kvEPIYU0nQ9HPeN280sEUv5ROU&m=Y51GaQT3fKSNuGmD- >9dRkeRq59DJDzTJ7yEh86VaPxCJP7TBLuK6eYKAw-fnG7- >c&s=NUdyCeltsQ_6_rZPnNvgfyPleIYGMaSIN1sC1zqpV7Y&e= >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 | 24 ++++++++++++++++++++++-- > 1 file changed, 22 insertions(+), 2 deletions(-)
On 04/12/2023 14:08, 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> > --- > net/nfc/llcp_core.c | 24 ++++++++++++++++++++++-- > 1 file changed, 22 insertions(+), 2 deletions(-) > > diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c > index 1dac28136e6a..9d45ce6dcdca 100644 > --- a/net/nfc/llcp_core.c > +++ b/net/nfc/llcp_core.c > @@ -145,6 +145,9 @@ 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) > { > + if (!nfc_get_device(local->dev->idx)) > + return NULL; > + > kref_get(&local->ref); > > return local; > @@ -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. > return kref_put(&local->ref, local_release); > } > > @@ -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. > + 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. > + 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. Best regards, Krzysztof
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. > > @@ -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. > > + 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. Thanks, Siddh
On Tue, 05 Dec 2023 22:51:17 +0530, Siddh Raman Pant wrote: > 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. Or nfc_find_device() instead of nfc_get_device_from_idx(). Thanks, Siddh
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
On Tue, 05 Dec 2023 22:57:28 +0530, Krzysztof Kozlowski wrote: > >> 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. Oops, my bad. What would be the valid reason? (if any) Thanks, Siddh
On 05/12/2023 19:01, Siddh Raman Pant wrote: > On Tue, 05 Dec 2023 22:57:28 +0530, Krzysztof Kozlowski wrote: >>>> 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. > > Oops, my bad. What would be the valid reason? (if any) Just store the local->dev pointer and use correct order. Best regards, Krzysztof
diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c index 1dac28136e6a..9d45ce6dcdca 100644 --- a/net/nfc/llcp_core.c +++ b/net/nfc/llcp_core.c @@ -145,6 +145,9 @@ 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) { + if (!nfc_get_device(local->dev->idx)) + return NULL; + kref_get(&local->ref); return local; @@ -180,6 +183,7 @@ int nfc_llcp_local_put(struct nfc_llcp_local *local) if (local == NULL) return 0; + nfc_put_device(local->dev); return kref_put(&local->ref, local_release); } @@ -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) { + 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. */ + 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);
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> --- net/nfc/llcp_core.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-)