Nouveau - Jan 2022 - [PATCH] drm/nouveau/acr: Fix undefined behavior in nvkm_acr_hsfw_load_bl()

On Fri, Jan 28, 2022 at 8:54 PM Alex Deucher <alexdeucher at gmail.com>
wrote:
>
> On Fri, Jan 28, 2022 at 2:20 PM Lyude Paul <lyude at redhat.com>
wrote:
> >
> > Sigh-thank you for catching this - I had totally forgot about the
umn.edu ban.
> > I pushed this already but I will go ahead and send a revert for this
patch.
> > Will cc you on it as well.
>
> This seems short-sighted.  If the patch is valid I see no reason to
> not accept it.  I'm not trying to downplay the mess umn got into, but
> as long as the patch is well scrutinized and fixes a valid issue, it
> should be applied rather than leaving potential bugs in place.
>
> Alex
>
Even though knowing that malicious code can be introduced via
perfectly fine looking patches, and sometimes one will never spot the
problem, this patch isn't all that bad tbh.

So should we reject patches out of "policies" or should we just be
extra careful? But not addressing the concerns as Greg pointed out is
also kind of a bad move, but also not knowing what the state of
resolving this mess is anyway.
>
> >
> > On Fri, 2022-01-28 at 11:18 +0100, Greg KH wrote:
> > > On Tue, Jan 25, 2022 at 12:58:55AM +0800, Zhou Qingyang wrote:
> > > > In nvkm_acr_hsfw_load_bl(), the return value of kmalloc() is
directly
> > > > passed to memcpy(), which could lead to undefined behavior
on failure
> > > > of kmalloc().
> > > >
> > > > Fix this bug by using kmemdup() instead of
kmalloc()+memcpy().
> > > >
> > > > This bug was found by a static analyzer.
> > > >
> > > > Builds with 'make allyesconfig' show no new
warnings,
> > > > and our static analyzer no longer warns about this code.
> > > >
> > > > Fixes: 22dcda45a3d1 ("drm/nouveau/acr: implement new
subdev to replace
> > > > "secure boot"")
> > > > Signed-off-by: Zhou Qingyang <zhou1615 at umn.edu>
> > > > ---
> > > > The analysis employs differential checking to identify
inconsistent
> > > > security operations (e.g., checks or kfrees) between two
code paths
> > > > and confirms that the inconsistent operations are not
recovered in the
> > > > current function or the callers, so they constitute bugs.
> > > >
> > > > Note that, as a bug found by static analysis, it can be a
false
> > > > positive or hard to trigger. Multiple researchers have
cross-reviewed
> > > > the bug.
> > > >
> > > >  drivers/gpu/drm/nouveau/nvkm/subdev/acr/hsfw.c | 9
+++++----
> > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/acr/hsfw.c
> > > > b/drivers/gpu/drm/nouveau/nvkm/subdev/acr/hsfw.c
> > > > index 667fa016496e..a6ea89a5d51a 100644
> > > > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/acr/hsfw.c
> > > > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/acr/hsfw.c
> > > > @@ -142,11 +142,12 @@ nvkm_acr_hsfw_load_bl(struct nvkm_acr
*acr, const
> > > > char *name, int ver,
> > > >
> > > >         hsfw->imem_size = desc->code_size;
> > > >         hsfw->imem_tag = desc->start_tag;
> > > > -       hsfw->imem = kmalloc(desc->code_size,
GFP_KERNEL);
> > > > -       memcpy(hsfw->imem, data + desc->code_off,
desc->code_size);
> > > > -
> > > > +       hsfw->imem = kmemdup(data + desc->code_off,
desc->code_size,
> > > > GFP_KERNEL);
> > > >         nvkm_firmware_put(fw);
> > > > -       return 0;
> > > > +       if (!hsfw->imem)
> > > > +               return -ENOMEM;
> > > > +       else
> > > > +               return 0;
> > > >  }
> > > >
> > > >  int
> > > > --
> > > > 2.25.1
> > > >
> > >
> > > As stated before, umn.edu is still not allowed to contribute to
the
> > > Linux kernel.  Please work with your administration to resolve
this
> > > issue.
> > >
> >
> > --
> > Cheers,
> >  Lyude Paul (she/her)
> >  Software Engineer at Red Hat
> >
>

On Fri, Jan 28, 2022 at 2:58 PM Karol Herbst <kherbst at redhat.com>
wrote:
>
> On Fri, Jan 28, 2022 at 8:54 PM Alex Deucher <alexdeucher at
gmail.com> wrote:
> >
> > On Fri, Jan 28, 2022 at 2:20 PM Lyude Paul <lyude at redhat.com>
wrote:
> > >
> > > Sigh-thank you for catching this - I had totally forgot about the
umn.edu ban.
> > > I pushed this already but I will go ahead and send a revert for
this patch.
> > > Will cc you on it as well.
> >
> > This seems short-sighted.  If the patch is valid I see no reason to
> > not accept it.  I'm not trying to downplay the mess umn got into,
but
> > as long as the patch is well scrutinized and fixes a valid issue, it
> > should be applied rather than leaving potential bugs in place.
> >
> > Alex
> >
>
> Even though knowing that malicious code can be introduced via
> perfectly fine looking patches, and sometimes one will never spot the
> problem, this patch isn't all that bad tbh.
>
> So should we reject patches out of "policies" or should we just
be
> extra careful? But not addressing the concerns as Greg pointed out is
> also kind of a bad move, but also not knowing what the state of
> resolving this mess is anyway.
I think if the umn mess taught us anything, it's the need for more
careful scrutiny.  But I certainly don't have the time to retype every
valid patch if it comes from a umn source.  There are also ethical
implications to that as well.  You didn't actually write the patch.

Alex
>
> >
> > >
> > > On Fri, 2022-01-28 at 11:18 +0100, Greg KH wrote:
> > > > On Tue, Jan 25, 2022 at 12:58:55AM +0800, Zhou Qingyang
wrote:
> > > > > In nvkm_acr_hsfw_load_bl(), the return value of
kmalloc() is directly
> > > > > passed to memcpy(), which could lead to undefined
behavior on failure
> > > > > of kmalloc().
> > > > >
> > > > > Fix this bug by using kmemdup() instead of
kmalloc()+memcpy().
> > > > >
> > > > > This bug was found by a static analyzer.
> > > > >
> > > > > Builds with 'make allyesconfig' show no new
warnings,
> > > > > and our static analyzer no longer warns about this
code.
> > > > >
> > > > > Fixes: 22dcda45a3d1 ("drm/nouveau/acr: implement
new subdev to replace
> > > > > "secure boot"")
> > > > > Signed-off-by: Zhou Qingyang <zhou1615 at
umn.edu>
> > > > > ---
> > > > > The analysis employs differential checking to identify
inconsistent
> > > > > security operations (e.g., checks or kfrees) between
two code paths
> > > > > and confirms that the inconsistent operations are not
recovered in the
> > > > > current function or the callers, so they constitute
bugs.
> > > > >
> > > > > Note that, as a bug found by static analysis, it can be
a false
> > > > > positive or hard to trigger. Multiple researchers have
cross-reviewed
> > > > > the bug.
> > > > >
> > > > >  drivers/gpu/drm/nouveau/nvkm/subdev/acr/hsfw.c | 9
+++++----
> > > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git
a/drivers/gpu/drm/nouveau/nvkm/subdev/acr/hsfw.c
> > > > > b/drivers/gpu/drm/nouveau/nvkm/subdev/acr/hsfw.c
> > > > > index 667fa016496e..a6ea89a5d51a 100644
> > > > > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/acr/hsfw.c
> > > > > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/acr/hsfw.c
> > > > > @@ -142,11 +142,12 @@ nvkm_acr_hsfw_load_bl(struct
nvkm_acr *acr, const
> > > > > char *name, int ver,
> > > > >
> > > > >         hsfw->imem_size = desc->code_size;
> > > > >         hsfw->imem_tag = desc->start_tag;
> > > > > -       hsfw->imem = kmalloc(desc->code_size,
GFP_KERNEL);
> > > > > -       memcpy(hsfw->imem, data + desc->code_off,
desc->code_size);
> > > > > -
> > > > > +       hsfw->imem = kmemdup(data +
desc->code_off, desc->code_size,
> > > > > GFP_KERNEL);
> > > > >         nvkm_firmware_put(fw);
> > > > > -       return 0;
> > > > > +       if (!hsfw->imem)
> > > > > +               return -ENOMEM;
> > > > > +       else
> > > > > +               return 0;
> > > > >  }
> > > > >
> > > > >  int
> > > > > --
> > > > > 2.25.1
> > > > >
> > > >
> > > > As stated before, umn.edu is still not allowed to contribute
to the
> > > > Linux kernel.  Please work with your administration to
resolve this
> > > > issue.
> > > >
> > >
> > > --
> > > Cheers,
> > >  Lyude Paul (she/her)
> > >  Software Engineer at Red Hat
> > >
> >
>

On Fri, Jan 28, 2022 at 08:57:56PM +0100, Karol Herbst
wrote:
> On Fri, Jan 28, 2022 at 8:54 PM Alex Deucher <alexdeucher at
gmail.com> wrote:
> >
> > On Fri, Jan 28, 2022 at 2:20 PM Lyude Paul <lyude at redhat.com>
wrote:
> > >
> > > Sigh-thank you for catching this - I had totally forgot about the
umn.edu ban.
> > > I pushed this already but I will go ahead and send a revert for
this patch.
> > > Will cc you on it as well.
> >
> > This seems short-sighted.  If the patch is valid I see no reason to
> > not accept it.  I'm not trying to downplay the mess umn got into,
but
> > as long as the patch is well scrutinized and fixes a valid issue, it
> > should be applied rather than leaving potential bugs in place.
> >
> > Alex
> >
> 
> Even though knowing that malicious code can be introduced via
> perfectly fine looking patches, and sometimes one will never spot the
> problem, this patch isn't all that bad tbh.
> 
> So should we reject patches out of "policies" or should we just
be
> extra careful? But not addressing the concerns as Greg pointed out is
> also kind of a bad move, but also not knowing what the state of
> resolving this mess is anyway.
If you think the change is correct, and are willing to sign off on it,
that's fine for now.

The big issue here is that the umn.edu "researchers" are not folling
the
very basic and simple proceedures that we worked to set up with them,
and have resumed sending buggy patches to the kernel community.  If you
think this one is not a problem, then feel free to keep it.

As I just now told their administration, it is very difficult to tell
malice from incompetence.  If this is malice, then we need to defend our
community.  If this is just incompetence, then the university has to
handle that as the contributions are coming in their name, _and_ we also
need to defend from that.  What we have tried in the past for this group
is obviously not working, so we now will need to do something else.

{sigh}

greg k-h

On Fri, Jan 28, 2022 at 1:58 PM Karol Herbst <kherbst at redhat.com>
wrote:
>
> On Fri, Jan 28, 2022 at 8:54 PM Alex Deucher <alexdeucher at
gmail.com> wrote:
> >
> > On Fri, Jan 28, 2022 at 2:20 PM Lyude Paul <lyude at redhat.com>
wrote:
> > >
> > > Sigh-thank you for catching this - I had totally forgot about the
umn.edu ban.
> > > I pushed this already but I will go ahead and send a revert for
this patch.
> > > Will cc you on it as well.
> >
> > This seems short-sighted.  If the patch is valid I see no reason to
> > not accept it.  I'm not trying to downplay the mess umn got into,
but
> > as long as the patch is well scrutinized and fixes a valid issue, it
> > should be applied rather than leaving potential bugs in place.
> >
> > Alex
> >
>
> Even though knowing that malicious code can be introduced via
> perfectly fine looking patches, and sometimes one will never spot the
> problem, this patch isn't all that bad tbh.
>
> So should we reject patches out of "policies" or should we just
be
> extra careful? But not addressing the concerns as Greg pointed out is
> also kind of a bad move, but also not knowing what the state of
> resolving this mess is anyway.

Seeing some discussion here, I feel I owe you some quick updates on
the state. We sent three testing patches in August 2020, which is a
serious mistake. We never did that again and will never do that again.
All other patches including recent ones were sent to fix real bugs,
not to introduce problems. Clearly, although most of the patches are
valid, some patches are still not good enough, but it is not about
malice. Fixing bugs in Linux isn't an easy task and takes so much
effort.

We did not ignore the concerns pointed out by Greg, and have seriously
taken some actions. For example, we explained how our static-analysis
tool found the bugs, and members in my research group have internally
cross-reviewed the found bugs. We sent these patches after contacting
Greg---I thought Greg allowed us to send patches, but also requested
us to work on the last process with our administration. Unfortunately,
the process has been slow during the pandemic, but I hope this can be
resolved soon. Of course, before this is resolved, we will not send
any more patches.



>
> >
> > >
> > > On Fri, 2022-01-28 at 11:18 +0100, Greg KH wrote:
> > > > On Tue, Jan 25, 2022 at 12:58:55AM +0800, Zhou Qingyang
wrote:
> > > > > In nvkm_acr_hsfw_load_bl(), the return value of
kmalloc() is directly
> > > > > passed to memcpy(), which could lead to undefined
behavior on failure
> > > > > of kmalloc().
> > > > >
> > > > > Fix this bug by using kmemdup() instead of
kmalloc()+memcpy().
> > > > >
> > > > > This bug was found by a static analyzer.
> > > > >
> > > > > Builds with 'make allyesconfig' show no new
warnings,
> > > > > and our static analyzer no longer warns about this
code.
> > > > >
> > > > > Fixes: 22dcda45a3d1 ("drm/nouveau/acr: implement
new subdev to replace
> > > > > "secure boot"")
> > > > > Signed-off-by: Zhou Qingyang <zhou1615 at
umn.edu>
> > > > > ---
> > > > > The analysis employs differential checking to identify
inconsistent
> > > > > security operations (e.g., checks or kfrees) between
two code paths
> > > > > and confirms that the inconsistent operations are not
recovered in the
> > > > > current function or the callers, so they constitute
bugs.
> > > > >
> > > > > Note that, as a bug found by static analysis, it can be
a false
> > > > > positive or hard to trigger. Multiple researchers have
cross-reviewed
> > > > > the bug.
> > > > >
> > > > >  drivers/gpu/drm/nouveau/nvkm/subdev/acr/hsfw.c | 9
+++++----
> > > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git
a/drivers/gpu/drm/nouveau/nvkm/subdev/acr/hsfw.c
> > > > > b/drivers/gpu/drm/nouveau/nvkm/subdev/acr/hsfw.c
> > > > > index 667fa016496e..a6ea89a5d51a 100644
> > > > > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/acr/hsfw.c
> > > > > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/acr/hsfw.c
> > > > > @@ -142,11 +142,12 @@ nvkm_acr_hsfw_load_bl(struct
nvkm_acr *acr, const
> > > > > char *name, int ver,
> > > > >
> > > > >         hsfw->imem_size = desc->code_size;
> > > > >         hsfw->imem_tag = desc->start_tag;
> > > > > -       hsfw->imem = kmalloc(desc->code_size,
GFP_KERNEL);
> > > > > -       memcpy(hsfw->imem, data + desc->code_off,
desc->code_size);
> > > > > -
> > > > > +       hsfw->imem = kmemdup(data +
desc->code_off, desc->code_size,
> > > > > GFP_KERNEL);
> > > > >         nvkm_firmware_put(fw);
> > > > > -       return 0;
> > > > > +       if (!hsfw->imem)
> > > > > +               return -ENOMEM;
> > > > > +       else
> > > > > +               return 0;
> > > > >  }
> > > > >
> > > > >  int
> > > > > --
> > > > > 2.25.1
> > > > >
> > > >
> > > > As stated before, umn.edu is still not allowed to contribute
to the
> > > > Linux kernel.  Please work with your administration to
resolve this
> > > > issue.
> > > >
> > >
> > > --
> > > Cheers,
> > >  Lyude Paul (she/her)
> > >  Software Engineer at Red Hat
> > >
> >
>

--
Kangjie Lu
Assistant Professor
Department of Computer Science and Engineering
University of Minnesota
https://www-users.cs.umn.edu/~kjlu

On Sat, Jan 29, 2022 at 08:18:55AM -0600, Kangjie Lu
wrote:
> On Fri, Jan 28, 2022 at 1:58 PM Karol Herbst <kherbst at redhat.com>
wrote:
> >
> > On Fri, Jan 28, 2022 at 8:54 PM Alex Deucher <alexdeucher at
gmail.com> wrote:
> > >
> > > On Fri, Jan 28, 2022 at 2:20 PM Lyude Paul <lyude at
redhat.com> wrote:
> > > >
> > > > Sigh-thank you for catching this - I had totally forgot
about the umn.edu ban.
> > > > I pushed this already but I will go ahead and send a revert
for this patch.
> > > > Will cc you on it as well.
> > >
> > > This seems short-sighted.  If the patch is valid I see no reason
to
> > > not accept it.  I'm not trying to downplay the mess umn got
into, but
> > > as long as the patch is well scrutinized and fixes a valid issue,
it
> > > should be applied rather than leaving potential bugs in place.
> > >
> > > Alex
> > >
> >
> > Even though knowing that malicious code can be introduced via
> > perfectly fine looking patches, and sometimes one will never spot the
> > problem, this patch isn't all that bad tbh.
> >
> > So should we reject patches out of "policies" or should we
just be
> > extra careful? But not addressing the concerns as Greg pointed out is
> > also kind of a bad move, but also not knowing what the state of
> > resolving this mess is anyway.
> 
> 
> Seeing some discussion here, I feel I owe you some quick updates on
> the state. We sent three testing patches in August 2020, which is a
> serious mistake. We never did that again and will never do that again.
> All other patches including recent ones were sent to fix real bugs,
> not to introduce problems. Clearly, although most of the patches are
> valid, some patches are still not good enough, but it is not about
> malice. Fixing bugs in Linux isn't an easy task and takes so much
> effort.
> 
> We did not ignore the concerns pointed out by Greg, and have seriously
> taken some actions. For example, we explained how our static-analysis
> tool found the bugs, and members in my research group have internally
> cross-reviewed the found bugs. We sent these patches after contacting
> Greg---I thought Greg allowed us to send patches, but also requested
> us to work on the last process with our administration.
I do not recall saying anything like this at all.

On January 4, I wrote to you and your coworkers on the mailing list
message https://lore.kernel.org/r/YdQfCGf8qr5zZJef at kroah.com by saying:

	Note that your university is still in many kernel maintainer's
	ignore-list (myself included, I dug this up as I saw Fei's response.)
	Please work with your administration and the process that is currently
	happening in order to give you all the needed training so you will not
	keep causing these types of basic errors that keep your patches from
	being accepted.

	*plonk*

And then later in a private email to you on January 5 where you emailed
Kees and me to try to see if you were allowed to start sending patches
again, I said:

	A kernel developer with lots of experience has already offered to work
	with your university.  Hopefully that process has already started, if
	not, I suggest contacting your administration as they should know who
	this is.

and then I closed with:

	Right now you all are still on my "ignore email" lists for patches.

The patches recently submitted have been shown to be incomplete and in
some places, completely wrong.  I have contacted your administration
about this issue because they asked to know if there were any problems
in the future at our last discussion.  In that response today, I wrote:

	I know that incompetence can often times be hard to distinguish from
	malice, but given the track-record here, we are now going to have to
	treat this as malice.  If it is just incompetence, well, that's
	something that your organization needs to overcome.

	Either way, this is not something that the Linux kernel community should
	be forced to endure.

So to be explicit, so you do not misunderstand me somehow:

	No more patches from umn.edu should be accepted into the Linux
	kernel until further public notice.  They should be considered a
	"bad actor" given their prior history of submissions and lack of
	complying with the kernel community's prior requirements to
	them.

Is this understood?

greg k-h