[PATCH v2] video: hyperv_fb: Fix a double free in hvfb_probe

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 <redacted>
---
 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

From: Lv Yunlong <redacted> 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 <redacted>

On Wed, Mar 24, 2021 at 01:46:39PM +0000, Michael Kelley wrote:

From: Lv Yunlong <redacted> Sent: Wednesday, March 24, 2021 3:37 AM

quoted

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 <redacted>