* [PATCH v2] video: hyperv_fb: Fix a double free in hvfb_probe
@ 2021-03-24 10:37 Lv Yunlong
2021-03-24 13:46 ` Michael Kelley
0 siblings, 1 reply; 3+ messages in thread
From: Lv Yunlong @ 2021-03-24 10:37 UTC (permalink / raw)
To: kys, haiyangz, sthemmin, wei.liu
Cc: linux-hyperv, dri-devel, linux-fbdev, linux-kernel, Lv Yunlong
In function hvfb_probe in hyperv_fb.c, it calls hvfb_getmem(hdev, info)
and return err when info->apertures is freed.
In the error1 label of hvfb_probe, info->apertures will be freed for the
second time in framebuffer_release(info).
My patch removes all kfree(info->apertures) instead of set info->apertures
to NULL. It is because that let framebuffer_release() handle freeing the
memory flows the fbdev pattern, and less code overall.
Signed-off-by: Lv Yunlong <lyl2019@mail.ustc.edu.cn>
---
drivers/video/fbdev/hyperv_fb.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
index c8b0ae676809..4dc9077dd2ac 100644
--- a/drivers/video/fbdev/hyperv_fb.c
+++ b/drivers/video/fbdev/hyperv_fb.c
@@ -1031,7 +1031,6 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
PCI_DEVICE_ID_HYPERV_VIDEO, NULL);
if (!pdev) {
pr_err("Unable to find PCI Hyper-V video\n");
- kfree(info->apertures);
return -ENODEV;
}
@@ -1129,7 +1128,6 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
} else {
pci_dev_put(pdev);
}
- kfree(info->apertures);
return 0;
@@ -1141,7 +1139,6 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
err1:
if (!gen2vm)
pci_dev_put(pdev);
- kfree(info->apertures);
return -ENOMEM;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* RE: [PATCH v2] video: hyperv_fb: Fix a double free in hvfb_probe
2021-03-24 10:37 [PATCH v2] video: hyperv_fb: Fix a double free in hvfb_probe Lv Yunlong
@ 2021-03-24 13:46 ` Michael Kelley
2021-03-25 13:30 ` Wei Liu
0 siblings, 1 reply; 3+ messages in thread
From: Michael Kelley @ 2021-03-24 13:46 UTC (permalink / raw)
To: Lv Yunlong, KY Srinivasan, Haiyang Zhang, Stephen Hemminger, wei.liu
Cc: linux-hyperv, dri-devel, linux-fbdev, linux-kernel
From: Lv Yunlong <lyl2019@mail.ustc.edu.cn> Sent: Wednesday, March 24, 2021 3:37 AM
>
> In function hvfb_probe in hyperv_fb.c, it calls hvfb_getmem(hdev, info)
> and return err when info->apertures is freed.
>
> In the error1 label of hvfb_probe, info->apertures will be freed for the
> second time in framebuffer_release(info).
>
> My patch removes all kfree(info->apertures) instead of set info->apertures
> to NULL. It is because that let framebuffer_release() handle freeing the
> memory flows the fbdev pattern, and less code overall.
Let me suggest some clarifications in the commit message. It's probably
better not to reference the initial approach of setting info->apertures to
NULL, since there won't be any record of that approach in the commit
history. Here's what I would suggest:
Function hvfb_probe() calls hvfb_getmem(), expecting upon return that
info->apertures is either NULL or points to memory that should be freed
by framebuffer_release(). But hvfb_getmem() is freeing the memory and
leaving the pointer non-NULL, resulting in a double free if an error
occurs or later if hvfb_remove() is called.
Fix this by removing all kfree(info->apertures) calls in hvfb_getmem().
This will allow framebuffer_release() to free the memory, which follows
the pattern of other fbdev drivers.
Modulo this revision to the commit message, which Wei Liu can
probably incorporate,
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
>
> Signed-off-by: Lv Yunlong <lyl2019@mail.ustc.edu.cn>
> ---
> drivers/video/fbdev/hyperv_fb.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
> index c8b0ae676809..4dc9077dd2ac 100644
> --- a/drivers/video/fbdev/hyperv_fb.c
> +++ b/drivers/video/fbdev/hyperv_fb.c
> @@ -1031,7 +1031,6 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info
> *info)
> PCI_DEVICE_ID_HYPERV_VIDEO, NULL);
> if (!pdev) {
> pr_err("Unable to find PCI Hyper-V video\n");
> - kfree(info->apertures);
> return -ENODEV;
> }
>
> @@ -1129,7 +1128,6 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info
> *info)
> } else {
> pci_dev_put(pdev);
> }
> - kfree(info->apertures);
>
> return 0;
>
> @@ -1141,7 +1139,6 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info
> *info)
> err1:
> if (!gen2vm)
> pci_dev_put(pdev);
> - kfree(info->apertures);
>
> return -ENOMEM;
> }
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] video: hyperv_fb: Fix a double free in hvfb_probe
2021-03-24 13:46 ` Michael Kelley
@ 2021-03-25 13:30 ` Wei Liu
0 siblings, 0 replies; 3+ messages in thread
From: Wei Liu @ 2021-03-25 13:30 UTC (permalink / raw)
To: Michael Kelley
Cc: Lv Yunlong, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
wei.liu, linux-hyperv, dri-devel, linux-fbdev, linux-kernel
On Wed, Mar 24, 2021 at 01:46:39PM +0000, Michael Kelley wrote:
> From: Lv Yunlong <lyl2019@mail.ustc.edu.cn> Sent: Wednesday, March 24, 2021 3:37 AM
> >
> > In function hvfb_probe in hyperv_fb.c, it calls hvfb_getmem(hdev, info)
> > and return err when info->apertures is freed.
> >
> > In the error1 label of hvfb_probe, info->apertures will be freed for the
> > second time in framebuffer_release(info).
> >
> > My patch removes all kfree(info->apertures) instead of set info->apertures
> > to NULL. It is because that let framebuffer_release() handle freeing the
> > memory flows the fbdev pattern, and less code overall.
>
> Let me suggest some clarifications in the commit message. It's probably
> better not to reference the initial approach of setting info->apertures to
> NULL, since there won't be any record of that approach in the commit
> history. Here's what I would suggest:
>
> Function hvfb_probe() calls hvfb_getmem(), expecting upon return that
> info->apertures is either NULL or points to memory that should be freed
> by framebuffer_release(). But hvfb_getmem() is freeing the memory and
> leaving the pointer non-NULL, resulting in a double free if an error
> occurs or later if hvfb_remove() is called.
>
> Fix this by removing all kfree(info->apertures) calls in hvfb_getmem().
> This will allow framebuffer_release() to free the memory, which follows
> the pattern of other fbdev drivers.
>
> Modulo this revision to the commit message, which Wei Liu can
> probably incorporate,
>
Yes. I surely can incorporate the changes.
I will also add the Fixes tag.
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-03-25 13:31 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-24 10:37 [PATCH v2] video: hyperv_fb: Fix a double free in hvfb_probe Lv Yunlong
2021-03-24 13:46 ` Michael Kelley
2021-03-25 13:30 ` Wei Liu
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).