| 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: <20110721071021.GC9216@elte.hu>
Date: Thu, 21 Jul 2011 09:10:22 +0200
From: Ingo Molnar <mingo@...e.hu>
To: Jan Beulich <JBeulich@...ell.com>,
Jesse Barnes <jbarnes@...tuousgeek.org>
Cc: tglx@...utronix.de, hpa@...or.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] x86: PCI config space accessor functions should not
ignore the segment argument
> The access method 1 accessor, as it can be used for extended accesses
> (on AMD systems) instead gets added checks for the passed in segment to
> be zero (returning an error just like out of range values of the other
> arguments would cause).
* Jan Beulich <JBeulich@...ell.com> wrote:
> Without this change, the majority of the raw PCI config space access
> functions silently ignore a non-zero segment argument, which is
> certainly wrong.
>
> Apart from pci_direct_conf1, all other non-MMCFG access methods get
> used only for non-extended accesses (i.e. assigned to raw_pci_ops
> only). Consequently, with the way raw_pci_{read,write}() work, it would
> be a coding error to call these functions with a non-zero segment (with
> the current call flow this cannot happen afaict).
>
>
> Signed-off-by: Jan Beulich <jbeulich@...ell.com>
>
> ---
> arch/x86/pci/ce4100.c | 4 ++++
> arch/x86/pci/direct.c | 6 ++++--
> arch/x86/pci/numaq_32.c | 2 ++
> arch/x86/pci/olpc.c | 4 ++++
> arch/x86/pci/pcbios.c | 2 ++
> 5 files changed, 16 insertions(+), 2 deletions(-)
>
> --- 3.0-rc7/arch/x86/pci/ce4100.c
> +++ 3.0-rc7-x86-pci-access-seg/arch/x86/pci/ce4100.c
> @@ -257,6 +257,8 @@ static int ce4100_conf_read(unsigned int
> {
> int i;
>
> + BUG_ON(seg);
> +
> if (bus == 1) {
> for (i = 0; i < ARRAY_SIZE(bus1_fixups); i++) {
> if (bus1_fixups[i].dev_func == devfn &&
> @@ -282,6 +284,8 @@ static int ce4100_conf_write(unsigned in
> {
> int i;
>
> + BUG_ON(seg);
> +
> if (bus == 1) {
> for (i = 0; i < ARRAY_SIZE(bus1_fixups); i++) {
> if (bus1_fixups[i].dev_func == devfn &&
> --- 3.0-rc7/arch/x86/pci/direct.c
> +++ 3.0-rc7-x86-pci-access-seg/arch/x86/pci/direct.c
> @@ -22,7 +22,7 @@ static int pci_conf1_read(unsigned int s
> {
> unsigned long flags;
>
> - if ((bus > 255) || (devfn > 255) || (reg > 4095)) {
> + if (seg || (bus > 255) || (devfn > 255) || (reg > 4095)) {
> *value = -1;
> return -EINVAL;
> }
> @@ -53,7 +53,7 @@ static int pci_conf1_write(unsigned int
> {
> unsigned long flags;
>
> - if ((bus > 255) || (devfn > 255) || (reg > 4095))
> + if (seg || (bus > 255) || (devfn > 255) || (reg > 4095))
> return -EINVAL;
>
> raw_spin_lock_irqsave(&pci_config_lock, flags);
> @@ -97,6 +97,7 @@ static int pci_conf2_read(unsigned int s
> unsigned long flags;
> int dev, fn;
>
> + BUG_ON(seg);
> if ((bus > 255) || (devfn > 255) || (reg > 255)) {
> *value = -1;
> return -EINVAL;
> @@ -138,6 +139,7 @@ static int pci_conf2_write(unsigned int
> unsigned long flags;
> int dev, fn;
>
> + BUG_ON(seg);
> if ((bus > 255) || (devfn > 255) || (reg > 255))
> return -EINVAL;
>
> --- 3.0-rc7/arch/x86/pci/numaq_32.c
> +++ 3.0-rc7-x86-pci-access-seg/arch/x86/pci/numaq_32.c
> @@ -34,6 +34,7 @@ static int pci_conf1_mq_read(unsigned in
> unsigned long flags;
> void *adr __iomem = XQUAD_PORT_ADDR(0xcfc, BUS2QUAD(bus));
>
> + BUG_ON(seg);
> if (!value || (bus >= MAX_MP_BUSSES) || (devfn > 255) || (reg > 255))
> return -EINVAL;
>
> @@ -73,6 +74,7 @@ static int pci_conf1_mq_write(unsigned i
> unsigned long flags;
> void *adr __iomem = XQUAD_PORT_ADDR(0xcfc, BUS2QUAD(bus));
>
> + BUG_ON(seg);
> if ((bus >= MAX_MP_BUSSES) || (devfn > 255) || (reg > 255))
> return -EINVAL;
>
> --- 3.0-rc7/arch/x86/pci/olpc.c
> +++ 3.0-rc7-x86-pci-access-seg/arch/x86/pci/olpc.c
> @@ -206,6 +206,8 @@ static int pci_olpc_read(unsigned int se
> {
> uint32_t *addr;
>
> + BUG_ON(seg);
> +
> /* Use the hardware mechanism for non-simulated devices */
> if (!is_simulated(bus, devfn))
> return pci_direct_conf1.read(seg, bus, devfn, reg, len, value);
> @@ -264,6 +266,8 @@ static int pci_olpc_read(unsigned int se
> static int pci_olpc_write(unsigned int seg, unsigned int bus,
> unsigned int devfn, int reg, int len, uint32_t value)
> {
> + BUG_ON(seg);
> +
> /* Use the hardware mechanism for non-simulated devices */
> if (!is_simulated(bus, devfn))
> return pci_direct_conf1.write(seg, bus, devfn, reg, len, value);
> --- 3.0-rc7/arch/x86/pci/pcbios.c
> +++ 3.0-rc7-x86-pci-access-seg/arch/x86/pci/pcbios.c
> @@ -181,6 +181,7 @@ static int pci_bios_read(unsigned int se
> unsigned long flags;
> unsigned long bx = (bus << 8) | devfn;
>
> + BUG_ON(seg);
> if (!value || (bus > 255) || (devfn > 255) || (reg > 255))
> return -EINVAL;
>
> @@ -247,6 +248,7 @@ static int pci_bios_write(unsigned int s
> unsigned long flags;
> unsigned long bx = (bus << 8) | devfn;
>
> + BUG_ON(seg);
> if ((bus > 255) || (devfn > 255) || (reg > 255))
> return -EINVAL;
Not sure we want a BUG_ON() which crashes the box - wouldn't a
WARN_ON() suffice?
also, the analysis/explanation is a bit incomplete:
> The access method 1 accessor, as it can be used for extended accesses
> (on AMD systems) instead gets added checks for the passed in segment to
> be zero (returning an error just like out of range values of the other
> arguments would cause).
Under what circumstances can this trigger in practice, with the
current code?
Thanks,
Ingo
--
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