Yuran Pereira
2023-Oct-26 17:04 UTC
[PATCH] drm/nouveau: Prevents NULL pointer dereference in nouveau_uvmm_sm_prepare
There are instances where the "args" argument passed to nouveau_uvmm_sm_prepare() is NULL. I.e. when nouveau_uvmm_sm_prepare() is called from nouveau_uvmm_sm_unmap_prepare() ``` static int nouveau_uvmm_sm_unmap_prepare(struct nouveau_uvmm *uvmm, ... { return nouveau_uvmm_sm_prepare(uvmm, new, ops, NULL); } ``` The problem is that op_map_prepare() which nouveau_uvmm_sm_prepare calls, dereferences this value, which can lead to a NULL pointer dereference. ``` static int op_map_prepare(struct nouveau_uvmm *uvmm, ... { ... uvma->region = args->region; <-- Dereferencing of possibly NULL pointer uvma->kind = args->kind; <-- ... } ``` ``` static int nouveau_uvmm_sm_prepare(struct nouveau_uvmm *uvmm, ... struct uvmm_map_args *args) { struct drm_gpuva_op *op; u64 vmm_get_start = args ? args->addr : 0; u64 vmm_get_end = args ? args->addr + args->range : 0; int ret; drm_gpuva_for_each_op(op, ops) { switch (op->op) { case DRM_GPUVA_OP_MAP: { u64 vmm_get_range = vmm_get_end - vmm_get_start; ret = op_map_prepare(uvmm, &new->map, &op->map, args); <--- if (ret) goto unwind; if (args && vmm_get_range) { ret = nouveau_uvmm_vmm_get(uvmm, vmm_get_start, vmm_get_range); if (ret) { op_map_prepare_unwind(new->map); goto unwind; } } ... ``` Since the switch "case DRM_GPUVA_OP_MAP", also NULL checks "args" after the call to op_map_prepare(), my guess is that we should probably relocate this check to a point before op_map_prepare() is called. This patch ensures that the value of args is checked before calling op_map_prepare() Addresses-Coverity-ID: 1544574 ("Dereference after null check") Signed-off-by: Yuran Pereira <yuran.pereira at hotmail.com> --- drivers/gpu/drm/nouveau/nouveau_uvmm.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c index aae780e4a4aa..6baa481eb2c8 100644 --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c @@ -620,11 +620,14 @@ nouveau_uvmm_sm_prepare(struct nouveau_uvmm *uvmm, case DRM_GPUVA_OP_MAP: { u64 vmm_get_range = vmm_get_end - vmm_get_start; + if (!args) + goto unwind; + ret = op_map_prepare(uvmm, &new->map, &op->map, args); if (ret) goto unwind; - if (args && vmm_get_range) { + if (vmm_get_range) { ret = nouveau_uvmm_vmm_get(uvmm, vmm_get_start, vmm_get_range); if (ret) { -- 2.25.1
Danilo Krummrich
2023-Nov-14 16:23 UTC
[Nouveau] [PATCH] drm/nouveau: Prevents NULL pointer dereference in nouveau_uvmm_sm_prepare
Hi Yuran, On 10/26/23 19:03, Yuran Pereira wrote:> There are instances where the "args" argument passed to > nouveau_uvmm_sm_prepare() is NULL. > > I.e. when nouveau_uvmm_sm_prepare() is called from > nouveau_uvmm_sm_unmap_prepare() > > ``` > static int > nouveau_uvmm_sm_unmap_prepare(struct nouveau_uvmm *uvmm, > ... > { > return nouveau_uvmm_sm_prepare(uvmm, new, ops, NULL); > } > ``` > > The problem is that op_map_prepare() which nouveau_uvmm_sm_prepare > calls, dereferences this value, which can lead to a NULL pointer > dereference.op_map_prepare() can't be called with `args` being NULL, since when called through nouveau_uvmm_sm_unmap_prepare() we can't hit the DRM_GPUVA_OP_MAP case at all. Unmapping something never leads to a new mapping being created, it can lead to remaps though.> > ``` > static int > op_map_prepare(struct nouveau_uvmm *uvmm, > ... > { > ... > uvma->region = args->region; <-- Dereferencing of possibly NULL pointer > uvma->kind = args->kind; <-- > ... > } > ``` > > ``` > static int > nouveau_uvmm_sm_prepare(struct nouveau_uvmm *uvmm, > ... > struct uvmm_map_args *args) > { > struct drm_gpuva_op *op; > u64 vmm_get_start = args ? args->addr : 0; > u64 vmm_get_end = args ? args->addr + args->range : 0; > int ret; > > drm_gpuva_for_each_op(op, ops) { > switch (op->op) { > case DRM_GPUVA_OP_MAP: { > u64 vmm_get_range = vmm_get_end - vmm_get_start; > > ret = op_map_prepare(uvmm, &new->map, &op->map, args); <--- > if (ret) > goto unwind; > > if (args && vmm_get_range) { > ret = nouveau_uvmm_vmm_get(uvmm, vmm_get_start, > vmm_get_range); > if (ret) { > op_map_prepare_unwind(new->map); > goto unwind; > } > } > ... > ``` > > Since the switch "case DRM_GPUVA_OP_MAP", also NULL checks "args"This check is not required for the reason given above. If you like, you can change this patch up to remove the args check and add a comment like: /* args can't be NULL when called for a map operation. */> after the call to op_map_prepare(), my guess is that we should > probably relocate this check to a point before op_map_prepare() > is called.Yeah, I see how this unnecessary check made you think so. - Danilo> > This patch ensures that the value of args is checked before > calling op_map_prepare() > > Addresses-Coverity-ID: 1544574 ("Dereference after null check") > Signed-off-by: Yuran Pereira <yuran.pereira at hotmail.com> > --- > drivers/gpu/drm/nouveau/nouveau_uvmm.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c > index aae780e4a4aa..6baa481eb2c8 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c > +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c > @@ -620,11 +620,14 @@ nouveau_uvmm_sm_prepare(struct nouveau_uvmm *uvmm, > case DRM_GPUVA_OP_MAP: { > u64 vmm_get_range = vmm_get_end - vmm_get_start; > > + if (!args) > + goto unwind; > + > ret = op_map_prepare(uvmm, &new->map, &op->map, args); > if (ret) > goto unwind; > > - if (args && vmm_get_range) { > + if (vmm_get_range) { > ret = nouveau_uvmm_vmm_get(uvmm, vmm_get_start, > vmm_get_range); > if (ret) {