netdev - Re: [PATCH net v2 2/5] ice: fix possible leak in ice_plug_aux

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] [day] [month] [year] [list]

Message-ID: <c180694a-7841-4f72-9810-f5e6c959c33e@intel.com>
Date: Thu, 21 Aug 2025 10:46:34 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Jakub Kicinski <kuba@...nel.org>, Tony Nguyen
	<anthony.l.nguyen@...el.com>, Jerome Brunet <jbrunet@...libre.com>, "Emil
 Tantilov" <emil.s.tantilov@...el.com>
CC: <davem@...emloft.net>, <pabeni@...hat.com>, <edumazet@...gle.com>,
	<andrew+netdev@...n.ch>, <netdev@...r.kernel.org>,
	<david.m.ertman@...el.com>, <tatyana.e.nikolova@...el.com>, "Aleksandr
 Loktionov" <aleksandr.loktionov@...el.com>
Subject: Re: [PATCH net v2 2/5] ice: fix possible leak in ice_plug_aux_dev()
 error path

On 8/21/25 03:45, Jakub Kicinski wrote:
> On Tue, 19 Aug 2025 15:19:56 -0700 Tony Nguyen wrote:
>>   	ret = auxiliary_device_init(adev);
>> -	if (ret) {
>> -		kfree(iadev);
>> -		return ret;
>> -	}
>> +	if (ret)
>> +		goto free_iadev;
>>   
>>   	ret = auxiliary_device_add(adev);
>> -	if (ret) {
>> -		auxiliary_device_uninit(adev);
>> -		return ret;
> 
> I think the code is correct as is. Once auxiliary_device_init()
> returns the device is refcounted, auxiliary_device_uninit()
> will call release, which is ice_adev_release(), which in turn
> frees iadev.

you are right

It's nice, that a recent wrapper [1] added notes that exact bit of
wisdom as comment (what only proves such wrapper is a great abstraction,
thanks @Jerome Brunet):

drivers/base/auxiliary.c:
444│         ret = __auxiliary_device_add(auxdev, modname);
445│         if (ret) {
446│                 /*
447│                  * It may look odd but auxdev should not be freed here.
448│                  * auxiliary_device_uninit() calls device_put() 
which call
449│                  * the device release function, freeing auxdev.
450│                  */
451│                 auxiliary_device_uninit(auxdev);
452│                 return NULL;
453│         }

[1] eaa0d30216c1 ("driver core: auxiliary bus: add device creation helpers")

> 
>> -	}
>> +	if (ret)
>> +		goto aux_dev_uninit;
>>   
>>   	mutex_lock(&pf->adev_mutex);
>>   	cdev->adev = adev;
>> @@ -339,6 +335,13 @@ int ice_plug_aux_dev(struct ice_pf *pf)
>>   	set_bit(ICE_FLAG_AUX_DEV_CREATED, pf->flags);
>>   
>>   	return 0;
>> +
>> +aux_dev_uninit:
>> +	auxiliary_device_uninit(adev);
>> +free_iadev:
>> +	kfree(iadev);
>> +
>> +	return ret;