| Message ID | 20210427132734.5212-1-lyl2019@mail.ustc.edu.cn |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] media:exynos4-is: Fix a use after free in isp_video_release | expand |
Hi Lv Yunlong, On 27/04/2021 15:27, Lv Yunlong wrote: > In isp_video_release, file->private_data is freed via > _vb2_fop_release()->v4l2_fh_release(). But the freed > file->private_data is still used in v4l2_fh_is_singular_file() > ->v4l2_fh_is_singular(file->private_data), which is a use > after free bug. > > My patch sets file->private_data to NULL after _vb2_fop_release() > to avoid the use after free, and uses a variable 'is_singular_file' > to keep the original function unchanged. Actually, it is the use of 'is_singular_file' that fixes the bug, the 'file->private_data = NULL;' is unnecessary here. That said, it would be a really good idea if in a separate patch you make v4l2_fh_release() more robust by setting filp->private_data to NULL after the kfree(fh). Regards, Hans > > Fixes: 34947b8aebe3f ("[media] exynos4-is: Add the FIMC-IS ISP capture DMA driver") > Signed-off-by: Lv Yunlong <lyl2019@mail.ustc.edu.cn> > --- > drivers/media/platform/exynos4-is/fimc-isp-video.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/exynos4-is/fimc-isp-video.c b/drivers/media/platform/exynos4-is/fimc-isp-video.c > index 612b9872afc8..c07dcb0bccc2 100644 > --- a/drivers/media/platform/exynos4-is/fimc-isp-video.c > +++ b/drivers/media/platform/exynos4-is/fimc-isp-video.c > @@ -306,17 +306,21 @@ static int isp_video_release(struct file *file) > struct fimc_is_video *ivc = &isp->video_capture; > struct media_entity *entity = &ivc->ve.vdev.entity; > struct media_device *mdev = entity->graph_obj.mdev; > + bool is_singular_file; > > mutex_lock(&isp->video_lock); > > - if (v4l2_fh_is_singular_file(file) && ivc->streaming) { > + is_singular_file = v4l2_fh_is_singular_file(file); > + > + if (is_singular_file && ivc->streaming) { > media_pipeline_stop(entity); > ivc->streaming = 0; > } > > _vb2_fop_release(file, NULL); > + file->private_data = NULL; > > - if (v4l2_fh_is_singular_file(file)) { > + if (is_singular_file) { > fimc_pipeline_call(&ivc->ve, close); > > mutex_lock(&mdev->graph_mutex); >
> -----原始邮件----- > 发件人: "Hans Verkuil" <hverkuil@xs4all.nl> > 发送时间: 2021-05-05 17:31:04 (星期三) > 收件人: "Lv Yunlong" <lyl2019@mail.ustc.edu.cn>, s.nawrocki@samsung.com, mchehab@kernel.org, krzk@kernel.org > 抄送: linux-media@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org > 主题: Re: [PATCH v3] media:exynos4-is: Fix a use after free in isp_video_release > > Hi Lv Yunlong, > > On 27/04/2021 15:27, Lv Yunlong wrote: > > In isp_video_release, file->private_data is freed via > > _vb2_fop_release()->v4l2_fh_release(). But the freed > > file->private_data is still used in v4l2_fh_is_singular_file() > > ->v4l2_fh_is_singular(file->private_data), which is a use > > after free bug. > > > > My patch sets file->private_data to NULL after _vb2_fop_release() > > to avoid the use after free, and uses a variable 'is_singular_file' > > to keep the original function unchanged. > > Actually, it is the use of 'is_singular_file' that fixes the bug, > the 'file->private_data = NULL;' is unnecessary here. > > That said, it would be a really good idea if in a separate patch you > make v4l2_fh_release() more robust by setting filp->private_data to > NULL after the kfree(fh). > > Regards, > > Hans > > > > > Fixes: 34947b8aebe3f ("[media] exynos4-is: Add the FIMC-IS ISP capture DMA driver") > > Signed-off-by: Lv Yunlong <lyl2019@mail.ustc.edu.cn> > > --- > > drivers/media/platform/exynos4-is/fimc-isp-video.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/platform/exynos4-is/fimc-isp-video.c b/drivers/media/platform/exynos4-is/fimc-isp-video.c > > index 612b9872afc8..c07dcb0bccc2 100644 > > --- a/drivers/media/platform/exynos4-is/fimc-isp-video.c > > +++ b/drivers/media/platform/exynos4-is/fimc-isp-video.c > > @@ -306,17 +306,21 @@ static int isp_video_release(struct file *file) > > struct fimc_is_video *ivc = &isp->video_capture; > > struct media_entity *entity = &ivc->ve.vdev.entity; > > struct media_device *mdev = entity->graph_obj.mdev; > > + bool is_singular_file; > > > > mutex_lock(&isp->video_lock); > > > > - if (v4l2_fh_is_singular_file(file) && ivc->streaming) { > > + is_singular_file = v4l2_fh_is_singular_file(file); > > + > > + if (is_singular_file && ivc->streaming) { > > media_pipeline_stop(entity); > > ivc->streaming = 0; > > } > > > > _vb2_fop_release(file, NULL); > > + file->private_data = NULL; > > > > - if (v4l2_fh_is_singular_file(file)) { > > + if (is_singular_file) { > > fimc_pipeline_call(&ivc->ve, close); > > > > mutex_lock(&mdev->graph_mutex); > > > Ok, thanks for your suggestion. Do you means i need submit a new path to set filp->private_data = NULL after kfree(fh) in v4l2_fh_release() ? Lv Yunlong
On 06/05/2021 09:19, lyl2019@mail.ustc.edu.cn wrote: > > > >> -----原始邮件----- >> 发件人: "Hans Verkuil" <hverkuil@xs4all.nl> >> 发送时间: 2021-05-05 17:31:04 (星期三) >> 收件人: "Lv Yunlong" <lyl2019@mail.ustc.edu.cn>, s.nawrocki@samsung.com, mchehab@kernel.org, krzk@kernel.org >> 抄送: linux-media@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org >> 主题: Re: [PATCH v3] media:exynos4-is: Fix a use after free in isp_video_release >> >> Hi Lv Yunlong, >> >> On 27/04/2021 15:27, Lv Yunlong wrote: >>> In isp_video_release, file->private_data is freed via >>> _vb2_fop_release()->v4l2_fh_release(). But the freed >>> file->private_data is still used in v4l2_fh_is_singular_file() >>> ->v4l2_fh_is_singular(file->private_data), which is a use >>> after free bug. >>> >>> My patch sets file->private_data to NULL after _vb2_fop_release() >>> to avoid the use after free, and uses a variable 'is_singular_file' >>> to keep the original function unchanged. >> >> Actually, it is the use of 'is_singular_file' that fixes the bug, >> the 'file->private_data = NULL;' is unnecessary here. >> >> That said, it would be a really good idea if in a separate patch you >> make v4l2_fh_release() more robust by setting filp->private_data to >> NULL after the kfree(fh). >> >> Regards, >> >> Hans >> >>> >>> Fixes: 34947b8aebe3f ("[media] exynos4-is: Add the FIMC-IS ISP capture DMA driver") >>> Signed-off-by: Lv Yunlong <lyl2019@mail.ustc.edu.cn> >>> --- >>> drivers/media/platform/exynos4-is/fimc-isp-video.c | 8 ++++++-- >>> 1 file changed, 6 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/media/platform/exynos4-is/fimc-isp-video.c b/drivers/media/platform/exynos4-is/fimc-isp-video.c >>> index 612b9872afc8..c07dcb0bccc2 100644 >>> --- a/drivers/media/platform/exynos4-is/fimc-isp-video.c >>> +++ b/drivers/media/platform/exynos4-is/fimc-isp-video.c >>> @@ -306,17 +306,21 @@ static int isp_video_release(struct file *file) >>> struct fimc_is_video *ivc = &isp->video_capture; >>> struct media_entity *entity = &ivc->ve.vdev.entity; >>> struct media_device *mdev = entity->graph_obj.mdev; >>> + bool is_singular_file; >>> >>> mutex_lock(&isp->video_lock); >>> >>> - if (v4l2_fh_is_singular_file(file) && ivc->streaming) { >>> + is_singular_file = v4l2_fh_is_singular_file(file); >>> + >>> + if (is_singular_file && ivc->streaming) { >>> media_pipeline_stop(entity); >>> ivc->streaming = 0; >>> } >>> >>> _vb2_fop_release(file, NULL); >>> + file->private_data = NULL; >>> >>> - if (v4l2_fh_is_singular_file(file)) { >>> + if (is_singular_file) { >>> fimc_pipeline_call(&ivc->ve, close); >>> >>> mutex_lock(&mdev->graph_mutex); >>> >> > > > Ok, thanks for your suggestion. > > Do you means i need submit a new path to set filp->private_data = NULL > after kfree(fh) in v4l2_fh_release() ? Yes, so one updated patch for fimc-isp-video.c and a second patch for v4l2-fh.c. Regards, Hans > > Lv Yunlong >
diff --git a/drivers/media/platform/exynos4-is/fimc-isp-video.c b/drivers/media/platform/exynos4-is/fimc-isp-video.c index 612b9872afc8..c07dcb0bccc2 100644 --- a/drivers/media/platform/exynos4-is/fimc-isp-video.c +++ b/drivers/media/platform/exynos4-is/fimc-isp-video.c @@ -306,17 +306,21 @@ static int isp_video_release(struct file *file) struct fimc_is_video *ivc = &isp->video_capture; struct media_entity *entity = &ivc->ve.vdev.entity; struct media_device *mdev = entity->graph_obj.mdev; + bool is_singular_file; mutex_lock(&isp->video_lock); - if (v4l2_fh_is_singular_file(file) && ivc->streaming) { + is_singular_file = v4l2_fh_is_singular_file(file); + + if (is_singular_file && ivc->streaming) { media_pipeline_stop(entity); ivc->streaming = 0; } _vb2_fop_release(file, NULL); + file->private_data = NULL; - if (v4l2_fh_is_singular_file(file)) { + if (is_singular_file) { fimc_pipeline_call(&ivc->ve, close); mutex_lock(&mdev->graph_mutex);
In isp_video_release, file->private_data is freed via _vb2_fop_release()->v4l2_fh_release(). But the freed file->private_data is still used in v4l2_fh_is_singular_file() ->v4l2_fh_is_singular(file->private_data), which is a use after free bug. My patch sets file->private_data to NULL after _vb2_fop_release() to avoid the use after free, and uses a variable 'is_singular_file' to keep the original function unchanged. Fixes: 34947b8aebe3f ("[media] exynos4-is: Add the FIMC-IS ISP capture DMA driver") Signed-off-by: Lv Yunlong <lyl2019@mail.ustc.edu.cn> --- drivers/media/platform/exynos4-is/fimc-isp-video.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)