A use-after-free bug may occur if init_imstt invokes framebuffer_release
and free the info ptr. The caller, imsttfb_probe didn't notice that and
still keep the ptr as private data in pdev.
If we remove the driver which will call imsttfb_remove to make cleanup,
UAF happens.
Fix it by return error code if bad case happens in init_imstt.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Zheng Wang <[email protected]>
---
v2:
- add return error code in another location.
---
drivers/video/fbdev/imsttfb.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/video/fbdev/imsttfb.c b/drivers/video/fbdev/imsttfb.c
index bea45647184e..975dd682fae4 100644
--- a/drivers/video/fbdev/imsttfb.c
+++ b/drivers/video/fbdev/imsttfb.c
@@ -1347,7 +1347,7 @@ static const struct fb_ops imsttfb_ops = {
.fb_ioctl = imsttfb_ioctl,
};
-static void init_imstt(struct fb_info *info)
+static int init_imstt(struct fb_info *info)
{
struct imstt_par *par = info->par;
__u32 i, tmp, *ip, *end;
@@ -1420,7 +1420,7 @@ static void init_imstt(struct fb_info *info)
|| !(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;
+ return -ENODEV;
}
sprintf(info->fix.id, "IMS TT (%s)", par->ramdac == IBM ? "IBM" : "TVP");
@@ -1456,12 +1456,13 @@ static void init_imstt(struct fb_info *info)
if (register_framebuffer(info) < 0) {
framebuffer_release(info);
- return;
+ return -ENODEV;
}
tmp = (read_reg_le32(par->dc_regs, SSTATUS) & 0x0f00) >> 8;
fb_info(info, "%s frame buffer; %uMB vram; chip version %u\n",
info->fix.id, info->fix.smem_len >> 20, tmp);
+ return 0;
}
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)
--
2.25.1
On 4/27/23 05:08, Zheng Wang wrote:
> A use-after-free bug may occur if init_imstt invokes framebuffer_release
> and free the info ptr. The caller, imsttfb_probe didn't notice that and
> still keep the ptr as private data in pdev.
>
> If we remove the driver which will call imsttfb_remove to make cleanup,
> UAF happens.
>
> Fix it by return error code if bad case happens in init_imstt.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Zheng Wang <[email protected]>
applied.
Thanks!
Helge
> ---
> v2:
> - add return error code in another location.
> ---
> drivers/video/fbdev/imsttfb.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/video/fbdev/imsttfb.c b/drivers/video/fbdev/imsttfb.c
> index bea45647184e..975dd682fae4 100644
> --- a/drivers/video/fbdev/imsttfb.c
> +++ b/drivers/video/fbdev/imsttfb.c
> @@ -1347,7 +1347,7 @@ static const struct fb_ops imsttfb_ops = {
> .fb_ioctl = imsttfb_ioctl,
> };
>
> -static void init_imstt(struct fb_info *info)
> +static int init_imstt(struct fb_info *info)
> {
> struct imstt_par *par = info->par;
> __u32 i, tmp, *ip, *end;
> @@ -1420,7 +1420,7 @@ static void init_imstt(struct fb_info *info)
> || !(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;
> + return -ENODEV;
> }
>
> sprintf(info->fix.id, "IMS TT (%s)", par->ramdac == IBM ? "IBM" : "TVP");
> @@ -1456,12 +1456,13 @@ static void init_imstt(struct fb_info *info)
>
> if (register_framebuffer(info) < 0) {
> framebuffer_release(info);
> - return;
> + return -ENODEV;
> }
>
> tmp = (read_reg_le32(par->dc_regs, SSTATUS) & 0x0f00) >> 8;
> fb_info(info, "%s frame buffer; %uMB vram; chip version %u\n",
> info->fix.id, info->fix.smem_len >> 20, tmp);
> + return 0;
> }
>
> 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)
Hello.
On Thu, Apr 27, 2023 at 11:08:41AM +0800, Zheng Wang <[email protected]> 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.)
Shouldn't there be something like the diff below on top of the existing code?
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);
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:
Hi Michal,
On 5/22/23 17:36, Michal Koutný wrote:
> On Thu, Apr 27, 2023 at 11:08:41AM +0800, Zheng Wang <[email protected]> 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