Re: [PATCH 073/190] Revert "media: rcar_drif: fix a memory disclosure"

[Hans Verkuil]

On 22/04/2021 08:57, Geert Uytterhoeven wrote:
> Hi Laurent,
> 
> On Wed, Apr 21, 2021 at 11:22 PM Laurent Pinchart
> <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
>> On Wed, Apr 21, 2021 at 08:58:22PM +0200, Geert Uytterhoeven wrote:
>>> On Wed, Apr 21, 2021 at 3:06 PM Greg Kroah-Hartman wrote:
>>>> This reverts commit d39083234c60519724c6ed59509a2129fd2aed41.
>>>>
>>>> Commits from @umn.edu addresses have been found to be submitted in "bad
>>>> faith" to try to test the kernel community's ability to review "known
>>>> malicious" changes.  The result of these submissions can be found in a
>>>> paper published at the 42nd IEEE Symposium on Security and Privacy
>>>> entitled, "Open Source Insecurity: Stealthily Introducing
>>>> Vulnerabilities via Hypocrite Commits" written by Qiushi Wu (University
>>>> of Minnesota) and Kangjie Lu (University of Minnesota).
>>>>
>>>> Because of this, all submissions from this group must be reverted from
>>>> the kernel tree and will need to be re-reviewed again to determine if
>>>> they actually are a valid fix.  Until that work is complete, remove this
>>>> change to ensure that no problems are being introduced into the
>>>> codebase.
>>>>
>>>> Cc: Kangjie Lu <kjlu@xxxxxxx>
>>>> Cc: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
>>>> Cc: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
>>>> Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
>>>> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>>>
>>> Upon a second look, I still see nothing wrong with the original commit.
>>> However, as I'm no v4l expert, I'd like to defer to the experts for final
>>> judgement.
>>
>> It seems fine to me, but it also seems unneeded, as the V4L2 core clears
>> the whole f->fmt union before calling this operation. The revert will
>> this improve performance very slightly.
> 
> Hmm, that means very recent commit f12b81e47f48940a ("media: core
> headers: fix kernel-doc warnings") is not fully correct, as it added
> kerneldoc stating this is the responsibility of the driver:
> 
> + * @reserved:          drivers and applications must zero this array

Actually, it is the V4L2 core used by the driver that zeroes this. So
drivers don't need to do this, it's done for them. It used to be the
responsibility of the driver itself, but this was all moved to the core
framework a long time ago since, duh!, drivers always forgot this :-)

> 
> Anyway, it doesn't look like this umn.edu patch introduced a bug.

I haven't seen any bugs introduced by the media patches from umn.edu.

Regards,

	Hans

> 
>>>> --- a/drivers/media/platform/rcar_drif.c
>>>> +++ b/drivers/media/platform/rcar_drif.c
>>>> @@ -915,7 +915,6 @@ static int rcar_drif_g_fmt_sdr_cap(struct file *file, void *priv,
>>>>  {
>>>>         struct rcar_drif_sdr *sdr = video_drvdata(file);
>>>>
>>>> -       memset(f->fmt.sdr.reserved, 0, sizeof(f->fmt.sdr.reserved));
>>>>         f->fmt.sdr.pixelformat = sdr->fmt->pixelformat;
>>>>         f->fmt.sdr.buffersize = sdr->fmt->buffersize;
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
>