linux-kernel - RE: [PATCH 1/2] uio_hv_generic: Fix kernel NULL pointer dereference in hv_uio

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:
 <SN6PR02MB4157FDCAE52019E13DB97229D48A2@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Sun, 25 Aug 2024 02:51:40 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Naman Jain <namjain@...ux.microsoft.com>, "K . Y . Srinivasan"
	<kys@...rosoft.com>, Haiyang Zhang <haiyangz@...rosoft.com>, Wei Liu
	<wei.liu@...nel.org>, Dexuan Cui <decui@...rosoft.com>, Greg Kroah-Hartman
	<gregkh@...uxfoundation.org>, Stephen Hemminger <stephen@...workplumber.org>
CC: "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Saurabh Sengar
	<ssengar@...ux.microsoft.com>
Subject: RE: [PATCH 1/2] uio_hv_generic: Fix kernel NULL pointer dereference
 in hv_uio_rescind

From: Naman Jain <namjain@...ux.microsoft.com> Sent: Thursday, August 22, 2024 4:09 AM
> 
> From: Saurabh Sengar <ssengar@...ux.microsoft.com>
> 
> For primary VMBus channels primary_channel pointer is always NULL. This
> pointer is valid only for the secondry channels.
> 
> Fix NULL pointer dereference by retrieving the device_obj from the parent
> in the absence of a valid primary_channel pointer.
> 
> Fixes: ca3cda6fcf1e ("uio_hv_generic: add rescind support")
> Signed-off-by: Saurabh Sengar <ssengar@...ux.microsoft.com>
> Signed-off-by: Naman Jain <namjain@...ux.microsoft.com>
> ---
>  drivers/uio/uio_hv_generic.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
> index b45653752301..c99890c16d29 100644
> --- a/drivers/uio/uio_hv_generic.c
> +++ b/drivers/uio/uio_hv_generic.c
> @@ -109,7 +109,8 @@ static void hv_uio_channel_cb(void *context)
>   */
>  static void hv_uio_rescind(struct vmbus_channel *channel)
>  {
> -	struct hv_device *hv_dev = channel->primary_channel->device_obj;
> +	struct hv_device *hv_dev = channel->primary_channel ?
> +				   channel->primary_channel->device_obj : channel->device_obj;

It looks to me like hv_uio_rescind() is called only for the primary
channel. That makes sense, because waking up the reader should
presumably be done once for the device, not once for each channel.

Rather than generalizing the function so it works for both primary
and secondary channels, I'd suggest checking if the channel is a
secondary channel. If so, output a warning message or do WARN(),
and then return immediately, as some there's some kind of
programming error.

Looking at the history of the code, it appears that rescinding a UIO
device could never have worked. Is that your conclusion as well,
or am I missing something?

Michael