Re: [PATCH 1/2] misc: pci_endpoint_test: validate BAR index in doorbell test

[Carlos Bilbao]

Hello,

On 4/6/26 09:39, Koichiro Den wrote:
> On Fri, Apr 03, 2026 at 06:20:01PM -0700, carlos.bilbao@xxxxxxxxxx wrote:
> > From: Carlos Bilbao <carlos.bilbao@xxxxxxxxxx>
> >
> > pci_endpoint_test_doorbell() reads the BAR number directly from an endpoint
> > test register and uses it as an index into test->bar[] without bounds
> > checking. Since the value is a raw u32 from device MMIO, any value is
> > possible and if greater than or equal to PCI_STD_NUM_BARS the access goes
> > out of bounds.
> >
> > Add a bounds check before dereferencing test->bar[bar].
> >
> > Fixes: eefb83790a0d ("misc: pci_endpoint_test: Add doorbell test case")
> > Signed-off-by: Carlos Bilbao (Lambda) <carlos.bilbao@xxxxxxxxxx>
> > ---
> >   drivers/misc/pci_endpoint_test.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> > index 74ab5b5b9011..276bed3f1c18 100644
> > --- a/drivers/misc/pci_endpoint_test.c
> > +++ b/drivers/misc/pci_endpoint_test.c
> > @@ -1089,6 +1089,11 @@ static int pci_endpoint_test_doorbell(struct pci_endpoint_test *test)
> >   	pci_endpoint_test_writel(test, PCI_ENDPOINT_TEST_STATUS, 0);
> >   
> >   	bar = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_DB_BAR);
> > +	if (bar >= PCI_STD_NUM_BARS) {
> > +		dev_err(dev, "BAR %u reported by endpoint out of range [0, %u]\n",
> > +			bar, PCI_STD_NUM_BARS - 1);
> > +		return -EINVAL;
> > +	}
> Hi,
>
> On the EP-side (pci-epf-test.c), doorbell_bar is set to NO_BAR or a valid BAR
> number in the range 0..(PCI_STD_NUM_BARS-1), so if we add such a safeguard here,
> we might as well handle NO_BAR explicitly.
>
> That said, I'm not sure this check is really needed. At this point
> COMMAND_ENABLE_DOORBELL should have succeeded, so bar is expected to always be
> within the valid range. Whether such defensive checks are desirable probably
> depends on the maintainers' preference. In any case, I don't think a Fixes tag
> is appropriate here.
>
> Also, even if the intention is also to guard against an "all 1's" value scenario
> on errors, then I think it might be better to handle that in a more centralized
> way, since the same concern would apply to other similar reads as well.


Thanks for the feedback. I'll send a v2 with changes here to handle bar
< BAR_0 as well as dropping the Fixes tag.


> Best regards,
> Koichiro
>
> >   
> >   	writel(data, test->bar[bar] + addr);
> >   
> > --
> > 2.43.0

Thanks,

Carlos