On Fri, 2023-08-18 at 00:42 +0200, Jann Horn wrote:
> *** CAUTION: This email originates from a source not known to Imagination Technologies. Think before you click a link or open an attachment ***
>
> On Wed, Aug 16, 2023 at 10:25 AM Sarah Walker <sarah.walker@xxxxxxxxxx> wrote:
> > Implement ioctls for the creation and destruction of contexts. Contexts are
> > used for job submission and each is associated with a particular job type.
> [...]
> > +/**
> > + * pvr_context_create() - Create a context.
> > + * @pvr_file: File to attach the created context to.
> > + * @args: Context creation arguments.
> > + *
> > + * Return:
> > + * * 0 on success, or
> > + * * A negative error code on failure.
> > + */
> > +int pvr_context_create(struct pvr_file *pvr_file, struct drm_pvr_ioctl_create_context_args *args)
> > +{
> > + struct pvr_device *pvr_dev = pvr_file->pvr_dev;
> > + struct pvr_context *ctx;
> > + int ctx_size;
> > + int err;
> > +
> > + /* Context creation flags are currently unused and must be zero. */
> > + if (args->flags)
> > + return -EINVAL;
> > +
> > + ctx_size = get_fw_obj_size(args->type);
> > + if (ctx_size < 0)
> > + return ctx_size;
> > +
> > + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> > + if (!ctx)
> > + return -ENOMEM;
> > +
> > + ctx->data_size = ctx_size;
> > + ctx->type = args->type;
> > + ctx->flags = args->flags;
> > + ctx->pvr_dev = pvr_dev;
> > + kref_init(&ctx->ref_count);
> > +
> > + err = remap_priority(pvr_file, args->priority, &ctx->priority);
> > + if (err)
> > + goto err_free_ctx;
> > +
> > + ctx->vm_ctx = pvr_vm_context_lookup(pvr_file, args->vm_context_handle);
> > + if (IS_ERR(ctx->vm_ctx)) {
> > + err = PTR_ERR(ctx->vm_ctx);
> > + goto err_free_ctx;
> > + }
> > +
> > + ctx->data = kzalloc(ctx_size, GFP_KERNEL);
> > + if (!ctx->data) {
> > + err = -ENOMEM;
> > + goto err_put_vm;
> > + }
> > +
> > + err = init_fw_objs(ctx, args, ctx->data);
> > + if (err)
> > + goto err_free_ctx_data;
> > +
> > + err = pvr_fw_object_create(pvr_dev, ctx_size, PVR_BO_FW_FLAGS_DEVICE_UNCACHED,
> > + ctx_fw_data_init, ctx, &ctx->fw_obj);
> > + if (err)
> > + goto err_free_ctx_data;
> > +
> > + err = xa_alloc(&pvr_dev->ctx_ids, &ctx->ctx_id, ctx, xa_limit_32b, GFP_KERNEL);
> > + if (err)
> > + goto err_destroy_fw_obj;
> > +
> > + err = xa_alloc(&pvr_file->ctx_handles, &args->handle, ctx, xa_limit_32b, GFP_KERNEL);
> > + if (err)
> > + goto err_release_id;
>
> This bailout looks a bit dodgy. We have already inserted ctx into
> &pvr_dev->ctx_ids, and now we just take it out again. If someone could
> concurrently call pvr_context_lookup_id() on the ID we just allocated
> (I don't understand enough about what's going on here at a high level
> to be able to tell if that's possible), I think they would be able to
> elevate the ctx->ref_count from 1 to 2, and then on the bailout path
> we'll just free the ctx without looking at the refcount.
>
> If this can't happen, it might be a good idea to add a comment
> explaining why. If it can happen, I guess one way to fix it would be
> to replace this last bailout with a call to pvr_context_put()?
Yes, I think you're correct here. I don't think there's anything in the current
patch set that can actually trigger this, but it definitely needs fixing.
Thanks,
Sarah
>
>
> > +
> > + return 0;
> > +
> > +err_release_id:
> > + xa_erase(&pvr_dev->ctx_ids, ctx->ctx_id);
> > +
> > +err_destroy_fw_obj:
> > + pvr_fw_object_destroy(ctx->fw_obj);
> > +
> > +err_free_ctx_data:
> > + kfree(ctx->data);
> > +
> > +err_put_vm:
> > + pvr_vm_context_put(ctx->vm_ctx);
> > +
> > +err_free_ctx:
> > + kfree(ctx);
> > + return err;
> > +}
