Skip to contentSkip to conversation

mailbox: Add generic mechanism for testing Mailbox Controllers

DISCUSSION Dan Carpenter 2 participants 2 messages Oct 21, 2015 – Oct 26, 2015 Kernel janitors

lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

Hello Lee Jones,

The patch 8ea4484d0c2b: "mailbox: Add generic mechanism for testing Mailbox Controllers" from Oct 16, 2015, leads to the following static checker warning:

drivers/mailbox/mailbox-test.c:71 mbox_test_signal_write()
warn: copy_to/from_user() returns a positive value

drivers/mailbox/mailbox-test.c

    42  static ssize_t mbox_test_signal_write(struct file *filp,
    43                                         const char __user *userbuf,
    44                                         size_t count, loff_t *ppos)
    45  {
    46          struct mbox_test_device *tdev = filp->private_data;
    47          int ret;
    48  
    49          if (!tdev->tx_channel) {
    50                  dev_err(tdev->dev, "Channel cannot do Tx\n");
    51                  return -EINVAL;
    52          }
    53  
    54          if (count > MBOX_MAX_SIG_LEN) {
    55                  dev_err(tdev->dev,
    56                          "Signal length %zd greater than max allowed %d\n",
    57                          count, MBOX_MAX_SIG_LEN);
    58                  return -EINVAL;
    59          }
    60  
    61          tdev->signal = kzalloc(MBOX_MAX_SIG_LEN, GFP_KERNEL);
    62          if (!tdev->signal)
    63                  return -ENOMEM;

This feels racy. Also if ->signal has already been allocated then this leaks.

    64  
    65          ret = copy_from_user(tdev->signal, userbuf, count);
    66          if (ret) {
    67                  kfree(tdev->signal);
    68                  return -EFAULT;
    69          }

Normally we do it like this:

	if (copy_from_user(tdev->signal, userbuf, count)) {
		kfree(tdev->signal);
		tdev->signal;  <-- also let's set it to NULL or it leads
				   to a use after free.
		return -EFAULT;
	}



    70  
    71          return ret < 0 ? ret : count;

"ret" is always zero here. But we can just get rid of that variable completely.

    72  }

regards,
dan carpenter

#1 lore raw
Quoted 58 lines · Dan Carpenter

Hello Lee Jones,

The patch 8ea4484d0c2b: "mailbox: Add generic mechanism for testing Mailbox Controllers" from Oct 16, 2015, leads to the following static checker warning:

drivers/mailbox/mailbox-test.c:71 mbox_test_signal_write()
warn: copy_to/from_user() returns a positive value

drivers/mailbox/mailbox-test.c

    42  static ssize_t mbox_test_signal_write(struct file *filp,
    43                                         const char __user *userbuf,
    44                                         size_t count, loff_t *ppos)
    45  {
    46          struct mbox_test_device *tdev = filp->private_data;
    47          int ret;
    48  
    49          if (!tdev->tx_channel) {
    50                  dev_err(tdev->dev, "Channel cannot do Tx\n");
    51                  return -EINVAL;
    52          }
    53  
    54          if (count > MBOX_MAX_SIG_LEN) {
    55                  dev_err(tdev->dev,
    56                          "Signal length %zd greater than max allowed %d\n",
    57                          count, MBOX_MAX_SIG_LEN);
    58                  return -EINVAL;
    59          }
    60  
    61          tdev->signal = kzalloc(MBOX_MAX_SIG_LEN, GFP_KERNEL);
    62          if (!tdev->signal)
    63                  return -ENOMEM;

This feels racy. Also if ->signal has already been allocated then this leaks.

    64  
    65          ret = copy_from_user(tdev->signal, userbuf, count);
    66          if (ret) {
    67                  kfree(tdev->signal);
    68                  return -EFAULT;
    69          }

Normally we do it like this:

	if (copy_from_user(tdev->signal, userbuf, count)) {
		kfree(tdev->signal);
		tdev->signal;  <-- also let's set it to NULL or it leads
				   to a use after free.
		return -EFAULT;
	}



    70  
    71          return ret < 0 ? ret : count;
Quoted 2 lines

"ret" is always zero here. But we can just get rid of that variable completely.

Thanks.

Will follow-up with patches.

Signature · 8 lines
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html