[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <069f2f78-01f3-9476-d860-2b695c122649@gmx.de>
Date: Mon, 22 May 2023 20:02:27 +0200
From: Helge Deller <deller@....de>
To: Michal Koutný <mkoutny@...e.com>,
Zheng Wang <zyytlz.wz@....com>
Cc: alex000young@...il.com, linux-fbdev@...r.kernel.org,
linux-kernel@...r.kernel.org, hackerzheng666@...il.com,
dri-devel@...ts.freedesktop.org, javierm@...hat.com,
1395428693sheep@...il.com, tzimmermann@...e.de
Subject: Re: [PATCH v2] video: imsttfb: Fix use after free bug in
imsttfb_probe due to lack of error-handling of init_imstt
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@....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
Powered by blists - more mailing lists