linux-kernel - Re: [PATCH v2 04/18] nitro_enclaves: Init PCI device driver

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: <68b86d32-1255-f9ce-4366-12219ce07ba6@amazon.de>
Date:   Sat, 23 May 2020 22:25:25 +0200
From:   Alexander Graf <graf@...zon.de>
To:     Greg KH <greg@...ah.com>, Andra Paraschiv <andraprs@...zon.com>
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>,
        "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 v2 04/18] nitro_enclaves: Init PCI device driver

Hey Greg,

On 22.05.20 09:04, Greg KH wrote:
> 
> On Fri, May 22, 2020 at 09:29:32AM +0300, Andra Paraschiv wrote:
>> +/**
>> + * ne_setup_msix - Setup MSI-X vectors for the PCI device.
>> + *
>> + * @pdev: PCI device to setup the MSI-X for.
>> + *
>> + * @returns: 0 on success, negative return value on failure.
>> + */
>> +static int ne_setup_msix(struct pci_dev *pdev)
>> +{
>> +     struct ne_pci_dev *ne_pci_dev = NULL;
>> +     int nr_vecs = 0;
>> +     int rc = -EINVAL;
>> +
>> +     if (WARN_ON(!pdev))
>> +             return -EINVAL;
> 
> How can this ever happen?  If it can not, don't test for it.  If it can,
> don't warn for it as that will crash systems that do panic-on-warn, just
> test and return an error.

I think the point here is to catch situations that should never happen, 
but keep a sanity check in in case they do happen. This would've usually 
been a BUG_ON, but people tend to dislike those these days because they 
can bring down your system ...

So in this particular case here I agree that it's a bit silly to check 
whether pdev is != NULL. In other device code internal APIs though it's 
not quite as clear of a cut. I by far prefer code that tells me it's 
broken over reverse engineering stray pointer accesses ...

> 
>> +
>> +     ne_pci_dev = pci_get_drvdata(pdev);
>> +     if (WARN_ON(!ne_pci_dev))
>> +             return -EINVAL;
> 
> Same here, don't use WARN_ON if at all possible.
> 
>> +
>> +     nr_vecs = pci_msix_vec_count(pdev);
>> +     if (nr_vecs < 0) {
>> +             rc = nr_vecs;
>> +
>> +             dev_err_ratelimited(&pdev->dev,
>> +                                 NE "Error in getting vec count [rc=%d]\n",
>> +                                 rc);
>> +
> 
> Why ratelimited, can this happen over and over and over?

In this particular function, no, so here it really should just be 
dev_err. Other functions are implicitly callable from user space through 
an ioctl, which means they really need to stay rate limited.

Thanks a lot for looking through the code and pointing all those bits out :)


Alex



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879