| Message ID | 20240821071842.8591-9-pstanner@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: Remove pcim_iounmap_regions() | expand |
Le 21/08/2024 à 09:18, Philipp Stanner a écrit : > In psnet_open_pf_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 string 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 | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/vdpa/solidrun/snet_main.c b/drivers/vdpa/solidrun/snet_main.c > index 99428a04068d..4d42a05d70fc 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) snet_open_vf_bar() also needs the same modification (see Andy's comment on patch 8/9) CJ > { > - 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");
On Wed, Aug 21, 2024 at 09:18:40AM +0200, Philipp Stanner wrote: > In psnet_open_pf_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 string 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> I don't get why is this a part of a cleanup series - looks like an unrelated bugfix? > --- > drivers/vdpa/solidrun/snet_main.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/vdpa/solidrun/snet_main.c b/drivers/vdpa/solidrun/snet_main.c > index 99428a04068d..4d42a05d70fc 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"); > -- > 2.46.0
On Wed, 2024-08-21 at 08:12 -0400, Michael S. Tsirkin wrote: > On Wed, Aug 21, 2024 at 09:18:40AM +0200, Philipp Stanner wrote: > > In psnet_open_pf_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 string 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> > > I don't get why is this a part of a cleanup series - > looks like an unrelated bugfix? It was discovered in the discussion of v1 of this series. It indeed is an unrelated bugfix and could be merged separately. But my patch #8 depends on it. So it would be convenient to merge it into mainline through this series, and have stable just pick patch #7. Or should it be done differently, in your opinion? P. > > > > --- > > drivers/vdpa/solidrun/snet_main.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/vdpa/solidrun/snet_main.c > > b/drivers/vdpa/solidrun/snet_main.c > > index 99428a04068d..4d42a05d70fc 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"); > > -- > > 2.46.0 >
diff --git a/drivers/vdpa/solidrun/snet_main.c b/drivers/vdpa/solidrun/snet_main.c index 99428a04068d..4d42a05d70fc 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");
In psnet_open_pf_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 string 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 | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)