Greg KH
2022-Jan-28 10:18 UTC
[Nouveau] [PATCH] drm/nouveau/acr: Fix undefined behavior in nvkm_acr_hsfw_load_bl()
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.
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