linux-kernel - Re: [PATCH] PCI: Fix a device reference count leakage issue in pci_dev

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: <CAErSpo7kZAeAzr4UxDGvviQtwou5t8RduWe-SmzOPvO2UJpgeg@mail.gmail.com>
Date:	Wed, 25 Apr 2012 11:01:07 -0600
From:	Bjorn Helgaas <bhelgaas@...gle.com>
To:	Jiang Liu <liuj97@...il.com>
Cc:	Yinghai Lu <yinghai@...nel.org>, Jiang Liu <jiang.liu@...wei.com>,
	Keping Chen <chenkeping@...wei.com>, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] PCI: Fix a device reference count leakage issue in pci_dev_present()

On Sat, Apr 7, 2012 at 8:51 AM, Jiang Liu <liuj97@...il.com> wrote:
> On 04/07/2012 07:23 AM, Bjorn Helgaas wrote:
>> On Fri, Apr 6, 2012 at 9:31 AM, Jiang Liu <liuj97@...il.com> wrote:
>>> Function pci_get_dev_by_id() will hold a reference count on the pci device
>>> returned, so pci_dev_present() should release the corresponding reference
>>> count to avoid memory leakage.
>>>
>>> Signed-off-by: Jiang Liu <jiang.liu@...wei.com>
>>> ---
>>>  drivers/pci/search.c |   10 +++++-----
>>>  1 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/pci/search.c b/drivers/pci/search.c
>>> index 9d75dc8..b572730 100644
>>> --- a/drivers/pci/search.c
>>> +++ b/drivers/pci/search.c
>>> @@ -338,13 +338,13 @@ int pci_dev_present(const struct pci_device_id *ids)
>>>        WARN_ON(in_interrupt());
>>>        while (ids->vendor || ids->subvendor || ids->class_mask) {
>>>                found = pci_get_dev_by_id(ids, NULL);
>>> -               if (found)
>>> -                       goto exit;
>>> +               if (found) {
>>> +                       pci_dev_put(found);
>>> +                       return 1;
>>> +               }
>>>                ids++;
>>>        }
>>> -exit:
>>> -       if (found)
>>> -               return 1;
>>> +
>>>        return 0;
>>>  }
>>>  EXPORT_SYMBOL(pci_dev_present);
>>
>> This might be the right thing to do, but I'd like to understand what's
>> going on, so let's talk about it a bit first.
>>
>> I agree, there appears to be a leak here.  Or at least, the fact that
>> we keep a reference when a device is found doesn't match the comment.
>> What problem are you seeing from this leak?
>>
>> There are not many callers, and most appear to be one-time things done
>> at boot, looking for built-in devices known to be defective.  These
>> devices won't be removable, so the leak shouldn't be causing
>> hot-remove issues.
> I noticed this issue when reading code, no real issue disclosed yet.
> As you have pointed out, this interface is used for built-in devices only,
> there should be no real issue currently and the patch is just for purity.
>
>>
>> IMO, this is a bogus interface that leads to poor code, and I don't
>> want to encourage its use.  For device defect workarounds, I think
>> it'd be better to use PCI quirks to catch the defective device.  Some
>> chipset defects affect all downstream devices, and a quirk could make
>> the defect visible to all the drivers, not just the ones that use
>> pci_dev_present().  For example, look at tg3_write_reorder_chipsets
>> and tg3_dma_wait_state_chipsets.  Those aren't for tg3 bugs, they're
>> for chipset bugs that might affect other devices, too.  But right now,
>> that knowledge is buried in the tg3 driver.
> Thanks for pointing out the real issue behind this bogus interface.
> It would be better to solve this type of chip bugs in PCI core instead
> of in individual drivers.

I applied this to the -next branch, thanks.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/