| 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
| ||
|
Date: Sat, 16 Jul 2022 14:36:46 +0200
From: Jernej Škrabec <jernej.skrabec@...il.com>
To: Nicolas Dufresne <nicolas@...fresne.ca>,
Ezequiel Garcia <ezequiel@...guardiasur.com.ar>
Cc: linux-media <linux-media@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Hans Verkuil <hverkuil-cisco@...all.nl>,
Tomasz Figa <tfiga@...omium.org>,
Marek Szyprowski <m.szyprowski@...sung.com>,
Maxime Ripard <mripard@...nel.org>,
Paul Kocialkowski <paul.kocialkowski@...tlin.com>
Subject: Re: [PATCH v2 7/8] cedrus: Use vb2_find_buffer
Dne petek, 15. julij 2022 ob 13:48:34 CEST je Ezequiel Garcia napisal(a):
> Hi Nicolas,
>
> Thanks a lot for the test and the bug report.
>
> On Thu, Jul 14, 2022 at 4:26 PM Nicolas Dufresne <nicolas@...fresne.ca>
wrote:
> > Hi Ezequiel,
> >
> > I started testing with these patches and found some NULL dreferences, see
> > my comment inline...
> >
> > Le lundi 11 juillet 2022 à 18:11 -0300, Ezequiel Garcia a écrit :
> > > Use the newly introduced vb2_find_buffer API to get a vb2_buffer
> > > given a buffer timestamp.
> > >
> > > Cc: Maxime Ripard <mripard@...nel.org>
> > > Cc: Paul Kocialkowski <paul.kocialkowski@...tlin.com>
> > > Cc: Jernej Skrabec <jernej.skrabec@...il.com>
> > > Signed-off-by: Ezequiel Garcia <ezequiel@...guardiasur.com.ar>
> > > Acked-by: Tomasz Figa <tfiga@...omium.org>
> > > ---
> > >
> > > drivers/staging/media/sunxi/cedrus/cedrus.h | 24 ++++++-----
> > > .../staging/media/sunxi/cedrus/cedrus_h264.c | 16 +++----
> > > .../staging/media/sunxi/cedrus/cedrus_h265.c | 16 +++----
> > > .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 28 ++++--------
> > > .../staging/media/sunxi/cedrus/cedrus_vp8.c | 43 ++++---------------
> > > 5 files changed, 46 insertions(+), 81 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h
> > > b/drivers/staging/media/sunxi/cedrus/cedrus.h index
> > > 3bc094eb497f..c054dbe3d3bc 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> > > @@ -233,19 +233,23 @@ static inline dma_addr_t cedrus_buf_addr(struct
> > > vb2_buffer *buf,> >
> > > }
> > >
> > > static inline dma_addr_t cedrus_dst_buf_addr(struct cedrus_ctx *ctx,
> > >
> > > - int index, unsigned int
> > > plane)
> > > + struct vb2_buffer *buf,
> > > + unsigned int plane)
> > >
> > > {
> > >
> > > - struct vb2_buffer *buf = NULL;
> > > - struct vb2_queue *vq;
> > > -
> > > - if (index < 0)
> > > - return 0;
> > > + return buf ? cedrus_buf_addr(buf, &ctx->dst_fmt, plane) : 0;
> > > +}
> > >
> > > - vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
> > > V4L2_BUF_TYPE_VIDEO_CAPTURE);
> > > - if (vq)
> > > - buf = vb2_get_buffer(vq, index);
> > > +static inline void cedrus_write_ref_buf_addr(struct cedrus_ctx *ctx,
> > > + struct vb2_queue *q,
> > > + u64 timestamp,
> > > + u32 luma_reg,
> > > + u32 chroma_reg)
> > > +{
> > > + struct cedrus_dev *dev = ctx->dev;
> > > + struct vb2_buffer *buf = vb2_find_buffer(q, timestamp);
> > >
> > > - return buf ? cedrus_buf_addr(buf, &ctx->dst_fmt, plane) : 0;
> > > + cedrus_write(dev, luma_reg, cedrus_dst_buf_addr(ctx, buf, 0));
> > > + cedrus_write(dev, chroma_reg, cedrus_dst_buf_addr(ctx, buf, 1));
> > >
> > > }
> > >
> > > static inline struct cedrus_buffer *
> > >
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index
> > > d8fb93035470..0559efeac125 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > @@ -111,16 +111,16 @@ static void cedrus_write_frame_list(struct
> > > cedrus_ctx *ctx,> >
> > > for (i = 0; i < ARRAY_SIZE(decode->dpb); i++) {
> > >
> > > const struct v4l2_h264_dpb_entry *dpb = &decode->dpb[i];
> > > struct cedrus_buffer *cedrus_buf;
> > >
> > > - int buf_idx;
> > > + struct vb2_buffer *buf;
> > >
> > > if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_VALID))
> > >
> > > continue;
> > >
> > > - buf_idx = vb2_find_timestamp(cap_q, dpb->reference_ts, 0);
> > > - if (buf_idx < 0)
> > > + buf = vb2_find_buffer(cap_q, dpb->reference_ts);
> > > + if (!buf)
> > >
> > > continue;
> > >
> > > - cedrus_buf = vb2_to_cedrus_buffer(cap_q->bufs[buf_idx]);
> > > + cedrus_buf = vb2_to_cedrus_buffer(buf);
> > >
> > > position = cedrus_buf->codec.h264.position;
> > > used_dpbs |= BIT(position);
> > >
> > > @@ -186,7 +186,7 @@ static void _cedrus_write_ref_list(struct cedrus_ctx
> > > *ctx,> >
> > > const struct v4l2_h264_dpb_entry *dpb;
> > > const struct cedrus_buffer *cedrus_buf;
> > > unsigned int position;
> > >
> > > - int buf_idx;
> > > + struct vb2_buffer *buf;
> > >
> > > u8 dpb_idx;
> > >
> > > dpb_idx = ref_list[i].index;
> > >
> > > @@ -195,11 +195,11 @@ static void _cedrus_write_ref_list(struct
> > > cedrus_ctx *ctx,> >
> > > if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
> > >
> > > continue;
> > >
> > > - buf_idx = vb2_find_timestamp(cap_q, dpb->reference_ts, 0);
> > > - if (buf_idx < 0)
> > > + buf = vb2_find_buffer(cap_q, dpb->reference_ts);
> > > + if (!buf)
> > >
> > > continue;
> > >
> > > - cedrus_buf = vb2_to_cedrus_buffer(cap_q->bufs[buf_idx]);
> > > + cedrus_buf = vb2_to_cedrus_buffer(buf);
> > >
> > > position = cedrus_buf->codec.h264.position;
> > >
> > > sram_array[i] |= position << 1;
> > >
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > > b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c index
> > > 44f385be9f6c..60cc13e4d0a9 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > > @@ -102,14 +102,14 @@ static void
> > > cedrus_h265_frame_info_write_single(struct cedrus_ctx *ctx,> >
> > > unsigned int index,
> > > bool field_pic,
> > > u32 pic_order_cnt[],
> > >
> > > - int buffer_index)
> > > + struct vb2_buffer *buf)
> > >
> > > {
> > >
> > > struct cedrus_dev *dev = ctx->dev;
> > >
> > > - dma_addr_t dst_luma_addr = cedrus_dst_buf_addr(ctx, buffer_index,
> > > 0);
> > > - dma_addr_t dst_chroma_addr = cedrus_dst_buf_addr(ctx,
> > > buffer_index, 1); + dma_addr_t dst_luma_addr =
> > > cedrus_dst_buf_addr(ctx, buf, 0); + dma_addr_t dst_chroma_addr =
> > > cedrus_dst_buf_addr(ctx, buf, 1);> >
> > > dma_addr_t mv_col_buf_addr[2] = {
> > >
> > > - cedrus_h265_frame_info_mv_col_buf_addr(ctx, buffer_index,
> > > 0),
> > > - cedrus_h265_frame_info_mv_col_buf_addr(ctx, buffer_index,
> > > + cedrus_h265_frame_info_mv_col_buf_addr(ctx, buf->index,
> > > 0),
> > > + cedrus_h265_frame_info_mv_col_buf_addr(ctx, buf->index,
> > >
> > > field_pic ? 1 : 0)
> >
> > Previously, -1 would be passed to
> > cedrus_h265_frame_info_mv_col_buf_addr(),
> > which would not find a buffer at that index, and would return 0. Now the
> > code will crash with a NULL pointer deref.
>
> Is it really correct to pass -1 to cedrus_h265_frame_info_mv_col_buf_addr?
> It seems it get casted into an unsigned type and then used to calculate
> an address for DMA.
I totally agree that this is a latent bug and it should be fixed. H264 checks
for negative value, but not HEVC. Current code just makes out of bounds read,
which is tolerated, but yours causes NULL pointer dereference, which is not.
I suggest that following check is added in cedrus_h265_frame_info_write_dpb():
if (buffer_index < 0)
continue;
I can send it as a fix so it gets backported and you update this patch so above
if is changed to NULL pointer check.
Do you all agree? Nicolas, do you prefer to send such patch, since you're first
who noticed something odd with the code?
Best regards,
Jernej
>
> static inline dma_addr_t
> cedrus_h265_frame_info_mv_col_buf_addr(struct cedrus_ctx *ctx,
> unsigned int index, unsigned int
> field) {
> return ctx->codec.h265.mv_col_buf_addr + index *
> ctx->codec.h265.mv_col_buf_unit_size +
> field * ctx->codec.h265.mv_col_buf_unit_size / 2;
> }
>
> Fixing the driver to go back to the previous behavior is trivial,
> but this looks odd.
>
> Jernej, Paul, any thoughts?
>
> Thanks,
> Ezequiel
>
> > > };
> > > u32 offset = VE_DEC_H265_SRAM_OFFSET_FRAME_INFO +
> > >
> > > @@ -141,7 +141,7 @@ static void cedrus_h265_frame_info_write_dpb(struct
> > > cedrus_ctx *ctx,> >
> > > unsigned int i;
> > >
> > > for (i = 0; i < num_active_dpb_entries; i++) {
> > >
> > > - int buffer_index = vb2_find_timestamp(vq,
> > > dpb[i].timestamp, 0); + struct vb2_buffer *buf =
> > > vb2_find_buffer(vq, dpb[i].timestamp);> >
> > > u32 pic_order_cnt[2] = {
> > >
> > > dpb[i].pic_order_cnt[0],
> > > dpb[i].pic_order_cnt[1]
> > >
> > > @@ -149,7 +149,7 @@ static void cedrus_h265_frame_info_write_dpb(struct
> > > cedrus_ctx *ctx,> >
> > > cedrus_h265_frame_info_write_single(ctx, i,
> > > dpb[i].field_pic,
> > >
> > > pic_order_cnt,
> > >
> > > - buffer_index);
> > > + buf);
> > >
> > > }
> > >
> > > }
> > >
> > > @@ -616,7 +616,7 @@ static void cedrus_h265_setup(struct cedrus_ctx
> > > *ctx,
> > >
> > > cedrus_h265_frame_info_write_single(ctx, output_pic_list_index,
> > >
> > > slice_params->pic_struct != 0,
> > > pic_order_cnt,
> > >
> > > - run->dst->vb2_buf.index);
> > > + &run->dst->vb2_buf);
> > >
> > > cedrus_write(dev, VE_DEC_H265_OUTPUT_FRAME_IDX,
> > > output_pic_list_index);
> > >
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > > b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c index
> > > 5dad2f296c6d..22d6cae9a710 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > > @@ -54,13 +54,9 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx
> > > *ctx, struct cedrus_run *run)> >
> > > const struct v4l2_ctrl_mpeg2_picture *pic;
> > > const struct v4l2_ctrl_mpeg2_quantisation *quantisation;
> > > dma_addr_t src_buf_addr, dst_luma_addr, dst_chroma_addr;
> > >
> > > - dma_addr_t fwd_luma_addr, fwd_chroma_addr;
> > > - dma_addr_t bwd_luma_addr, bwd_chroma_addr;
> > >
> > > struct cedrus_dev *dev = ctx->dev;
> > > struct vb2_queue *vq;
> > > const u8 *matrix;
> > >
> > > - int forward_idx;
> > > - int backward_idx;
> > >
> > > unsigned int i;
> > > u32 reg;
> > >
> > > @@ -123,27 +119,19 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx
> > > *ctx, struct cedrus_run *run)> >
> > > cedrus_write(dev, VE_DEC_MPEG_PICBOUNDSIZE, reg);
> > >
> > > /* Forward and backward prediction reference buffers. */
> > >
> > > -
> > >
> > > vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
> > > V4L2_BUF_TYPE_VIDEO_CAPTURE);
> > >
> > > - forward_idx = vb2_find_timestamp(vq, pic->forward_ref_ts, 0);
> > > - fwd_luma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 0);
> > > - fwd_chroma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 1);
> > > -
> > > - cedrus_write(dev, VE_DEC_MPEG_FWD_REF_LUMA_ADDR, fwd_luma_addr);
> > > - cedrus_write(dev, VE_DEC_MPEG_FWD_REF_CHROMA_ADDR,
> > > fwd_chroma_addr);
> > > -
> > > - backward_idx = vb2_find_timestamp(vq, pic->backward_ref_ts, 0);
> > > - bwd_luma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 0);
> > > - bwd_chroma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 1);
> > > -
> > > - cedrus_write(dev, VE_DEC_MPEG_BWD_REF_LUMA_ADDR, bwd_luma_addr);
> > > - cedrus_write(dev, VE_DEC_MPEG_BWD_REF_CHROMA_ADDR,
> > > bwd_chroma_addr);
> > > + cedrus_write_ref_buf_addr(ctx, vq, pic->forward_ref_ts,
> > > + VE_DEC_MPEG_FWD_REF_LUMA_ADDR,
> > > + VE_DEC_MPEG_FWD_REF_CHROMA_ADDR);
> > > + cedrus_write_ref_buf_addr(ctx, vq, pic->backward_ref_ts,
> > > + VE_DEC_MPEG_BWD_REF_LUMA_ADDR,
> > > + VE_DEC_MPEG_BWD_REF_CHROMA_ADDR);
> > >
> > > /* Destination luma and chroma buffers. */
> > >
> > > - dst_luma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index,
> > > 0);
> > > - dst_chroma_addr = cedrus_dst_buf_addr(ctx,
> > > run->dst->vb2_buf.index, 1); + dst_luma_addr =
> > > cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 0); + dst_chroma_addr
> > > = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 1);> >
> > > cedrus_write(dev, VE_DEC_MPEG_REC_LUMA, dst_luma_addr);
> > > cedrus_write(dev, VE_DEC_MPEG_REC_CHROMA, dst_chroma_addr);
> > >
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> > > b/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c index
> > > f4016684b32d..196cf692186d 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_vp8.c
> > > @@ -661,7 +661,6 @@ static void cedrus_vp8_setup(struct cedrus_ctx *ctx,
> > >
> > > dma_addr_t luma_addr, chroma_addr;
> > > dma_addr_t src_buf_addr;
> > > int header_size;
> > >
> > > - int qindex;
> > >
> > > u32 reg;
> > >
> > > cedrus_engine_enable(ctx, CEDRUS_CODEC_VP8);
> > >
> > > @@ -805,43 +804,17 @@ static void cedrus_vp8_setup(struct cedrus_ctx
> > > *ctx,
> > >
> > > reg |= VE_VP8_LF_DELTA0(slice->lf.mb_mode_delta[0]);
> > > cedrus_write(dev, VE_VP8_MODE_LF_DELTA, reg);
> > >
> > > - luma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index, 0);
> > > - chroma_addr = cedrus_dst_buf_addr(ctx, run->dst->vb2_buf.index,
> > > 1);
> > > + luma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 0);
> > > + chroma_addr = cedrus_dst_buf_addr(ctx, &run->dst->vb2_buf, 1);
> > >
> > > cedrus_write(dev, VE_VP8_REC_LUMA, luma_addr);
> > > cedrus_write(dev, VE_VP8_REC_CHROMA, chroma_addr);
> > >
> > > - qindex = vb2_find_timestamp(cap_q, slice->last_frame_ts, 0);
> > > - if (qindex >= 0) {
> > > - luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0);
> > > - chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1);
> > > - cedrus_write(dev, VE_VP8_FWD_LUMA, luma_addr);
> > > - cedrus_write(dev, VE_VP8_FWD_CHROMA, chroma_addr);
> > > - } else {
> > > - cedrus_write(dev, VE_VP8_FWD_LUMA, 0);
> > > - cedrus_write(dev, VE_VP8_FWD_CHROMA, 0);
> > > - }
> > > -
> > > - qindex = vb2_find_timestamp(cap_q, slice->golden_frame_ts, 0);
> > > - if (qindex >= 0) {
> > > - luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0);
> > > - chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1);
> > > - cedrus_write(dev, VE_VP8_BWD_LUMA, luma_addr);
> > > - cedrus_write(dev, VE_VP8_BWD_CHROMA, chroma_addr);
> > > - } else {
> > > - cedrus_write(dev, VE_VP8_BWD_LUMA, 0);
> > > - cedrus_write(dev, VE_VP8_BWD_CHROMA, 0);
> > > - }
> > > -
> > > - qindex = vb2_find_timestamp(cap_q, slice->alt_frame_ts, 0);
> > > - if (qindex >= 0) {
> > > - luma_addr = cedrus_dst_buf_addr(ctx, qindex, 0);
> > > - chroma_addr = cedrus_dst_buf_addr(ctx, qindex, 1);
> > > - cedrus_write(dev, VE_VP8_ALT_LUMA, luma_addr);
> > > - cedrus_write(dev, VE_VP8_ALT_CHROMA, chroma_addr);
> > > - } else {
> > > - cedrus_write(dev, VE_VP8_ALT_LUMA, 0);
> > > - cedrus_write(dev, VE_VP8_ALT_CHROMA, 0);
> > > - }
> > > + cedrus_write_ref_buf_addr(ctx, cap_q, slice->last_frame_ts,
> > > + VE_VP8_FWD_LUMA, VE_VP8_FWD_CHROMA);
> > > + cedrus_write_ref_buf_addr(ctx, cap_q, slice->golden_frame_ts,
> > > + VE_VP8_BWD_LUMA, VE_VP8_BWD_CHROMA);
> > > + cedrus_write_ref_buf_addr(ctx, cap_q, slice->alt_frame_ts,
> > > + VE_VP8_ALT_LUMA, VE_VP8_ALT_CHROMA);
> > >
> > > cedrus_write(dev, VE_H264_CTRL, VE_H264_CTRL_VP8 |
> > >
> > > VE_H264_CTRL_DECODE_ERR_INT |
Powered by blists - more mailing lists