| 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:
<SI2PR01MB4393F2E837009717A766DF0CDC62A@SI2PR01MB4393.apcprd01.prod.exchangelabs.com>
Date: Tue, 10 Feb 2026 23:02:00 +0800
From: Wei Wang <wei.w.wang@...mail.com>
To: Jonathan Cameron <jonathan.cameron@...wei.com>
Cc: bhelgaas@...gle.com, jgg@...dia.com, akpm@...ux-foundation.org,
bp@...en8.de, rdunlap@...radead.org, alex@...zbot.org, kevin.tian@...el.com,
linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org
Subject: Re: [PATCH v4 1/2] PCI: Enable the enhanced ACS controls introduced
by PCI_ACS_ECAP
On 2/10/26 12:43 AM, Jonathan Cameron wrote:
> On Sat, 7 Feb 2026 19:30:58 +0800
> Wei Wang <wei.w.wang@...mail.com> wrote:
>
>> The ACS Enhanced Capability introduces several new access controls to
>> improve device isolation. These new controls are particularly important
>> for device passthrough in virtualization scenarios.
>>
>> For example, a DMA transaction from a device may target a guest physical
>> address that lies within the memory aperture of the switch's upstream
>> port, but not within any memory aperture or BAR space of a downstream
>> port. In such cases, the switch would generate an Unsupported Request (UR)
>> response to the device, which is undesirable. Enabling Unclaimed Request
>> Redirect Control ensures that these DMA requests are forwarded upstream
>> instead of being rejected.
>>
>> The ACS DSP and USP Memory Target Access Control and ACS I/O Request
>> Blocking features similarly enhance device isolation. Device grouping in
>> Linux assumes that devices are properly isolated. Therefore, enable these
>> controls by default if PCI_ACS_ECAP is supported by the hardware. As with
>> other basic ACS access controls, these new controls can be configured via
>> the "config_acs = " boot parameter.
>>
>> Signed-off-by: Wei Wang <wei.w.wang@...mail.com>
>> Reviewed-by: Jason Gunthorpe <jgg@...dia.com>
>
> Hi Wei Wang,
>
> I'm late to the game here and may be missing something but
> I think the validation and defines for the 2 bit fields are both
> misleading and not sufficiently careful to reject values that
> are not allowed. Maybe we do want to allow the reserved value
> but if so use a 2 bit mask to validate, not two specific
> values that the filed can take.
>
Hi Jonathan,
Thanks for your comments. Yes, I think it's better to reject the
reserved encoding (0b11). I'll add masks and enum values for the
encodings, and validate that the value is not 0b11.
#define PCI_ACS_DMAC_MASK 0x0300
#define PCI_ACS_UMAC_MASK 0x0C00
enum {
PCI_ACS_MAC_DA = 0x0,
PCI_ACS_MAC_RB = 0x1,
PCI_ACS_MAC_RR = 0x2,
PCI_ACS_MAC_RSVD = 0x3,
};
> Thanks,
>
> Jonathan
>
>> ---
>> .../admin-guide/kernel-parameters.txt | 23 +++++++++++++------
>> drivers/pci/pci.c | 13 ++++++++++-
>> include/uapi/linux/pci_regs.h | 7 ++++++
>> 3 files changed, 35 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 7cfbf1102152..fd895716b07d 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -5301,13 +5301,22 @@ Kernel parameters
>> flags.
>>
>> ACS Flags is defined as follows:
>> - bit-0 : ACS Source Validation
>> - bit-1 : ACS Translation Blocking
>> - bit-2 : ACS P2P Request Redirect
>> - bit-3 : ACS P2P Completion Redirect
>> - bit-4 : ACS Upstream Forwarding
>> - bit-5 : ACS P2P Egress Control
>> - bit-6 : ACS Direct Translated P2P
>> + bit-0 : ACS Source Validation
>> + bit-1 : ACS Translation Blocking
>> + bit-2 : ACS P2P Request Redirect
>> + bit-3 : ACS P2P Completion Redirect
>> + bit-4 : ACS Upstream Forwarding
>> + bit-5 : ACS P2P Egress Control
>> + bit-6 : ACS Direct Translated P2P
>> + bit-7 : ACS I/O Request Blocking
>> + bit-9:8 : ACS DSP Memory Target Access Ctrl
>> + 00 : Direct Request access enabled
>> + 01 : Request blocking enabled
>> + 10 : Request redirect enabled
>> + 11 : Reserved
>> + bit-11:10 : ACS USP Memory Target Access Ctrl
>> + Same encoding as bit-9:8
>> + bit-12 : ACS Unclaimed Request Redirect Ctrl
>> Each bit can be marked as:
>> '0' – force disabled
>> '1' – force enabled
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index d2810eb97bda..1714e29ce099 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -942,7 +942,10 @@ static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
>> }
>>
>> if (mask & ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR |
>> - PCI_ACS_UF | PCI_ACS_EC | PCI_ACS_DT)) {
>> + PCI_ACS_UF | PCI_ACS_EC | PCI_ACS_DT | PCI_ACS_IB |
>> + PCI_ACS_DMAC_RB | PCI_ACS_DMAC_RR |
>> + PCI_ACS_UMAC_RB | PCI_ACS_UMAC_RR |
>> + PCI_ACS_URRC)) {
> This used of the RB and RR Bits seems misleading to me as they form parts
> of 2 bit field that can only take values 0, 1 and 2.
>
> So if the command line set PCI_ACS_UMAC_RB and PCI_ACS_UMAC_RR it
> would have hit the reserved value and that should be detected.
>
> Hence I think you need to do this sort of masking + additional checks that these
> 2 bit fields have valid content.
Yes, will do.
>
>> pci_err(dev, "Invalid ACS flags specified\n");
>> return;
>> }
>> @@ -1002,6 +1005,14 @@ static void pci_std_enable_acs(struct pci_dev *dev, struct pci_acs *caps)
>> /* Upstream Forwarding */
>> caps->ctrl |= (caps->cap & PCI_ACS_UF);
>>
>> + /*
>> + * Downstream and Upstream Port Memory Target Access Redirect,
>> + * Redirect Unclaimed Request Redirect Control
>> + */
>> + if (caps->cap & PCI_ACS_ECAP)
>> + caps->ctrl |= PCI_ACS_DMAC_RR | PCI_ACS_UMAC_RR |
>> + PCI_ACS_URRC | PCI_ACS_IB;
>> +
>> /* Enable Translation Blocking for external devices and noats */
>> if (pci_ats_disabled() || dev->external_facing || dev->untrusted)
>> caps->ctrl |= (caps->cap & PCI_ACS_TB);
>> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
>> index ec1c54b5a310..593ef522d2ba 100644
>> --- a/include/uapi/linux/pci_regs.h
>> +++ b/include/uapi/linux/pci_regs.h
>> @@ -1016,6 +1016,7 @@
>>
>> /* Access Control Service */
>> #define PCI_ACS_CAP 0x04 /* ACS Capability Register */
>> +#define PCI_ACS_ECAP 0x0080 /* ACS Enhanced Capability */
>> #define PCI_ACS_SV 0x0001 /* Source Validation */
>> #define PCI_ACS_TB 0x0002 /* Translation Blocking */
>> #define PCI_ACS_RR 0x0004 /* P2P Request Redirect */
>> @@ -1023,6 +1024,12 @@
>> #define PCI_ACS_UF 0x0010 /* Upstream Forwarding */
>> #define PCI_ACS_EC 0x0020 /* P2P Egress Control */
>> #define PCI_ACS_DT 0x0040 /* Direct Translated P2P */
>> +#define PCI_ACS_IB 0x0080 /* I/O Request Blocking */
>> +#define PCI_ACS_DMAC_RB 0x0100 /* DSP Memory Target Access Blocking */
>> +#define PCI_ACS_DMAC_RR 0x0200 /* DSP Memory Target Access Redirect */
>> +#define PCI_ACS_UMAC_RB 0x0400 /* USP Memory Target Access Blocking */
>
> If I'm reading this write, those are specific values in the
> ACS USP Memory Target Access Control, not what they appear to be here
> which is bits that can be combined.
> I think you need a _MASK and separate definitions for the 3 values that
> are allowed.
Yes.
>
>> +#define PCI_ACS_UMAC_RR 0x0800 /* USP Memory Target Access Redirect */
>> +#define PCI_ACS_URRC 0x1000 /* Unclaimed Request Redirect Ctrl */
>> #define PCI_ACS_EGRESS_BITS 0x05 /* ACS Egress Control Vector Size */
>> #define PCI_ACS_CTRL 0x06 /* ACS Control Register */
>> #define PCI_ACS_EGRESS_CTL_V 0x08 /* ACS Egress Control Vector */
>
Powered by blists - more mailing lists