[PATCH RESEND] vdpa: solidrun: Fix UB bug with devres

[PATCH RESEND] vdpa: solidrun: Fix UB bug with devres

[Philipp Stanner]

In psnet_open_pf_bar() and snet_open_vf_bar() a string later passed to
pcim_iomap_regions() is placed on the stack. Neither
pcim_iomap_regions() nor the functions it calls copy that string.

Should the string later ever be used, this, consequently, causes
undefined behavior since the stack frame will by then have disappeared.

Fix the bug by allocating the strings on the heap through
devm_kasprintf().

Cc: stable@vger.kernel.org	# v6.3
Fixes: 51a8f9d7f587 ("virtio: vdpa: new SolidNET DPU driver.")
Reported-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Closes: https://lore.kernel.org/all/74e9109a-ac59-49e2-9b1d-d825c9c9f891@wanadoo.fr/
Suggested-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 drivers/vdpa/solidrun/snet_main.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/vdpa/solidrun/snet_main.c b/drivers/vdpa/solidrun/snet_main.c
index 99428a04068d..c8b74980dbd1 100644
--- a/drivers/vdpa/solidrun/snet_main.c
+++ b/drivers/vdpa/solidrun/snet_main.c
@@ -555,7 +555,7 @@ static const struct vdpa_config_ops snet_config_ops = {
 
 static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet *psnet)
 {
-	char name[50];
+	char *name;
 	int ret, i, mask = 0;
 	/* We don't know which BAR will be used to communicate..
 	 * We will map every bar with len > 0.
@@ -573,7 +573,10 @@ static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet *psnet)
 		return -ENODEV;
 	}
 
-	snprintf(name, sizeof(name), "psnet[%s]-bars", pci_name(pdev));
+	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "psnet[%s]-bars", pci_name(pdev));
+	if (!name)
+		return -ENOMEM;
+
 	ret = pcim_iomap_regions(pdev, mask, name);
 	if (ret) {
 		SNET_ERR(pdev, "Failed to request and map PCI BARs\n");
@@ -590,10 +593,13 @@ static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet *psnet)
 
 static int snet_open_vf_bar(struct pci_dev *pdev, struct snet *snet)
 {
-	char name[50];
+	char *name;
 	int ret;
 
-	snprintf(name, sizeof(name), "snet[%s]-bar", pci_name(pdev));
+	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "snet[%s]-bars", pci_name(pdev));
+	if (!name)
+		return -ENOMEM;
+
 	/* Request and map BAR */
 	ret = pcim_iomap_regions(pdev, BIT(snet->psnet->cfg.vf_bar), name);
 	if (ret) {
-- 
2.46.1

Re: [PATCH RESEND] vdpa: solidrun: Fix UB bug with devres

[Andy Shevchenko]

On Wed, Oct 16, 2024 at 09:25:54AM +0200, Philipp Stanner wrote:
> In psnet_open_pf_bar() and snet_open_vf_bar() a string later passed to
> pcim_iomap_regions() is placed on the stack. Neither
> pcim_iomap_regions() nor the functions it calls copy that string.
> 
> Should the string later ever be used, this, consequently, causes
> undefined behavior since the stack frame will by then have disappeared.
> 
> Fix the bug by allocating the strings on the heap through
> devm_kasprintf().

> ---

I haven't found the reason for resending. Can you elaborate here?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RESEND] vdpa: solidrun: Fix UB bug with devres

[Philipp Stanner]

On Wed, 2024-10-16 at 12:08 +0300, Andy Shevchenko wrote:
> On Wed, Oct 16, 2024 at 09:25:54AM +0200, Philipp Stanner wrote:
> > In psnet_open_pf_bar() and snet_open_vf_bar() a string later passed
> > to
> > pcim_iomap_regions() is placed on the stack. Neither
> > pcim_iomap_regions() nor the functions it calls copy that string.
> > 
> > Should the string later ever be used, this, consequently, causes
> > undefined behavior since the stack frame will by then have
> > disappeared.
> > 
> > Fix the bug by allocating the strings on the heap through
> > devm_kasprintf().
> 
> > ---
> 
> I haven't found the reason for resending. Can you elaborate here?

Impatience ;p

This is not a v2.

I mean, it's a bug, easy to fix and merge [and it's blocking my other
PCI work, *cough*]. Should contributors wait longer than 8 days until
resending in your opinion?


P.

Re: [PATCH RESEND] vdpa: solidrun: Fix UB bug with devres

[Andy Shevchenko]

On Wed, Oct 16, 2024 at 12:22 PM Philipp Stanner <pstanner@redhat.com> wrote:
> On Wed, 2024-10-16 at 12:08 +0300, Andy Shevchenko wrote:
> > On Wed, Oct 16, 2024 at 09:25:54AM +0200, Philipp Stanner wrote:

...

> > > ---
> >
> > I haven't found the reason for resending. Can you elaborate here?
>
> Impatience ;p
>
> This is not a v2.

It doesn't matter, the reviewers and maintainers should get a clue
which version is to be used (even if it's a simple resend) and why it
has been sent.

> I mean, it's a bug, easy to fix and merge [and it's blocking my other
> PCI work, *cough*]. Should contributors wait longer than 8 days until
> resending in your opinion?

Usually we may ping for that. And yeah, depending on the maintainers
it takes a while to get it to the point. In some (quite rare I
believe) cases we may escalate up to Linus about this. I probably has
only one case like that in my full period of working on the Linux
kernel project (several years).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RESEND] vdpa: solidrun: Fix UB bug with devres

[Greg KH]

On Wed, Oct 16, 2024 at 11:22:48AM +0200, Philipp Stanner wrote:
> On Wed, 2024-10-16 at 12:08 +0300, Andy Shevchenko wrote:
> > On Wed, Oct 16, 2024 at 09:25:54AM +0200, Philipp Stanner wrote:
> > > In psnet_open_pf_bar() and snet_open_vf_bar() a string later passed
> > > to
> > > pcim_iomap_regions() is placed on the stack. Neither
> > > pcim_iomap_regions() nor the functions it calls copy that string.
> > > 
> > > Should the string later ever be used, this, consequently, causes
> > > undefined behavior since the stack frame will by then have
> > > disappeared.
> > > 
> > > Fix the bug by allocating the strings on the heap through
> > > devm_kasprintf().
> > 
> > > ---
> > 
> > I haven't found the reason for resending. Can you elaborate here?
> 
> Impatience ;p
> 
> This is not a v2.
> 
> I mean, it's a bug, easy to fix and merge [and it's blocking my other
> PCI work, *cough*]. Should contributors wait longer than 8 days until
> resending in your opinion?

2 weeks is normally the expected response time, but each subsystem might
have other time limites, the documentation should show those that do.

While you wait, take the time to review other pending patches for that
maintainer, that will ensure that your patches move to the top as they
will be the only ones remaining.

thanks,

greg k-h

Re: [PATCH RESEND] vdpa: solidrun: Fix UB bug with devres

[Philipp Stanner]

On Wed, 2024-10-16 at 12:51 +0200, Greg KH wrote:
> On Wed, Oct 16, 2024 at 11:22:48AM +0200, Philipp Stanner wrote:
> > On Wed, 2024-10-16 at 12:08 +0300, Andy Shevchenko wrote:
> > > On Wed, Oct 16, 2024 at 09:25:54AM +0200, Philipp Stanner wrote:
> > > > In psnet_open_pf_bar() and snet_open_vf_bar() a string later
> > > > passed
> > > > to
> > > > pcim_iomap_regions() is placed on the stack. Neither
> > > > pcim_iomap_regions() nor the functions it calls copy that
> > > > string.
> > > > 
> > > > Should the string later ever be used, this, consequently,
> > > > causes
> > > > undefined behavior since the stack frame will by then have
> > > > disappeared.
> > > > 
> > > > Fix the bug by allocating the strings on the heap through
> > > > devm_kasprintf().
> > > 
> > > > ---
> > > 
> > > I haven't found the reason for resending. Can you elaborate here?
> > 
> > Impatience ;p
> > 
> > This is not a v2.
> > 
> > I mean, it's a bug, easy to fix and merge [and it's blocking my
> > other
> > PCI work, *cough*]. Should contributors wait longer than 8 days
> > until
> > resending in your opinion?
> 
> 2 weeks is normally the expected response time, but each subsystem
> might
> have other time limites, the documentation should show those that do.

Where do we document that?

Regarding resend intervals, the official guide line is contradictory:
"You should receive comments within a few weeks (typically 2-3)" <->
"Wait for a minimum of one week before resubmitting or pinging
reviewers" <--> "It’s also ok to resend the patch or the patch series
after a couple of weeks"

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#don-t-get-discouraged-or-impatient


We could make the docu more consistent and specify 2 weeks as the
minimum time.

P.


> 
> While you wait, take the time to review other pending patches for
> that
> maintainer, that will ensure that your patches move to the top as
> they
> will be the only ones remaining.
> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH RESEND] vdpa: solidrun: Fix UB bug with devres

[Greg KH]

On Wed, Oct 16, 2024 at 01:16:32PM +0200, Philipp Stanner wrote:
> On Wed, 2024-10-16 at 12:51 +0200, Greg KH wrote:
> > On Wed, Oct 16, 2024 at 11:22:48AM +0200, Philipp Stanner wrote:
> > > On Wed, 2024-10-16 at 12:08 +0300, Andy Shevchenko wrote:
> > > > On Wed, Oct 16, 2024 at 09:25:54AM +0200, Philipp Stanner wrote:
> > > > > In psnet_open_pf_bar() and snet_open_vf_bar() a string later
> > > > > passed
> > > > > to
> > > > > pcim_iomap_regions() is placed on the stack. Neither
> > > > > pcim_iomap_regions() nor the functions it calls copy that
> > > > > string.
> > > > > 
> > > > > Should the string later ever be used, this, consequently,
> > > > > causes
> > > > > undefined behavior since the stack frame will by then have
> > > > > disappeared.
> > > > > 
> > > > > Fix the bug by allocating the strings on the heap through
> > > > > devm_kasprintf().
> > > > 
> > > > > ---
> > > > 
> > > > I haven't found the reason for resending. Can you elaborate here?
> > > 
> > > Impatience ;p
> > > 
> > > This is not a v2.
> > > 
> > > I mean, it's a bug, easy to fix and merge [and it's blocking my
> > > other
> > > PCI work, *cough*]. Should contributors wait longer than 8 days
> > > until
> > > resending in your opinion?
> > 
> > 2 weeks is normally the expected response time, but each subsystem
> > might
> > have other time limites, the documentation should show those that do.
> 
> Where do we document that?

Documentation/process/maintainer-*

> Regarding resend intervals, the official guide line is contradictory:
> "You should receive comments within a few weeks (typically 2-3)" <->
> "Wait for a minimum of one week before resubmitting or pinging
> reviewers" <--> "It’s also ok to resend the patch or the patch series
> after a couple of weeks"
> 
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#don-t-get-discouraged-or-impatient
> 
> 
> We could make the docu more consistent and specify 2 weeks as the
> minimum time.

Trying to tell other people what they are required to do, when you don't
pay them, is going to be a bit difficult :)

Just leave it as-is, and again, take the time to do reviews for the
maintainers you are trying to get patches accepted for.  That's the
simplest way to make forward progress faster.

good luck!

greg k-h

Re: [PATCH RESEND] vdpa: solidrun: Fix UB bug with devres

[Stefano Garzarella]

On Wed, Oct 16, 2024 at 09:25:54AM +0200, Philipp Stanner wrote:
>In psnet_open_pf_bar() and snet_open_vf_bar() a string later passed to
>pcim_iomap_regions() is placed on the stack. Neither
>pcim_iomap_regions() nor the functions it calls copy that string.
>
>Should the string later ever be used, this, consequently, causes
>undefined behavior since the stack frame will by then have disappeared.
>
>Fix the bug by allocating the strings on the heap through
>devm_kasprintf().
>
>Cc: stable@vger.kernel.org	# v6.3
>Fixes: 51a8f9d7f587 ("virtio: vdpa: new SolidNET DPU driver.")
>Reported-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>Closes: https://lore.kernel.org/all/74e9109a-ac59-49e2-9b1d-d825c9c9f891@wanadoo.fr/
>Suggested-by: Andy Shevchenko <andy@kernel.org>
>Signed-off-by: Philipp Stanner <pstanner@redhat.com>
>---
> drivers/vdpa/solidrun/snet_main.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)

LGTM!

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

>
>diff --git a/drivers/vdpa/solidrun/snet_main.c b/drivers/vdpa/solidrun/snet_main.c
>index 99428a04068d..c8b74980dbd1 100644
>--- a/drivers/vdpa/solidrun/snet_main.c
>+++ b/drivers/vdpa/solidrun/snet_main.c
>@@ -555,7 +555,7 @@ static const struct vdpa_config_ops snet_config_ops = {
>
> static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet *psnet)
> {
>-	char name[50];
>+	char *name;
> 	int ret, i, mask = 0;
> 	/* We don't know which BAR will be used to communicate..
> 	 * We will map every bar with len > 0.
>@@ -573,7 +573,10 @@ static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet *psnet)
> 		return -ENODEV;
> 	}
>
>-	snprintf(name, sizeof(name), "psnet[%s]-bars", pci_name(pdev));
>+	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "psnet[%s]-bars", pci_name(pdev));
>+	if (!name)
>+		return -ENOMEM;
>+
> 	ret = pcim_iomap_regions(pdev, mask, name);
> 	if (ret) {
> 		SNET_ERR(pdev, "Failed to request and map PCI BARs\n");
>@@ -590,10 +593,13 @@ static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet *psnet)
>
> static int snet_open_vf_bar(struct pci_dev *pdev, struct snet *snet)
> {
>-	char name[50];
>+	char *name;
> 	int ret;
>
>-	snprintf(name, sizeof(name), "snet[%s]-bar", pci_name(pdev));
>+	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "snet[%s]-bars", pci_name(pdev));
>+	if (!name)
>+		return -ENOMEM;
>+
> 	/* Request and map BAR */
> 	ret = pcim_iomap_regions(pdev, BIT(snet->psnet->cfg.vf_bar), name);
> 	if (ret) {
>-- 
>2.46.1
>
>