[PATCH v2] hv_sock: Initializing vsk->trans to NULL to prevent a dangling pointer

[PATCH v2] hv_sock: Initializing vsk->trans to NULL to prevent a dangling pointer

[Hyunwoo Kim]

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(+)

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

Re: [PATCH v2] hv_sock: Initializing vsk->trans to NULL to prevent a dangling pointer

[Stefano Garzarella]

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
>

Re: [PATCH v2] hv_sock: Initializing vsk->trans to NULL to prevent a dangling pointer

[Michael S. Tsirkin]

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

Re: [PATCH v2] hv_sock: Initializing vsk->trans to NULL to prevent a dangling pointer

[Jakub Kicinski]

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?

Re: [PATCH v2] hv_sock: Initializing vsk->trans to NULL to prevent a dangling pointer

[Michael S. Tsirkin]

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".

-- 
MST

Re: [PATCH v2] hv_sock: Initializing vsk->trans to NULL to prevent a dangling pointer

[Jakub Kicinski]

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?

Re: [PATCH v2] hv_sock: Initializing vsk->trans to NULL to prevent a dangling pointer

[Hyunwoo Kim]

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

Re: [PATCH v2] hv_sock: Initializing vsk->trans to NULL to prevent a dangling pointer

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html