Lyude Paul
2022-Jan-28 19:20 UTC
[Nouveau] [PATCH] drm/nouveau/acr: Fix undefined behavior in nvkm_acr_hsfw_load_bl()
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. 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
Alex Deucher
2022-Jan-28 19:53 UTC
[Nouveau] [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 >
Greg KH
2022-Jan-29 08:24 UTC
[Nouveau] [PATCH] drm/nouveau/acr: Fix undefined behavior in nvkm_acr_hsfw_load_bl()
On Fri, Jan 28, 2022 at 02:20:24PM -0500, Lyude Paul 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.No worries, thanks for doing this. greg k-h