* [PATCH v2] video: imsttfb: Fix use after free bug in imsttfb_probe due to lack of error-handling of init_imstt
@ 2023-04-27 3:08 Zheng Wang
2023-05-11 17:03 ` Helge Deller
2023-05-22 15:36 ` Michal Koutný
0 siblings, 2 replies; 7+ messages in thread
From: Zheng Wang @ 2023-04-27 3:08 UTC (permalink / raw)
To: deller
Cc: javierm, tzimmermann, linux-fbdev, dri-devel, linux-kernel,
hackerzheng666, 1395428693sheep, alex000young, Zheng Wang
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 <zyytlz.wz@163.com>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] video: imsttfb: Fix use after free bug in imsttfb_probe due to lack of error-handling of init_imstt
2023-04-27 3:08 [PATCH v2] video: imsttfb: Fix use after free bug in imsttfb_probe due to lack of error-handling of init_imstt Zheng Wang
@ 2023-05-11 17:03 ` Helge Deller
2023-05-22 15:36 ` Michal Koutný
1 sibling, 0 replies; 7+ messages in thread
From: Helge Deller @ 2023-05-11 17:03 UTC (permalink / raw)
To: Zheng Wang
Cc: javierm, tzimmermann, linux-fbdev, dri-devel, linux-kernel,
hackerzheng666, 1395428693sheep, alex000young
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 <zyytlz.wz@163.com>
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)
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] video: imsttfb: Fix use after free bug in imsttfb_probe due to lack of error-handling of init_imstt
2023-04-27 3:08 [PATCH v2] video: imsttfb: Fix use after free bug in imsttfb_probe due to lack of error-handling of init_imstt Zheng Wang
2023-05-11 17:03 ` Helge Deller
@ 2023-05-22 15:36 ` Michal Koutný
2023-05-22 18:02 ` Helge Deller
1 sibling, 1 reply; 7+ messages in thread
From: Michal Koutný @ 2023-05-22 15:36 UTC (permalink / raw)
To: Zheng Wang
Cc: deller, alex000young, linux-fbdev, linux-kernel, hackerzheng666,
dri-devel, javierm, 1395428693sheep, tzimmermann
[-- Attachment #1: Type: text/plain, Size: 2032 bytes --]
Hello.
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.)
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:
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] video: imsttfb: Fix use after free bug in imsttfb_probe due to lack of error-handling of init_imstt
2023-05-22 15:36 ` Michal Koutný
@ 2023-05-22 18:02 ` Helge Deller
[not found] ` <97807a2d-ccf2-1fbf-06f7-085bb1bdf451@web.de>
0 siblings, 1 reply; 7+ messages in thread
From: Helge Deller @ 2023-05-22 18:02 UTC (permalink / raw)
To: Michal Koutný, Zheng Wang
Cc: alex000young, linux-fbdev, linux-kernel, hackerzheng666,
dri-devel, javierm, 1395428693sheep, tzimmermann
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 3/4] fbdev: imsttfb: Move a variable assignment for an error code in imsttfb_probe()
[not found] ` <38cd2e4b-3df4-6c69-b790-5762d24e2c29@web.de>
@ 2023-05-24 18:11 ` Helge Deller
0 siblings, 0 replies; 7+ messages in thread
From: Helge Deller @ 2023-05-24 18:11 UTC (permalink / raw)
To: Markus Elfring, kernel-janitors, linux-fbdev, dri-devel,
Javier Martinez Canillas, Michal Koutný,
Thomas Zimmermann, Zheng Wang
Cc: LKML, cocci, 1395428693sheep, alex000young, hackerzheng666
On 5/23/23 19:42, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 23 May 2023 18:30:26 +0200
>
> The value “-ENOMEM” was assigned to the variable “ret”
> at the beginning.
> Move this statement directly before the first ioremap() call.
Please do not move such variables without real need.
It makes backporting (of this and maybe follow-up patches) much more
complicated and the compiler will optimize it anyway.
Thanks!
Helge
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> drivers/video/fbdev/imsttfb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/imsttfb.c b/drivers/video/fbdev/imsttfb.c
> index 6490f544f8eb..90c72428e8d7 100644
> --- a/drivers/video/fbdev/imsttfb.c
> +++ b/drivers/video/fbdev/imsttfb.c
> @@ -1484,7 +1484,6 @@ static int imsttfb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> ret = aperture_remove_conflicting_pci_devices(pdev, "imsttfb");
> if (ret)
> return ret;
> - ret = -ENOMEM;
>
> dp = pci_device_to_OF_node(pdev);
> if(dp)
> @@ -1525,6 +1524,7 @@ static int imsttfb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> }
>
> info->fix.smem_start = addr;
> + ret = -ENOMEM;
> info->screen_base = (__u8 *)ioremap(addr, par->ramdac == IBM ?
> 0x400000 : 0x800000);
> if (!info->screen_base)
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/4] fbdev: imsttfb: Fix error handling in init_imstt()
[not found] ` <c551c670-7458-ed50-eb2f-5a2b7ba421a8@web.de>
@ 2023-05-24 18:20 ` Helge Deller
[not found] ` <1d7228fb-f1f8-364c-aa29-5719a9da1fc6@web.de>
0 siblings, 1 reply; 7+ messages in thread
From: Helge Deller @ 2023-05-24 18:20 UTC (permalink / raw)
To: Markus Elfring, kernel-janitors, linux-fbdev, dri-devel,
Javier Martinez Canillas, Michal Koutný,
Thomas Zimmermann, Zheng Wang
Cc: LKML, cocci, 1395428693sheep, alex000young, hackerzheng666
On 5/23/23 19:38, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 23 May 2023 14:32:39 +0200
>
> The return value was overlooked from a call of
> the function “fb_alloc_cmap”.
>
> * Thus use a corresponding error check.
>
> * Add two jump targets so that a bit of exception handling
> can be better reused at the end of this function.
>
>
> Reported-by: Helge Deller <deller@gmx.de>
> Link: https://lore.kernel.org/dri-devel/069f2f78-01f3-9476-d860-2b695c122649@gmx.de/
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Fixes: c75f5a550610 ("fbdev: imsttfb: Fix use after free bug in imsttfb_probe")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> drivers/video/fbdev/imsttfb.c | 18 +++++++++++++-----
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/video/fbdev/imsttfb.c b/drivers/video/fbdev/imsttfb.c
> index 975dd682fae4..d3532def4707 100644
> --- a/drivers/video/fbdev/imsttfb.c
> +++ b/drivers/video/fbdev/imsttfb.c
> @@ -1351,6 +1351,7 @@ static int init_imstt(struct fb_info *info)
> {
> struct imstt_par *par = info->par;
> __u32 i, tmp, *ip, *end;
> + int ret;
>
> tmp = read_reg_le32(par->dc_regs, PRC);
> if (par->ramdac == IBM)
> @@ -1419,8 +1420,7 @@ 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;
> + goto e_nodev;
> }
>
> sprintf(info->fix.id, "IMS TT (%s)", par->ramdac == IBM ? "IBM" : "TVP");
> @@ -1452,17 +1452,25 @@ static int init_imstt(struct fb_info *info)
> FBINFO_HWACCEL_FILLRECT |
> FBINFO_HWACCEL_YPAN;
>
> - fb_alloc_cmap(&info->cmap, 0, 0);
> + ret = fb_alloc_cmap(&info->cmap, 0, 0);
> + if (ret)
> + goto release_framebuffer;
>
> if (register_framebuffer(info) < 0) {
> - framebuffer_release(info);
> - return -ENODEV;
> + fb_dealloc_cmap(&info->cmap);
> + goto e_nodev;
> }
>
> 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;
> +
> +e_nodev:
> + ret = -ENODEV;
I think the return value isn't checked at all, so you could
simply return below "-ENODEV" for all cases (instead of "return ret").
Then you don't need the e_nodev label and can simplify the flow.
Helge
> +release_framebuffer:
> + framebuffer_release(info);
> + return ret;
> }
>
> static int imsttfb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/4] fbdev: imsttfb: Fix error handling in init_imstt()
[not found] ` <1d7228fb-f1f8-364c-aa29-5719a9da1fc6@web.de>
@ 2023-05-25 17:39 ` Helge Deller
0 siblings, 0 replies; 7+ messages in thread
From: Helge Deller @ 2023-05-25 17:39 UTC (permalink / raw)
To: Markus Elfring, kernel-janitors, linux-fbdev, dri-devel,
Javier Martinez Canillas, Michal Koutný,
Thomas Zimmermann, Zheng Wang
Cc: LKML, cocci, 1395428693sheep, alex000young, hackerzheng666
On 5/25/23 07:33, Markus Elfring wrote:
>>> The return value was overlooked from a call of
>>> the function “fb_alloc_cmap”.
>>>
>>> * Thus use a corresponding error check.
>>>
>>> * Add two jump targets so that a bit of exception handling
>>> can be better reused at the end of this function.
> …
>>> +++ b/drivers/video/fbdev/imsttfb.c
> …
>>> @@ -1452,17 +1452,25 @@ static int init_imstt(struct fb_info *info)
>>> FBINFO_HWACCEL_FILLRECT |
>>> FBINFO_HWACCEL_YPAN;
>>>
>>> - fb_alloc_cmap(&info->cmap, 0, 0);
>>> + ret = fb_alloc_cmap(&info->cmap, 0, 0);
>>> + if (ret)
>>> + goto release_framebuffer;
>>>
>>> if (register_framebuffer(info) < 0) {
>>> - framebuffer_release(info);
>>> - return -ENODEV;
>>> + fb_dealloc_cmap(&info->cmap);
>>> + goto e_nodev;
>>> }
>>>
>>> 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;
>>> +
>>> +e_nodev:
>>> + ret = -ENODEV;
>>
>> I think the return value isn't checked at all, so you could
>> simply return below "-ENODEV" for all cases (instead of "return ret").
>> Then you don't need the e_nodev label and can simplify the flow.
>
> Can it be helpful to distinguish involved error codes better?
No.
Helge
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-05-25 17:39 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-27 3:08 [PATCH v2] video: imsttfb: Fix use after free bug in imsttfb_probe due to lack of error-handling of init_imstt Zheng Wang
2023-05-11 17:03 ` Helge Deller
2023-05-22 15:36 ` Michal Koutný
2023-05-22 18:02 ` Helge Deller
[not found] ` <97807a2d-ccf2-1fbf-06f7-085bb1bdf451@web.de>
[not found] ` <38cd2e4b-3df4-6c69-b790-5762d24e2c29@web.de>
2023-05-24 18:11 ` [PATCH 3/4] fbdev: imsttfb: Move a variable assignment for an error code in imsttfb_probe() Helge Deller
[not found] ` <c551c670-7458-ed50-eb2f-5a2b7ba421a8@web.de>
2023-05-24 18:20 ` [PATCH 1/4] fbdev: imsttfb: Fix error handling in init_imstt() Helge Deller
[not found] ` <1d7228fb-f1f8-364c-aa29-5719a9da1fc6@web.de>
2023-05-25 17:39 ` Helge Deller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).