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) {
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?
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.
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).
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
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...
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
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...
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
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
linux-stable-mirror@lists.linaro.org
-
Andy Shevchenko -
Andy Shevchenko -
Greg KH -
Philipp Stanner -
Stefano Garzarella