| 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
| ||
|
Date: Wed, 28 Oct 2015 14:46:50 -0700
From: Alexander Duyck <alexander.duyck@...il.com>
To: Bjorn Helgaas <helgaas@...nel.org>,
Alexander Duyck <aduyck@...antis.com>
Cc: bhelgaas@...gle.com, linux-pci@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/5] iov: Update virtfn_max_buses to validate offset and
stride
On 10/28/2015 11:43 AM, Bjorn Helgaas wrote:
> On Wed, Oct 28, 2015 at 11:32:16AM -0500, Bjorn Helgaas wrote:
>> Hi Alex,
>>
>> Thanks a lot for cleaning this up. I think this is a great
>> improvement over what I did.
>>
>> On Tue, Oct 27, 2015 at 01:52:15PM -0700, Alexander Duyck wrote:
>>> This patch pulls the validation of offset and stride into virtfn_max_buses.
>>> The general idea is to validate offset and stride for each possible value
>>> of numvfs in addition to still determining the maximum bus value for the
>>> VFs.
>>>
>>> I also reversed the loop as the most likely maximum will be when numvfs is
>>> set to total_VFs. In addition this makes it so that we loop down to a
>>> value of 0 for numvfs which should be the resting state for the register.
>>>
>>> Fixes: 8e20e89658f2 ("PCI: Set SR-IOV NumVFs to zero after enumeration")
>>> Signed-off-by: Alexander Duyck <aduyck@...antis.com>
>>
>> I'd like to squash this together with my patch instead of having fixes
>> on top of fixes. What do you think of the following? (This applies
>> on top of 70675e0b6a1a ("PCI: Don't try to restore VF BARs")).
>>
>>
>> commit c20e11b572c5d4e4f01c86580a133122fbd13cfa
>> Author: Alexander Duyck <aduyck@...antis.com>
>> Date: Wed Oct 28 10:54:32 2015 -0500
>>
>> PCI: Set SR-IOV NumVFs to zero after enumeration
>>
>> The enumeration path should leave NumVFs set to zero. But after
>> 4449f079722c ("PCI: Calculate maximum number of buses required for VFs"),
>> we call virtfn_max_buses() in the enumeration path, which changes NumVFs.
>> This NumVFs change is visible via lspci and sysfs until a driver enables
>> SR-IOV.
>>
>> Iterate from TotalVFs down to zero so NumVFs is zero when we're finished
>> computing the maximum number of buses. Validate offset and stride in
>> the loop, so we can test it at every possible NumVFs setting. Rename
>> virtfn_max_buses() to compute_max_vf_buses() to hint that it does have a
>> side effect of updating iov->max_VF_buses.
>>
>> [bhelgaas: changelog, rename, reverse sense of error path]
>> Fixes: 4449f079722c ("PCI: Calculate maximum number of buses required for VFs")
>> Based-on-patch-by: Ethan Zhao <ethan.zhao@...cle.com>
>> Signed-off-by: Alexander Duyck <aduyck@...antis.com>
>> Signed-off-by: Bjorn Helgaas <bhelgaas@...gle.com>
>>
>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>> index ee0ebff..120cfb3 100644
>> --- a/drivers/pci/iov.c
>> +++ b/drivers/pci/iov.c
>> @@ -54,24 +54,33 @@ static inline void pci_iov_set_numvfs(struct pci_dev *dev, int nr_virtfn)
>> * The PF consumes one bus number. NumVFs, First VF Offset, and VF Stride
>> * determine how many additional bus numbers will be consumed by VFs.
>> *
>> - * Iterate over all valid NumVFs and calculate the maximum number of bus
>> - * numbers that could ever be required.
>> + * Iterate over all valid NumVFs, validate offset and stride, and calculate
>> + * the maximum number of bus numbers that could ever be required.
>> */
>> -static inline u8 virtfn_max_buses(struct pci_dev *dev)
>> +static int compute_max_vf_buses(struct pci_dev *dev)
>> {
>> struct pci_sriov *iov = dev->sriov;
>> - int nr_virtfn;
>> - u8 max = 0;
>> + int nr_virtfn = iov->total_VFs;
>> int busnr;
>>
>> - for (nr_virtfn = 1; nr_virtfn <= iov->total_VFs; nr_virtfn++) {
>> - pci_iov_set_numvfs(dev, nr_virtfn);
>> + pci_iov_set_numvfs(dev, nr_virtfn);
>> +
>> + while (nr_virtfn--) {
>> + if (!iov->offset || !iov->stride)
>> + goto err;
>
> I think we have a minor problem here. In sriov_enable(), we return an
> error if "nr_virtfn > 1 && !iov->stride", so it's legal for stride to
> be zero if NumVF is 1. Here we don't allow that. Sec 3.3.10 says:
>
> Note: VF Stride is unused if NumVFs is 0 or 1. If NumVFs is greater
> than 1, VF Stride must not be zero."
>
> So I think we should allow "stride == 0" here when NumVFs is 1.
Right, we shouldn't be testing it if NumVFs is 1 or less.
>> +
>> busnr = pci_iov_virtfn_bus(dev, nr_virtfn - 1);
>
> I think this loop management is slightly wrong: I don't think we ever
> compute busnr for the highest VF because we always decrement nr_virtfn
> after calling pci_iov_set_numvfs(), and then we subtract one again.
> E.g., if Total VFs is 8, the VFs are numbered VF0..VF7, and we have
> this, which doesn't check VF7:
>
> nr_virtfn = iov->total_VFs # nr_virtfn == 8
> pci_iov_set_numvfs(..., nr_virtfn) # passes 8 (correct)
> while (nr_virtfn--) {
> # nr_virtfn == 7 in loop body
> pci_iov_virtfn_bus(..., nr_virtfn - 1) # passes 6 (wrong)
>
Yeah, that was supposed to just be nr_virtfn.
>> - if (busnr > max)
>> - max = busnr;
>> + if (busnr > iov->max_VF_buses)
>> + iov->max_VF_buses = busnr;
>> +
>> + pci_iov_set_numvfs(dev, nr_virtfn);
>> }
>>
>> - return max;
>> + return 0;
>> +
>> +err:
>> + pci_iov_set_numvfs(dev, 0);
>> + return -EIO;
>> }
>
> Here's my new proposal:
>
> static int compute_max_vf_buses(struct pci_dev *dev)
> {
> struct pci_sriov *iov = dev->sriov;
> int nr_virtfn, busnr, rc = 0;
>
> for (nr_virtfn = iov->total_VFs; nr_virtfn; nr_virtfn--) {
> pci_iov_set_numvfs(dev, nr_virtfn);
> if (!iov->offset || (nr_virtfn > 1 && !iov->stride)) {
> rc = -EIO;
> goto out;
> }
>
> busnr = pci_iov_virtfn_bus(dev, nr_virtfn - 1);
> if (busnr > iov->max_VF_buses)
> iov->max_VF_buses = busnr;
> }
>
> out:
> pci_iov_set_numvfs(dev, 0);
> return rc;
> }
>
This looks good to me. In theory you could save yourself a pair of MMIO
reads at the end of the loop by just writing numvfs without the offset
and stride read, but this should work.
- Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists