linux-kernel - Re: [PATCH] PCI: kirin: fix memory leak in kirin_pcie_parse

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: <d061d545-4694-4d5b-86fa-03d1f7251b45@gmail.com>
Date: Fri, 5 Jul 2024 13:29:10 +0200
From: Javier Carrasco <javier.carrasco.cruz@...il.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: Xiaowei Song <songxiaowei@...ilicon.com>,
 Binghui Wang <wangbinghui@...ilicon.com>,
 Lorenzo Pieralisi <lpieralisi@...nel.org>,
 Krzysztof Wilczyński <kw@...ux.com>,
 Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
 Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
 Linus Walleij <linus.walleij@...aro.org>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Krzysztof Wilczyński <kwilczynski@...nel.org>,
 linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] PCI: kirin: fix memory leak in kirin_pcie_parse_port()

On 05/07/2024 12:18, Jonathan Cameron wrote:
> On Sun, 09 Jun 2024 12:56:14 +0200
> Javier Carrasco <javier.carrasco.cruz@...il.com> wrote:
> 
>> The conversion of this file to use the agnostic GPIO API has introduced
>> a new early return where the refcounts of two device nodes (parent and
>> child) are not decremented.
>>
>> Given that the device nodes are not required outside the loops where
>> they are used, and to avoid potential bugs every time a new error path
>> is introduced to the loop, the _scoped() versions of the macros have
>> been applied. The bug was introduced recently, and the fix is not
>> relevant for old stable kernels that might not support the scoped()
>> variant.
>>
>> Fixes: 1d38f9d89f85 ("PCI: kirin: Convert to use agnostic GPIO API")
>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@...il.com>
> Diff on this on is irritating as it doesn't actually show the
> buggy code...  Ah well.
> 
> Change is valid, but one suggestion inline.
> 
> Looks like it's queued now already, but if not.
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> 
> 
>> ---
>> This bug was found while analyzing the code and I don't have hardware to
>> validate it beyond compilation and static analysis. Any test with real
>> hardware to make sure there are no regressions is always welcome.
>>
>> The dev_err() messages have not been converted into dev_err_probe() to
>> keep the current format, but I am open to convert them if preferred.
>> ---
>>  drivers/pci/controller/dwc/pcie-kirin.c | 21 ++++++---------------
>>  1 file changed, 6 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
>> index d1f54f188e71..0a29136491b8 100644
>> --- a/drivers/pci/controller/dwc/pcie-kirin.c
>> +++ b/drivers/pci/controller/dwc/pcie-kirin.c
>> @@ -403,11 +403,10 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie,
>>  				 struct device_node *node)
>>  {
>>  	struct device *dev = &pdev->dev;
>> -	struct device_node *parent, *child;
>>  	int ret, slot, i;
>>  
>> -	for_each_available_child_of_node(node, parent) {
>> -		for_each_available_child_of_node(parent, child) {
>> +	for_each_available_child_of_node_scoped(node, parent) {
>> +		for_each_available_child_of_node_scoped(parent, child) {
>>  			i = pcie->num_slots;
>>  
>>  			pcie->id_reset_gpio[i] = devm_fwnode_gpiod_get_index(dev,
>> @@ -424,14 +423,13 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie,
>>  			pcie->num_slots++;
>>  			if (pcie->num_slots > MAX_PCI_SLOTS) {
>>  				dev_err(dev, "Too many PCI slots!\n");
>> -				ret = -EINVAL;
>> -				goto put_node;
>> +				return -EINVAL;
> Perhaps a future change, but this would be nicer as
> 				return dev_err_probe(dev, -EINVAL,
> 						     "Too many PCI slots!\n");
> Maybe as part of a general change to this driver to use
> dev_err_probe() for all the error prints in paths only called
> from probe().
> 

Yeah, it seems that other paths that have nothing to do with this fix
would require the same modification.

Best regards,
Javier Carrasco