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

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

>
> 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, 2022-01-28 at 14:53 -0500, Alex Deucher 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.
> 
Yeah - I sent a revert for this, but that was mainly just to make sure I
didn't cause problems with Linus or something like that. If it's all the
same
I'd much rather just leave this patch in, as looking at the code the fix
seems
completely valid.
> 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
> > 
> 
-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

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
> >
>