Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 92721C432C2
Subject: Re: [PATCH v1 3/3] bus: mhi: clients: Add user space client interface
 driver
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
References: <1589840619-18520-1-git-send-email-hemantk@codeaurora.org>
 <1589840619-18520-4-git-send-email-hemantk@codeaurora.org>
From:   Jeffrey Hugo <jhugo@codeaurora.org>
Message-ID: <3ee9c396-d1e3-d50a-cd27-7b88f84cb37f@codeaurora.org>
Date:   Tue, 19 May 2020 08:23:25 -0600
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <1589840619-18520-4-git-send-email-hemantk@codeaurora.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.