linux-kernel - Re: [EXTERNAL] Re: [PATCH v5 13/17] drm/imagination: Implement context creation/destruction ioctls

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8fb0a30a06e31357128b4d2248a923beff80d029.camel@imgtec.com>
Date:   Fri, 18 Aug 2023 11:00:04 +0000
From:   Sarah Walker <Sarah.Walker@...tec.com>
To:     "jannh@...gle.com" <jannh@...gle.com>
CC:     "tzimmermann@...e.de" <tzimmermann@...e.de>,
        "afd@...com" <afd@...com>,
        Donald Robson <Donald.Robson@...tec.com>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "matthew.brost@...el.com" <matthew.brost@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "luben.tuikov@....com" <luben.tuikov@....com>,
        "boris.brezillon@...labora.com" <boris.brezillon@...labora.com>,
        "faith.ekstrand@...labora.com" <faith.ekstrand@...labora.com>,
        "dakr@...hat.com" <dakr@...hat.com>,
        "mripard@...nel.org" <mripard@...nel.org>,
        "hns@...delico.com" <hns@...delico.com>,
        "christian.koenig@....com" <christian.koenig@....com>
Subject: Re: [EXTERNAL] Re: [PATCH v5 13/17] drm/imagination: Implement
 context creation/destruction ioctls

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@...tec.com> 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;
> > +}