[PATCH] misc: pci_endpoint_test: fixed pci_resource_len return value out of bounds.

[PATCH] misc: pci_endpoint_test: fixed pci_resource_len return value out of bounds.

[Hans Zhang]

The return type of the API is inconsistent. Inconsistencies may
result in out-of-bounds. If the bar size of the EP device exceeds
4G, this bar_Size will be equal to 0.

For example, there is an EP device, the bar0 size is 16MB, bar1
size is 32MB, bar2 size is 8GB. When testing bar2, barno equals
BAR2. Then run pcitest -b 2, console will output "TEST FAILED".

Variable declaration of bar_size is int, the range less than or
equal 2G. The return value of pci_resource_len is resource_size_t.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
 drivers/misc/pci_endpoint_test.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 3aaaf47fa4ee..414c4e55fb0a 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -280,10 +280,11 @@ static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
 static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
 				  enum pci_barno barno)
 {
-	int j, bar_size, buf_size, iters, remain;
 	void *write_buf __free(kfree) = NULL;
 	void *read_buf __free(kfree) = NULL;
 	struct pci_dev *pdev = test->pdev;
+	int j, buf_size, iters, remain;
+	resource_size_t bar_size;
 
 	if (!test->bar[barno])
 		return false;
-- 
2.17.1

Re: [PATCH] misc: pci_endpoint_test: fixed pci_resource_len return value out of bounds.

[Niklas Cassel]

On Tue, Dec 17, 2024 at 07:12:20AM -0500, Hans Zhang wrote:
> The return type of the API is inconsistent. Inconsistencies may
> result in out-of-bounds. If the bar size of the EP device exceeds
> 4G, this bar_Size will be equal to 0.
> 
> For example, there is an EP device, the bar0 size is 16MB, bar1
> size is 32MB, bar2 size is 8GB. When testing bar2, barno equals
> BAR2. Then run pcitest -b 2, console will output "TEST FAILED".
> 
> Variable declaration of bar_size is int, the range less than or
> equal 2G. The return value of pci_resource_len is resource_size_t.
> 
> Signed-off-by: Hans Zhang <18255117159@163.com>
> ---
>  drivers/misc/pci_endpoint_test.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 3aaaf47fa4ee..414c4e55fb0a 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -280,10 +280,11 @@ static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
>  static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
>  				  enum pci_barno barno)
>  {
> -	int j, bar_size, buf_size, iters, remain;
>  	void *write_buf __free(kfree) = NULL;
>  	void *read_buf __free(kfree) = NULL;
>  	struct pci_dev *pdev = test->pdev;
> +	int j, buf_size, iters, remain;
> +	resource_size_t bar_size;
>  
>  	if (!test->bar[barno])
>  		return false;
> -- 
> 2.17.1
> 

Reviewed-by: Niklas Cassel <cassel@kernel.org>

Re: [PATCH] misc: pci_endpoint_test: fixed pci_resource_len return value out of bounds.

[Ilpo Järvinen]

On Tue, 17 Dec 2024, Hans Zhang wrote:

> The return type of the API is inconsistent. Inconsistencies may
> result in out-of-bounds.

I'm not sure how out-of-bounds access would happen. On which line you see 
that possibility?

> If the bar size of the EP device exceeds

BAR size

> 4G, this bar_Size will be equal to 0.

bar_size

> For example, there is an EP device, the bar0 size is 16MB, bar1
> size is 32MB, bar2 size is 8GB. When testing bar2, barno equals
> BAR2. Then run pcitest -b 2, console will output "TEST FAILED".

I think bar0 and bar1 information could simply be dropped since they're 
unrelated. I think this would be enough information:

With 8GB BAR2, running pcitest -b 2 fails with "TEST FAILED".

> Variable declaration of bar_size is int, the range less than or
> equal 2G. The return value of pci_resource_len is resource_size_t.
> 
> Signed-off-by: Hans Zhang <18255117159@163.com>
> ---
>  drivers/misc/pci_endpoint_test.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 3aaaf47fa4ee..414c4e55fb0a 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -280,10 +280,11 @@ static int pci_endpoint_test_bar_memcmp(struct pci_endpoint_test *test,
>  static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
>  				  enum pci_barno barno)
>  {
> -	int j, bar_size, buf_size, iters, remain;
>  	void *write_buf __free(kfree) = NULL;
>  	void *read_buf __free(kfree) = NULL;
>  	struct pci_dev *pdev = test->pdev;
> +	int j, buf_size, iters, remain;
> +	resource_size_t bar_size;
>  
>  	if (!test->bar[barno])
>  		return false;
> 

The code change itself is good.

-- 
 i.

Re: [PATCH] misc: pci_endpoint_test: fixed pci_resource_len return value out of bounds.

[Hans Zhang]

On 12/18/24 10:00, Ilpo Järvinen wrote:

> I'm not sure how out-of-bounds access would happen. On which line you see
> that possibility?
Please see #L291
https://elixir.bootlin.com/linux/v6.13-rc3/source/drivers/misc/pci_endpoint_test.c#L291

> 
>> If the bar size of the EP device exceeds
> 
> BAR size
> 
>> 4G, this bar_Size will be equal to 0.
> 
> bar_size
> 
> I think bar0 and bar1 information could simply be dropped since they're
> unrelated. I think this would be enough information:
> 
> With 8GB BAR2, running pcitest -b 2 fails with "TEST FAILED".
> 

Do I need to add the patch version?

Regards
Hans

Re: [PATCH] misc: pci_endpoint_test: fixed pci_resource_len return value out of bounds.

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1418 bytes --]

On Thu, 19 Dec 2024, Hans Zhang wrote:
> On 12/18/24 10:00, Ilpo Järvinen wrote:
> 
> > I'm not sure how out-of-bounds access would happen. On which line you see
> > that possibility?
> Please see #L291
> https://elixir.bootlin.com/linux/v6.13-rc3/source/drivers/misc/pci_endpoint_test.c#L291

Ah, so this was just a language confusion (understandable, we're not 
perfect in expressing ourselves in English, me included).

So you didn't mean "out-of-bound access" at all but that the value is too 
large to fit the int sized variable.

Could you please try to phrase it better so it's clear what you mean with 
it. And it feels that said multiple times as there's also the 2G thing 
mentioned elsewhere in the changelog. It would be nice if you could try to 
avoid such duplication. So try to structure changelog so that you first 
describe the problem and its impact. And the how the patch solves the 
issue.

> > > If the bar size of the EP device exceeds
> > 
> > BAR size
> > 
> > > 4G, this bar_Size will be equal to 0.
> > 
> > bar_size
> > 
> > I think bar0 and bar1 information could simply be dropped since they're
> > unrelated. I think this would be enough information:
> > 
> > With 8GB BAR2, running pcitest -b 2 fails with "TEST FAILED".
> > 
> 
> Do I need to add the patch version?

Yes, when you change the patch in anyway, please create a new version.

-- 
 i.

Re: [PATCH] misc: pci_endpoint_test: fixed pci_resource_len return value out of bounds.

[Hans Zhang]

On 12/19/24 07:36, Ilpo Järvinen wrote:
> 
> Yes, when you change the patch in anyway, please create a new version.
> 

Thanks Ilpo Järvinen for feedback!