Re: [PATCH 04/15] PCI: mvebu: Handle invalid size of read config request

From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: "Pali Rohár" <pali@kernel.org>,
	"Thomas Petazzoni" <thomas.petazzoni@bootlin.com>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	"Rob Herring" <robh@kernel.org>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Marek Behún" <kabel@kernel.org>,
	linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 04/15] PCI: mvebu: Handle invalid size of read config request
Date: Fri, 7 Jan 2022 19:15:26 +0000	[thread overview]
Message-ID: <YdiRTjGGWelHZ9rA@shell.armlinux.org.uk> (raw)
In-Reply-To: <20220107184548.GA390934@bhelgaas>

On Fri, Jan 07, 2022 at 12:45:48PM -0600, Bjorn Helgaas wrote:
> On Thu, Nov 25, 2021 at 01:45:54PM +0100, Pali Rohár wrote:
> > Function mvebu_pcie_hw_rd_conf() does not handle invalid size. So correctly
> > set read value to all-ones and return appropriate error return value
> > PCIBIOS_BAD_REGISTER_NUMBER like in mvebu_pcie_hw_wr_conf() function.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Cc: stable@vger.kernel.org
> 
> Is there a bug that this fixes?  If not, I would drop the stable tag
> (as I see Lorenzo already did, thanks!).
> 
> > ---
> >  drivers/pci/controller/pci-mvebu.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
> > index 08274132cdfb..19c6ee298442 100644
> > --- a/drivers/pci/controller/pci-mvebu.c
> > +++ b/drivers/pci/controller/pci-mvebu.c
> > @@ -261,6 +261,9 @@ static int mvebu_pcie_hw_rd_conf(struct mvebu_pcie_port *port,
> >  	case 4:
> >  		*val = readl_relaxed(conf_data);
> >  		break;
> > +	default:
> > +		*val = 0xffffffff;
> > +		return PCIBIOS_BAD_REGISTER_NUMBER;
> 
> Might be the right thing to do, but there are many config accessors
> that do not set *val to ~0 before returning
> PCIBIOS_BAD_REGISTER_NUMBER:

I think a better question would be - how can this function be called
with a size that isn't 1, 2 or 4? I suppose if someone were to add
another PCI_OP_READ/PCI_OP_WRITE. However... they really need to audit
every implementation if they do that.

The generic implementation does this:

        if (size == 1)
                *val = readb(addr);
        else if (size == 2)
                *val = readw(addr);
        else
                *val = readl(addr);

and therefore completely ignores the size if it isn't 1 or 2. So I
don't think this is something that needs fixing.

If we're going to fix this in drivers, shouldn't we fix the generic
implementation too?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!