* [PATCH] media: iris: fix use-after-free of fmt_src during MBPF check @ 2026-02-27 17:33 ` Vishnu Reddy 2026-02-27 21:45 ` Bryan O'Donoghue 0 siblings, 1 reply; 3+ messages in thread From: Vishnu Reddy @ 2026-02-27 17:33 UTC (permalink / raw) To: Vikash Garodia, Dikshita Agarwal, Abhinav Kumar, Bryan O'Donoghue, Mauro Carvalho Chehab Cc: linux-media, linux-arm-msm, linux-kernel, Vishnu Reddy A race condition was observed during concurrency testing. the core MBPF check walks the list of active instances and reads fields such as fmt_src height and width. At the same time, iris_close() could free these format structures before the instance was removed from core list. this creates a use-after-free window where the MBPF checker access the freed memory and read invalid values. To fix this, the freeing of fmt_src and fmt_dst is moved to the end of iris_close(), after the instance has been removed from the core list and teardown is complete. This avoids accessing dangling pointers during the MBPF check. Signed-off-by: Vishnu Reddy <busanna.reddy@oss.qualcomm.com> --- drivers/media/platform/qcom/iris/iris_vdec.c | 6 ------ drivers/media/platform/qcom/iris/iris_vdec.h | 1 - drivers/media/platform/qcom/iris/iris_venc.c | 6 ------ drivers/media/platform/qcom/iris/iris_venc.h | 1 - drivers/media/platform/qcom/iris/iris_vidc.c | 6 ++---- 5 files changed, 2 insertions(+), 18 deletions(-) diff --git a/drivers/media/platform/qcom/iris/iris_vdec.c b/drivers/media/platform/qcom/iris/iris_vdec.c index 719217399a30..99d544e2af4f 100644 --- a/drivers/media/platform/qcom/iris/iris_vdec.c +++ b/drivers/media/platform/qcom/iris/iris_vdec.c @@ -61,12 +61,6 @@ int iris_vdec_inst_init(struct iris_inst *inst) return iris_ctrls_init(inst); } -void iris_vdec_inst_deinit(struct iris_inst *inst) -{ - kfree(inst->fmt_dst); - kfree(inst->fmt_src); -} - static const struct iris_fmt iris_vdec_formats_cap[] = { [IRIS_FMT_NV12] = { .pixfmt = V4L2_PIX_FMT_NV12, diff --git a/drivers/media/platform/qcom/iris/iris_vdec.h b/drivers/media/platform/qcom/iris/iris_vdec.h index ec1ce55d1375..5123d2a340e1 100644 --- a/drivers/media/platform/qcom/iris/iris_vdec.h +++ b/drivers/media/platform/qcom/iris/iris_vdec.h @@ -9,7 +9,6 @@ struct iris_inst; int iris_vdec_inst_init(struct iris_inst *inst); -void iris_vdec_inst_deinit(struct iris_inst *inst); int iris_vdec_enum_fmt(struct iris_inst *inst, struct v4l2_fmtdesc *f); int iris_vdec_try_fmt(struct iris_inst *inst, struct v4l2_format *f); int iris_vdec_s_fmt(struct iris_inst *inst, struct v4l2_format *f); diff --git a/drivers/media/platform/qcom/iris/iris_venc.c b/drivers/media/platform/qcom/iris/iris_venc.c index aa27b22704eb..4d886769d958 100644 --- a/drivers/media/platform/qcom/iris/iris_venc.c +++ b/drivers/media/platform/qcom/iris/iris_venc.c @@ -79,12 +79,6 @@ int iris_venc_inst_init(struct iris_inst *inst) return iris_ctrls_init(inst); } -void iris_venc_inst_deinit(struct iris_inst *inst) -{ - kfree(inst->fmt_dst); - kfree(inst->fmt_src); -} - static const struct iris_fmt iris_venc_formats_cap[] = { [IRIS_FMT_H264] = { .pixfmt = V4L2_PIX_FMT_H264, diff --git a/drivers/media/platform/qcom/iris/iris_venc.h b/drivers/media/platform/qcom/iris/iris_venc.h index c4db7433da53..00c1716b2747 100644 --- a/drivers/media/platform/qcom/iris/iris_venc.h +++ b/drivers/media/platform/qcom/iris/iris_venc.h @@ -9,7 +9,6 @@ struct iris_inst; int iris_venc_inst_init(struct iris_inst *inst); -void iris_venc_inst_deinit(struct iris_inst *inst); int iris_venc_enum_fmt(struct iris_inst *inst, struct v4l2_fmtdesc *f); int iris_venc_try_fmt(struct iris_inst *inst, struct v4l2_format *f); int iris_venc_s_fmt(struct iris_inst *inst, struct v4l2_format *f); diff --git a/drivers/media/platform/qcom/iris/iris_vidc.c b/drivers/media/platform/qcom/iris/iris_vidc.c index bd38d84c9cc7..5eb1786b0737 100644 --- a/drivers/media/platform/qcom/iris/iris_vidc.c +++ b/drivers/media/platform/qcom/iris/iris_vidc.c @@ -289,10 +289,6 @@ int iris_close(struct file *filp) v4l2_m2m_ctx_release(inst->m2m_ctx); v4l2_m2m_release(inst->m2m_dev); mutex_lock(&inst->lock); - if (inst->domain == DECODER) - iris_vdec_inst_deinit(inst); - else if (inst->domain == ENCODER) - iris_venc_inst_deinit(inst); iris_session_close(inst); iris_inst_change_state(inst, IRIS_INST_DEINIT); iris_v4l2_fh_deinit(inst, filp); @@ -304,6 +300,8 @@ int iris_close(struct file *filp) mutex_unlock(&inst->lock); mutex_destroy(&inst->ctx_q_lock); mutex_destroy(&inst->lock); + kfree(inst->fmt_src); + kfree(inst->fmt_dst); kfree(inst); return 0; --- base-commit: 6de23f81a5e08be8fbf5e8d7e9febc72a5b5f27f change-id: 20260226-fix-use-after-free-of-fmt_src-during-mbpf-abc27f573400 Best regards, -- Vishnu Reddy <busanna.reddy@oss.qualcomm.com> ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] media: iris: fix use-after-free of fmt_src during MBPF check 2026-02-27 17:33 ` [PATCH] media: iris: fix use-after-free of fmt_src during MBPF check Vishnu Reddy @ 2026-02-27 21:45 ` Bryan O'Donoghue 2026-03-03 4:11 ` Vishnu Reddy 0 siblings, 1 reply; 3+ messages in thread From: Bryan O'Donoghue @ 2026-02-27 21:45 UTC (permalink / raw) To: Vishnu Reddy, Vikash Garodia, Dikshita Agarwal, Abhinav Kumar, Mauro Carvalho Chehab Cc: linux-media, linux-arm-msm, linux-kernel On 27/02/2026 17:33, Vishnu Reddy wrote: > A race condition was observed during concurrency testing. the core MBPF > check walks the list of active instances and reads fields such as fmt_src > height and width. Where does that happen - you highlight iris_close() below - that's good, what's the method or methods that can run concurrently with iris_close() - you should state those in the commit log so that reviewers like myself and people reading the commit in the future know where to look. At the same time, iris_close() could free these format > structures before the instance was removed from core list. this creates a > use-after-free window where the MBPF checker access the freed memory and > read invalid values. Without looking at the code this description seems suspect. &inst->lock ought to protect inst && inst->thing if it doesn't, then the lock isn't being used correctly. > > To fix this, the freeing of fmt_src and fmt_dst is moved to the end > of iris_close(), after the instance has been removed from the core > list and teardown is complete. This avoids accessing dangling pointers > during the MBPF check. > > Signed-off-by: Vishnu Reddy <busanna.reddy@oss.qualcomm.com> Needs - Fixes: - Cc: stable > --- > drivers/media/platform/qcom/iris/iris_vdec.c | 6 ------ > drivers/media/platform/qcom/iris/iris_vdec.h | 1 - > drivers/media/platform/qcom/iris/iris_venc.c | 6 ------ > drivers/media/platform/qcom/iris/iris_venc.h | 1 - > drivers/media/platform/qcom/iris/iris_vidc.c | 6 ++---- > 5 files changed, 2 insertions(+), 18 deletions(-) > > diff --git a/drivers/media/platform/qcom/iris/iris_vdec.c b/drivers/media/platform/qcom/iris/iris_vdec.c > index 719217399a30..99d544e2af4f 100644 > --- a/drivers/media/platform/qcom/iris/iris_vdec.c > +++ b/drivers/media/platform/qcom/iris/iris_vdec.c > @@ -61,12 +61,6 @@ int iris_vdec_inst_init(struct iris_inst *inst) > return iris_ctrls_init(inst); > } > > -void iris_vdec_inst_deinit(struct iris_inst *inst) > -{ > - kfree(inst->fmt_dst); > - kfree(inst->fmt_src); > -} > - > static const struct iris_fmt iris_vdec_formats_cap[] = { > [IRIS_FMT_NV12] = { > .pixfmt = V4L2_PIX_FMT_NV12, > diff --git a/drivers/media/platform/qcom/iris/iris_vdec.h b/drivers/media/platform/qcom/iris/iris_vdec.h > index ec1ce55d1375..5123d2a340e1 100644 > --- a/drivers/media/platform/qcom/iris/iris_vdec.h > +++ b/drivers/media/platform/qcom/iris/iris_vdec.h > @@ -9,7 +9,6 @@ > struct iris_inst; > > int iris_vdec_inst_init(struct iris_inst *inst); > -void iris_vdec_inst_deinit(struct iris_inst *inst); > int iris_vdec_enum_fmt(struct iris_inst *inst, struct v4l2_fmtdesc *f); > int iris_vdec_try_fmt(struct iris_inst *inst, struct v4l2_format *f); > int iris_vdec_s_fmt(struct iris_inst *inst, struct v4l2_format *f); > diff --git a/drivers/media/platform/qcom/iris/iris_venc.c b/drivers/media/platform/qcom/iris/iris_venc.c > index aa27b22704eb..4d886769d958 100644 > --- a/drivers/media/platform/qcom/iris/iris_venc.c > +++ b/drivers/media/platform/qcom/iris/iris_venc.c > @@ -79,12 +79,6 @@ int iris_venc_inst_init(struct iris_inst *inst) > return iris_ctrls_init(inst); > } > > -void iris_venc_inst_deinit(struct iris_inst *inst) > -{ > - kfree(inst->fmt_dst); > - kfree(inst->fmt_src); > -} > - > static const struct iris_fmt iris_venc_formats_cap[] = { > [IRIS_FMT_H264] = { > .pixfmt = V4L2_PIX_FMT_H264, > diff --git a/drivers/media/platform/qcom/iris/iris_venc.h b/drivers/media/platform/qcom/iris/iris_venc.h > index c4db7433da53..00c1716b2747 100644 > --- a/drivers/media/platform/qcom/iris/iris_venc.h > +++ b/drivers/media/platform/qcom/iris/iris_venc.h > @@ -9,7 +9,6 @@ > struct iris_inst; > > int iris_venc_inst_init(struct iris_inst *inst); > -void iris_venc_inst_deinit(struct iris_inst *inst); > int iris_venc_enum_fmt(struct iris_inst *inst, struct v4l2_fmtdesc *f); > int iris_venc_try_fmt(struct iris_inst *inst, struct v4l2_format *f); > int iris_venc_s_fmt(struct iris_inst *inst, struct v4l2_format *f); > diff --git a/drivers/media/platform/qcom/iris/iris_vidc.c b/drivers/media/platform/qcom/iris/iris_vidc.c > index bd38d84c9cc7..5eb1786b0737 100644 > --- a/drivers/media/platform/qcom/iris/iris_vidc.c > +++ b/drivers/media/platform/qcom/iris/iris_vidc.c > @@ -289,10 +289,6 @@ int iris_close(struct file *filp) > v4l2_m2m_ctx_release(inst->m2m_ctx); > v4l2_m2m_release(inst->m2m_dev); > mutex_lock(&inst->lock); > - if (inst->domain == DECODER) > - iris_vdec_inst_deinit(inst); > - else if (inst->domain == ENCODER) > - iris_venc_inst_deinit(inst); > iris_session_close(inst); > iris_inst_change_state(inst, IRIS_INST_DEINIT); > iris_v4l2_fh_deinit(inst, filp); > @@ -304,6 +300,8 @@ int iris_close(struct file *filp) > mutex_unlock(&inst->lock); > mutex_destroy(&inst->ctx_q_lock); > mutex_destroy(&inst->lock); > + kfree(inst->fmt_src); > + kfree(inst->fmt_dst); > kfree(inst); On the face of it I like the logic of moving the kfree() after destruction of the various other bits - however the description in the log makes me question of the two locks we have are being used correctly .. Please provide more detail. > > return 0; > > --- > base-commit: 6de23f81a5e08be8fbf5e8d7e9febc72a5b5f27f > change-id: 20260226-fix-use-after-free-of-fmt_src-during-mbpf-abc27f573400 > > Best regards, > -- > Vishnu Reddy <busanna.reddy@oss.qualcomm.com> > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] media: iris: fix use-after-free of fmt_src during MBPF check 2026-02-27 21:45 ` Bryan O'Donoghue @ 2026-03-03 4:11 ` Vishnu Reddy 0 siblings, 0 replies; 3+ messages in thread From: Vishnu Reddy @ 2026-03-03 4:11 UTC (permalink / raw) To: Bryan O'Donoghue, Vikash Garodia, Dikshita Agarwal, Abhinav Kumar, Mauro Carvalho Chehab Cc: linux-media, linux-arm-msm, linux-kernel On 2/28/2026 3:15 AM, Bryan O'Donoghue wrote: > On 27/02/2026 17:33, Vishnu Reddy wrote: >> A race condition was observed during concurrency testing. the core MBPF >> check walks the list of active instances and reads fields such as >> fmt_src >> height and width. > > Where does that happen - you highlight iris_close() below - that's > good, what's the method or methods that can run concurrently with > iris_close() - you should state those in the commit log so that > reviewers like myself and people reading the commit in the future know > where to look. > > At the same time, iris_close() could free these format >> structures before the instance was removed from core list. this >> creates a >> use-after-free window where the MBPF checker access the freed memory and >> read invalid values. > > Without looking at the code this description seems suspect. > > &inst->lock ought to protect inst && inst->thing if it doesn't, then > the lock isn't being used correctly. > During concurrency testing, multiple instances can be created in parallel. Each instance has its own inst->lock, while the core list is protected by the global core->lock. The race occurs because these two locks protect different scopes: inst->lock protects fields within a single instance core->lock protects the list of active instances The MBPF checker walks the core instance list under the core->lock. While doing so, it reads fields such as fmt_src->width and fmt_src->height from each instance. Now consider what happens concurrently: Instance A (MBPF check): Acquires core->lock Iterates over the instance list Reads inst->fmt_src->width, inst->fmt_src->height, etc. Instance B (iris_close() for a different instance): Acquires inst->lock Frees inst->fmt_src and inst->fmt_dst (At this moment, the instance is still in the core list.) Releases inst->lock. Later acquires core->lock and removes that instance from the core list The problem is that fmt_src is freed before the instance is removed from the core list. This creates a use‑after‑free window: MBPF check (Instance A) is still walking the core list It reaches the instance whose fmt_src has already been freed by Instance B It dereferences a dangling pointer → use-after-free This situation occurs because: inst->lock only protects the instance internals MBPF checker does not take inst->lock when reading fmt_src It only relies on the core->lock, the instance is valid as long as it is in the core list Therefore, ordering of freeing inst->fmt_src and inst->fmt_dest in iris_close() is incorrect. The fix is to postpone freeing fmt_src and fmt_dst until after: The instance has been removed from the core list, and All teardown under the core lock is complete. Will update commit description in v2. >> >> To fix this, the freeing of fmt_src and fmt_dst is moved to the end >> of iris_close(), after the instance has been removed from the core >> list and teardown is complete. This avoids accessing dangling pointers >> during the MBPF check. >> >> Signed-off-by: Vishnu Reddy <busanna.reddy@oss.qualcomm.com> > > Needs > > - Fixes: > - Cc: stable ACK > >> --- >> drivers/media/platform/qcom/iris/iris_vdec.c | 6 ------ >> drivers/media/platform/qcom/iris/iris_vdec.h | 1 - >> drivers/media/platform/qcom/iris/iris_venc.c | 6 ------ >> drivers/media/platform/qcom/iris/iris_venc.h | 1 - >> drivers/media/platform/qcom/iris/iris_vidc.c | 6 ++---- >> 5 files changed, 2 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/iris/iris_vdec.c >> b/drivers/media/platform/qcom/iris/iris_vdec.c >> index 719217399a30..99d544e2af4f 100644 >> --- a/drivers/media/platform/qcom/iris/iris_vdec.c >> +++ b/drivers/media/platform/qcom/iris/iris_vdec.c >> @@ -61,12 +61,6 @@ int iris_vdec_inst_init(struct iris_inst *inst) >> return iris_ctrls_init(inst); >> } >> >> -void iris_vdec_inst_deinit(struct iris_inst *inst) >> -{ >> - kfree(inst->fmt_dst); >> - kfree(inst->fmt_src); >> -} >> - >> static const struct iris_fmt iris_vdec_formats_cap[] = { >> [IRIS_FMT_NV12] = { >> .pixfmt = V4L2_PIX_FMT_NV12, >> diff --git a/drivers/media/platform/qcom/iris/iris_vdec.h >> b/drivers/media/platform/qcom/iris/iris_vdec.h >> index ec1ce55d1375..5123d2a340e1 100644 >> --- a/drivers/media/platform/qcom/iris/iris_vdec.h >> +++ b/drivers/media/platform/qcom/iris/iris_vdec.h >> @@ -9,7 +9,6 @@ >> struct iris_inst; >> >> int iris_vdec_inst_init(struct iris_inst *inst); >> -void iris_vdec_inst_deinit(struct iris_inst *inst); >> int iris_vdec_enum_fmt(struct iris_inst *inst, struct v4l2_fmtdesc >> *f); >> int iris_vdec_try_fmt(struct iris_inst *inst, struct v4l2_format *f); >> int iris_vdec_s_fmt(struct iris_inst *inst, struct v4l2_format *f); >> diff --git a/drivers/media/platform/qcom/iris/iris_venc.c >> b/drivers/media/platform/qcom/iris/iris_venc.c >> index aa27b22704eb..4d886769d958 100644 >> --- a/drivers/media/platform/qcom/iris/iris_venc.c >> +++ b/drivers/media/platform/qcom/iris/iris_venc.c >> @@ -79,12 +79,6 @@ int iris_venc_inst_init(struct iris_inst *inst) >> return iris_ctrls_init(inst); >> } >> >> -void iris_venc_inst_deinit(struct iris_inst *inst) >> -{ >> - kfree(inst->fmt_dst); >> - kfree(inst->fmt_src); >> -} >> - >> static const struct iris_fmt iris_venc_formats_cap[] = { >> [IRIS_FMT_H264] = { >> .pixfmt = V4L2_PIX_FMT_H264, >> diff --git a/drivers/media/platform/qcom/iris/iris_venc.h >> b/drivers/media/platform/qcom/iris/iris_venc.h >> index c4db7433da53..00c1716b2747 100644 >> --- a/drivers/media/platform/qcom/iris/iris_venc.h >> +++ b/drivers/media/platform/qcom/iris/iris_venc.h >> @@ -9,7 +9,6 @@ >> struct iris_inst; >> >> int iris_venc_inst_init(struct iris_inst *inst); >> -void iris_venc_inst_deinit(struct iris_inst *inst); >> int iris_venc_enum_fmt(struct iris_inst *inst, struct v4l2_fmtdesc >> *f); >> int iris_venc_try_fmt(struct iris_inst *inst, struct v4l2_format *f); >> int iris_venc_s_fmt(struct iris_inst *inst, struct v4l2_format *f); >> diff --git a/drivers/media/platform/qcom/iris/iris_vidc.c >> b/drivers/media/platform/qcom/iris/iris_vidc.c >> index bd38d84c9cc7..5eb1786b0737 100644 >> --- a/drivers/media/platform/qcom/iris/iris_vidc.c >> +++ b/drivers/media/platform/qcom/iris/iris_vidc.c >> @@ -289,10 +289,6 @@ int iris_close(struct file *filp) >> v4l2_m2m_ctx_release(inst->m2m_ctx); >> v4l2_m2m_release(inst->m2m_dev); >> mutex_lock(&inst->lock); >> - if (inst->domain == DECODER) >> - iris_vdec_inst_deinit(inst); >> - else if (inst->domain == ENCODER) >> - iris_venc_inst_deinit(inst); >> iris_session_close(inst); >> iris_inst_change_state(inst, IRIS_INST_DEINIT); >> iris_v4l2_fh_deinit(inst, filp); >> @@ -304,6 +300,8 @@ int iris_close(struct file *filp) >> mutex_unlock(&inst->lock); >> mutex_destroy(&inst->ctx_q_lock); >> mutex_destroy(&inst->lock); >> + kfree(inst->fmt_src); >> + kfree(inst->fmt_dst); >> kfree(inst); > > On the face of it I like the logic of moving the kfree() after > destruction of the various other bits - however the description in the > log makes me question of the two locks we have are being used > correctly .. > > Please provide more detail. Given details above and will update commit description in v2. >> >> return 0; >> >> --- >> base-commit: 6de23f81a5e08be8fbf5e8d7e9febc72a5b5f27f >> change-id: >> 20260226-fix-use-after-free-of-fmt_src-during-mbpf-abc27f573400 >> >> Best regards, >> -- >> Vishnu Reddy <busanna.reddy@oss.qualcomm.com> >> > Regards, Vishnu Reddy ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-03-03 4:12 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <rUXTfoyGnkGklkQTcUpGww1LY3k1mQifzKFqJaChQkC1jTWg-_3dxJVTkYOp45jUmL45pc7DKHXZvEHaEm-Evw==@protonmail.internalid>
2026-02-27 17:33 ` [PATCH] media: iris: fix use-after-free of fmt_src during MBPF check Vishnu Reddy
2026-02-27 21:45 ` Bryan O'Donoghue
2026-03-03 4:11 ` Vishnu Reddy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®