Re: [PATCH] pci: Warn if BME cannot be turned off during kexec

[Deepa Dinamani]

On Mon, Jan 6, 2020 at 11:38 AM Deepa Dinamani <deepa.kernel@xxxxxxxxx> wrote:
>
> On Mon, Jan 6, 2020 at 5:54 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> >
> > Hi Deepa,
> >
> > Thanks for the patches.  Since these two patches touch the same piece
> > of code in pci_device_shutdown(), they conflict with each other.  I
> > could resolve this myself, but maybe you could make them a series that
> > applies cleanly together?
>
> Sure, will make this a series.
>
> > Can you also please edit the subject lines so they match the
> > convention (use "git log --oneline drivers/pci/pci-driver.c" to see
> > it).
>
> Will do.
>
> > On Sat, Jan 04, 2020 at 02:50:52PM -0800, Deepa Dinamani wrote:
> > > BME not being off is a security risk, so for whatever
> > > reason if we cannot disable it, print a warning.
> >
> > "BME" is not a common term in drivers/pci; can you use "Bus Master
> > Enable" (to match the PCIe spec) or "PCI_COMMAND_MASTER" (to match the
> > Linux code)?
>
> Will do.
>
> > Can you also explain why this is a security risk?  It looks like we
> > disable bus mastering if the device is in D0-D3hot.  If the device is
> > in D3cold, it's powered off, so we can't read/write config space.  But
> > if it's in D3cold, the device is powered off, so it can't be a bus
> > master either, so why would we warn about it?
>
> I was mainly concerned about the PCI_UNKNOWN state here. Maybe we can
> add a specific check for the unknown state if that is preferable.

I did some more testing. You are right, these messages are printed
more often than I had remembered.

For some more context on what I am trying to do: I recently merged
another patch that disable iommu unconditionally at kexec:
https://lkml.org/lkml/2019/11/10/146
And, if we do not have IOMMU on and BME is on then we have no way of
controlling memory accesses from devices. This is why I wanted these
warning messages printed. Alternatively, it might just be enough to
warn if we cannot turn off BME on root ports rather than all the
devices. Does this seem like a better compromise?

-Deepa