Messages in this thread Patch in this message | | | Date | Mon, 22 May 2023 20:02:27 +0200 | Subject | Re: [PATCH v2] video: imsttfb: Fix use after free bug in imsttfb_probe due to lack of error-handling of init_imstt | From | Helge Deller <> |
| |
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@163.com> 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
| |