Message-Id: <20160323144851.8a50bad7648ae3ae8e412fb4@linux-foundation.org>
Date:	Wed, 23 Mar 2016 14:48:51 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Alexandre Bounine <alexandre.bounine@....com>
Cc:	Matt Porter <mporter@...nel.crashing.org>,
	Aurelien Jacquiot <a-jacquiot@...com>,
	Andre van Herk <andre.van.herk@...drive-technologies.com>,
	Barry Wood <barry.wood@....com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rapidio: add RapidIO channelized messaging driver

On Mon, 21 Mar 2016 07:38:01 -0400 Alexandre Bounine <alexandre.bounine@....com> wrote:

> Add channelized messaging driver to support native RapidIO messaging
> exchange between multiple senders/recipients.
> 
> This device driver is the result of collaboration within the RapidIO.org
> Software Task Group (STG) between Texas Instruments, Freescale,
> Prodrive Technologies, Nokia Networks, BAE and IDT.  Additional input was
> received from other members of RapidIO.org. The objective was to create a
> character mode driver interface which exposes messaging capabilities
> of RapidIO endpoint devices (mports) directly to applications, in a manner
> that allows the numerous and varied RapidIO implementations
> to interoperate.
> 
> This new char mode device driver allows user-space applications to setup
> messaging communication channels using a single shared RapidIO mailbox.
> 
> By default this driver uses RapidIO MBOX_1 (MBOX_0 is reserved for use
> by RIONET Ethernet emulation driver).
> 
> ...
>
> --- /dev/null
> +++ b/Documentation/rapidio/rio_cm.txt
> @@ -0,0 +1,94 @@
> +RapidIO subsystem Channelized Messaging character device driver (rio_cm.c)
> +==========================================================================
> +
> +Version History:
> +----------------
> +  1.0.0 - Initial driver release.
> +
> +==========================================================================
> +
> +I. Overview
> +
> +This device driver is the result of collaboration within the RapidIO.org
> +Software Task Group (STG) between Texas Instruments, Freescale,
> +Prodrive Technologies, Nokia Networks, BAE and IDT.  Additional input was
> +received from other members of RapidIO.org. The objective was to create a
> +character mode driver interface which exposes messaging capabilities of RapidIO
> +endpoint devices (mports) directly to applications, in a manner that allows
> +the numerous and varied RapidIO implementations to interoperate.
> +
> +This driver (RIO_CM) provides to user-space applications shared access to
> +RapidIO mailbox messaging resources.
> +
> +RapidIO specification (Part 2) defines that endpoint devices may have up to four
> +messaging mailboxes in case of multi-packet message (up to 4KB) and
> +up to 64 mailboxes if single-packet messages (up to 256 B) are used. In addition
> +to protocol definition limitations, a particular hardware implementation can
> +have reduced number of messaging mailboxes.  RapidIO aware applications must
> +therefore share the messaging resources of a RapidIO endpoint.
> +
> +Main purpose of this device driver is to provide RapidIO mailbox messaging
> +capability to large number of user-space processes by introducing socket-like
> +operations using a single messaging mailbox.  This allows applications to
> +use the limited RapidIO hardware resources efficiently and effectively.
> +
> +Most of device driver's operations are supported through 'ioctl' system calls.
> +
> +When loaded this device driver creates a single filesystem node named rio_cm
> +in /dev directory common for all registered RapidIO mport devices.
> +
> +Using available set of ioctl commands user-space applications can perform
> +following operations:
> +
> +- Obtain list of local mport devices that support messaging operations
> +    (RIO_CM_MPORT_GET_LIST).
> +- Obtain list of messaging capable remote endpoints (peers) in a RapidIO network
> +    (RIO_CM_EP_GET_LIST_SIZE/RIO_CM_EP_GET_LIST).
> +- Create RapidIO messaging channel (RIO_MPORT_MAINT_HDID_SET).
> +- Bind created channel to the specified mport device (RIO_CM_CHAN_BIND).
> +- Listen on the specified channel (RIO_CM_CHAN_LISTEN)
> +- Accept a connection request on the specified channel (RIO_CM_CHAN_ACCEPT)
> +- Send a connection request to a remote node/channel (RIO_CM_CHAN_CONNECT).
> +- Send a data message to a connected channel (RIO_CM_CHAN_SEND).
> +- Receive a data message on a connected channel (RIO_CM_CHAN_RECEIVE).
> +- Close an active channel (RIO_CM_CHAN_CLOSE).

The ioctl interface isn't really documented?

> +II. Hardware Compatibility
> +
> +This device driver uses standard interfaces defined by kernel RapidIO subsystem
> +and therefore it can be used with any mport device driver registered by RapidIO
> +subsystem with limitations set by available mport HW implementation of messaging
> +mailboxes.
> +
> +III. Module parameters
> +
> +- 'dbg_level' - This parameter allows to control amount of debug information
> +        generated by this device driver. This parameter is formed by set of
> +        bit masks that correspond to the specific functional block.
> +        For mask definitions see 'drivers/rapidio/devices/rio_cm.c'
> +        This parameter can be changed dynamically.
> +        Use CONFIG_RAPIDIO_DEBUG=y to enable debug output at the top level.
> +
> +- 'cmbox' - Number of RapidIO mailbox to use (default value is 1).
> +        This parameter allows to set messaging mailbox number that will be used
> +        within entire RapidIO network. It can be used when default mailbox is
> +        used by other device drivers or is not supported by some nodes in the
> +        RapidIO network.
> +
> +- 'chstart' - Start channel number for dynamic assignment. Default value - 256.
> +        Allows to exclude channel numbers below this parameter from dynamic
> +		allocation to avoid conflicts with software components that use
> +		reserved predefined channel numbers.
> +
> 
> ...
>
> +/*
> + * rio_cm_handler - worker thread that services request (non-data) packets
> + */
> +static void rio_cm_handler(struct work_struct *_work)
> +{
> +	struct rio_cm_work *work;
> +	void *data;
> +	struct rio_ch_chan_hdr *hdr;
> +
> +	work = container_of(_work, struct rio_cm_work, work);
> +	data = work->data;
> +
> +	if (!rio_mport_is_running(work->cm->mport))
> +		goto out;
> +
> +	hdr = (struct rio_ch_chan_hdr *)data;

Nit: the driver does this cast in quite a few places.  Casting of void*
is unnecessary and is in fact a bit harmful: if, later on, some `int
data' comes into scope, you want the warning to come out.  But the cast
will prevent that from happening.

> +	riocm_debug(RX_CMD, "OP=%x for ch=%d from %d",
> +		    hdr->ch_op, ntohs(hdr->dst_ch), ntohs(hdr->src_ch));
> +
> +	switch (hdr->ch_op) {
> +	case CM_CONN_REQ:
> +		riocm_req_handler(work->cm, data);
> +		break;
> +	case CM_CONN_ACK:
> +		riocm_resp_handler(data);
> +		break;
> +	case CM_CONN_CLOSE:
> +		riocm_close_handler(data);
> +		break;
> +	default:
> +		riocm_error("Invalid packet header");
> +		break;
> +	}
> +out:
> +	kfree(data);
> +	kfree(work);
> +}
> +
> 
> ...
>
> +static int riocm_post_send(struct cm_dev *cm, struct rio_dev *rdev,
> +			   void *buffer, size_t len, int req)
> +{
> +	int rc;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&cm->tx_lock, flags);
> +
> +	if (cm->mport == NULL) {
> +		rc = -ENODEV;
> +		goto err_out;
> +	}
> +
> +	if (cm->tx_cnt == RIOCM_TX_RING_SIZE) {
> +		if (req) {
> +			struct tx_req *treq;
> +
> +			treq = kzalloc(sizeof(struct tx_req), GFP_ATOMIC);

The GFP_ATOMIC is unfortunate - it is unreliable.  As far as I can
tell, this function *could* use something stronger (even GFP_KERNEL) if
it weren't for that spin_lock.  Perhaps do this:

	if (cm->tx_cnt == RIOCM_TX_RING_SIZE) {
		treq = kzalloc(..., GFP_KERNEL);
		spin_lock_irqsave(&cm->tx_lock, flags);
		if (cm->tx_cnt == RIOCM_TX_RING_SIZE) {
			...
		} else {
			/* race happened */
			kfree(treq);
			...
		}
	}

> +			if (treq == NULL) {
> +				rc = -ENOMEM;
> +				goto err_out;
> +			}
> +			treq->rdev = rdev;
> +			treq->buffer = buffer;
> +			treq->len = len;
> +			list_add_tail(&treq->node, &cm->tx_reqs);
> +		}
> +		riocm_debug(TX, "Tx Queue is full");
> +		rc = -EBUSY;
> +		goto err_out;
> +	}
> +
> +	cm->tx_buf[cm->tx_slot] = buffer;
> +	rc = rio_add_outb_message(cm->mport, rdev, cmbox, buffer, len);
> +
> +	riocm_debug(TX, "Add buf@%p destid=%x tx_slot=%d tx_cnt=%d",
> +		 buffer, rdev->destid, cm->tx_slot, cm->tx_cnt);
> +
> +	++cm->tx_cnt;
> +	++cm->tx_slot;
> +	cm->tx_slot &= (RIOCM_TX_RING_SIZE - 1);
> +
> +err_out:
> +	spin_unlock_irqrestore(&cm->tx_lock, flags);
> +	return rc;
> +}
> +
> 
> ...
>
> +static int riocm_send_close(struct rio_channel *ch)
> +{
> +	struct rio_ch_chan_hdr *hdr;
> +	int ret;
> +
> +	/*
> +	 * Send CH_CLOSE notification to the remote RapidIO device
> +	 */
> +
> +	hdr = kzalloc(sizeof(struct rio_ch_chan_hdr), GFP_KERNEL);

Nit: I prefer

	hdr = kzalloc(*hdr, GFP_KERNEL);

so the poor old reviewer doesn't have to check the type every time.

> +	if (hdr == NULL)
> +		return -ENOMEM;
> +
> +	hdr->bhdr.src_id = htonl(ch->loc_destid);
> +	hdr->bhdr.dst_id = htonl(ch->rem_destid);
> +	hdr->bhdr.src_mbox = cmbox;
> +	hdr->bhdr.dst_mbox = cmbox;
> +	hdr->bhdr.type = RIO_CM_CHAN;
> +	hdr->ch_op = CM_CONN_CLOSE;
> +	hdr->dst_ch = htons(ch->rem_channel);
> +	hdr->src_ch = htons(ch->id);
> +
> +	/* ATTN: the function call below relies on the fact that underlying
> +	 * add_outb_message() routine copies TX data into its internal transfer
> +	 * buffer. Needs to be reviewed if switched to direct buffer mode.
> +	 */
> +	ret = riocm_post_send(ch->cmdev, ch->rdev, hdr, sizeof(*hdr), 1);
> +	if (ret == -EBUSY)
> +		ret = 0;
> +	else
> +		kfree(hdr);
> +
> +	if (ret)
> +		riocm_error("ch(%d) send CLOSE failed (ret=%d)", ch->id, ret);
> +
> +	return ret;
> +}
> +
> 
> ...
>
> +static int cm_ep_get_list(void __user *arg)
> +{
> +	int ret = 0;
> +	uint32_t info[2];
> +	void *buf;
> +
> +	if (copy_from_user(&info, arg, sizeof(info)))
> +		return -EFAULT;
> +
> +	buf = kcalloc(info[0] + 2, sizeof(u32), GFP_KERNEL);

We're giving userspace the ability to make the kernel kmalloc()
arbitrary amounts of memory.  Even "negative" amounts.  Sounds a bit
shaky.  Some sanity checking in here would be good to have.

> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = riocm_get_peer_list(info[1], (u8 *)buf + 2*sizeof(u32), &info[0]);
> +	if (ret)
> +		goto out;
> +
> +	((u32 *)buf)[0] = info[0]; /* report an updated number of entries */
> +	((u32 *)buf)[1] = info[1]; /* put back an mport ID */
> +	if (copy_to_user(arg, buf, sizeof(u32) * (info[0] + 2)))
> +		ret = -EFAULT;
> +out:
> +	kfree(buf);
> +	return ret;
> +}
> +
> +/*
> + * cm_mport_get_list() - Returns list of attached endpoints
> + */
> +static int cm_mport_get_list(void __user *arg)
> +{
> +	int ret = 0;
> +	uint32_t entries;
> +	void *buf;
> +	struct cm_dev *cm;
> +	u32 *entry_ptr;
> +	int count = 0;
> +
> +	if (copy_from_user(&entries, arg, sizeof(entries)))
> +		return -EFAULT;
> +	if (entries == 0)
> +		return -ENOMEM;
> +	buf = kcalloc(entries + 1, sizeof(u32), GFP_KERNEL);

Again.

> +	if (!buf)
> +		return -ENOMEM;
> +
> +	/* Scan all registered cm_dev objects */
> +	entry_ptr = (u32 *)((u8 *)buf + sizeof(u32));
> +	down_read(&rdev_sem);
> +	list_for_each_entry(cm, &cm_dev_list, list) {
> +		if (count++ < entries) {
> +			*entry_ptr = (cm->mport->id << 16) |
> +				      cm->mport->host_deviceid;
> +			entry_ptr++;
> +		}
> +	}
> +	up_read(&rdev_sem);
> +
> +	*((u32 *)buf) = count; /* report a real number of entries */
> +	if (copy_to_user(arg, buf, sizeof(u32) * (count + 1)))
> +		ret = -EFAULT;
> +
> +	kfree(buf);
> +	return ret;
> +}
> +
> 
> ...
>
> +static int cm_chan_connect(void __user *arg)
> +{
> +	struct rio_cm_channel chan;
> +
> +	if (copy_from_user(&chan, arg, sizeof(struct rio_cm_channel)))
> +		return -EFAULT;
> +
> +	return riocm_ch_connect(chan.id, chan.mport_id,
> +				chan.remote_destid, chan.remote_channel);

Again, does all this stuff we're accepting from userspace get a
suitably paranoid level of checking?  If not, this driver will be
adding all sorts of security holes.

Is the ioctl interface privileged?  root-only?  This should be
mentioned in the interface documentation!

> +}
> +
> +/*
> + * cm_chan_msg_send() - Connect on channel
> + * @arg:	Outbound message information
> + */
> +static int cm_chan_msg_send(void __user *arg)
> +{
> +	struct rio_cm_msg msg;
> +	void *buf;
> +	int ret = 0;
> +
> +	if (copy_from_user(&msg, arg, sizeof(struct rio_cm_msg)))
> +		return -EFAULT;
> +
> +	buf = kmalloc(RIO_MAX_MSG_SIZE, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	if (copy_from_user(buf, msg.msg, msg.size)) {

Big bad bug.  If msg.size is greater than RIO_MAX_MSG_SIZE, userspace
will write userspace-controlled data into the memory beyond the end of
`buf'.  This is the sort of thing of which root exploits are made.

This whole ioctl interface needs a careful going over, please.  I
should be able to read this code and easily know "yup, this is safe".

> +		ret = -EFAULT;
> +		goto out;
> +	}
> +
> +	ret = riocm_ch_send(msg.ch_num, buf, msg.size);
> +out:
> +	kfree(buf);
> +	return ret;
> +}
>
> ...
>

Powered by blists - more mailing lists