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

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: <YdiRTjGGWelHZ9rA@shell.armlinux.org.uk>
Date:   Fri, 7 Jan 2022 19:15:26 +0000
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     Pali Rohár <pali@...nel.org>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Marek Behún <kabel@...nel.org>,
        linux-pci@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 04/15] PCI: mvebu: Handle invalid size of read config
 request

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@...nel.org>
> > Cc: stable@...r.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!