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] [thread-next>] [day] [month] [year] [list]

Message-ID: <DM6PR11MB3819B7E06DB9A4BC7FBDABD285E10@DM6PR11MB3819.namprd11.prod.outlook.com>
Date:   Wed, 18 Nov 2020 01:54:31 +0000
From:   "Wu, Hao" <hao.wu@...el.com>
To:     "matthew.gerlach@...ux.intel.com" <matthew.gerlach@...ux.intel.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 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?

[...]

> >> +	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)
...

> 
> >
> >> +				__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?

Thanks
Hao