On Wed, Dec 04, 2024 at 11:17:16AM -0600, Bjorn Helgaas wrote:
> [+cc NTB list, since NTB seems to be the main user of .set_bar()]
>
> Can we say something specific in the subject line, like "prevent
> changing size/flags" or whatever?
Ok. Will fix in v6.
>
> On Wed, Nov 27, 2024 at 11:30:18AM +0100, Niklas Cassel wrote:
> > In commit 4284c88fff0e ("PCI: designware-ep: Allow pci_epc_set_bar() update
> > inbound map address") set_bar() was modified to support dynamically
> > changing the physical address of a BAR.
> >
> > This means that set_bar() can be called twice, without ever calling
> > clear_bar(), as calling clear_bar() would clear the BAR's PCI address
> > assigned by the host).
>
> Unpaired parenthesis at end.
Ok. Will fix in v6.
>
> Apparently calling .set_bar() twice without calling .clear_bar() is a
> problem? What problem does it cause? Sorry about my poor
> understanding of the endpoint and NTB code; I'm sure this would be
> obvious if I understood more.
Calling .set_bar() without calling .clear_bar() is not a problem.
In fact, that is how the NTB EPF dynamically switches the inbound address
translation for a BAR on the fly, after the host has assigned a PCI address
for the BAR.
It is just that for most EPF drivers, you would normally call .clear_bar()
before calling .set_bar() again.
>
> Maybe a hint about the reason why we need to call .set_bar() twice
> would help me understand.
The NTB EPF does this to change the inbound address translation from one
address to another on the fly. For details about why the NTB EPF driver does
this, you would need to ask Frank who implemented this.
>
> > This can only be done if the new BAR configuration doesn't fundamentally
> > differ from the existing BAR configuration. Add these missing checks.
>
> Can you elaborate a bit on what "fundamentally differ" means? Based
> on the patch, I guess it has to do with changing the size or type?
Correct, BAR size and BAR type has to be the same.
I don't know why the NTB EPF does this, but it is obvious that very bad
things could happen if you would dynamically change from one BAR size to
another. (This is not a resizable BAR...)
>
> And the problem this would cause is ...? I guess it's a problem for
> some other entity that knows about the BAR and is doing MMIO or DMA to
> it?
If we allowed the BAR sizes of the old area and the new area to differ,
that could mean that we could leak kernel memory, since if e.g. the new area
is smaller than the old area, a read past the new area size would potentially
read memory that wasn't allocated by the EPF.
To me, is seems like quite a big oversight that these safe guards wasn't
added as part of commit 4284c88fff0e ("PCI: designware-ep: Allow
pci_epc_set_bar() update inbound map address"), but then as you might have
seen, that patch was also merged via the NTB tree without any Ack from the
PCI endpoint maintainers. This patch has a Fixes-tag against that commit,
so stable should pick this up. I will also add an explict Cc: stable in v6.
Kind regards,
Niklas
