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