| Message ID | Zys4hCj61V+mQfX2@v4bel-B760M-AORUS-ELITE-AX (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e629295bd60abf4da1db85b82819ca6a4f6c1e79 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] hv_sock: Initializing vsk->trans to NULL to prevent a dangling pointer | expand |
On Wed, Nov 06, 2024 at 04:36:04AM -0500, Hyunwoo Kim wrote: >When hvs is released, there is a possibility that vsk->trans may not >be initialized to NULL, which could lead to a dangling pointer. >This issue is resolved by initializing vsk->trans to NULL. > >Fixes: ae0078fcf0a5 ("hv_sock: implements Hyper-V transport for Virtual Sockets (AF_VSOCK)") >Cc: stable@vger.kernel.org >Signed-off-by: Hyunwoo Kim <v4bel@theori.io> >--- >v1 -> v2: Add fixes and cc tags >--- > net/vmw_vsock/hyperv_transport.c | 1 + > 1 file changed, 1 insertion(+) Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> > >diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c >index e2157e387217..56c232cf5b0f 100644 >--- a/net/vmw_vsock/hyperv_transport.c >+++ b/net/vmw_vsock/hyperv_transport.c >@@ -549,6 +549,7 @@ static void hvs_destruct(struct vsock_sock *vsk) > vmbus_hvsock_device_unregister(chan); > > kfree(hvs); >+ vsk->trans = NULL; > } > > static int hvs_dgram_bind(struct vsock_sock *vsk, struct sockaddr_vm *addr) >-- >2.34.1 >
On Wed, Nov 06, 2024 at 04:36:04AM -0500, Hyunwoo Kim wrote: > When hvs is released, there is a possibility that vsk->trans may not > be initialized to NULL, which could lead to a dangling pointer. > This issue is resolved by initializing vsk->trans to NULL. > > Fixes: ae0078fcf0a5 ("hv_sock: implements Hyper-V transport for Virtual Sockets (AF_VSOCK)") > Cc: stable@vger.kernel.org > Signed-off-by: Hyunwoo Kim <v4bel@theori.io> Acked-by: Michael S. Tsirkin <mst@redhat.com> > --- > v1 -> v2: Add fixes and cc tags > --- > net/vmw_vsock/hyperv_transport.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c > index e2157e387217..56c232cf5b0f 100644 > --- a/net/vmw_vsock/hyperv_transport.c > +++ b/net/vmw_vsock/hyperv_transport.c > @@ -549,6 +549,7 @@ static void hvs_destruct(struct vsock_sock *vsk) > vmbus_hvsock_device_unregister(chan); > > kfree(hvs); > + vsk->trans = NULL; > } > > static int hvs_dgram_bind(struct vsock_sock *vsk, struct sockaddr_vm *addr) > -- > 2.34.1
On Wed, 6 Nov 2024 04:36:04 -0500 Hyunwoo Kim wrote: > When hvs is released, there is a possibility that vsk->trans may not > be initialized to NULL, which could lead to a dangling pointer. > This issue is resolved by initializing vsk->trans to NULL. > > Fixes: ae0078fcf0a5 ("hv_sock: implements Hyper-V transport for Virtual Sockets (AF_VSOCK)") > Cc: stable@vger.kernel.org I don't see the v1 on netdev@, nor a link to it in the change log so I may be missing the context, but the commit message is a bit sparse. The stable and Fixes tags indicate this is a fix. But the commit message reads like currently no such crash is observed, quote: which could lead to a dangling pointer. ^^^^^ ? Could someone clarify?
On Thu, Nov 07, 2024 at 11:29:42AM -0800, Jakub Kicinski wrote: > On Wed, 6 Nov 2024 04:36:04 -0500 Hyunwoo Kim wrote: > > When hvs is released, there is a possibility that vsk->trans may not > > be initialized to NULL, which could lead to a dangling pointer. > > This issue is resolved by initializing vsk->trans to NULL. > > > > Fixes: ae0078fcf0a5 ("hv_sock: implements Hyper-V transport for Virtual Sockets (AF_VSOCK)") > > Cc: stable@vger.kernel.org > > I don't see the v1 on netdev@, nor a link to it in the change log > so I may be missing the context, but the commit message is a bit > sparse. > > The stable and Fixes tags indicate this is a fix. But the commit > message reads like currently no such crash is observed, quote: > > which could lead to a dangling pointer. > ^^^^^ > ? > > Could someone clarify? I think it's just an accent, in certain languages/cultures expressing uncertainty is considered polite. Should be "can".
On Thu, 7 Nov 2024 16:41:02 -0500 Michael S. Tsirkin wrote: > On Thu, Nov 07, 2024 at 11:29:42AM -0800, Jakub Kicinski wrote: > > On Wed, 6 Nov 2024 04:36:04 -0500 Hyunwoo Kim wrote: > > > When hvs is released, there is a possibility that vsk->trans may not > > > be initialized to NULL, which could lead to a dangling pointer. > > > This issue is resolved by initializing vsk->trans to NULL. > > > > > > Fixes: ae0078fcf0a5 ("hv_sock: implements Hyper-V transport for Virtual Sockets (AF_VSOCK)") > > > Cc: stable@vger.kernel.org > > > > I don't see the v1 on netdev@, nor a link to it in the change log > > so I may be missing the context, but the commit message is a bit > > sparse. > > > > The stable and Fixes tags indicate this is a fix. But the commit > > message reads like currently no such crash is observed, quote: > > > > which could lead to a dangling pointer. > > ^^^^^ > > ? > > > > Could someone clarify? > > I think it's just an accent, in certain languages/cultures expressing > uncertainty is considered polite. Should be "can". You're probably right, the issue perhaps isn't the phrasing as much as the lack of pointing out the code path in which the dangling pointer would be deferenced. Hyunwoo Kim, can you provide one?
Dear, On Thu, Nov 07, 2024 at 01:52:33PM -0800, Jakub Kicinski wrote: > On Thu, 7 Nov 2024 16:41:02 -0500 Michael S. Tsirkin wrote: > > On Thu, Nov 07, 2024 at 11:29:42AM -0800, Jakub Kicinski wrote: > > > On Wed, 6 Nov 2024 04:36:04 -0500 Hyunwoo Kim wrote: > > > > When hvs is released, there is a possibility that vsk->trans may not > > > > be initialized to NULL, which could lead to a dangling pointer. > > > > This issue is resolved by initializing vsk->trans to NULL. > > > > > > > > Fixes: ae0078fcf0a5 ("hv_sock: implements Hyper-V transport for Virtual Sockets (AF_VSOCK)") > > > > Cc: stable@vger.kernel.org > > > > > > I don't see the v1 on netdev@, nor a link to it in the change log > > > so I may be missing the context, but the commit message is a bit > > > sparse. > > > > > > The stable and Fixes tags indicate this is a fix. But the commit > > > message reads like currently no such crash is observed, quote: > > > > > > which could lead to a dangling pointer. > > > ^^^^^ > > > ? > > > > > > Could someone clarify? > > > > I think it's just an accent, in certain languages/cultures expressing > > uncertainty is considered polite. Should be "can". > > You're probably right, the issue perhaps isn't the phrasing as much > as the lack of pointing out the code path in which the dangling pointer > would be deferenced. Hyunwoo Kim, can you provide one? This is a potential issue. Initially, I reported a patch for a dangling pointer in virtio_transport_destruct() within virtio_transport_common.c to the security team. The vulnerability in virtio_transport_destruct() was actually exploited for root privilege escalation, and its exploitability was confirmed (Google kernelCTF). Afterward, the maintainers recommended patching the hvs_destruct() function, which has a similar form to virtio_transport_destruct(), so I created and submitted this patch. Unlike virtio_transport_destruct(), this has not been actually triggered, so there is no call stack available. However, I still believe it’s good to patch it since it is a potential issue. Additionally, the v1 patch only exists in the security mailing list, which is why it might not be visible. Best Regards, Hyunwoo Kim
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Wed, 6 Nov 2024 04:36:04 -0500 you wrote: > When hvs is released, there is a possibility that vsk->trans may not > be initialized to NULL, which could lead to a dangling pointer. > This issue is resolved by initializing vsk->trans to NULL. > > Fixes: ae0078fcf0a5 ("hv_sock: implements Hyper-V transport for Virtual Sockets (AF_VSOCK)") > Cc: stable@vger.kernel.org > Signed-off-by: Hyunwoo Kim <v4bel@theori.io> > > [...] Here is the summary with links: - [v2] hv_sock: Initializing vsk->trans to NULL to prevent a dangling pointer https://git.kernel.org/netdev/net-next/c/e629295bd60a You are awesome, thank you!
diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c index e2157e387217..56c232cf5b0f 100644 --- a/net/vmw_vsock/hyperv_transport.c +++ b/net/vmw_vsock/hyperv_transport.c @@ -549,6 +549,7 @@ static void hvs_destruct(struct vsock_sock *vsk) vmbus_hvsock_device_unregister(chan); kfree(hvs); + vsk->trans = NULL; } static int hvs_dgram_bind(struct vsock_sock *vsk, struct sockaddr_vm *addr)
When hvs is released, there is a possibility that vsk->trans may not be initialized to NULL, which could lead to a dangling pointer. This issue is resolved by initializing vsk->trans to NULL. Fixes: ae0078fcf0a5 ("hv_sock: implements Hyper-V transport for Virtual Sockets (AF_VSOCK)") Cc: stable@vger.kernel.org Signed-off-by: Hyunwoo Kim <v4bel@theori.io> --- v1 -> v2: Add fixes and cc tags --- net/vmw_vsock/hyperv_transport.c | 1 + 1 file changed, 1 insertion(+)