linux-kernel - Re: [PATCH] venus: vdec: fix decoded data size

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: <CAPBb6MUBi+Dn5v4PKngxztFgKd6CA7bC1pKvWd1GMY9NJFoyZQ@mail.gmail.com>
Date:   Mon, 2 Jul 2018 17:51:03 +0900
From:   Alexandre Courbot <acourbot@...omium.org>
To:     vgarodia@...eaurora.org
Cc:     Stanimir Varbanov <stanimir.varbanov@...aro.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] venus: vdec: fix decoded data size

On Mon, Jul 2, 2018 at 4:44 PM Vikash Garodia <vgarodia@...eaurora.org> wrote:
>
> Exisiting code returns the max of the decoded

s/Exisiting/Existing

Also the lines of your commit message look pretty short - I think the
standard for kernel log messges is 72 chars?

> size and buffer size. It turns out that buffer
> size is always greater due to hardware alignment
> requirement. As a result, payload size given to
> client is incorrect. This change ensures that
> the bytesused is assigned to actual payload size.
>
> Change-Id: Ie6f3429c0cb23f682544748d181fa4fa63ca2e28
> Signed-off-by: Vikash Garodia <vgarodia@...eaurora.org>
> ---
>  drivers/media/platform/qcom/venus/vdec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index d079aeb..ada1d2f 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -890,7 +890,7 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
>
>                 vb = &vbuf->vb2_buf;
>                 vb->planes[0].bytesused =
> -                       max_t(unsigned int, opb_sz, bytesused);
> +                       min_t(unsigned int, opb_sz, bytesused);

Reviewed-by: Alexandre Courbot <acourbot@...omium.org>
Tested-by: Alexandre Courbot <acourbot@...omium.org>

This indeed reports the correct size to the client. If bytesused were
larger than the size of the buffer we would be having some trouble
anyway.

Actually in my tree I was using the following patch:

--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -924,13 +924,12 @@ static void vdec_buf_done(struct venus_inst
*inst, unsigned int buf_type,

               vb = &vbuf->vb2_buf;
               vb->planes[0].bytesused =
-                       max_t(unsigned int, opb_sz, bytesused);
+                       min_t(unsigned int, opb_sz, bytesused);
               vb->planes[0].data_offset = data_offset;
               vb->timestamp = timestamp_us * NSEC_PER_USEC;
               vbuf->sequence = inst->sequence_cap++;
               if (vbuf->flags & V4L2_BUF_FLAG_LAST) {
                       const struct v4l2_event ev = { .type = V4L2_EVENT_EOS };
-                       vb->planes[0].bytesused = bytesused;
                       v4l2_event_queue_fh(&inst->fh, &ev);

Given that we are now taking the minimum of these two values, it seems
to me that we don't need to set bytesused again in case we are dealing
with the last buffer? Stanimir, what do you think?