linux-kernel - Re: [PATCH v2 1/2] PCI: dwc: Perform host

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:   Tue, 24 Aug 2021 14:23:14 -0700
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     Bjorn Helgaas <bhelgaas@...gle.com>,
        Rob Herring <robh+dt@...nel.org>,
        Jingoo Han <jingoohan1@...il.com>,
        Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Krzysztof Wilczy??ski <kw@...ux.com>,
        Stanimir Varbanov <svarbanov@...sol.com>,
        linux-arm-msm@...r.kernel.org, linux-pci@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] PCI: dwc: Perform host_init() before registering
 msi

On Tue 24 Aug 13:29 PDT 2021, Bjorn Helgaas wrote:

> On Tue, Aug 24, 2021 at 01:15:49PM -0700, Bjorn Andersson wrote:
> > On Tue 24 Aug 12:05 PDT 2021, Bjorn Helgaas wrote:
> > 
> > > On Mon, Aug 23, 2021 at 08:49:57AM -0700, Bjorn Andersson wrote:
> > > > On the Qualcomm sc8180x platform the bootloader does something related
> > > > to PCI that leaves a pending "msi" interrupt, which with the current
> > > > ordering often fires before init has a chance to enable the clocks that
> > > > are necessary for the interrupt handler to access the hardware.
> > > > 
> > > > Move the host_init() call before the registration of the "msi" interrupt
> > > > handler to ensure the host driver has a chance to enable the clocks.
> > > 
> > > Did you audit other drivers for similar issues?  If they do, we should
> > > fix them all at once.
> > 
> > I only looked at the DesignWware drivers, in an attempt to find any
> > issues the proposed reordering.
> > 
> > The set of bugs causes by drivers registering interrupts before critical
> > resources tends to be rather visible and I don't know if there's much
> > value in speculatively "fixing" drivers.
> > 
> > E.g. a quick look through the drivers I see a similar pattern in
> > pci-tegra.c, but it's unlikely that they have the similar problem in
> > practice and I have no way to validate that a change to the order would
> > have a positive effect - or any side effects.
> > 
> > Or am I misunderstanding your request?
> 
> That is exactly my request.

Okay.

> I'm not sure if the potential issue you
> noticed in pci-tegra.c is similar to the one I mentioned here:
> 
>   https://lore.kernel.org/linux-pci/20210624224040.GA3567297@bjorn-Precision-5520/
> 

As I still have the tegra driver open, I share your concern about the
use of potentially uninitialized variables.

The problem I was concerned about was however the same as in my patch
and the rockchip one, that if the tegra hardware isn't clocked the
pm_runtime_get_sync() (which would turn on power and clock) happens
after setting up the msi chain handler...

> but I am actually in favor of speculatively fixing drivers even though
> they're hard to test.  Code like this tends to get copied to other
> places, and fixing several drivers sometimes exposes opportunities for
> refactoring and sharing code.
> 

Looking through the other cases mentioned in your reply above certainly
gives a feeling that this problem has been inherited from driver to
driver...

I've added a ticket to my backlog to take a deeper look at this.

Regards,
Bjorn