linux-kernel - Re: [PATCH 4.19 047/125] media: davinci: vpif

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1304121599035106@mail.yandex.ru>
Date:   Wed, 02 Sep 2020 11:42:33 +0300
From:   Evgeny Novikov <novikov@...ras.ru>
To:     Pavel Machek <pavel@...x.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
        Sasha Levin <sashal@...nel.org>
Subject: Re: [PATCH 4.19 047/125] media: davinci: vpif_capture: fix potential double free

Hi Pavel,

Maybe I miss something, but it seems that in 4.19 vpif_probe() ignores
error codes from vpif_probe_complete() and returns 0 even if it fails.
That was fixed by commit 64f883cd98c6, but it was not backported to 4.19.
In addition, this commit contains a fix of one more bug.

Regarding your second note. I investigated other drivers that use the same
mechanism and did not find out any driver that performs clean up in the
complete handler. In particular, this is the case for very similar driver
vpif_display.c. I am not an expert in this subsystem, so, I can not reason
why this is so.

-- 
Best regards,
Evgeny Novikov



01.09.2020, 21:43, "Pavel Machek" <pavel@...x.de>:
> Hi!
>
>>  [ Upstream commit 602649eadaa0c977e362e641f51ec306bc1d365d ]
>>
>>  In case of errors vpif_probe_complete() releases memory for vpif_obj.sd
>>  and unregisters the V4L2 device. But then this is done again by
>>  vpif_probe() itself. The patch removes the cleaning from
>>  vpif_probe_complete().
>
>>  Signed-off-by: Evgeny Novikov <novikov@...ras.ru>
>>  Signed-off-by: Hans Verkuil <hverkuil-cisco@...all.nl>
>>  Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
>>  Signed-off-by: Sasha Levin <sashal@...nel.org>
>>  ---
>>   drivers/media/platform/davinci/vpif_capture.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>>  diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
>>  index a96f53ce80886..cf1d11e6dd8c4 100644
>>  --- a/drivers/media/platform/davinci/vpif_capture.c
>>  +++ b/drivers/media/platform/davinci/vpif_capture.c
>>  @@ -1489,8 +1489,6 @@ probe_out:
>>                   /* Unregister video device */
>>                   video_unregister_device(&ch->video_dev);
>>           }
>>  - kfree(vpif_obj.sd);
>>  - v4l2_device_unregister(&vpif_obj.v4l2_dev);
>>
>>           return err;
>>   }
>
> This one is wrong. Unlike mainline, 4.19 does check return value of
> vpif_probe_complete(), and thus it will lead to memory leak in 4.19.
>
> Furthermore, I believe mainline still has a problems after this
> patch. There is sync and async path where vpif_probe_complete(), and
> while this fixes the sync path in mainline, I believe it will cause
> memory leak on the async path.
>
> Best regards,
>                                                                         Pavel
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html