| Message ID | 20230516091209.3098262-1-m.tretter@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: verisilicon: Fix null pointer dereference in try_fmt | expand |
On Tuesday, 16 May 2023 11:12:09 CEST Michael Tretter wrote: > Since commit db6f68b51e5c ("media: verisilicon: Do not set context > src/dst formats in reset functions"), vpu_src_fmt is not set in the > reset function, but only set in hantro_set_fmt_out, which calls > hantro_try_fmt before setting the format. Therefore, hantro_try_fmt > might be called with vpu_src_fmt still being null. > > Add a test if the format is actually set before checking the format. > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > Fixes: db6f68b51e5c ("media: verisilicon: Do not set context src/dst formats > in reset functions") --- > drivers/media/platform/verisilicon/hantro_v4l2.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c > b/drivers/media/platform/verisilicon/hantro_v4l2.c index > 835518534e3b..ec37d2646fde 100644 > --- a/drivers/media/platform/verisilicon/hantro_v4l2.c > +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c > @@ -313,17 +313,20 @@ static int hantro_try_fmt(const struct hantro_ctx > *ctx, /* Fill remaining fields */ > v4l2_fill_pixfmt_mp(pix_mp, fmt->fourcc, pix_mp->width, > pix_mp->height); > - if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE && > + if (ctx->vpu_src_fmt && > + ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE && > !hantro_needs_postproc(ctx, fmt)) > pix_mp->plane_fmt[0].sizeimage += > hantro_h264_mv_size(pix_mp- >width, > pix_mp- >height); > - else if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_VP9_FRAME && > + else if (ctx->vpu_src_fmt && > + ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_VP9_FRAME && > !hantro_needs_postproc(ctx, fmt)) > pix_mp->plane_fmt[0].sizeimage += > hantro_vp9_mv_size(pix_mp->width, > pix_mp- >height); > - else if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_HEVC_SLICE && > + else if (ctx->vpu_src_fmt && > + ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_HEVC_SLICE && > !hantro_needs_postproc(ctx, fmt)) > pix_mp->plane_fmt[0].sizeimage += > hantro_hevc_mv_size(pix_mp- >width, I have verified that this patch fixes the crash I was seeing since 6.4. https://lore.kernel.org/linux-media/12724349.O9o76ZdvQC@bagend/ is a/the other patch which also fixed the crash.
Hi Diederik, Le mardi 23 mai 2023 à 16:22 +0200, Diederik de Haas a écrit : > On Tuesday, 16 May 2023 11:12:09 CEST Michael Tretter wrote: > > Since commit db6f68b51e5c ("media: verisilicon: Do not set context > > src/dst formats in reset functions"), vpu_src_fmt is not set in the > > reset function, but only set in hantro_set_fmt_out, which calls > > hantro_try_fmt before setting the format. Therefore, hantro_try_fmt > > might be called with vpu_src_fmt still being null. > > > > Add a test if the format is actually set before checking the format. > > > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > > Fixes: db6f68b51e5c ("media: verisilicon: Do not set context src/dst formats > > in reset functions") --- > > drivers/media/platform/verisilicon/hantro_v4l2.c | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c > > b/drivers/media/platform/verisilicon/hantro_v4l2.c index > > 835518534e3b..ec37d2646fde 100644 > > --- a/drivers/media/platform/verisilicon/hantro_v4l2.c > > +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c > > @@ -313,17 +313,20 @@ static int hantro_try_fmt(const struct hantro_ctx > > *ctx, /* Fill remaining fields */ > > v4l2_fill_pixfmt_mp(pix_mp, fmt->fourcc, pix_mp->width, > > pix_mp->height); > > - if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE > && > > + if (ctx->vpu_src_fmt && > > + ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE > && > > !hantro_needs_postproc(ctx, fmt)) > > pix_mp->plane_fmt[0].sizeimage += > > hantro_h264_mv_size(pix_mp- > > width, > > pix_mp- > > height); > > - else if (ctx->vpu_src_fmt->fourcc == > V4L2_PIX_FMT_VP9_FRAME && > > + else if (ctx->vpu_src_fmt && > > + ctx->vpu_src_fmt->fourcc == > V4L2_PIX_FMT_VP9_FRAME && > > !hantro_needs_postproc(ctx, fmt)) > > pix_mp->plane_fmt[0].sizeimage += > > hantro_vp9_mv_size(pix_mp->width, > > pix_mp- > > height); > > - else if (ctx->vpu_src_fmt->fourcc == > V4L2_PIX_FMT_HEVC_SLICE && > > + else if (ctx->vpu_src_fmt && > > + ctx->vpu_src_fmt->fourcc == > V4L2_PIX_FMT_HEVC_SLICE && > > !hantro_needs_postproc(ctx, fmt)) > > pix_mp->plane_fmt[0].sizeimage += > > hantro_hevc_mv_size(pix_mp- > > width, > > I have verified that this patch fixes the crash I was seeing since 6.4. > https://lore.kernel.org/linux-media/12724349.O9o76ZdvQC@bagend/ is a/the other > patch which also fixed the crash. An official form of this comment would be Tested-by: <name> <email>, would you agree to have this added ? Nicolas
Le mardi 16 mai 2023 à 11:12 +0200, Michael Tretter a écrit : > Since commit db6f68b51e5c ("media: verisilicon: Do not set context > src/dst formats in reset functions"), vpu_src_fmt is not set in the > reset function, but only set in hantro_set_fmt_out, which calls > hantro_try_fmt before setting the format. Therefore, hantro_try_fmt > might be called with vpu_src_fmt still being null. > > Add a test if the format is actually set before checking the format. > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > Fixes: db6f68b51e5c ("media: verisilicon: Do not set context src/dst formats in reset functions") This patch highlights yet more issues in the driver default format handling, but the remaining bug is extremely minor (too small sizeimage before S_FMT is called, rather then kernel oops.). Considering how long this has been going, please consider merging this. Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > --- > drivers/media/platform/verisilicon/hantro_v4l2.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c > index 835518534e3b..ec37d2646fde 100644 > --- a/drivers/media/platform/verisilicon/hantro_v4l2.c > +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c > @@ -313,17 +313,20 @@ static int hantro_try_fmt(const struct hantro_ctx *ctx, > /* Fill remaining fields */ > v4l2_fill_pixfmt_mp(pix_mp, fmt->fourcc, pix_mp->width, > pix_mp->height); > - if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE && > + if (ctx->vpu_src_fmt && > + ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE && > !hantro_needs_postproc(ctx, fmt)) > pix_mp->plane_fmt[0].sizeimage += > hantro_h264_mv_size(pix_mp->width, > pix_mp->height); > - else if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_VP9_FRAME && > + else if (ctx->vpu_src_fmt && > + ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_VP9_FRAME && > !hantro_needs_postproc(ctx, fmt)) > pix_mp->plane_fmt[0].sizeimage += > hantro_vp9_mv_size(pix_mp->width, > pix_mp->height); > - else if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_HEVC_SLICE && > + else if (ctx->vpu_src_fmt && > + ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_HEVC_SLICE && > !hantro_needs_postproc(ctx, fmt)) > pix_mp->plane_fmt[0].sizeimage += > hantro_hevc_mv_size(pix_mp->width,
On 25/05/2023 16:38, Nicolas Dufresne wrote: > Le mardi 16 mai 2023 à 11:12 +0200, Michael Tretter a écrit : >> Since commit db6f68b51e5c ("media: verisilicon: Do not set context >> src/dst formats in reset functions"), vpu_src_fmt is not set in the >> reset function, but only set in hantro_set_fmt_out, which calls >> hantro_try_fmt before setting the format. Therefore, hantro_try_fmt >> might be called with vpu_src_fmt still being null. >> >> Add a test if the format is actually set before checking the format. >> >> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> >> Fixes: db6f68b51e5c ("media: verisilicon: Do not set context src/dst formats in reset functions") > > This patch highlights yet more issues in the driver default format handling, but > the remaining bug is extremely minor (too small sizeimage before S_FMT is > called, rather then kernel oops.). Considering how long this has been going, > please consider merging this. I went with this fix: https://patchwork.linuxtv.org/project/linux-media/patch/20230523162515.993862-1-benjamin.gaignard@collabora.com/ Part of this pull request: https://patchwork.linuxtv.org/project/linux-media/patch/d4b08420-f7c0-4950-2d20-385d98f3cad9@xs4all.nl/ If you disagree, then please let me know. This particular patch has been marked as Superseded in patchwork. Regards, Hans > > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > >> --- >> drivers/media/platform/verisilicon/hantro_v4l2.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c >> index 835518534e3b..ec37d2646fde 100644 >> --- a/drivers/media/platform/verisilicon/hantro_v4l2.c >> +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c >> @@ -313,17 +313,20 @@ static int hantro_try_fmt(const struct hantro_ctx *ctx, >> /* Fill remaining fields */ >> v4l2_fill_pixfmt_mp(pix_mp, fmt->fourcc, pix_mp->width, >> pix_mp->height); >> - if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE && >> + if (ctx->vpu_src_fmt && >> + ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE && >> !hantro_needs_postproc(ctx, fmt)) >> pix_mp->plane_fmt[0].sizeimage += >> hantro_h264_mv_size(pix_mp->width, >> pix_mp->height); >> - else if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_VP9_FRAME && >> + else if (ctx->vpu_src_fmt && >> + ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_VP9_FRAME && >> !hantro_needs_postproc(ctx, fmt)) >> pix_mp->plane_fmt[0].sizeimage += >> hantro_vp9_mv_size(pix_mp->width, >> pix_mp->height); >> - else if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_HEVC_SLICE && >> + else if (ctx->vpu_src_fmt && >> + ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_HEVC_SLICE && >> !hantro_needs_postproc(ctx, fmt)) >> pix_mp->plane_fmt[0].sizeimage += >> hantro_hevc_mv_size(pix_mp->width, >
Hi Nicolas, On Thursday, 25 May 2023 16:36:13 CEST Nicolas Dufresne wrote: > Le mardi 23 mai 2023 à 16:22 +0200, Diederik de Haas a écrit : > > On Tuesday, 16 May 2023 11:12:09 CEST Michael Tretter wrote: > > > Since commit db6f68b51e5c ("media: verisilicon: Do not set context > > > src/dst formats in reset functions"), vpu_src_fmt is not set in the > > > reset function, but only set in hantro_set_fmt_out, which calls > > > hantro_try_fmt before setting the format. Therefore, hantro_try_fmt > > > might be called with vpu_src_fmt still being null. > > > > > > Add a test if the format is actually set before checking the format. > > > > > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > > > Fixes: db6f68b51e5c ("media: verisilicon: Do not set context src/dst > > > formats in reset functions") --- > > > > > > drivers/media/platform/verisilicon/hantro_v4l2.c | 9 ++++++--- > > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c > > > b/drivers/media/platform/verisilicon/hantro_v4l2.c index > > > 835518534e3b..ec37d2646fde 100644 > > > --- a/drivers/media/platform/verisilicon/hantro_v4l2.c > > > +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c > > > @@ -313,17 +313,20 @@ static int hantro_try_fmt(const struct hantro_ctx > > > *ctx, /* Fill remaining fields */ > > > > > > v4l2_fill_pixfmt_mp(pix_mp, fmt->fourcc, pix_mp->width, > > > > > > pix_mp->height); > > > > > > - if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE > > > > && > > > > > + if (ctx->vpu_src_fmt && > > > + ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE > > > > && > > > > > !hantro_needs_postproc(ctx, fmt)) > > > > > > pix_mp->plane_fmt[0].sizeimage += > > > > > > hantro_h264_mv_size(pix_mp- > > > > > > width, > > > > > > pix_mp- > > > > > > height); > > > - else if (ctx->vpu_src_fmt->fourcc == > > > > V4L2_PIX_FMT_VP9_FRAME && > > > > > + else if (ctx->vpu_src_fmt && > > > + ctx->vpu_src_fmt->fourcc == > > > > V4L2_PIX_FMT_VP9_FRAME && > > > > > !hantro_needs_postproc(ctx, fmt)) > > > > > > pix_mp->plane_fmt[0].sizeimage += > > > > > > hantro_vp9_mv_size(pix_mp- >width, > > > > > > pix_mp- > > > > > > height); > > > - else if (ctx->vpu_src_fmt->fourcc == > > > > V4L2_PIX_FMT_HEVC_SLICE && > > > > > + else if (ctx->vpu_src_fmt && > > > + ctx->vpu_src_fmt->fourcc == > > > > V4L2_PIX_FMT_HEVC_SLICE && > > > > > !hantro_needs_postproc(ctx, fmt)) > > > > > > pix_mp->plane_fmt[0].sizeimage += > > > > > > hantro_hevc_mv_size(pix_mp- > > > > > > width, > > > > I have verified that this patch fixes the crash I was seeing since 6.4. > > https://lore.kernel.org/linux-media/12724349.O9o76ZdvQC@bagend/ is a/the > > other patch which also fixed the crash. > > An official form of this comment would be Tested-by: <name> <email>, would > you agree to have this added ? I don't have a principle objection to that, but I already did give my "Tested-by" for what I think is the latest version to fix this issue: https://lore.kernel.org/linux-media/3187393.mvXUDI8C0e@bagend/ I was about to 'raise concern', but I just saw that Hans Verkuil already went with the patch for which I had (officially) added my "Tested-by" Cheers, Diederik
diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c index 835518534e3b..ec37d2646fde 100644 --- a/drivers/media/platform/verisilicon/hantro_v4l2.c +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c @@ -313,17 +313,20 @@ static int hantro_try_fmt(const struct hantro_ctx *ctx, /* Fill remaining fields */ v4l2_fill_pixfmt_mp(pix_mp, fmt->fourcc, pix_mp->width, pix_mp->height); - if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE && + if (ctx->vpu_src_fmt && + ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE && !hantro_needs_postproc(ctx, fmt)) pix_mp->plane_fmt[0].sizeimage += hantro_h264_mv_size(pix_mp->width, pix_mp->height); - else if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_VP9_FRAME && + else if (ctx->vpu_src_fmt && + ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_VP9_FRAME && !hantro_needs_postproc(ctx, fmt)) pix_mp->plane_fmt[0].sizeimage += hantro_vp9_mv_size(pix_mp->width, pix_mp->height); - else if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_HEVC_SLICE && + else if (ctx->vpu_src_fmt && + ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_HEVC_SLICE && !hantro_needs_postproc(ctx, fmt)) pix_mp->plane_fmt[0].sizeimage += hantro_hevc_mv_size(pix_mp->width,
Since commit db6f68b51e5c ("media: verisilicon: Do not set context src/dst formats in reset functions"), vpu_src_fmt is not set in the reset function, but only set in hantro_set_fmt_out, which calls hantro_try_fmt before setting the format. Therefore, hantro_try_fmt might be called with vpu_src_fmt still being null. Add a test if the format is actually set before checking the format. Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> Fixes: db6f68b51e5c ("media: verisilicon: Do not set context src/dst formats in reset functions") --- drivers/media/platform/verisilicon/hantro_v4l2.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)