[073/190] Revert "media: rcar_drif: fix a memory disclosure"
diff mbox series

Message ID 20210421130105.1226686-74-gregkh@linuxfoundation.org
State New, archived
Headers show
Series
  • Revertion of all of the umn.edu commits
Related show

Commit Message

Greg KH April 21, 2021, 12:59 p.m. UTC
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@umn.edu>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/media/platform/rcar_drif.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Geert Uytterhoeven April 21, 2021, 6:58 p.m. UTC | #1
Hi Greg,

On Wed, Apr 21, 2021 at 3:06 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> 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@umn.edu>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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.

> --- 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
Laurent Pinchart April 21, 2021, 9:21 p.m. UTC | #2
Hi Geert,

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@umn.edu>
> > Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> > Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> 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.

> > --- 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;
> >
Geert Uytterhoeven April 22, 2021, 6:57 a.m. UTC | #3
Hi Laurent,

On Wed, Apr 21, 2021 at 11:22 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> 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@umn.edu>
> > > Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> > > Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >
> > 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

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

> > > --- 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
Hans Verkuil April 22, 2021, 7:29 a.m. UTC | #4
On 22/04/2021 08:57, Geert Uytterhoeven wrote:
> Hi Laurent,
> 
> On Wed, Apr 21, 2021 at 11:22 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> 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@umn.edu>
>>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>>>> Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>>> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
>>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>
>>> 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
>
Mauro Carvalho Chehab April 22, 2021, 9:03 a.m. UTC | #5
Em Thu, 22 Apr 2021 09:29:36 +0200
Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:

> On 22/04/2021 08:57, Geert Uytterhoeven wrote:
> > Hi Laurent,
> > 
> > On Wed, Apr 21, 2021 at 11:22 PM Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> 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@umn.edu>
> >>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> >>>> Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >>>> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> >>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>  
> >>>
> >>> 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.

Hi Greg,

I also double-checked all media revert patches from:

	git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git umn.edu-reverts

currently on this patch:
	6f4747a872ad Revert "ethtool: fix a potential missing-check bug"

That's a summary of what I found:

All of those should be dropped from your tree:

	84fdb5856edd	Revert "media: si2165: fix a missing check of return value"
	867043f2206e	Revert "media: video-mux: fix null pointer dereferences"
	78ae4b621297	Revert "media: cx231xx: replace BUG_ON with recovery code"
	5be328a55817	Revert "media: saa7146: Avoid using BUG_ON as an assertion"
	81ce83158d22	Revert "media: davinci/vpfe_capture.c: Avoid BUG_ON for register failure"
	3319b39504b8	Revert "media: media/saa7146: fix incorrect assertion in saa7146_buffer_finish"
	b393f7cb29a2	Revert "media: rcar-vin: Fix a reference count leak."
	197bc5d03682	Revert "media: rcar-vin: Fix a reference count leak."
	2fd9cf68bbb6	Revert "media: rockchip/rga: Fix a reference count leak."
	d1e4614eca24	Revert "media: platform: fcp: Fix a reference count leak."
	416e8a6ae07f	Revert "media: camss: Fix a reference count leak."
	06b793ae497b	Revert "media: s5p-mfc: Fix a reference count leak"
	8f9fc14a7cc9	Revert "media: stm32-dcmi: Fix a reference count leak"
	556e1f86ba24	Revert "media: ti-vpe: Fix a missing check and reference count leak"
	5f5b1722ad0d	Revert "media: exynos4-is: Fix a reference count leak"
	f4c758c6c1cb	Revert "media: exynos4-is: Fix a reference count leak due to pm_runtime_get_sync"
	beb717878c73	Revert "media: exynos4-is: Fix several reference count leaks due to pm_runtime_get_sync
	7066ec748bfd	Revert "media: sti: Fix reference count leaks"
	cdd117093b19	Revert "media: st-delta: Fix reference count leak in delta_run_work"

As, after my re-check, they all seem to be addressing real issues. So,
NACK on those.

This patch (073/190):

	899ab4671bc0	Revert "media: rcar_drif: fix a memory disclosure"

While it doesn't hurt, it is useless, as the media core already
prevents memory disclosure. So, it should be reverted.

So, for patch 073/190:

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Thanks,
Mauro
Fabrizio Castro April 22, 2021, 9:21 a.m. UTC | #6
Dear All,

> From: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> Sent: 22 April 2021 10:04
> Subject: Re: [PATCH 073/190] Revert "media: rcar_drif: fix a memory
> disclosure"
> 
> Em Thu, 22 Apr 2021 09:29:36 +0200
> Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:
> 
> > On 22/04/2021 08:57, Geert Uytterhoeven wrote:
> > > Hi Laurent,
> > >
> > > On Wed, Apr 21, 2021 at 11:22 PM Laurent Pinchart
> > > <laurent.pinchart@ideasonboard.com> 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@umn.edu>
> > >>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> > >>>> Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > >>>> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > >>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > >>>
> > >>> 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.
> 
> Hi Greg,
> 
> I also double-checked all media revert patches from:
> 
> 	git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git
> umn.edu-reverts
> 
> currently on this patch:
> 	6f4747a872ad Revert "ethtool: fix a potential missing-check bug"
> 
> That's a summary of what I found:
> 
> All of those should be dropped from your tree:
> 
> 	84fdb5856edd	Revert "media: si2165: fix a missing check of
> return value"
> 	867043f2206e	Revert "media: video-mux: fix null pointer
> dereferences"
> 	78ae4b621297	Revert "media: cx231xx: replace BUG_ON with
> recovery code"
> 	5be328a55817	Revert "media: saa7146: Avoid using BUG_ON as an
> assertion"
> 	81ce83158d22	Revert "media: davinci/vpfe_capture.c: Avoid
> BUG_ON for register failure"
> 	3319b39504b8	Revert "media: media/saa7146: fix incorrect
> assertion in saa7146_buffer_finish"
> 	b393f7cb29a2	Revert "media: rcar-vin: Fix a reference count
> leak."
> 	197bc5d03682	Revert "media: rcar-vin: Fix a reference count
> leak."
> 	2fd9cf68bbb6	Revert "media: rockchip/rga: Fix a reference count
> leak."
> 	d1e4614eca24	Revert "media: platform: fcp: Fix a reference
> count leak."
> 	416e8a6ae07f	Revert "media: camss: Fix a reference count leak."
> 	06b793ae497b	Revert "media: s5p-mfc: Fix a reference count
> leak"
> 	8f9fc14a7cc9	Revert "media: stm32-dcmi: Fix a reference count
> leak"
> 	556e1f86ba24	Revert "media: ti-vpe: Fix a missing check and
> reference count leak"
> 	5f5b1722ad0d	Revert "media: exynos4-is: Fix a reference count
> leak"
> 	f4c758c6c1cb	Revert "media: exynos4-is: Fix a reference count
> leak due to pm_runtime_get_sync"
> 	beb717878c73	Revert "media: exynos4-is: Fix several reference
> count leaks due to pm_runtime_get_sync
> 	7066ec748bfd	Revert "media: sti: Fix reference count leaks"
> 	cdd117093b19	Revert "media: st-delta: Fix reference count leak
> in delta_run_work"
> 
> As, after my re-check, they all seem to be addressing real issues. So,
> NACK on those.
> 
> This patch (073/190):
> 
> 	899ab4671bc0	Revert "media: rcar_drif: fix a memory disclosure"
> 
> While it doesn't hurt, it is useless, as the media core already
> prevents memory disclosure. So, it should be reverted.
> 
> So, for patch 073/190:
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

I agree, this patch should be reverted.

Reviewed-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>

> 
> Thanks,
> Mauro
Greg KH April 22, 2021, 9:29 a.m. UTC | #7
On Thu, Apr 22, 2021 at 11:03:36AM +0200, Mauro Carvalho Chehab wrote:
> Em Thu, 22 Apr 2021 09:29:36 +0200
> Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:
> 
> > On 22/04/2021 08:57, Geert Uytterhoeven wrote:
> > > Hi Laurent,
> > > 
> > > On Wed, Apr 21, 2021 at 11:22 PM Laurent Pinchart
> > > <laurent.pinchart@ideasonboard.com> 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@umn.edu>
> > >>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> > >>>> Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > >>>> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > >>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>  
> > >>>
> > >>> 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.
> 
> Hi Greg,
> 
> I also double-checked all media revert patches from:
> 
> 	git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git umn.edu-reverts
> 
> currently on this patch:
> 	6f4747a872ad Revert "ethtool: fix a potential missing-check bug"
> 
> That's a summary of what I found:
> 
> All of those should be dropped from your tree:
> 
> 	84fdb5856edd	Revert "media: si2165: fix a missing check of return value"
> 	867043f2206e	Revert "media: video-mux: fix null pointer dereferences"
> 	78ae4b621297	Revert "media: cx231xx: replace BUG_ON with recovery code"
> 	5be328a55817	Revert "media: saa7146: Avoid using BUG_ON as an assertion"
> 	81ce83158d22	Revert "media: davinci/vpfe_capture.c: Avoid BUG_ON for register failure"
> 	3319b39504b8	Revert "media: media/saa7146: fix incorrect assertion in saa7146_buffer_finish"
> 	b393f7cb29a2	Revert "media: rcar-vin: Fix a reference count leak."
> 	197bc5d03682	Revert "media: rcar-vin: Fix a reference count leak."
> 	2fd9cf68bbb6	Revert "media: rockchip/rga: Fix a reference count leak."
> 	d1e4614eca24	Revert "media: platform: fcp: Fix a reference count leak."
> 	416e8a6ae07f	Revert "media: camss: Fix a reference count leak."
> 	06b793ae497b	Revert "media: s5p-mfc: Fix a reference count leak"
> 	8f9fc14a7cc9	Revert "media: stm32-dcmi: Fix a reference count leak"
> 	556e1f86ba24	Revert "media: ti-vpe: Fix a missing check and reference count leak"
> 	5f5b1722ad0d	Revert "media: exynos4-is: Fix a reference count leak"
> 	f4c758c6c1cb	Revert "media: exynos4-is: Fix a reference count leak due to pm_runtime_get_sync"
> 	beb717878c73	Revert "media: exynos4-is: Fix several reference count leaks due to pm_runtime_get_sync
> 	7066ec748bfd	Revert "media: sti: Fix reference count leaks"
> 	cdd117093b19	Revert "media: st-delta: Fix reference count leak in delta_run_work"
> 
> As, after my re-check, they all seem to be addressing real issues. So,
> NACK on those.
> 
> This patch (073/190):
> 
> 	899ab4671bc0	Revert "media: rcar_drif: fix a memory disclosure"
> 
> While it doesn't hurt, it is useless, as the media core already
> prevents memory disclosure. So, it should be reverted.
> 
> So, for patch 073/190:
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Wonderful, thank you so much for doing this review and letting me know.
I'll drop the ones you mention here.

greg k-h
Greg KH April 27, 2021, 1:08 p.m. UTC | #8
On Thu, Apr 22, 2021 at 09:21:13AM +0000, Fabrizio Castro wrote:
> Dear All,
> 
> > From: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > Sent: 22 April 2021 10:04
> > Subject: Re: [PATCH 073/190] Revert "media: rcar_drif: fix a memory
> > disclosure"
> > 
> > Em Thu, 22 Apr 2021 09:29:36 +0200
> > Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:
> > 
> > > On 22/04/2021 08:57, Geert Uytterhoeven wrote:
> > > > Hi Laurent,
> > > >
> > > > On Wed, Apr 21, 2021 at 11:22 PM Laurent Pinchart
> > > > <laurent.pinchart@ideasonboard.com> 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@umn.edu>
> > > >>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> > > >>>> Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > > >>>> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > > >>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > >>>
> > > >>> 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.
> > 
> > Hi Greg,
> > 
> > I also double-checked all media revert patches from:
> > 
> > 	git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git
> > umn.edu-reverts
> > 
> > currently on this patch:
> > 	6f4747a872ad Revert "ethtool: fix a potential missing-check bug"
> > 
> > That's a summary of what I found:
> > 
> > All of those should be dropped from your tree:
> > 
> > 	84fdb5856edd	Revert "media: si2165: fix a missing check of
> > return value"
> > 	867043f2206e	Revert "media: video-mux: fix null pointer
> > dereferences"
> > 	78ae4b621297	Revert "media: cx231xx: replace BUG_ON with
> > recovery code"
> > 	5be328a55817	Revert "media: saa7146: Avoid using BUG_ON as an
> > assertion"
> > 	81ce83158d22	Revert "media: davinci/vpfe_capture.c: Avoid
> > BUG_ON for register failure"
> > 	3319b39504b8	Revert "media: media/saa7146: fix incorrect
> > assertion in saa7146_buffer_finish"
> > 	b393f7cb29a2	Revert "media: rcar-vin: Fix a reference count
> > leak."
> > 	197bc5d03682	Revert "media: rcar-vin: Fix a reference count
> > leak."
> > 	2fd9cf68bbb6	Revert "media: rockchip/rga: Fix a reference count
> > leak."
> > 	d1e4614eca24	Revert "media: platform: fcp: Fix a reference
> > count leak."
> > 	416e8a6ae07f	Revert "media: camss: Fix a reference count leak."
> > 	06b793ae497b	Revert "media: s5p-mfc: Fix a reference count
> > leak"
> > 	8f9fc14a7cc9	Revert "media: stm32-dcmi: Fix a reference count
> > leak"
> > 	556e1f86ba24	Revert "media: ti-vpe: Fix a missing check and
> > reference count leak"
> > 	5f5b1722ad0d	Revert "media: exynos4-is: Fix a reference count
> > leak"
> > 	f4c758c6c1cb	Revert "media: exynos4-is: Fix a reference count
> > leak due to pm_runtime_get_sync"
> > 	beb717878c73	Revert "media: exynos4-is: Fix several reference
> > count leaks due to pm_runtime_get_sync
> > 	7066ec748bfd	Revert "media: sti: Fix reference count leaks"
> > 	cdd117093b19	Revert "media: st-delta: Fix reference count leak
> > in delta_run_work"
> > 
> > As, after my re-check, they all seem to be addressing real issues. So,
> > NACK on those.
> > 
> > This patch (073/190):
> > 
> > 	899ab4671bc0	Revert "media: rcar_drif: fix a memory disclosure"
> > 
> > While it doesn't hurt, it is useless, as the media core already
> > prevents memory disclosure. So, it should be reverted.
> > 
> > So, for patch 073/190:
> > 
> > Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> 
> I agree, this patch should be reverted.
> 
> Reviewed-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>

Thanks for the review, I've now dropped the media patches listed above,
and kept this one and added both of your r-b to it.

greg k-h

Patch
diff mbox series

diff --git a/drivers/media/platform/rcar_drif.c b/drivers/media/platform/rcar_drif.c
index 83bd9a412a56..1e3b68a8743a 100644
--- 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;