From 95d601bb7f9d7fef4b146d2188205f8783c4043d Mon Sep 17 00:00:00 2001 From: Deepak Kumar Singh Date: Thu, 3 Dec 2020 13:32:30 +0530 Subject: [PATCH 1/2] soc: qcom: glink_probe: Use ref count for holding ssr context During ssr rpmsg ssr device will unregister, which can result in use of stale device pointer in ssr callback function. Now incrementing refcount inside ssr callback function to prevent release of rpmsg device. CRs-Fixed: 2551255 Change-Id: If3fc57c4635378dc6fdcb7df120c83dca0e83758 Signed-off-by: Deepak Kumar Singh --- drivers/soc/qcom/msm_glink_ssr.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/drivers/soc/qcom/msm_glink_ssr.c b/drivers/soc/qcom/msm_glink_ssr.c index 491c2105adfd..b64496b24d1a 100644 --- a/drivers/soc/qcom/msm_glink_ssr.c +++ b/drivers/soc/qcom/msm_glink_ssr.c @@ -64,8 +64,21 @@ struct glink_ssr { u32 seq_num; struct completion completion; struct work_struct unreg_work; + struct kref refcount; }; +static void glink_ssr_release(struct kref *ref) +{ + struct glink_ssr *ssr = container_of(ref, struct glink_ssr, + refcount); + struct glink_ssr_nb *nb, *tmp; + + list_for_each_entry_safe(nb, tmp, &ssr->notify_list, list) + kfree(nb); + + kfree(ssr); +} + static void glink_ssr_ssr_unreg_work(struct work_struct *work) { struct glink_ssr *ssr = container_of(work, struct glink_ssr, @@ -75,9 +88,8 @@ static void glink_ssr_ssr_unreg_work(struct work_struct *work) list_for_each_entry_safe(nb, tmp, &ssr->notify_list, list) { subsys_notif_unregister_notifier(nb->ssr_register_handle, &nb->nb); - kfree(nb); } - kfree(ssr); + kref_put(&ssr->refcount, glink_ssr_release); } static int glink_ssr_ssr_cb(struct notifier_block *this, @@ -92,6 +104,8 @@ static int glink_ssr_ssr_cb(struct notifier_block *this, if (!dev || !ssr->ept) return NOTIFY_DONE; + kref_get(&ssr->refcount); + if (code == SUBSYS_AFTER_SHUTDOWN || code == SUBSYS_POWERUP_FAILURE) { ssr->seq_num++; reinit_completion(&ssr->completion); @@ -110,6 +124,7 @@ static int glink_ssr_ssr_cb(struct notifier_block *this, if (ret) { MSM_SSR_ERR(dev, "fail to send do cleanup to %s %d\n", nb->ssr_label, ret); + kref_put(&ssr->refcount, glink_ssr_release); return NOTIFY_DONE; } @@ -117,6 +132,7 @@ static int glink_ssr_ssr_cb(struct notifier_block *this, if (!ret) MSM_SSR_ERR(dev, "timeout waiting for cleanup resp\n"); } + kref_put(&ssr->refcount, glink_ssr_release); return NOTIFY_DONE; } @@ -217,6 +233,7 @@ static int glink_ssr_probe(struct rpmsg_device *rpdev) INIT_LIST_HEAD(&ssr->notify_list); init_completion(&ssr->completion); INIT_WORK(&ssr->unreg_work, glink_ssr_ssr_unreg_work); + kref_init(&ssr->refcount); ssr->dev = &rpdev->dev; ssr->ept = rpdev->ept; From c3c08ac7a1bde6bb28279627c905ec6f2fe6ebec Mon Sep 17 00:00:00 2001 From: Deepak Kumar Singh Date: Thu, 3 Dec 2020 13:45:26 +0530 Subject: [PATCH 2/2] soc: qcom: glink_probe: use lock while sending ssr notification In ssr handling of remote subsystem down, all rpmsg devices are removed for that edge. If at the same time ssr down is received for other remote subsystem, it may try to notify already down remote and end up using invalid rpmsg device which causes use after free. Store device state in ssr context and update this with holding a lock. SSR notification function should also use the same lock and perform check for validity of rpmsg device. CRs-Fixed: 2580433 Change-Id: I339e228a527f7b6a737944d8e9e53caa31992914 Signed-off-by: Deepak Kumar Singh --- drivers/soc/qcom/msm_glink_ssr.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/drivers/soc/qcom/msm_glink_ssr.c b/drivers/soc/qcom/msm_glink_ssr.c index b64496b24d1a..2ba7aa017c39 100644 --- a/drivers/soc/qcom/msm_glink_ssr.c +++ b/drivers/soc/qcom/msm_glink_ssr.c @@ -16,6 +16,7 @@ #define MSM_SSR_LOG_PAGE_CNT 4 static void *ssr_ilc; +static DEFINE_MUTEX(ssr_lock); #define MSM_SSR_INFO(x, ...) ipc_log_string(ssr_ilc, x, ##__VA_ARGS__) @@ -97,14 +98,15 @@ static int glink_ssr_ssr_cb(struct notifier_block *this, { struct glink_ssr_nb *nb = container_of(this, struct glink_ssr_nb, nb); struct glink_ssr *ssr = nb->ssr; - struct device *dev = ssr->dev; + struct device *dev; struct do_cleanup_msg msg; int ret; - if (!dev || !ssr->ept) - return NOTIFY_DONE; - kref_get(&ssr->refcount); + mutex_lock(&ssr_lock); + dev = ssr->dev; + if (!dev || !ssr->ept) + goto out; if (code == SUBSYS_AFTER_SHUTDOWN || code == SUBSYS_POWERUP_FAILURE) { ssr->seq_num++; @@ -124,14 +126,15 @@ static int glink_ssr_ssr_cb(struct notifier_block *this, if (ret) { MSM_SSR_ERR(dev, "fail to send do cleanup to %s %d\n", nb->ssr_label, ret); - kref_put(&ssr->refcount, glink_ssr_release); - return NOTIFY_DONE; + goto out; } ret = wait_for_completion_timeout(&ssr->completion, HZ); if (!ret) MSM_SSR_ERR(dev, "timeout waiting for cleanup resp\n"); } +out: + mutex_unlock(&ssr_lock); kref_put(&ssr->refcount, glink_ssr_release); return NOTIFY_DONE; } @@ -252,11 +255,12 @@ static void glink_ssr_remove(struct rpmsg_device *rpdev) { struct glink_ssr *ssr = dev_get_drvdata(&rpdev->dev); + mutex_lock(&ssr_lock); ssr->dev = NULL; ssr->ept = NULL; + mutex_unlock(&ssr_lock); dev_set_drvdata(&rpdev->dev, NULL); - schedule_work(&ssr->unreg_work); }