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: <0271e241-3bbb-4d88-8da5-d3d3b8b54e38@linux.microsoft.com>
Date: Mon, 26 Aug 2024 11:01:41 +0530
From: Naman Jain <namjain@...ux.microsoft.com>
To: Michael Kelley <mhklinux@...look.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



On 8/25/2024 8:21 AM, Michael Kelley wrote:
> 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.

Thanks for reviewing Michael. By design, there is different handling for 
secondary channel rescind in channel_mgmt.c [1] and this callback is not 
expected to be called. I will make the change in my next patch to make 
it a warning.


[1]
         } else if (channel->primary_channel != NULL) {
                 /*
                  * Sub-channel is being rescinded. Following is the channel
                  * close sequence when initiated from the driveri (refer to
                  * vmbus_close() for details):
                  * 1. Close all sub-channels first
                  * 2. Then close the primary channel.
                  */

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

Most likely, yes.


Regards,
Naman