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]

Date:   Mon, 25 May 2020 14:15:02 +0300
From:   "Paraschiv, Andra-Irina" <andraprs@...zon.com>
To:     Greg KH <greg@...ah.com>, Alexander Graf <graf@...zon.de>
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



On 24/05/2020 09:32, Greg KH wrote:
> On Sat, May 23, 2020 at 10:25:25PM +0200, Alexander Graf wrote:
>> 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 ...
> Same for WARN_ON when you run with panic-on-warn enabled :(
>
>> 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 ...
> For static calls where you control the callers, don't do checks like
> this.  Otherwise the kernel would just be full of these all over the
> place and things would slow down.  It's just not needed.
>
>>>> +     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.
> Think through these as the driver seems to ONLY use these ratelimited
> calls right now, which is not correct.
>
> Also, if a user can create a printk, that almost always is not a good
> idea.  But yes, those should be ratelimited.

I updated the static calls checks and removed the WARN_ONs. And 
ratelimited is used now only in the ioctl call paths.

Thank you both.

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.