| 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
| ||
|
Message-ID: <8963a3ee-532f-eb2a-f417-ce62c8d18bd2@collabora.com>
Date: Wed, 12 Apr 2023 16:13:20 +0300
From: Dmitry Osipenko <dmitry.osipenko@...labora.com>
To: Emil Velikov <emil.l.velikov@...il.com>
Cc: David Airlie <airlied@...hat.com>,
Gerd Hoffmann <kraxel@...hat.com>,
Gurchetan Singh <gurchetansingh@...omium.org>,
Chia-I Wu <olvaffe@...il.com>, Daniel Vetter <daniel@...ll.ch>,
Rob Clark <robdclark@...il.com>,
Marek Olšák <maraeo@...il.com>,
Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@....com>,
Emil Velikov <emil.velikov@...labora.com>,
kernel@...labora.com, linux-kernel@...r.kernel.org,
dri-devel@...ts.freedesktop.org,
virtualization@...ts.linux-foundation.org
Subject: Re: [PATCH v5 3/3] drm/virtio: Support sync objects
Hello,
On 4/11/23 14:07, Emil Velikov wrote:
> Hi Dmitry,
>
> On Sun, 9 Apr 2023 at 13:40, Dmitry Osipenko
> <dmitry.osipenko@...labora.com> wrote:
>
>> +static void virtio_gpu_free_syncobjs(struct drm_syncobj **syncobjs,
>> + uint32_t nr_syncobjs)
>> +{
>> + uint32_t i = nr_syncobjs;
>> +
>> + while (i--) {
>> + if (syncobjs[i])
>> + drm_syncobj_put(syncobjs[i]);
>> + }
>> +
>> + kvfree(syncobjs);
>> +}
>> +
>
>> +static void virtio_gpu_reset_syncobjs(struct drm_syncobj **syncobjs,
>> + uint32_t nr_syncobjs)
>> +{
>> + uint32_t i;
>> +
>> + for (i = 0; i < nr_syncobjs; i++) {
>> + if (syncobjs[i])
>> + drm_syncobj_replace_fence(syncobjs[i], NULL);
>> + }
>> +}
>> +
>
> Can I bribe you (with cookies) about dropping the NULL checks above?
> They're dead code and rather misleading IMHO.
When userspace doesn't set the VIRTGPU_EXECBUF_SYNCOBJ_RESET flag in
virtio_gpu_parse_deps(), the syncobjs[i] is NULL. Hence not a dead code
at all :)
>> +static void
>> +virtio_gpu_free_post_deps(struct virtio_gpu_submit_post_dep *post_deps,
>> + uint32_t nr_syncobjs)
>> +{
>> + uint32_t i = nr_syncobjs;
>> +
>> + while (i--) {
>> + kfree(post_deps[i].chain);
>> + drm_syncobj_put(post_deps[i].syncobj);
>> + }
>> +
>> + kvfree(post_deps);
>> +}
>> +
>> +static int virtio_gpu_parse_post_deps(struct virtio_gpu_submit *submit)
>> +{
>> + struct drm_virtgpu_execbuffer *exbuf = submit->exbuf;
>> + struct drm_virtgpu_execbuffer_syncobj syncobj_desc;
>> + struct virtio_gpu_submit_post_dep *post_deps;
>> + u32 num_out_syncobjs = exbuf->num_out_syncobjs;
>> + size_t syncobj_stride = exbuf->syncobj_stride;
>> + int ret = 0, i;
>> +
>> + if (!num_out_syncobjs)
>> + return 0;
>> +
>> + post_deps = kvcalloc(num_out_syncobjs, sizeof(*post_deps), GFP_KERNEL);
>> + if (!post_deps)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < num_out_syncobjs; i++) {
>> + uint64_t address = exbuf->out_syncobjs + i * syncobj_stride;
>> +
>
> For my own education: what's the specifics/requirements behind the
> stride? is there a use-case for the stride to vary across in/out
> syncobj?
Stride is the same for in/out syncobjs as the same struct
drm_virtgpu_execbuffer_syncobj is used by both.
The out-syncobj don't use the "flags" field of
drm_virtgpu_execbuffer_syncobj. We could use separate strides and desc
for in/out syncobjs, but in practice it's unlikely that we will be
extending the desc anytime soon and usually there are not many
out-syncobj to care about the wasted field.
On the other hand, if we will ever need to extend desc for in-syncobjs,
there will be more wasted fields. Maybe it indeed won't hurt to split
in/out syncobjs, for consistency. I'll think about it for v6, thanks.
> Off the top of my head: userspace could have an array of larger
> structs, each embedding an syncobj. Thus passing the stride, the
> kernel will fetch/update them in-place w/o caring about the other
> data.
> Or perhaps there is another trick that userspace utilises the stride for?
Stride is only about potential future expansion of the struct
drm_virtgpu_execbuffer_syncobj with new fields. There shouldn't be any
special tricks for userspace to use.
>> + if (copy_from_user(&syncobj_desc,
>> + u64_to_user_ptr(address),
>> + min(syncobj_stride, sizeof(syncobj_desc)))) {
>> + ret = -EFAULT;
>> + break;
>> + }
>> +
>
> We seem to be parsing garbage(?) stack data in the syncobj_stride <
> sizeof(syncobj_desc) case.
>
> Zeroing/reseting the syncobj_desc on each iteration is one approach -
> be that fully or in part. Alternatively we could error out on
> syncobj_stride < sizeof(syncobj_desc).
Good catch! It indeed needs to be zeroed. Nothing terrible will happen
today for kernel if it will use garbage data, but a malfunctioning
userspace may happen to appear working properly.
>> + post_deps[i].point = syncobj_desc.point;
>> +
>> + if (syncobj_desc.flags) {
>> + ret = -EINVAL;
>> + break;
>> + }
>> +
>> + if (syncobj_desc.point) {
>> + post_deps[i].chain = dma_fence_chain_alloc();
>> + if (!post_deps[i].chain) {
>> + ret = -ENOMEM;
>> + break;
>> + }
>> + }
>> +
>> + post_deps[i].syncobj = drm_syncobj_find(submit->file,
>> + syncobj_desc.handle);
>> + if (!post_deps[i].syncobj) {
>> + ret = -EINVAL;
>
> I think we want a kfree(chain) here. Otherwise we'll leak it, right?
I'm sure there was a kfree here in one of previous version of the patch.
Another good catch, thanks :)
>> + break;
>> + }
>> + }
>> +
>> + if (ret) {
>> + virtio_gpu_free_post_deps(post_deps, i);
>> + return ret;
>> + }
>> +
>> + submit->num_out_syncobjs = num_out_syncobjs;
>> + submit->post_deps = post_deps;
>> +
>> + return 0;
>> +}
>> +
>
>
> With the two issues in virtio_gpu_parse_post_deps() addressed, the series is:
> Reviewed-by; Emil Velikov <emil.velikov@...labora.com>
Thanks you for the review!
--
Best regards,
Dmitry
Powered by blists - more mailing lists