| 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: <20260209164349.00003e41@huawei.com>
Date: Mon, 9 Feb 2026 16:43:49 +0000
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: Wei Wang <wei.w.wang@...mail.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 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.
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.
> 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.
> +#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