linux-kernel - Re: [PATCH] media: venus: fix reported size of 0-length buffers

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: <cce537955998a62d4fa36e466940fb3b5a9f21cf.camel@ndufresne.ca>
Date:   Thu, 22 Nov 2018 18:53:39 -0500
From:   Nicolas Dufresne <nicolas@...fresne.ca>
To:     Alexandre Courbot <acourbot@...omium.org>
Cc:     Stanimir Varbanov <stanimir.varbanov@...aro.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        linux-arm-msm@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] media: venus: fix reported size of 0-length buffers

Le jeudi 22 novembre 2018 à 17:31 +0900, Alexandre Courbot a écrit :
> On Fri, Nov 16, 2018 at 1:49 AM Nicolas Dufresne <nicolas@...fresne.ca> wrote:
> > Le mercredi 14 novembre 2018 à 13:12 +0900, Alexandre Courbot a écrit :
> > > On Wed, Nov 14, 2018 at 3:54 AM Nicolas Dufresne <nicolas@...fresne.ca> wrote:
> > > > 
> > > > Le mar. 13 nov. 2018 04 h 30, Alexandre Courbot <acourbot@...omium.org> a écrit :
> > > > > The last buffer is often signaled by an empty buffer with the
> > > > > V4L2_BUF_FLAG_LAST flag set. Such buffers were returned with the
> > > > > bytesused field set to the full size of the OPB, which leads
> > > > > user-space to believe that the buffer actually contains useful data. Fix
> > > > > this by passing the number of bytes reported used by the firmware.
> > > > 
> > > > That means the driver does not know on time which one is last. Why not just returned EPIPE to userspace on DQBUF and ovoid this useless roundtrip ?
> > > 
> > > Sorry, I don't understand what you mean. EPIPE is supposed to be
> > > returned after a buffer with V4L2_BUF_FLAG_LAST is made available for
> > > dequeue. This patch amends the code that prepares this LAST-flagged
> > > buffer. How could we avoid a roundtrip in this case?
> > 
> > Maybe it has changed, but when this was introduced, we found that some
> > firmware (Exynos MFC) could not know which one is last. Instead, it
> > gets an event saying there will be no more buffers.
> > 
> > Sending buffers with payload size to 0 just for the sake of setting the
> > V4L2_BUF_FLAG_LAST was considered a waste. Specially that after that,
> > every polls should return EPIPE. So in the end, we decided the it
> > should just unblock the userspace and return EPIPE.
> > 
> > If you look at the related GStreamer code, it completely ignores the
> > LAST flag. With fake buffer of size 0, userspace will endup dequeuing
> > and throwing away. This is not useful to the process of terminating the
> > decoding. To me, this LAST flag is not useful in this context.
> 
> Note that this patch does not interfere with DQBUF returning -EPIPE
> after the last buffer has been dequeued. It just fixes an invalid size
> that was returned for the last buffer.
> 
> Note also that if I understand the doc properly, the kernel driver
> *must* set the V4L2_BUF_FLAG_LAST on the last buffer. With Venus the
> last buffer is signaled by the firmware with an empty buffer. That's
> not something we can change or predict earlier, so in order to respect
> the specification we need to return that empty buffer. After that
> DQBUF will behave as expected (returning -EPIPE), so GStreamer should
> be happy as well.
> 
> Without the proposed fix however, GStreamer would receive the last
> buffer with an incorrect size, and thus interpret random data as a
> frame.
> 
> So to me this fix seems to be both correct, and needed. Isn't it?

Totally, thanks for the extra clarification.

> 
> Cheers,
> Alex.