linux-kernel - Re: [PATCH] MIPS: pci-legacy: Override pci_address_to

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]

Message-Id: <23f46c23-a58e-4a85-8733-480b5ebf993b@app.fastmail.com>
Date: Wed, 15 Jan 2025 08:38:36 +0100
From: "Arnd Bergmann" <arnd@...db.de>
To: "Jiaxun Yang" <jiaxun.yang@...goat.com>,
 "Thomas Bogendoerfer" <tsbogend@...ha.franken.de>,
 "Baoquan He" <bhe@...hat.com>
Cc: "linux-mips@...r.kernel.org" <linux-mips@...r.kernel.org>,
 linux-kernel@...r.kernel.org,
 "stable@...r.kernel.org" <stable@...r.kernel.org>,
 Mateusz Jończyk <mat.jonczyk@...pl>
Subject: Re: [PATCH] MIPS: pci-legacy: Override pci_address_to_pio

On Tue, Jan 14, 2025, at 20:20, Jiaxun Yang wrote:
> 在2025年1月14日一月 下午7:03，Arnd Bergmann写道：
>> On Tue, Jan 14, 2025, at 19:11, Jiaxun Yang wrote:
>>> 
>>> +unsigned long pci_address_to_pio(phys_addr_t address)
>>> +{
>>> +	if (address > IO_SPACE_LIMIT)
>>> +		return (unsigned long)-1;
>>> +
>>> +	return (unsigned long) address;
>>> +}
>>> +
>>>  /*
>>
>> Isn't the argument to this function a CPU physical address? I
>> don't think there is a point comparing it to IO_SPACE_LIMIT
>> on architectures where I/O space is memory mapped.
>
> Actually not. It seems like the argument here is just raw PIO offset,
> without applying mips_io_port_base.
>
> We should validate it to ensure it's within the range specified by
> mips_io_port_base (which is sized by IO_SPACE_LIMIT).

I don't know what you mean with "raw PIO offset", but I don't think
that's how it's supopsed to be used. Here is how this gets called
in of_pci_range_to_resource()

        if (res->flags & IORESOURCE_IO) {
                unsigned long port;
                err = pci_register_io_range(&np->fwnode, range->cpu_addr,
                                range->size);
                if (err)
                        goto invalid_range;
                port = pci_address_to_pio(range->cpu_addr);
                if (port == (unsigned long)-1) {
                        err = -EINVAL;
                        goto invalid_range;
                }
                start = port;
       }

Where "range->cpu_addr" is the phys_addr_t location of the
MMIO window that maps the host controllers port ranges, i.e.
the fully translated address from the "ranges" property.

The point of the pci_address_to_pio() function is to convert
this into the Linux-internal virtual port number that gets
used as an argument to inb()/outb() and reported in
/proc/ioports and that may or may not be the same as the
address on the bus itself, depending on the how the translation
gets set up.

On loongson, we seem to have two port ranges that are set up
like

                isa@...00000 {
                        compatible = "isa";
                        ranges = <1 0x0 0x0 0x18000000 0x4000>;
                };

                pci@...00000 {
                        ranges = <0x01000000 0x0 0x00020000 0x0 0x18020000 0x0 0x00020000>,
                                 <0x02000000 0x0 0x40000000 0x0 0x40000000 0x0 0x40000000>;
                }

Here, the cpu_addr is 0x18000000 for the isa bus and 
0x18020000 for the PCI bus, apparently the intention being that
these are consecutive in physical space, though Linux is free
to rearrange the logical port numbers in a different way, e.g.
to ensure that the PCI bus can use port numbers below 0x10000.

On Malta, I see a very strange

        isa {
                compatible = "isa";
                ranges = <1 0 0 0x1000>;
        };

which maps the first 4096 port numbers into cpu_addr=0x0. The
actual port window appears to be at a board specific location

#define MALTA_GT_PORT_BASE      get_gt_port_base(GT_PCI0IOLD_OFS)
#define MALTA_BONITO_PORT_BASE  ((unsigned long)ioremap (0x1fd00000, 0x10000))
#define MALTA_MSC_PORT_BASE     get_msc_port_base(MSC01_PCI_SC2PIOBASL)

So e.g. on Bonito, the ranges property would have to be

      ranges = <1 0 0x1fd00000 0x1000>;

Not sure if this is patched in by the bootloader, or where the
corresponding window for PCI gets defined, but I suspect that
the reason for the regression is that the caller of
pci_address_to_pio() accidentally passed in '0' instead of
the physical address, and it happened to work because of the
missing PCI_IOBASE definition but broke after that got defined.

       Arnd