[bug report] drm/imagination: Implement firmware infrastructure and META FW support

* [bug report] drm/imagination: Implement firmware infrastructure and META FW support
@ 2023-12-06 12:50 Dan Carpenter
  0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2023-12-06 12:50 UTC (permalink / raw)
  To: sarah.walker; +Cc: dri-devel

Hello Sarah Walker,

The patch cc1aeedb98ad: "drm/imagination: Implement firmware
infrastructure and META FW support" from Nov 22, 2023 (linux-next),
leads to the following Smatch static checker warning:

	drivers/gpu/drm/imagination/pvr_vm.c:631 pvr_vm_create_context()
	error: 'vm_ctx->mmu_ctx' dereferencing possible ERR_PTR()

drivers/gpu/drm/imagination/pvr_vm.c
    597         vm_ctx = kzalloc(sizeof(*vm_ctx), GFP_KERNEL);
    598         if (!vm_ctx)
    599                 return ERR_PTR(-ENOMEM);
    600 
    601         drm_gem_private_object_init(&pvr_dev->base, &vm_ctx->dummy_gem, 0);
    602 
    603         vm_ctx->pvr_dev = pvr_dev;
    604         kref_init(&vm_ctx->ref_count);
    605         mutex_init(&vm_ctx->lock);
    606 
    607         drm_gpuvm_init(&vm_ctx->gpuvm_mgr,
    608                        is_userspace_context ? "PowerVR-user-VM" : "PowerVR-FW-VM",
    609                        0, &pvr_dev->base, &vm_ctx->dummy_gem,
    610                        0, 1ULL << device_addr_bits, 0, 0, &pvr_vm_gpuva_ops);
    611 
    612         vm_ctx->mmu_ctx = pvr_mmu_context_create(pvr_dev);
    613         err = PTR_ERR_OR_ZERO(&vm_ctx->mmu_ctx);
                                      ^
s/&//.

The address is never an error pointer so this will always return 0.
Fixing this will silence the static checker but there are some other
issues as well.

    614         if (err) {
    615                 vm_ctx->mmu_ctx = NULL;

Setting vm_ctx->mmu_ctx is not sufficient.

    616                 goto err_put_ctx;
    617         }
    618 
    619         if (is_userspace_context) {
    620                 err = pvr_fw_object_create(pvr_dev, sizeof(struct rogue_fwif_fwmemcontext),
    621                                            PVR_BO_FW_FLAGS_DEVICE_UNCACHED,
    622                                            fw_mem_context_init, vm_ctx, &vm_ctx->fw_mem_ctx_obj);
    623 
    624                 if (err)
    625                         goto err_page_table_destroy;
    626         }
    627 
    628         return vm_ctx;
    629 
    630 err_page_table_destroy:
--> 631         pvr_mmu_context_destroy(vm_ctx->mmu_ctx);

This will lead to a double free.  Delete.

    632 
    633 err_put_ctx:
    634         pvr_vm_context_put(vm_ctx);

The pvr_vm_context_put() function does:

	kref_put(&vm_ctx->ref_count, pvr_vm_context_release);

I really think that with kref free functions the way to do it is to
wait until the last possible momement when everything has been allocated
before we set up the kref release code.  Here the pvr_vm_context_release()
will call:

	pvr_mmu_context_destroy(vm_ctx->mmu_ctx);

We already did that on line 631 as mentioned so it's a double free.  But
also imagine if vm_ctx->mmu_ctx is NULL, then it will lead to a NULL
dereference.

The pvr_vm_context_release() function has several WARN() functions that
trigger if not everything is set up.  It's complicated to review.  So I
kind of always think that people should manually kfree() things in their
allocation functions and then do a kref_init() at the end.

    635 
    636         return ERR_PTR(err);
    637 }

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 3+ messages in thread