Re: [PATCH v2] video: imsttfb: Fix use after free bug in imsttfb_probe due to lack of error-handling of init_imstt

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Michal,

On 5/22/23 17:36, Michal Koutný wrote:
On Thu, Apr 27, 2023 at 11:08:41AM +0800, Zheng Wang <zyytlz.wz@xxxxxxx> wrote:
  static int imsttfb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
@@ -1529,10 +1530,10 @@ static int imsttfb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
  	if (!par->cmap_regs)
  		goto error;
  	info->pseudo_palette = par->palette;
-	init_imstt(info);
-
-	pci_set_drvdata(pdev, info);
-	return 0;
+	ret = init_imstt(info);
+	if (!ret)
+		pci_set_drvdata(pdev, info);
+	return ret;

  error:
  	if (par->dc_regs)

This part caught my eye -- shouldn't the -ENODEV from init_imstt go
through the standard error with proper cleanup? (It seems like a leak
from my 30000 ft view, i.e. not sure about imsttfb_{probe,remove}
pairing.)

Yes, you seem to be right.

Shouldn't there be something like the diff below on top of the existing code?

Yes, but ....


Regards,
Michal

diff --git a/drivers/video/fbdev/imsttfb.c b/drivers/video/fbdev/imsttfb.c
index 975dd682fae4..a116ac8ca020 100644
--- a/drivers/video/fbdev/imsttfb.c
+++ b/drivers/video/fbdev/imsttfb.c
@@ -1419,7 +1419,6 @@ static int init_imstt(struct fb_info *info)
  	if ((info->var.xres * info->var.yres) * (info->var.bits_per_pixel >> 3) > info->fix.smem_len
  	    || !(compute_imstt_regvals(par, info->var.xres, info->var.yres))) {
  		printk("imsttfb: %ux%ux%u not supported\n", info->var.xres, info->var.yres, info->var.bits_per_pixel);
-		framebuffer_release(info);
  		return -ENODEV;
  	}

@@ -1455,7 +1454,6 @@ static int init_imstt(struct fb_info *info)
  	fb_alloc_cmap(&info->cmap, 0, 0);

  	if (register_framebuffer(info) < 0) {
-		framebuffer_release(info);

That's ^^^  ok, but I think a
	fb_dealloc_cmap()
is missing here ...

... and in the error path of imsttfb_probe() ....

  		return -ENODEV;
  	}

@@ -1531,8 +1529,10 @@ static int imsttfb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
  		goto error;
  	info->pseudo_palette = par->palette;
  	ret = init_imstt(info);
-	if (!ret)
-		pci_set_drvdata(pdev, info);
+	if (ret)
+		goto error;
+
+	pci_set_drvdata(pdev, info);
  	return ret;

  error:

Would you mind sending a proper patch?

Thanks for noticing!
Helge




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux