linux-kernel - Re: [PATCH 1/1] PCI: acpiphp:: use pci_assign_unassigned_bridge

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 4 Aug 2023 18:27:09 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Igor Mammedov <imammedo@...hat.com>
Cc:     linux-kernel@...r.kernel.org, terraluna977@...il.com,
        bhelgaas@...gle.com, linux-pci@...r.kernel.org, mst@...hat.com,
        rafael@...nel.org, linux-acpi@...r.kernel.org
Subject: Re: [PATCH 1/1] PCI: acpiphp:: use
 pci_assign_unassigned_bridge_resources() only if bus->self not NULL

On Wed, Jul 26, 2023 at 02:35:18PM +0200, Igor Mammedov wrote:
> Commit [1] switched acpiphp hotplug to use
>    pci_assign_unassigned_bridge_resources()
> which depends on bridge being available, however in some cases
> when acpiphp is in use, enable_slot() can get a slot without
> bridge associated.

acpiphp is *always* in use if we get to enable_slot(), so that doesn't
really add information here.

>   1. legitimate case of hotplug on root bus
>       (likely not exiting on real hw, but widely used in virt world)
>   2. broken firmware, that sends 'Bus check' events to non
>      existing root ports (Dell Inspiron 7352/0W6WV0), which somehow
>      endup at acpiphp:enable_slot(..., bridge = 0) and with bus
>      without bridge assigned to it.

IIUC, the Inspiron problem happens when:

  - acpiphp_context->bridge is NULL, so hotplug_event() calls
    enable_slot() instead of acpiphp_check_bridge(), AND

  - acpiphp_slot->bus->self is also NULL, because enable_slot() calls
    pci_assign_unassigned_bridge_resources() with that NULL pointer,
    which dereferences "bridge->subordinate"

But I can't figure out why acpiphp_context->bridge is NULL for RP07
and RP08 (which don't exist), but not for RP03 (which does).

I guess all the acpiphp_contexts (RP03, RP07, RP08) must be allocated in
acpiphp_add_context() by acpiphp_init_context().

Woody's lspci from [1] shows only one Root Port:

  00:1c.0 Wildcat Point-LP PCI Express Root Port #3

The DSDT.DSL includes:

  Device (RP01) _ADR 0x001C0000		# 1c.0
  Device (RP02) _ADR 0x001C0001		# 1c.1
  Device (RP03) _ADR 0x001C0002		# 1c.2
  Device (RP04) _ADR 0x001C0003		# 1c.3
  Device (RP05) _ADR 0x001C0004		# 1c.4
  Device (RP06) _ADR 0x001C0005		# 1c.5
  Device (RP07) _ADR 0x001C0006		# 1c.6
  Device (RP08) _ADR 0x001C0007		# 1c.7

I can see why we might need a Bus Check after resume to see if
something got added while we were suspended.  But I don't see why we
handle RP03 differently from RP07 and RP08.

Can you help me out?  I'm lost in a maze of twisty passages, all
alike.

Bjorn

[1] https://lore.kernel.org/r/92150d8d-8a3a-d600-a996-f60a8e4c876c@gmail.com