Re: [PATCH 1/2] uio_hv_generic: Fix kernel NULL pointer dereference in hv_uio_rescind

[Naman Jain <namjain@linux.microsoft.com>]

On 8/25/2024 8:21 AM, Michael Kelley wrote:
> From: Naman Jain <namjain@linux.microsoft.com> Sent: Thursday, August 22, 2024 4:09 AM
>>
>> From: Saurabh Sengar <ssengar@linux.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@linux.microsoft.com>
>> Signed-off-by: Naman Jain <namjain@linux.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