linux-kernel - Re: [PATCH 1/1] PCI: dwc: Fix BAR0 wrong map to iATU6 after root complex reinit endpoint

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: <ZeY3XqgUf84qAMCL@fedora>
Date: Mon, 4 Mar 2024 21:04:30 +0000
From: Niklas Cassel <Niklas.Cassel@....com>
To: Frank Li <Frank.li@....com>
CC: Manivannan Sadhasivam <mani@...nel.org>, "imx@...ts.linux.dev"
	<imx@...ts.linux.dev>, Jingoo Han <jingoohan1@...il.com>, Gustavo Pimentel
	<gustavo.pimentel@...opsys.com>, Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Krzysztof Wilczyński <kw@...ux.com>, Rob Herring
	<robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>, Jon Mason
	<jdmason@...zu.us>, "open list:PCI DRIVER FOR SYNOPSYS DESIGNWARE"
	<linux-pci@...r.kernel.org>, open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/1] PCI: dwc: Fix BAR0 wrong map to iATU6 after root
 complex reinit endpoint

On Mon, Mar 04, 2024 at 02:13:27PM -0500, Frank Li wrote:
> > 
> > Niklas's initial suggestion of keeping u8 for the array and 0 as the unallocated
> > placeholder sounds good to me. Please use that instead.
> > 
> 
> It is impossible to keep u8, because 255 + 1 will 0 for u8. Previously
> Niklas's initial suggestion have not consider this condition. If u8 have to
> change to u16 or s16 anyways, I prefer use -1 as free.

Well, to be fair, my suggestion was:
"If we continue to use a u8, and offset the saved value by one,
we will at least be able to support 255-1 == 254 iATUs."

But we have this define:
drivers/pci/controller/dwc/pcie-designware.h:#define MAX_IATU_IN 256
(Even if it isn't used anywhere.)

But as ridiculous as it may seem to support that many inbound ranges,
that is the max number of windows supported by the hardware, so why
not just let the driver support the max supported by the hardware?

We are talking about:
struct dw_pcie_ep {
	...
        u8                      bar_to_atu[PCI_STD_NUM_BARS];
	...
}

where PCI_STD_NUM_BARS == 6.

And where struct dw_pcie_ep is kzalloced for what I assume is all drivers.

So I'm actually for your idea of changing it to u16, or even unsigned int.

If the code is simplified if we use a u16 or unsigned int (because we don't
need any weird if (val < MAX_IATU_IN - 1) check), then I'm all for it.

What I personally don't like with your patch in $subject,
was that you changed both dw_pcie_ep_clear_bar() to set the "clear value"
to -1, but you also need a
memset(ep->bar_to_atu, -1, sizeof(ep->bar_to_atu)); in dw_pcie_ep_init().

I much prefer to have 0 as the default/unused value, because it will
automatically get set when you do kzalloc().
Seeing a memset -1 just looks a bit hackish to be, but I realize that
it is personal preference.

If it is super important to save 8 bytes from the heap, then I would
even prefer changing the MAX_IATU_IN 256 to something smaller, like
127 or whatever, just as long as we don't need that extra memset -1 :)

Kind regards,
Niklas