[Linaro-mm-sig] Re: [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@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) {