| Message ID | 20240925103351.1628788-1-quic_mojha@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | remoteproc: qcom: Fix NULL pointer in glink_subdev_stop() | expand |
On Wed, Sep 25, 2024 at 04:03:51PM +0530, Mukesh Ojha wrote: > Multiple call to glink_subdev_stop() for the same remoteproc can happen > if rproc_stop() fails from Process-A that leaves the rproc state to > RPROC_CRASHED state later a call to recovery_store from user space in > Process B triggers rproc_trigger_recovery() of the same remoteproc to > recover it results in NULL pointer dereference issue in > qcom_glink_smem_unregister(). > > Fix it by having a NULL check in glink_subdev_stop(). > > Process-A Process-B > > fatal error interrupt happens > > rproc_crash_handler_work() > mutex_lock_interruptible(&rproc->lock); > ... > > rproc->state = RPROC_CRASHED; > ... > mutex_unlock(&rproc->lock); > > rproc_trigger_recovery() > mutex_lock_interruptible(&rproc->lock); > > adsp_stop() > qcom_q6v5_pas 20c00000.remoteproc: failed to shutdown: -22 > remoteproc remoteproc3: can't stop rproc: -22 I presume that at this point this remoteproc is in some undefined state and the only way to recover is for the user to reboot the machine? The check for glink->edge avoids one pitfall following this, but I'd prefer to see a solution that avoids issues in this scenario in the remoteproc core - rather than working around side effects of this in different places. Regards, Bjorn > mutex_unlock(&rproc->lock); > > echo enabled > /sys/class/remoteproc/remoteprocX/recovery > recovery_store() > rproc_trigger_recovery() > mutex_lock_interruptible(&rproc->lock); > rproc_stop() > glink_subdev_stop() > qcom_glink_smem_unregister() ==| > | > V > Unable to handle kernel NULL pointer dereference > at virtual address 0000000000000358 > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > --- > - We can do this NULL check in qcom_glink_smem_unregister() as it is > exported function however, there is only one user of this. So, doing > it with current approach should also be fine. > > drivers/remoteproc/qcom_common.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c > index 8c8688f99f0a..52d6c9b99fdb 100644 > --- a/drivers/remoteproc/qcom_common.c > +++ b/drivers/remoteproc/qcom_common.c > @@ -209,6 +209,9 @@ static void glink_subdev_stop(struct rproc_subdev *subdev, bool crashed) > { > struct qcom_rproc_glink *glink = to_glink_subdev(subdev); > > + if (!glink->edge) > + return; > + > qcom_glink_smem_unregister(glink->edge); > glink->edge = NULL; > } > -- > 2.34.1 > >
diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c index 8c8688f99f0a..52d6c9b99fdb 100644 --- a/drivers/remoteproc/qcom_common.c +++ b/drivers/remoteproc/qcom_common.c @@ -209,6 +209,9 @@ static void glink_subdev_stop(struct rproc_subdev *subdev, bool crashed) { struct qcom_rproc_glink *glink = to_glink_subdev(subdev); + if (!glink->edge) + return; + qcom_glink_smem_unregister(glink->edge); glink->edge = NULL; }
Multiple call to glink_subdev_stop() for the same remoteproc can happen if rproc_stop() fails from Process-A that leaves the rproc state to RPROC_CRASHED state later a call to recovery_store from user space in Process B triggers rproc_trigger_recovery() of the same remoteproc to recover it results in NULL pointer dereference issue in qcom_glink_smem_unregister(). Fix it by having a NULL check in glink_subdev_stop(). Process-A Process-B fatal error interrupt happens rproc_crash_handler_work() mutex_lock_interruptible(&rproc->lock); ... rproc->state = RPROC_CRASHED; ... mutex_unlock(&rproc->lock); rproc_trigger_recovery() mutex_lock_interruptible(&rproc->lock); adsp_stop() qcom_q6v5_pas 20c00000.remoteproc: failed to shutdown: -22 remoteproc remoteproc3: can't stop rproc: -22 mutex_unlock(&rproc->lock); echo enabled > /sys/class/remoteproc/remoteprocX/recovery recovery_store() rproc_trigger_recovery() mutex_lock_interruptible(&rproc->lock); rproc_stop() glink_subdev_stop() qcom_glink_smem_unregister() ==| | V Unable to handle kernel NULL pointer dereference at virtual address 0000000000000358 Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> --- - We can do this NULL check in qcom_glink_smem_unregister() as it is exported function however, there is only one user of this. So, doing it with current approach should also be fine. drivers/remoteproc/qcom_common.c | 3 +++ 1 file changed, 3 insertions(+)