Re: [PATCH v5 2/6] PCI: dwc: ep: Add missing checks when dynamically changing a BAR

Re: [PATCH v5 2/6] PCI: dwc: ep: Add missing checks when dynamically changing a BAR

[Bjorn Helgaas]

[+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?

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.

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.

Maybe a hint about the reason why we need to call .set_bar() twice
would help me understand.

> 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?

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?

> While at it, add comments which clarifies the support for dynamically
> changing the physical address of a BAR. (Which was also missing.)
> 
> Fixes: 4284c88fff0e ("PCI: designware-ep: Allow pci_epc_set_bar() update inbound map address")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  .../pci/controller/dwc/pcie-designware-ep.c   | 22 ++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index bad588ef69a4..01c739aaf61a 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -222,8 +222,28 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  	if ((flags & PCI_BASE_ADDRESS_MEM_TYPE_64) && (bar & 1))
>  		return -EINVAL;
>  
> -	if (ep->epf_bar[bar])
> +	/*
> +	 * Certain EPF drivers dynamically change the physical address of a BAR
> +	 * (i.e. they call set_bar() twice, without ever calling clear_bar(), as
> +	 * calling clear_bar() would clear the BAR's PCI address assigned by the
> +	 * host).
> +	 */
> +	if (ep->epf_bar[bar]) {
> +		/*
> +		 * We can only dynamically change a BAR if the new configuration
> +		 * doesn't fundamentally differ from the existing configuration.
> +		 */
> +		if (ep->epf_bar[bar]->barno != bar ||
> +		    ep->epf_bar[bar]->size != size ||
> +		    ep->epf_bar[bar]->flags != flags)
> +			return -EINVAL;
> +
> +		/*
> +		 * When dynamically changing a BAR, skip writing the BAR reg, as
> +		 * that would clear the BAR's PCI address assigned by the host.
> +		 */
>  		goto config_atu;
> +	}
>  
>  	reg = PCI_BASE_ADDRESS_0 + (4 * bar);
>  
> -- 
> 2.47.0
> 

Re: [PATCH v5 2/6] PCI: dwc: ep: Add missing checks when dynamically changing a BAR

[Niklas Cassel]

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