linux-kernel - Re: [PATCH v8 2/4] vcodec: mediatek: Add Mediatek JPEG Decoder Driver

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]

Date:   Mon, 12 Dec 2016 12:34:01 +0800
From:   Ricky Liang <jcliang@...omium.org>
To:     Rick Chang <rick.chang@...iatek.com>
Cc:     Hans Verkuil <hans.verkuil@...co.com>,
        Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        open list <linux-kernel@...r.kernel.org>,
        linux-media@...r.kernel.org, srv_heupstream@...iatek.com,
        "moderated list:ARM/Mediatek SoC..." 
        <linux-mediatek@...ts.infradead.org>,
        "moderated list:ARM/Mediatek SoC..." 
        <linux-arm-kernel@...ts.infradead.org>,
        "open list:OPEN FIRMWARE AND..." <devicetree@...r.kernel.org>,
        Minghsiu Tsai <minghsiu.tsai@...iatek.com>,
        Bin Liu <bin.liu@...iatek.com>
Subject: Re: [PATCH v8 2/4] vcodec: mediatek: Add Mediatek JPEG Decoder Driver

Hi Rick,

On Wed, Nov 30, 2016 at 11:08 AM, Rick Chang <rick.chang@...iatek.com> wrote:
> Add v4l2 driver for Mediatek JPEG Decoder
>
> Signed-off-by: Rick Chang <rick.chang@...iatek.com>
> Signed-off-by: Minghsiu Tsai <minghsiu.tsai@...iatek.com>

<snip...>

> +static bool mtk_jpeg_check_resolution_change(struct mtk_jpeg_ctx *ctx,
> +                                            struct mtk_jpeg_dec_param *param)
> +{
> +       struct mtk_jpeg_dev *jpeg = ctx->jpeg;
> +       struct mtk_jpeg_q_data *q_data;
> +
> +       q_data = &ctx->out_q;
> +       if (q_data->w != param->pic_w || q_data->h != param->pic_h) {
> +               v4l2_dbg(1, debug, &jpeg->v4l2_dev, "Picture size change\n");
> +               return true;
> +       }
> +
> +       q_data = &ctx->cap_q;
> +       if (q_data->fmt != mtk_jpeg_find_format(ctx, param->dst_fourcc,
> +                                               MTK_JPEG_FMT_TYPE_CAPTURE)) {
> +               v4l2_dbg(1, debug, &jpeg->v4l2_dev, "format change\n");
> +               return true;
> +       }
> +       return false;

<snip...>

> +static void mtk_jpeg_device_run(void *priv)
> +{
> +       struct mtk_jpeg_ctx *ctx = priv;
> +       struct mtk_jpeg_dev *jpeg = ctx->jpeg;
> +       struct vb2_buffer *src_buf, *dst_buf;
> +       enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
> +       unsigned long flags;
> +       struct mtk_jpeg_src_buf *jpeg_src_buf;
> +       struct mtk_jpeg_bs bs;
> +       struct mtk_jpeg_fb fb;
> +       int i;
> +
> +       src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> +       dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> +       jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(src_buf);
> +
> +       if (jpeg_src_buf->flags & MTK_JPEG_BUF_FLAGS_LAST_FRAME) {
> +               for (i = 0; i < dst_buf->num_planes; i++)
> +                       vb2_set_plane_payload(dst_buf, i, 0);
> +               buf_state = VB2_BUF_STATE_DONE;
> +               goto dec_end;
> +       }
> +
> +       if (mtk_jpeg_check_resolution_change(ctx, &jpeg_src_buf->dec_param)) {
> +               mtk_jpeg_queue_src_chg_event(ctx);
> +               ctx->state = MTK_JPEG_SOURCE_CHANGE;
> +               v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
> +               return;
> +       }

This only detects source change if multiple OUPUT buffers are queued.
It does not catch the source change in the following scenario:

- OUPUT buffers for jpeg1 enqueued
- OUTPUT queue STREAMON
- userspace creates CAPTURE buffers
- CAPTURE buffers enqueued
- CAPTURE queue STREAMON
- decode
- OUTPUT queue STREAMOFF
- userspace recreates OUTPUT buffers for jpeg2
- OUTPUT buffers for jpeg2 enqueued
- OUTPUT queue STREAMON

In the above sequence if jpeg2's decoded size is larger than jpeg1 the
function fails to detect that the existing CAPTURE buffers are not big
enough to hold the decoded data.

A possible fix is to pass *dst_buf to
mtk_jpeg_check_resolution_change(), and check in the function that all
the dst_buf planes are large enough to hold the decoded data.

> +
> +       mtk_jpeg_set_dec_src(ctx, src_buf, &bs);
> +       if (mtk_jpeg_set_dec_dst(ctx, &jpeg_src_buf->dec_param, dst_buf, &fb))
> +               goto dec_end;
> +
> +       spin_lock_irqsave(&jpeg->hw_lock, flags);
> +       mtk_jpeg_dec_reset(jpeg->dec_reg_base);
> +       mtk_jpeg_dec_set_config(jpeg->dec_reg_base,
> +                               &jpeg_src_buf->dec_param, &bs, &fb);
> +
> +       mtk_jpeg_dec_start(jpeg->dec_reg_base);
> +       spin_unlock_irqrestore(&jpeg->hw_lock, flags);
> +       return;
> +
> +dec_end:
> +       v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> +       v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> +       v4l2_m2m_buf_done(to_vb2_v4l2_buffer(src_buf), buf_state);
> +       v4l2_m2m_buf_done(to_vb2_v4l2_buffer(dst_buf), buf_state);
> +       v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
> +}

<snip...>