• Re: [PATCH] media: verisilicon: Fix null pointer dereference in try_fmt

    From Diederik de Haas@21:1/5 to All on Thu May 25 17:19:10 2023
    On Thursday, 25 May 2023 17:16:36 CEST Diederik de Haas wrote:
    Something not meant for the debian-qa list ...

    I'll purge KMail. Sorry again.
    -----BEGIN PGP SIGNATURE-----

    iHUEABYIAB0WIQT1sUPBYsyGmi4usy/XblvOeH7bbgUCZG98bgAKCRDXblvOeH7b btJOAQCvws7nOAmy55685JOA3PXGhp7Lc5z5mRoBdstSBxm07gEA1j1DAmSqFX3b Hmu+XC0Y8iWavcRH30AJoqsV6PlK4gU=
    =OZmJ
    -----END PGP SIGNATURE-----

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)
  • From Diederik de Haas@21:1/5 to All on Thu May 25 17:16:36 2023
    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
    -----BEGIN PGP SIGNATURE-----

    iHUEABYIAB0WIQT1sUPBYsyGmi4usy/XblvOeH7bbgUCZG971AAKCRDXblvOeH7b boF4AP0ZiatWpDLodWGpp9DAtCWHTEb2UX5e0q4FiDu3jtuucgEA3etbSX+6bSgq vyFyB6HXT/QTMbG26Dp2yDTLG8jwQAs=
    =1Pcb
    -----END PGP SIGNATURE-----

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)