[PATCH] drm/nouveau: Prevents NULL pointer dereference in nouveau_uvmm_sm_prepare

[PATCH] drm/nouveau: Prevents NULL pointer dereference in nouveau_uvmm_sm_prepare

[Yuran Pereira]

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

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [PATCH] drm/nouveau: Prevents NULL pointer dereference in nouveau_uvmm_sm_prepare

[Danilo Krummrich]

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

Re: [PATCH] drm/nouveau: Prevents NULL pointer dereference in nouveau_uvmm_sm_prepare

[Yuran Pereira]

Hello Danilo,
On Tue, Nov 14, 2023 at 05:23:59PM +0100, Danilo Krummrich wrote:
> Hi Yuran,
> 
> 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.
> 
Yes, you're right. I certainly hadn't noticed that when I first
submitted this patch.
> 
> 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. */
> 
Sure, I'll do that, sounds reasonable.

Thank you for your feedback.

Yuran
> 
> Yeah, I see how this unnecessary check made you think so.
> 
> - Danilo
> 
>