lkml.org 
[lkml]   [2016]   [Mar]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] rapidio: add RapidIO channelized messaging driver
On Mon, 21 Mar 2016 07:38:01 -0400 Alexandre Bounine <alexandre.bounine@idt.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;
> +}
>
> ...
>

\
 
 \ /
  Last update: 2016-03-23 23:01    [W:0.066 / U:5.524 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site