| Message ID | 20210323073350.17697-1-lyl2019@mail.ustc.edu.cn (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | video/fbdev: Fix a double free in hvfb_probe | expand |
Thanks for your patch. I would like to change the prefix to "video: hyperv_fb:" to be more specific. On Tue, Mar 23, 2021 at 12:33:50AM -0700, Lv Yunlong wrote: > 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 twice > by framebuffer_release(info). > I would say "freed for the second time" here. What you wrote reads to me fraembuffer_release frees the buffer twice all by itself. > My patch sets info->apertures to NULL after it was freed to avoid > double free. > I think this approach works. I would like to give other people a chance to comment though. Fixes: 3a6fb6c4255c ("video: hyperv: hyperv_fb: Use physical memory for fb on HyperV Gen 1 VMs.") > Signed-off-by: Lv Yunlong <lyl2019@mail.ustc.edu.cn> > --- > drivers/video/fbdev/hyperv_fb.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c > index c8b0ae676809..2fc9b507e73a 100644 > --- a/drivers/video/fbdev/hyperv_fb.c > +++ b/drivers/video/fbdev/hyperv_fb.c > @@ -1032,6 +1032,7 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info) > if (!pdev) { > pr_err("Unable to find PCI Hyper-V video\n"); > kfree(info->apertures); > + info->apertures = NULL; > return -ENODEV; > } > > @@ -1130,6 +1131,7 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info) > pci_dev_put(pdev); > } > kfree(info->apertures); > + info->apertures = NULL; > > return 0; > > @@ -1142,6 +1144,7 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info) > if (!gen2vm) > pci_dev_put(pdev); > kfree(info->apertures); > + info->apertures = NULL; > > return -ENOMEM; > } > -- > 2.25.1 > >
From: Lv Yunlong <lyl2019@mail.ustc.edu.cn> Sent: Tuesday, March 23, 2021 12:34 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 twice > by framebuffer_release(info). > > My patch sets info->apertures to NULL after it was freed to avoid > double free. > > Signed-off-by: Lv Yunlong <lyl2019@mail.ustc.edu.cn> > --- > drivers/video/fbdev/hyperv_fb.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c > index c8b0ae676809..2fc9b507e73a 100644 > --- a/drivers/video/fbdev/hyperv_fb.c > +++ b/drivers/video/fbdev/hyperv_fb.c > @@ -1032,6 +1032,7 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info > *info) > if (!pdev) { > pr_err("Unable to find PCI Hyper-V video\n"); > kfree(info->apertures); > + info->apertures = NULL; > return -ENODEV; > } > > @@ -1130,6 +1131,7 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info > *info) > pci_dev_put(pdev); > } > kfree(info->apertures); > + info->apertures = NULL; > > return 0; > > @@ -1142,6 +1144,7 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info > *info) > if (!gen2vm) > pci_dev_put(pdev); > kfree(info->apertures); > + info->apertures = NULL; > > return -ENOMEM; > } > -- > 2.25.1 > While I think this works, a slightly better solution might be to remove all calls to kfree(info->apertures) in hvfb_getmem(), and just let framebuffer_release() handle freeing the memory. That's what is done in other drivers that follow the fbdev pattern, and it's less code overall. Michael
> -----原始邮件----- > 发件人: "Michael Kelley" <mikelley@microsoft.com> > 发送时间: 2021-03-24 02:52:07 (星期三) > 收件人: "Lv Yunlong" <lyl2019@mail.ustc.edu.cn>, "KY Srinivasan" <kys@microsoft.com>, "Haiyang Zhang" <haiyangz@microsoft.com>, "Stephen Hemminger" <sthemmin@microsoft.com>, "wei.liu@kernel.org" <wei.liu@kernel.org> > 抄送: "linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>, "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>, "linux-fbdev@vger.kernel.org" <linux-fbdev@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org> > 主题: RE: [PATCH] video/fbdev: Fix a double free in hvfb_probe > > From: Lv Yunlong <lyl2019@mail.ustc.edu.cn> Sent: Tuesday, March 23, 2021 12:34 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 twice > > by framebuffer_release(info). > > > > My patch sets info->apertures to NULL after it was freed to avoid > > double free. > > > > Signed-off-by: Lv Yunlong <lyl2019@mail.ustc.edu.cn> > > --- > > drivers/video/fbdev/hyperv_fb.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c > > index c8b0ae676809..2fc9b507e73a 100644 > > --- a/drivers/video/fbdev/hyperv_fb.c > > +++ b/drivers/video/fbdev/hyperv_fb.c > > @@ -1032,6 +1032,7 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info > > *info) > > if (!pdev) { > > pr_err("Unable to find PCI Hyper-V video\n"); > > kfree(info->apertures); > > + info->apertures = NULL; > > return -ENODEV; > > } > > > > @@ -1130,6 +1131,7 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info > > *info) > > pci_dev_put(pdev); > > } > > kfree(info->apertures); > > + info->apertures = NULL; > > > > return 0; > > > > @@ -1142,6 +1144,7 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info > > *info) > > if (!gen2vm) > > pci_dev_put(pdev); > > kfree(info->apertures); > > + info->apertures = NULL; > > > > return -ENOMEM; > > } > > -- > > 2.25.1 > > > > While I think this works, a slightly better solution might be to remove > all calls to kfree(info->apertures) in hvfb_getmem(), and just let > framebuffer_release() handle freeing the memory. That's what is > done in other drivers that follow the fbdev pattern, and it's less > code overall. > > Michael Ok, i agree with you. Remove all calls to kfree(info->apertures) in hvfb_getmem() is a better solution. I will subimt a PATCH v2 for you to review. Thanks.
diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c index c8b0ae676809..2fc9b507e73a 100644 --- a/drivers/video/fbdev/hyperv_fb.c +++ b/drivers/video/fbdev/hyperv_fb.c @@ -1032,6 +1032,7 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info) if (!pdev) { pr_err("Unable to find PCI Hyper-V video\n"); kfree(info->apertures); + info->apertures = NULL; return -ENODEV; } @@ -1130,6 +1131,7 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info) pci_dev_put(pdev); } kfree(info->apertures); + info->apertures = NULL; return 0; @@ -1142,6 +1144,7 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info) if (!gen2vm) pci_dev_put(pdev); kfree(info->apertures); + info->apertures = NULL; return -ENOMEM; }
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 twice by framebuffer_release(info). My patch sets info->apertures to NULL after it was freed to avoid double free. Signed-off-by: Lv Yunlong <lyl2019@mail.ustc.edu.cn> --- drivers/video/fbdev/hyperv_fb.c | 3 +++ 1 file changed, 3 insertions(+)