From bbe692e349e2a1edf3fe0a29a0e05899c9c94d51 Mon Sep 17 00:00:00 2001 From: Navid Emamdoost Date: Mon, 16 Sep 2019 22:31:23 -0500 Subject: [PATCH 01/10] rpmsg: char: release allocated memory In rpmsg_eptdev_write_iter, if copy_from_iter_full fails the allocated buffer needs to be released. Signed-off-by: Navid Emamdoost Signed-off-by: Bjorn Andersson --- drivers/rpmsg/rpmsg_char.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c index eea5ebbb5119..c655074c07c2 100644 --- a/drivers/rpmsg/rpmsg_char.c +++ b/drivers/rpmsg/rpmsg_char.c @@ -227,8 +227,10 @@ static ssize_t rpmsg_eptdev_write_iter(struct kiocb *iocb, if (!kbuf) return -ENOMEM; - if (!copy_from_iter_full(kbuf, len, from)) - return -EFAULT; + if (!copy_from_iter_full(kbuf, len, from)) { + ret = -EFAULT; + goto free_kbuf; + } if (mutex_lock_interruptible(&eptdev->ept_lock)) { ret = -ERESTARTSYS; From 4623e8bf1de0b86e23a56cdb39a72f054e89c3bd Mon Sep 17 00:00:00 2001 From: Chris Lew Date: Wed, 27 Jun 2018 18:19:57 -0700 Subject: [PATCH 02/10] rpmsg: glink: Set tail pointer to 0 at end of FIFO When wrapping around the FIFO, the remote expects the tail pointer to be reset to 0 on the edge case where the tail equals the FIFO length. Fixes: caf989c350e8 ("rpmsg: glink: Introduce glink smem based transport") Cc: stable@vger.kernel.org Signed-off-by: Chris Lew Signed-off-by: Bjorn Andersson --- drivers/rpmsg/qcom_glink_smem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/rpmsg/qcom_glink_smem.c b/drivers/rpmsg/qcom_glink_smem.c index 4238383d8685..579bc4443f6d 100644 --- a/drivers/rpmsg/qcom_glink_smem.c +++ b/drivers/rpmsg/qcom_glink_smem.c @@ -105,7 +105,7 @@ static void glink_smem_rx_advance(struct qcom_glink_pipe *np, tail = le32_to_cpu(*pipe->tail); tail += count; - if (tail > pipe->native.length) + if (tail >= pipe->native.length) tail -= pipe->native.length; *pipe->tail = cpu_to_le32(tail); From b85f6b601407347f5425c4c058d1b7871f5bf4f0 Mon Sep 17 00:00:00 2001 From: Arun Kumar Neelakantam Date: Fri, 4 Oct 2019 15:26:57 -0700 Subject: [PATCH 03/10] rpmsg: glink: Fix reuse intents memory leak issue Memory allocated for re-usable intents are not freed during channel cleanup which causes memory leak in system. Check and free all re-usable memory to avoid memory leak. Fixes: 933b45da5d1d ("rpmsg: glink: Add support for TX intents") Cc: stable@vger.kernel.org Acked-By: Chris Lew Tested-by: Srinivas Kandagatla Signed-off-by: Arun Kumar Neelakantam Reported-by: Srinivas Kandagatla Signed-off-by: Bjorn Andersson --- drivers/rpmsg/qcom_glink_native.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c index 621f1afd4d6b..9355ce26fd98 100644 --- a/drivers/rpmsg/qcom_glink_native.c +++ b/drivers/rpmsg/qcom_glink_native.c @@ -241,10 +241,19 @@ static void qcom_glink_channel_release(struct kref *ref) { struct glink_channel *channel = container_of(ref, struct glink_channel, refcount); + struct glink_core_rx_intent *tmp; unsigned long flags; + int iid; spin_lock_irqsave(&channel->intent_lock, flags); + idr_for_each_entry(&channel->liids, tmp, iid) { + kfree(tmp->data); + kfree(tmp); + } idr_destroy(&channel->liids); + + idr_for_each_entry(&channel->riids, tmp, iid) + kfree(tmp); idr_destroy(&channel->riids); spin_unlock_irqrestore(&channel->intent_lock, flags); From ac74ea01860170699fb3b6ea80c0476774c8e94f Mon Sep 17 00:00:00 2001 From: Arun Kumar Neelakantam Date: Fri, 4 Oct 2019 15:26:58 -0700 Subject: [PATCH 04/10] rpmsg: glink: Fix use after free in open_ack TIMEOUT case Extra channel reference put when remote sending OPEN_ACK after timeout causes use-after-free while handling next remote CLOSE command. Remove extra reference put in timeout case to avoid use-after-free. Fixes: b4f8e52b89f6 ("rpmsg: Introduce Qualcomm RPM glink driver") Cc: stable@vger.kernel.org Tested-by: Srinivas Kandagatla Signed-off-by: Arun Kumar Neelakantam Signed-off-by: Bjorn Andersson --- drivers/rpmsg/qcom_glink_native.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c index 9355ce26fd98..72ed671f5dcd 100644 --- a/drivers/rpmsg/qcom_glink_native.c +++ b/drivers/rpmsg/qcom_glink_native.c @@ -1103,13 +1103,12 @@ static int qcom_glink_create_remote(struct qcom_glink *glink, close_link: /* * Send a close request to "undo" our open-ack. The close-ack will - * release the last reference. + * release qcom_glink_send_open_req() reference and the last reference + * will be relesed after receiving remote_close or transport unregister + * by calling qcom_glink_native_remove(). */ qcom_glink_send_close_req(glink, channel); - /* Release qcom_glink_send_open_req() reference */ - kref_put(&channel->refcount, qcom_glink_channel_release); - return ret; } From b646293e272816dd0719529dcebbd659de0722f7 Mon Sep 17 00:00:00 2001 From: Chris Lew Date: Fri, 4 Oct 2019 15:26:59 -0700 Subject: [PATCH 05/10] rpmsg: glink: Put an extra reference during cleanup In a remote processor crash scenario, there is no guarantee the remote processor sent close requests before it went into a bad state. Remove the reference that is normally handled by the close command in the so channel resources can be released. Fixes: b4f8e52b89f6 ("rpmsg: Introduce Qualcomm RPM glink driver") Cc: stable@vger.kernel.org Tested-by: Srinivas Kandagatla Signed-off-by: Chris Lew Reported-by: Srinivas Kandagatla Signed-off-by: Bjorn Andersson --- drivers/rpmsg/qcom_glink_native.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c index 72ed671f5dcd..21fd2ae5f7f1 100644 --- a/drivers/rpmsg/qcom_glink_native.c +++ b/drivers/rpmsg/qcom_glink_native.c @@ -1641,6 +1641,10 @@ void qcom_glink_native_remove(struct qcom_glink *glink) idr_for_each_entry(&glink->lcids, channel, cid) kref_put(&channel->refcount, qcom_glink_channel_release); + /* Release any defunct local channels, waiting for close-req */ + idr_for_each_entry(&glink->rcids, channel, cid) + kref_put(&channel->refcount, qcom_glink_channel_release); + idr_destroy(&glink->lcids); idr_destroy(&glink->rcids); spin_unlock_irqrestore(&glink->idr_lock, flags); From f7e714988edaffe6ac578318e99501149b067ba0 Mon Sep 17 00:00:00 2001 From: Chris Lew Date: Fri, 4 Oct 2019 15:27:00 -0700 Subject: [PATCH 06/10] rpmsg: glink: Fix rpmsg_register_device err handling The device release function is set before registering with rpmsg. If rpmsg registration fails, the framework will call device_put(), which invokes the release function. The channel create logic does not need to free rpdev if rpmsg_register_device() fails and release is called. Fixes: b4f8e52b89f6 ("rpmsg: Introduce Qualcomm RPM glink driver") Cc: stable@vger.kernel.org Tested-by: Srinivas Kandagatla Signed-off-by: Chris Lew Signed-off-by: Bjorn Andersson --- drivers/rpmsg/qcom_glink_native.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c index 21fd2ae5f7f1..89e02baea2d0 100644 --- a/drivers/rpmsg/qcom_glink_native.c +++ b/drivers/rpmsg/qcom_glink_native.c @@ -1423,15 +1423,13 @@ static int qcom_glink_rx_open(struct qcom_glink *glink, unsigned int rcid, ret = rpmsg_register_device(rpdev); if (ret) - goto free_rpdev; + goto rcid_remove; channel->rpdev = rpdev; } return 0; -free_rpdev: - kfree(rpdev); rcid_remove: spin_lock_irqsave(&glink->idr_lock, flags); idr_remove(&glink->rcids, channel->rcid); From c3dadc19b7564c732598b30d637c6f275c3b77b6 Mon Sep 17 00:00:00 2001 From: Bjorn Andersson Date: Fri, 4 Oct 2019 15:27:01 -0700 Subject: [PATCH 07/10] rpmsg: glink: Don't send pending rx_done during remove Attempting to transmit rx_done messages after the GLINK instance is being torn down will cause use after free and memory leaks. So cancel the intent_work and free up the pending intents. With this there are no concurrent accessors of the channel left during qcom_glink_native_remove() and there is therefor no need to hold the spinlock during this operation - which would prohibit the use of cancel_work_sync() in the release function. So remove this. Fixes: 1d2ea36eead9 ("rpmsg: glink: Add rx done command") Cc: stable@vger.kernel.org Acked-by: Chris Lew Tested-by: Srinivas Kandagatla Signed-off-by: Bjorn Andersson --- drivers/rpmsg/qcom_glink_native.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c index 89e02baea2d0..38599ff2b0a2 100644 --- a/drivers/rpmsg/qcom_glink_native.c +++ b/drivers/rpmsg/qcom_glink_native.c @@ -241,11 +241,23 @@ static void qcom_glink_channel_release(struct kref *ref) { struct glink_channel *channel = container_of(ref, struct glink_channel, refcount); + struct glink_core_rx_intent *intent; struct glink_core_rx_intent *tmp; unsigned long flags; int iid; + /* cancel pending rx_done work */ + cancel_work_sync(&channel->intent_work); + spin_lock_irqsave(&channel->intent_lock, flags); + /* Free all non-reuse intents pending rx_done work */ + list_for_each_entry_safe(intent, tmp, &channel->done_intents, node) { + if (!intent->reuse) { + kfree(intent->data); + kfree(intent); + } + } + idr_for_each_entry(&channel->liids, tmp, iid) { kfree(tmp->data); kfree(tmp); @@ -1625,7 +1637,6 @@ void qcom_glink_native_remove(struct qcom_glink *glink) struct glink_channel *channel; int cid; int ret; - unsigned long flags; disable_irq(glink->irq); cancel_work_sync(&glink->rx_work); @@ -1634,7 +1645,6 @@ void qcom_glink_native_remove(struct qcom_glink *glink) if (ret) dev_warn(glink->dev, "Can't remove GLINK devices: %d\n", ret); - spin_lock_irqsave(&glink->idr_lock, flags); /* Release any defunct local channels, waiting for close-ack */ idr_for_each_entry(&glink->lcids, channel, cid) kref_put(&channel->refcount, qcom_glink_channel_release); @@ -1645,7 +1655,6 @@ void qcom_glink_native_remove(struct qcom_glink *glink) idr_destroy(&glink->lcids); idr_destroy(&glink->rcids); - spin_unlock_irqrestore(&glink->idr_lock, flags); mbox_free_channel(glink->mbox_chan); } EXPORT_SYMBOL_GPL(qcom_glink_native_remove); From 278bcb7300f61785dba63840bd2a8cf79f14554c Mon Sep 17 00:00:00 2001 From: Bjorn Andersson Date: Fri, 4 Oct 2019 15:27:02 -0700 Subject: [PATCH 08/10] rpmsg: glink: Free pending deferred work on remove By just cancelling the deferred rx worker during GLINK instance teardown any pending deferred commands are leaked, so free them. Fixes: b4f8e52b89f6 ("rpmsg: Introduce Qualcomm RPM glink driver") Cc: stable@vger.kernel.org Acked-by: Chris Lew Tested-by: Srinivas Kandagatla Signed-off-by: Bjorn Andersson --- drivers/rpmsg/qcom_glink_native.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c index 38599ff2b0a2..1995f5b3ea67 100644 --- a/drivers/rpmsg/qcom_glink_native.c +++ b/drivers/rpmsg/qcom_glink_native.c @@ -1562,6 +1562,18 @@ static void qcom_glink_work(struct work_struct *work) } } +static void qcom_glink_cancel_rx_work(struct qcom_glink *glink) +{ + struct glink_defer_cmd *dcmd; + struct glink_defer_cmd *tmp; + + /* cancel any pending deferred rx_work */ + cancel_work_sync(&glink->rx_work); + + list_for_each_entry_safe(dcmd, tmp, &glink->rx_queue, node) + kfree(dcmd); +} + struct qcom_glink *qcom_glink_native_probe(struct device *dev, unsigned long features, struct qcom_glink_pipe *rx, @@ -1639,7 +1651,7 @@ void qcom_glink_native_remove(struct qcom_glink *glink) int ret; disable_irq(glink->irq); - cancel_work_sync(&glink->rx_work); + qcom_glink_cancel_rx_work(glink); ret = device_for_each_child(glink->dev, NULL, qcom_glink_remove_device); if (ret) From bb06a5ce88fd3489d2415470b20ab949550a62ec Mon Sep 17 00:00:00 2001 From: Christophe JAILLET Date: Tue, 29 Oct 2019 07:09:14 +0100 Subject: [PATCH 09/10] rpmsg: char: Simplify 'rpmsg_eptdev_release()' Use 'skb_queue_purge()' instead of re-implementing it. Signed-off-by: Christophe JAILLET Signed-off-by: Bjorn Andersson --- drivers/rpmsg/rpmsg_char.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c index c655074c07c2..876ce43df732 100644 --- a/drivers/rpmsg/rpmsg_char.c +++ b/drivers/rpmsg/rpmsg_char.c @@ -146,7 +146,6 @@ static int rpmsg_eptdev_release(struct inode *inode, struct file *filp) { struct rpmsg_eptdev *eptdev = cdev_to_eptdev(inode->i_cdev); struct device *dev = &eptdev->dev; - struct sk_buff *skb; /* Close the endpoint, if it's not already destroyed by the parent */ mutex_lock(&eptdev->ept_lock); @@ -157,10 +156,7 @@ static int rpmsg_eptdev_release(struct inode *inode, struct file *filp) mutex_unlock(&eptdev->ept_lock); /* Discard all SKBs */ - while (!skb_queue_empty(&eptdev->queue)) { - skb = skb_dequeue(&eptdev->queue); - kfree_skb(skb); - } + skb_queue_purge(&eptdev->queue); put_device(dev); From 8cf9b615653e40debdcf3b6f1d69e6b580d71173 Mon Sep 17 00:00:00 2001 From: Krzysztof Kozlowski Date: Wed, 20 Nov 2019 21:39:44 +0800 Subject: [PATCH 10/10] rpmsg: Fix Kconfig indentation Adjust indentation from spaces to tab (+optional two spaces) as in coding style with command like: $ sed -e 's/^ /\t/' -i */Kconfig Signed-off-by: Krzysztof Kozlowski Link: https://lore.kernel.org/r/20191120133945.13938-1-krzk@kernel.org Signed-off-by: Bjorn Andersson --- drivers/rpmsg/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig index d0322b41eca5..709276540ef1 100644 --- a/drivers/rpmsg/Kconfig +++ b/drivers/rpmsg/Kconfig @@ -21,7 +21,7 @@ config RPMSG_QCOM_GLINK_NATIVE config RPMSG_QCOM_GLINK_RPM tristate "Qualcomm RPM Glink driver" - select RPMSG_QCOM_GLINK_NATIVE + select RPMSG_QCOM_GLINK_NATIVE depends on HAS_IOMEM depends on MAILBOX help