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

[Carlos Bilbao <carlos.bilbao.osdev@gmail.com>]

Hello,

On 4/6/26 09:39, Koichiro Den wrote:
> On Fri, Apr 03, 2026 at 06:20:01PM -0700, carlos.bilbao@kernel.org wrote:
>> From: Carlos Bilbao <carlos.bilbao@kernel.org>
>>
>> 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@kernel.org>
>> ---
>>   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