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

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]

Message-ID: <Zy3Us4AV9DsgWAQO@v4bel-B760M-AORUS-ELITE-AX>
Date: Fri, 8 Nov 2024 04:06:59 -0500
From: Hyunwoo Kim <v4bel@...ori.io>
To: Jakub Kicinski <kuba@...nel.org>
Cc: "Michael S. Tsirkin" <mst@...hat.com>,
	"K. Y. Srinivasan" <kys@...rosoft.com>,
	Haiyang Zhang <haiyangz@...rosoft.com>,
	Wei Liu <wei.liu@...nel.org>, Dexuan Cui <decui@...rosoft.com>,
	Stefano Garzarella <sgarzare@...hat.com>, jasowang@...hat.com,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Simon Horman <horms@...nel.org>, linux-hyperv@...r.kernel.org,
	virtualization@...ts.linux.dev, netdev@...r.kernel.org,
	gregkh@...uxfoundation.org, imv4bel@...il.com, v4bel@...ori.io
Subject: Re: [PATCH v2] hv_sock: Initializing vsk->trans to NULL to prevent a
 dangling pointer

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@...r.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