From: Jeffrey Hugo <jhugo@codeaurora.org>
To: Hemant Kumar <hemantk@codeaurora.org>, manivannan.sadhasivam@linaro.org
Cc: linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
bbhatt@codeaurora.org
Subject: Re: [PATCH v1 3/3] bus: mhi: clients: Add user space client interface driver
Date: Tue, 19 May 2020 08:23:25 -0600 [thread overview]
Message-ID: <3ee9c396-d1e3-d50a-cd27-7b88f84cb37f@codeaurora.org> (raw)
In-Reply-To: <1589840619-18520-4-git-send-email-hemantk@codeaurora.org>
On 5/18/2020 4:23 PM, Hemant Kumar wrote:
> +static ssize_t mhi_uci_read(struct file *file,
> + char __user *buf,
> + size_t count,
> + loff_t *ppos)
> +{
> + struct uci_dev *uci_dev = file->private_data;
> + struct mhi_device *mhi_dev = uci_dev->mhi_dev;
> + struct uci_chan *uci_chan = &uci_dev->dl_chan;
> + struct device *dev = &mhi_dev->dev;
> + struct uci_buf *uci_buf;
> + char *ptr;
> + size_t to_copy;
> + int ret = 0;
> +
> + if (!buf)
> + return -EINVAL;
> +
> + dev_dbg(dev, "Client provided buf len:%lu\n", count);
> +
> + /* confirm channel is active */
> + spin_lock_bh(&uci_chan->lock);
> + if (!uci_dev->enabled) {
> + spin_unlock_bh(&uci_chan->lock);
> + return -ERESTARTSYS;
> + }
> +
> + /* No data available to read, wait */
> + if (!uci_chan->cur_buf && list_empty(&uci_chan->pending)) {
> + dev_dbg(dev, "No data available to read waiting\n");
> +
> + spin_unlock_bh(&uci_chan->lock);
> + ret = wait_event_interruptible(uci_chan->wq,
> + (!uci_dev->enabled ||
> + !list_empty(&uci_chan->pending)));
> + if (ret == -ERESTARTSYS) {
> + dev_dbg(dev, "Exit signal caught for node\n");
> + return -ERESTARTSYS;
> + }
> +
> + spin_lock_bh(&uci_chan->lock);
> + if (!uci_dev->enabled) {
> + dev_dbg(dev, "node is disabled\n");
> + ret = -ERESTARTSYS;
> + goto read_error;
> + }
> + }
> +
> + /* new read, get the next descriptor from the list */
> + if (!uci_chan->cur_buf) {
> + uci_buf = list_first_entry_or_null(&uci_chan->pending,
> + struct uci_buf, node);
> + if (unlikely(!uci_buf)) {
> + ret = -EIO;
> + goto read_error;
> + }
> +
> + list_del(&uci_buf->node);
> + uci_chan->cur_buf = uci_buf;
> + uci_chan->rx_size = uci_buf->len;
> + dev_dbg(dev, "Got pkt of size:%zu\n", uci_chan->rx_size);
> + }
> +
> + uci_buf = uci_chan->cur_buf;
> +
> + /* Copy the buffer to user space */
> + to_copy = min_t(size_t, count, uci_chan->rx_size);
> + ptr = uci_buf->data + (uci_buf->len - uci_chan->rx_size);
> + spin_unlock_bh(&uci_chan->lock);
This presents a race condition. Imagine if you will, two processes are
in read() at the same time. They will be contending for the lock. One
will win, and enter the above critical section. As soon as the winner
gets here and unlocks the lock, the loser will get the lock and enter
the critical section. There is a possibility that both processes get to
this point at roughly the same time, and thus both have a cached version
of the identical cur_buf pointer. At this point, bad things can happen.
If both processes consume the entire buffer, they will then both
re-queue the buffer to MHI. You then can cause memory corruption, and a
kernel crash if the buffer is consumed, then freed, then the memory is
allocated to somewhere else in the kernel, then MHI consumed the buffer
again (memory corruption). If the buffer gets freed again, slub may
trigger a BUG_ON if the allocation for elsewhere is completely different
in structure.
I think you need a mutex covering the entire read() operation to
serialize it.
> +
> + ret = copy_to_user(buf, ptr, to_copy);
> + if (ret)
> + return ret;
> +
> + spin_lock_bh(&uci_chan->lock);
> +
> + /* Buffer already queued from diff thread while we dropped lock */
> + if (to_copy && !uci_chan->rx_size) {
> + dev_dbg(dev, "Bailout as buffer already queued (%lu %lu)\n",
> + to_copy, uci_chan->rx_size);
> + goto read_error;
> + }
> +
> + dev_dbg(dev, "Copied %lu of %lu bytes\n", to_copy, uci_chan->rx_size);
> + uci_chan->rx_size -= to_copy;
> +
> + /* we finished with this buffer, queue it back to hardware */
> + if (!uci_chan->rx_size) {
> + uci_chan->cur_buf = NULL;
> +
> + if (uci_dev->enabled)
> + ret = mhi_queue_buf(mhi_dev, DMA_FROM_DEVICE,
> + uci_buf->data,
> + uci_dev->actual_mtu, MHI_EOT);
> + else
> + ret = -ERESTARTSYS;
> +
> + if (ret) {
> + dev_err(dev, "Failed to recycle element\n");
> + kfree(uci_buf->data);
> + goto read_error;
> + }
> + }
> + spin_unlock_bh(&uci_chan->lock);
> +
> + dev_dbg(dev, "Returning %lu bytes\n", to_copy);
> +
> + return to_copy;
> +
> +read_error:
> + spin_unlock_bh(&uci_chan->lock);
> +
> + return ret;
> +}
> +
--
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
prev parent reply other threads:[~2020-05-19 14:23 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-18 22:23 [PATCH v1 0/3] user space client interface driver Hemant Kumar
2020-05-18 22:23 ` [PATCH v1 1/3] bus: mhi: core: Add helper API to return number of free TREs Hemant Kumar
2020-05-18 22:23 ` [PATCH v1 2/3] bus: mhi: core: Move MHI_MAX_MTU to external header file Hemant Kumar
2020-05-18 22:23 ` [PATCH v1 3/3] bus: mhi: clients: Add user space client interface driver Hemant Kumar
2020-05-19 14:23 ` Jeffrey Hugo [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3ee9c396-d1e3-d50a-cd27-7b88f84cb37f@codeaurora.org \
--to=jhugo@codeaurora.org \
--cc=bbhatt@codeaurora.org \
--cc=hemantk@codeaurora.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=manivannan.sadhasivam@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).