linux-kernel - Re: [PATCH v4 07/18] nitro_enclaves: Init misc device providing the ioctl interface

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: <b87e2eeb-39cf-8de5-0f5f-1db04b3e2794@amazon.com>
Date:   Mon, 29 Jun 2020 20:45:25 +0300
From:   "Paraschiv, Andra-Irina" <andraprs@...zon.com>
To:     Greg KH <gregkh@...uxfoundation.org>
CC:     <linux-kernel@...r.kernel.org>,
        Anthony Liguori <aliguori@...zon.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Colm MacCarthaigh <colmmacc@...zon.com>,
        Bjoern Doebel <doebel@...zon.de>,
        David Woodhouse <dwmw@...zon.co.uk>,
        Frank van der Linden <fllinden@...zon.com>,
        "Alexander Graf" <graf@...zon.de>,
        Martin Pohlack <mpohlack@...zon.de>,
        Matt Wilson <msw@...zon.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Balbir Singh <sblbir@...zon.com>,
        Stefano Garzarella <sgarzare@...hat.com>,
        "Stefan Hajnoczi" <stefanha@...hat.com>,
        Stewart Smith <trawets@...zon.com>,
        "Uwe Dannowski" <uwed@...zon.de>, <kvm@...r.kernel.org>,
        <ne-devel-upstream@...zon.com>
Subject: Re: [PATCH v4 07/18] nitro_enclaves: Init misc device providing the
 ioctl interface



On 29/06/2020 19:20, Greg KH wrote:
>
> On Mon, Jun 22, 2020 at 11:03:18PM +0300, Andra Paraschiv wrote:
>> +static int __init ne_init(void)
>> +{
>> +     struct pci_dev *pdev = pci_get_device(PCI_VENDOR_ID_AMAZON,
>> +                                           PCI_DEVICE_ID_NE, NULL);
>> +     int rc = -EINVAL;
>> +
>> +     if (!pdev)
>> +             return -ENODEV;
> Ick, that's a _very_ old-school way of binding to a pci device.  Please
> just be a "real" pci driver and your probe function will be called if
> your hardware is present (or when it shows up.)  To do it this way
> prevents your driver from being auto-loaded for when your hardware is
> seen in the system, as well as lots of other things.

This check is mainly here in the case any codebase is added before the 
pci driver register call below.

And if we log any error with dev_err() instead of pr_err() before the 
driver register.

That check was only for logging purposes, if done with dev_err(). I 
removed the check in v5.

>> +
>> +     if (!zalloc_cpumask_var(&ne_cpu_pool.avail, GFP_KERNEL))
>> +             return -ENOMEM;
>> +
>> +     mutex_init(&ne_cpu_pool.mutex);
>> +
>> +     rc = pci_register_driver(&ne_pci_driver);
> Nice, you did it right here, but why the above crazy test?
>
>> +     if (rc < 0) {
>> +             dev_err(&pdev->dev,
>> +                     "Error in pci register driver [rc=%d]\n", rc);
>> +
>> +             goto free_cpumask;
>> +     }
>> +
>> +     return 0;
> You leaked a reference on that pci device, didn't you?  Not good :(

Yes, the pci get device call needs its pair - pci_dev_put(). I added it 
here and for the other occurrences where it was missing.

Thanks for review.

Andra



Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.