From: Lv Yunlong <[email protected]> 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 <[email protected]>
>
> Signed-off-by: Lv Yunlong <[email protected]>
> ---
> 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
>
On Wed, Mar 24, 2021 at 01:46:39PM +0000, Michael Kelley wrote:
> From: Lv Yunlong <[email protected]> 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 <[email protected]>