| 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
| ||
|
Message-ID: <1d72614f-25a1-b6f3-ed07-e2cac818b33a@linux.vnet.ibm.com>
Date: Tue, 26 Apr 2016 14:44:12 +0800
From: Yongji Xie <xyjxie@...ux.vnet.ibm.com>
To: Alexey Kardashevskiy <aik@...abs.ru>, kvm@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
linuxppc-dev@...ts.ozlabs.org, iommu@...ts.linux-foundation.org,
linux-doc@...r.kernel.org
Cc: alex.williamson@...hat.com, bhelgaas@...gle.com,
benh@...nel.crashing.org, paulus@...ba.org, mpe@...erman.id.au,
corbet@....net, warrier@...ux.vnet.ibm.com,
zhong@...ux.vnet.ibm.com, nikunj@...ux.vnet.ibm.com,
gwshan@...ux.vnet.ibm.com
Subject: Re: [RFC v6 04/10] PCI: Add support for enforcing all MMIO BARs to be
page aligned
On 2016/4/26 13:41, Alexey Kardashevskiy wrote:
> On 04/18/2016 08:56 PM, Yongji Xie wrote:
>> When vfio passthrough a PCI device of which MMIO BARs are
>> smaller than PAGE_SIZE, guest will not handle the mmio
>> accesses to the BARs which leads to mmio emulations in host.
>>
>> This is because vfio will not allow to passthrough one BAR's
>> mmio page which may be shared with other BARs. Otherwise,
>> there will be a backdoor that guest can use to access BARs
>> of other guest.
>>
>> To solve this issue, this patch modifies resource_alignment
>> to support syntax where multiple devices get the same
>> alignment. So we can use something like
>> "pci=resource_alignment=*:*:*.*:noresize" to enforce the
>> alignment of all MMIO BARs to be at least PAGE_SIZE so that
>> one BAR's mmio page would not be shared with other BARs.
>>
>> And we also define a macro PCIBIOS_MIN_ALIGNMENT to enable this
>> automatically on PPC64 platform which can easily hit this issue
>> because its PAGE_SIZE is 64KB.
>>
>> Signed-off-by: Yongji Xie <xyjxie@...ux.vnet.ibm.com>
>> ---
>> Documentation/kernel-parameters.txt | 2 ++
>> arch/powerpc/include/asm/pci.h | 2 ++
>> drivers/pci/pci.c | 64
>> +++++++++++++++++++++++++++++------
>> 3 files changed, 57 insertions(+), 11 deletions(-)
>>
>> diff --git a/Documentation/kernel-parameters.txt
>> b/Documentation/kernel-parameters.txt
>> index d8b29ab..542be4a 100644
>> --- a/Documentation/kernel-parameters.txt
>> +++ b/Documentation/kernel-parameters.txt
>> @@ -2918,6 +2918,8 @@ bytes respectively. Such letter suffixes can
>> also be entirely omitted.
>> aligned memory resources.
>> If <order of align> is not specified,
>> PAGE_SIZE is used as alignment.
>> + <domain>, <bus>, <slot> and <func> can be set to
>> + "*" which means match all values.
>> PCI-PCI bridge can be specified, if resource
>> windows need to be expanded.
>> noresize: Don't change the resources' sizes when
>> diff --git a/arch/powerpc/include/asm/pci.h
>> b/arch/powerpc/include/asm/pci.h
>> index 6f8065a..78f230f 100644
>> --- a/arch/powerpc/include/asm/pci.h
>> +++ b/arch/powerpc/include/asm/pci.h
>> @@ -30,6 +30,8 @@
>> #define PCIBIOS_MIN_IO 0x1000
>> #define PCIBIOS_MIN_MEM 0x10000000
>>
>> +#define PCIBIOS_MIN_ALIGNMENT PAGE_SIZE
>> +
>> struct pci_dev;
>>
>> /* Values for the `which' argument to sys_pciconfig_iobase
>> syscall. */
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 7564ccc..0381c28 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -4605,7 +4605,12 @@ static resource_size_t
>> pci_specified_resource_alignment(struct pci_dev *dev,
>> int seg, bus, slot, func, align_order, count;
>> resource_size_t align = 0;
>> char *p;
>> + bool invalid = false;
>>
>> +#ifdef PCIBIOS_MIN_ALIGNMENT
>> + align = PCIBIOS_MIN_ALIGNMENT;
>> + *resize = false;
>> +#endif
>> spin_lock(&resource_alignment_lock);
>> p = resource_alignment_param;
>> while (*p) {
>> @@ -4622,16 +4627,49 @@ static resource_size_t
>> pci_specified_resource_alignment(struct pci_dev *dev,
>> } else {
>> align_order = -1;
>> }
>> - if (sscanf(p, "%x:%x:%x.%x%n",
>> - &seg, &bus, &slot, &func, &count) != 4) {
>
>
> I'd replace the above lines with:
>
> char segstr[5] = "*", busstr[3] = "*";
> char slotstr[3] = "*", funstr[2] = "*";
>
> if (sscanf(p, "%4[^:]:%2[^:]:%2[^.].%1s%n",
> &segstr, &busstr, &slotstr, &funcstr, &count) != 4) {
>
>
It seems the current implement of sscanf() in kernel
is not able to support the syntax: "%4[^:]:%2[^:]:%2[^.]".
Thanks,
Yongji
> and add some wrapper like:
>
> static bool glob_match_hex(char const *pat, int val)
> {
> char valstr[5]; /* 5 should be enough for PCI */
> snprintf(valstr, sizeof(valstr) - 1, "%4x", val);
> return glob_match(pat, valstr);
> }
>
> and then use glob_match_hex() (or make a wrapper like above on top of
> fnmatch()), this would enable better mask handling.
>
> If anyone finds this useful (which I am not sure about).
>
>
>
>
>> + if (p[0] == '*' && p[1] == ':') {
>> + seg = -1;
>> + count = 1;
>> + } else if (sscanf(p, "%x%n", &seg, &count) != 1 ||
>> + p[count] != ':') {
>> + invalid = true;
>> + break;
>> + }
>> + p += count + 1;
>> + if (*p == '*') {
>> + bus = -1;
>> + count = 1;
>> + } else if (sscanf(p, "%x%n", &bus, &count) != 1) {
>> + invalid = true;
>> + break;
>> + }
>> + p += count;
>> + if (*p == '.') {
>> + slot = bus;
>> + bus = seg;
>> seg = 0;
>> - if (sscanf(p, "%x:%x.%x%n",
>> - &bus, &slot, &func, &count) != 3) {
>> - /* Invalid format */
>> - printk(KERN_ERR "PCI: Can't parse resource_alignment
>> parameter: %s\n",
>> - p);
>> + p++;
>> + } else if (*p == ':') {
>> + p++;
>> + if (p[0] == '*' && p[1] == '.') {
>> + slot = -1;
>> + count = 1;
>> + } else if (sscanf(p, "%x%n", &slot, &count) != 1 ||
>> + p[count] != '.') {
>> + invalid = true;
>> break;
>> }
>> + p += count + 1;
>> + } else {
>> + invalid = true;
>> + break;
>> + }
>> + if (*p == '*') {
>> + func = -1;
>> + count = 1;
>> + } else if (sscanf(p, "%x%n", &func, &count) != 1) {
>> + invalid = true;
>> + break;
>> }
>> p += count;
>> if (!strncmp(p, ":noresize", 9)) {
>> @@ -4639,10 +4677,10 @@ static resource_size_t
>> pci_specified_resource_alignment(struct pci_dev *dev,
>> p += 9;
>> } else
>> *resize = true;
>> - if (seg == pci_domain_nr(dev->bus) &&
>> - bus == dev->bus->number &&
>> - slot == PCI_SLOT(dev->devfn) &&
>> - func == PCI_FUNC(dev->devfn)) {
>> + if ((seg == pci_domain_nr(dev->bus) || seg == -1) &&
>> + (bus == dev->bus->number || bus == -1) &&
>> + (slot == PCI_SLOT(dev->devfn) || slot == -1) &&
>> + (func == PCI_FUNC(dev->devfn) || func == -1)) {
>> if (align_order == -1)
>> align = PAGE_SIZE;
>> else
>> @@ -4652,10 +4690,14 @@ static resource_size_t
>> pci_specified_resource_alignment(struct pci_dev *dev,
>> }
>> if (*p != ';' && *p != ',') {
>> /* End of param or invalid format */
>> + invalid = true;
>> break;
>> }
>> p++;
>> }
>> + if (invalid)
>> + printk(KERN_ERR "PCI: Can't parse resource_alignment
>> parameter:%s\n",
>> + p);
>> spin_unlock(&resource_alignment_lock);
>> return align;
>> }
>>
>
>
Powered by blists - more mailing lists