| Message ID | 20241217121220.19676-1-18255117159@163.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | misc: pci_endpoint_test: fixed pci_resource_len return value out of bounds. | expand |
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>
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.
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
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.
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!
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 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(-)