linux-kernel - Re: [PATCH v7 5/6] [media] vb2: add out-fence support to QBUF

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAPBb6MU-83QXHht_gLciGzfZtNxJL_=Fj5h1yfwPEt3vSKHVXg@mail.gmail.com>
Date:   Mon, 15 Jan 2018 16:12:37 +0900
From:   Alexandre Courbot <acourbot@...omium.org>
To:     Gustavo Padovan <gustavo@...ovan.org>
Cc:     Linux Media Mailing List <linux-media@...r.kernel.org>,
        Hans Verkuil <hverkuil@...all.nl>,
        Mauro Carvalho Chehab <mchehab@....samsung.com>,
        Shuah Khan <shuahkh@....samsung.com>,
        Pawel Osciak <pawel@...iak.com>,
        Sakari Ailus <sakari.ailus@....fi>,
        Brian Starkey <brian.starkey@....com>,
        Thierry Escande <thierry.escande@...labora.com>,
        linux-kernel@...r.kernel.org,
        Gustavo Padovan <gustavo.padovan@...labora.com>
Subject: Re: [PATCH v7 5/6] [media] vb2: add out-fence support to QBUF

On Thu, Jan 11, 2018 at 1:07 AM, Gustavo Padovan <gustavo@...ovan.org> wrote:
>  /*
>   * vb2_start_streaming() - Attempt to start streaming.
>   * @q:         videobuf2 queue
> @@ -1489,18 +1562,16 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
>         if (vb->in_fence) {
>                 ret = dma_fence_add_callback(vb->in_fence, &vb->fence_cb,
>                                              vb2_qbuf_fence_cb);
> -               if (ret == -EINVAL) {
> +               /* is the fence signaled? */
> +               if (ret == -ENOENT) {
> +                       dma_fence_put(vb->in_fence);
> +                       vb->in_fence = NULL;
> +               } else if (ret) {
>                         spin_unlock_irqrestore(&vb->fence_cb_lock, flags);
>                         goto err;
> -               } else if (!ret) {
> -                       goto fill;
>                 }
> -
> -               dma_fence_put(vb->in_fence);
> -               vb->in_fence = NULL;

This chunk seems to deal with input fences, shouldn't it be part of
the previous patch instead of this one?

>
> -       if ((b->fence_fd != 0 && b->fence_fd != -1) &&
> -           !(b->flags & V4L2_BUF_FLAG_IN_FENCE)) {
> +       if (b->fence_fd > 0 && !(b->flags & V4L2_BUF_FLAG_IN_FENCE)) {
>                 dprintk(1, "%s: fence_fd set without IN_FENCE flag\n", opname);
>                 return -EINVAL;
>         }
>
> +       if (b->fence_fd == -1 && (b->flags & V4L2_BUF_FLAG_IN_FENCE)) {
> +               dprintk(1, "%s: IN_FENCE flag set but no fence_fd\n", opname);
> +               return -EINVAL;
> +       }
> +

Same here?

>         return __verify_planes_array(q->bufs[b->index], b);
>  }
>
> @@ -212,7 +216,12 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
>         b->sequence = vbuf->sequence;
>         b->reserved = 0;
>
> -       b->fence_fd = 0;
> +       if (b->flags & V4L2_BUF_FLAG_OUT_FENCE) {
> +               b->fence_fd = vb->out_fence_fd;
> +       } else {
> +               b->fence_fd = 0;
> +       }

Sorry if this has already been discussed, but I don't remember the
outcome if it has.

I wonder if doing this here could not make out_fence_fd leak in
situations where we don't need/want it to. Let's take for instance a
multi-process user program. One process queues a buffer with an
OUT_FENCE and gets a valid fd in fence_fd upon return. Then the other
process performs a QUERYBUF and gets the same fence_fd - which is
invalid in its context. Would it not be preferable fill the out fence
information only when queuing buffers, since it is the only time where
we are guaranteed it will be usable by the caller?

Similarly, when a buffer is processed and user-space performs a DQBUF,
the V4L2_BUF_FLAG_OUT_FENCE will be set but fence_fd will be 0. Again,
limiting the return of out fence information to QBUF would prevent
this.

If we go that route, out_fence_fd could maybe become a local variable
of vb2_qbuf() instead of being a member of vb2_buffer, and would be
returned by vb2_setup_out_fence(). This would guarantee it does not
leak anywhere else.