linux-kernel - Re: [PATCH] Revert "PCI: Fix reference leak in pci_register_host

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: <c6fb3f34-4296-4b86-a08c-92b16cd8e1d0@nfschina.com>
Date: Mon, 14 Apr 2025 09:48:28 +0800
From: Su Hui <suhui@...china.com>
To: "liwei (JK)" <liwei728@...wei.com>
Cc: linux-kernel@...r.kernel.org, make24@...as.ac.cn, bhelgaas@...gle.com,
 linux-pci@...r.kernel.org, bobo.shaobowang@...wei.com,
 wangxiongfeng2@...wei.com
Subject: Re: [PATCH] Revert "PCI: Fix reference leak in
 pci_register_host_bridge()"

On 2025/4/12 11:18, liwei (JK) wrote:
> 在 2025/4/10 19:19, Su Hui 写道:
>> On 2025/4/10 11:28, Xiangwei Li wrote:
>>> This reverts commit 804443c1f27883926de94c849d91f5b7d7d696e9.
>>>
>>> The newly added logic incorrectly sets bus_registered to true even when
>>> device_register returns an error, this is incorrect.
>>>
>>> When device_register fails, there is no need to release the 
>>> reference count,
>> I think you missed some thing about device_register(). This patch is 
>> wrong.
>>
>> device_register()
>>      -> device_initialize()
>>                -> kobject_init()
>>                      -> kobject_init_internal()
>>                          -> kref_init(&kobj->kref); //set 
>> kref->refcount to 1
>>                             ^^^^^^^^^^^^^^^^^^^^^
>>
> Sorry, I missed the initialization of refcount in device_initialize,
> but I’m confused about the branch logic for bus_registered. Why isn’t
> free(bus) executed when bus_registered == true? My understanding is
> that the kobject_cleanup operation triggered when refcount reaches zero
> does not clean up the allocated bus. Could you clarify this further?
>
1020         dev_set_name(&bus->dev, "%04x:%02x", pci_domain_nr(bus), 
bus->number);
                   ^^^^^^^^^^^^^^^^^^^^
                 //device name is allocated, and should be freed when 
device_register() is failed.

1021         name = dev_name(&bus->dev);
1022
1023         err = device_register(&bus->dev);
1024         bus_registered = true;
1025         if (err)
1026                 goto unregister;
                         [...]
1117         if (bus_registered)
1118                 put_device(&bus->dev);
                            ^^^^^^^^^^^^^^^^
                            // decrement reference count  to zero and 
call release_pcibus_dev() to free bus.
                            // And call kfree_const() to free device 
name in kobject_cleanup().

1119         else
1120                 kfree(bus);

Commit 804443c1f278 fixes the memory leak of 'name' and consistent with 
the annotation of device_degister():

  '* NOTE: _Never_ directly free @dev after calling this function, even
  * if it returned an error! Always use put_device() to give up the
  * reference initialized in this function instead.'

Su Hui