netdev - Re: [PATCH net] ice: Fix freeing uninitialized pointers

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]

Date: Wed, 20 Mar 2024 16:33:02 +0000
From: Jonathan Cameron <jic23@...23.retrosnub.co.uk>
To: Julia Lawall <julia.lawall@...ia.fr>,
 Dan Carpenter <dan.carpenter@...aro.org>
CC: Jakub Kicinski <kuba@...nel.org>,
 Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
 Jesse Brandeburg <jesse.brandeburg@...el.com>,
 Tony Nguyen <anthony.l.nguyen@...el.com>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Paolo Abeni <pabeni@...hat.com>,
 Przemek Kitszel <przemyslaw.kitszel@...el.com>,
 intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
 Jonathan Cameron <jic23@...nel.org>
Subject: Re: [PATCH net] ice: Fix freeing uninitialized pointers

On 20 March 2024 07:32:17 GMT, Julia Lawall <julia.lawall@...ia.fr> wrote:
>
>
>On Wed, 20 Mar 2024, Dan Carpenter wrote:
>
>> On Tue, Mar 19, 2024 at 12:43:17PM -0700, Jakub Kicinski wrote:
>> > On Sat, 16 Mar 2024 12:44:40 +0300 Dan Carpenter wrote:
>> > > -	struct ice_aqc_get_phy_caps_data *pcaps __free(kfree);
>> > > -	void *mac_buf __free(kfree);
>> > > +	struct ice_aqc_get_phy_caps_data *pcaps __free(kfree) = NULL;
>> > > +	void *mac_buf __free(kfree) = NULL;
>> >
>> > This is just trading one kind of bug for another, and the __free()
>> > magic is at a cost of readability.
>> >
>> > I think we should ban the use of __free() in all of networking,
>> > until / unless it cleanly handles the NULL init case.
>>
>> Free handles the NULL init case, it doesn't handle the uninitialized
>> case.  I had previously argued that checkpatch should complain about
>> every __free() pointer if the declaration doesn't have an assignment.
>>
>> The = NULL assignment is unnecessary if the pointer is assigned to
>> something else before the first return, so this might cause "unused
>> assignment" warnings?  I don't know if there are any tools which
>> complain about that in that situation.  I think probably we should just
>> make that an exception and do the checkpatch thing because it's such a
>> simple rule to implement.
>
>My understanding from Jonathan Cameron was that Linus wants a NULL always,
>unless there is an initialization with the declaration.

I don't have thread to hand but Linus strongly preferred moving any declaration using this to
 where it is assigned so that it was obvious that the allocator and freer match.

Not checked if that makes sense here though 
>
>julia