linux-kernel - RE: [PATCH 2/2] fpga: dfl: look for vendor specific capability

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: <alpine.DEB.2.22.394.2011180948060.353557@rhweight-WRK1>
Date:   Wed, 18 Nov 2020 09:49:55 -0800 (PST)
From:   matthew.gerlach@...ux.intel.com
To:     "Wu, Hao" <hao.wu@...el.com>
cc:     "linux-fpga@...r.kernel.org" <linux-fpga@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "mdf@...nel.org" <mdf@...nel.org>,
        "trix@...hat.com" <trix@...hat.com>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "corbet@....net" <corbet@....net>
Subject: RE: [PATCH 2/2] fpga: dfl: look for vendor specific capability



On Wed, 18 Nov 2020, Wu, Hao wrote:

>> On Tue, 17 Nov 2020, Wu, Hao wrote:
>
> [...]
>
>>>>  Open discussion
>>>>  ===============
>>>> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
>>>> index b1b157b41942..5418e8bf2496 100644
>>>> --- a/drivers/fpga/dfl-pci.c
>>>> +++ b/drivers/fpga/dfl-pci.c
>>>> @@ -27,6 +27,13 @@
>>>>  #define DRV_VERSION	"0.8"
>>>>  #define DRV_NAME	"dfl-pci"
>>>>
>>>> +#define PCI_VNDR_ID_DFLS 0x43
>>>
>>> What about PCI_VSEC_ID_INTEL_DFLS?
>>>
>>> Is it possible a different ID chosen by different vendor?
>>
>> I think another vendor could choose their own ID.
>
> If another vendor could choose their own ID, so should we
> check vendor id as well?

Yes, the vendor id should be checked.  I will add it to v2.
>
> [...]
>
>>>> +	for (i = 0; i < dfl_cnt; i++) {
>>>> +		dfl_res_off = voff + PCI_VNDR_DFLS_RES_OFFSET +
>>>> +				      (i * sizeof(dfl_res));
>>>> +		pci_read_config_dword(pcidev, dfl_res_off, &dfl_res);
>>>> +
>>>> +		dev_dbg(&pcidev->dev, "dfl_res 0x%x\n", dfl_res);
>>>> +
>>>> +		bar = dfl_res & PCI_VND_DFLS_RES_BAR_MASK;
>>>> +
>>>> +		if (bar >= PCI_STD_NUM_BARS) {
>>>> +			dev_err(&pcidev->dev, "%s bad bar number %d\n",
>>>> +				__func__, bar);
>>>> +			return -EINVAL;
>>>> +		}
>>>> +
>>>> +		len = pci_resource_len(pcidev, bar);
>>>> +
>>>
>>> Remove this blank line.
>> OK, v2.
>>
>>>
>>>> +		if (len == 0) {
>>>> +			dev_err(&pcidev->dev, "%s unmapped bar
>>>> number %d\n",
>>>
>>> Why "unmapped bar"?
>>
>> How about, "zero length bar"?
>
> I think this checking can be covered by below one, right?
> if (offset >= len)
> ...

I agree. I will make the change in v2.
>
>>
>>>
>>>> +				__func__, bar);
>>>> +			return -EINVAL;
>>>> +		}
>>>> +
>>>> +		offset = dfl_res & ~PCI_VND_DFLS_RES_BAR_MASK;
>>>> +
>>>
>>> Remove this blank line.
>>
>> OK, v2.
>>
>>>
>>>> +		if (offset >= len) {
>>>> +			dev_err(&pcidev->dev, "%s bad offset %u >= %llu\n",
>>>> +				__func__, offset, len);
>>>> +			return -EINVAL;
>>>> +		}
>>>> +
>
> [....]
>
>>>> +
>>>> +		start = pci_resource_start(pcidev, bar) + offset;
>>>> +		len -= offset;
>>>> +
>>>> +		if (!PAGE_ALIGNED(start)) {
>>>
>>> Is this a hard requirement? Or offset should be page aligned per VSEC
>> definition?
>>> Or this is just the requirement from driver point of view. Actually we don't
>> like
>>> to add rules only in driver, so it's better we have this requirement in VSEC
>> definition
>>> with proper documentation.
>>
>> The DFL parsing code ioremaps the memory bounded by start/len.  I thought
>> this would require the start to be page aligned.
>
> If consider mmap the region to userspace, it requires page aligned, but do we
> need to apply this rule for everyone?

I will remove this check in v2.

>
> Thanks
> Hao
>
>