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.
