From: Markus Lidel <Markus.Lidel@shadowconnect.com>
To: Jesper Juhl <juhl-lkml@dif.dk>
Cc: Christoph Hellwig <hch@infradead.org>,
linux-kernel <linux-kernel@vger.kernel.org>,
Alan Cox <alan@lxorguk.ukuu.org.uk>,
Andrew Morton <akpm@osdl.org>
Subject: Re: [PATCH] add missing checks of __copy_to_user return value in i2o_config.c
Date: Thu, 07 Oct 2004 23:01:45 +0200 [thread overview]
Message-ID: <4165AEB9.8040608@shadowconnect.com> (raw)
In-Reply-To: <Pine.LNX.4.61.0410062159590.2975@dragon.hygekrogen.localhost>
[-- Attachment #1: Type: text/plain, Size: 522 bytes --]
Hello,
thanks for fixing i2o_config. In the meantime i've also made a patch,
which tries to reduce the many return's too.
Note: i've not touched the passthru stuff yet.
Please let me know, which you think of it.
Best regards,
Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)
Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany
Phone: +49 82 82/99 51-0
Fax: +49 82 82/99 51-11
E-Mail: Markus.Lidel@shadowconnect.com
URL: http://www.shadowconnect.com
[-- Attachment #2: i2o_config-copy_user-fix.patch --]
[-- Type: text/x-patch, Size: 12274 bytes --]
--- a/drivers/message/i2o/i2o_config.c 2004-09-24 11:05:12.044972000 +0200
+++ b/drivers/message/i2o/i2o_config.c 2004-10-07 22:49:59.786543596 +0200
@@ -178,18 +178,17 @@
struct i2o_controller *c;
u8 __user *user_iop_table = (void __user *)arg;
u8 tmp[MAX_I2O_CONTROLLERS];
+ int rc = 0;
memset(tmp, 0, MAX_I2O_CONTROLLERS);
- if (!access_ok(VERIFY_WRITE, user_iop_table, MAX_I2O_CONTROLLERS))
- return -EFAULT;
-
list_for_each_entry(c, &i2o_controllers, list)
tmp[c->unit] = 1;
- __copy_to_user(user_iop_table, tmp, MAX_I2O_CONTROLLERS);
+ if(copy_to_user(user_iop_table, tmp, MAX_I2O_CONTROLLERS))
+ rc = -EFAULT;
- return 0;
+ return rc;
};
static int i2o_cfg_gethrt(unsigned long arg)
@@ -200,20 +199,28 @@
i2o_hrt *hrt;
int len;
u32 reslen;
- int ret = 0;
+ int rc = 0;
- if (copy_from_user(&kcmd, cmd, sizeof(struct i2o_cmd_hrtlct)))
- return -EFAULT;
+ if (copy_from_user(&kcmd, cmd, sizeof(struct i2o_cmd_hrtlct))) {
+ rc = -EFAULT;
+ goto exit;
+ }
- if (get_user(reslen, kcmd.reslen) < 0)
- return -EFAULT;
+ if (get_user(reslen, kcmd.reslen) < 0) {
+ rc = -EFAULT;
+ goto exit;
+ }
- if (kcmd.resbuf == NULL)
- return -EFAULT;
+ if (kcmd.resbuf == NULL) {
+ rc = -EFAULT;
+ goto exit;
+ }
c = i2o_find_iop(kcmd.iop);
- if (!c)
- return -ENXIO;
+ if (!c) {
+ rc = -ENXIO;
+ goto exit;
+ }
hrt = (i2o_hrt *) c->hrt.virt;
@@ -221,12 +228,16 @@
/* We did a get user...so assuming mem is ok...is this bad? */
put_user(len, kcmd.reslen);
- if (len > reslen)
- ret = -ENOBUFS;
+ if (len > reslen) {
+ rc = -ENOBUFS;
+ goto exit;
+ }
+
if (copy_to_user(kcmd.resbuf, (void *)hrt, len))
- ret = -EFAULT;
+ rc = -EFAULT;
- return ret;
+exit:
+ return rc;
};
static int i2o_cfg_getlct(unsigned long arg)
@@ -236,37 +247,48 @@
struct i2o_cmd_hrtlct kcmd;
i2o_lct *lct;
int len;
- int ret = 0;
+ int rc = 0;
u32 reslen;
- if (copy_from_user(&kcmd, cmd, sizeof(struct i2o_cmd_hrtlct)))
- return -EFAULT;
+ if (copy_from_user(&kcmd, cmd, sizeof(struct i2o_cmd_hrtlct))) {
+ rc = -EFAULT;
+ goto exit;
+ }
- if (get_user(reslen, kcmd.reslen) < 0)
- return -EFAULT;
+ if (get_user(reslen, kcmd.reslen) < 0) {
+ rc = -EFAULT;
+ goto exit;
+ }
- if (kcmd.resbuf == NULL)
- return -EFAULT;
+ if (kcmd.resbuf == NULL) {
+ rc = -EFAULT;
+ goto exit;
+ }
c = i2o_find_iop(kcmd.iop);
- if (!c)
- return -ENXIO;
+ if (!c) {
+ rc = -ENXIO;
+ goto exit;
+ }
lct = (i2o_lct *) c->lct;
len = (unsigned int)lct->table_size << 2;
put_user(len, kcmd.reslen);
- if (len > reslen)
- ret = -ENOBUFS;
- else if (copy_to_user(kcmd.resbuf, lct, len))
- ret = -EFAULT;
+ if (len > reslen) {
+ rc = -ENOBUFS;
+ goto exit;
+ }
- return ret;
+ if (copy_to_user(kcmd.resbuf, lct, len))
+ rc = -EFAULT;
+
+exit:
+ return rc;
};
static int i2o_cfg_parms(unsigned long arg, unsigned int type)
{
- int ret = 0;
struct i2o_controller *c;
struct i2o_device *dev;
struct i2o_cmd_psetget __user *cmd =
@@ -276,31 +298,42 @@
u8 *ops;
u8 *res;
int len = 0;
+ int rc = 0;
u32 i2o_cmd = (type == I2OPARMGET ?
I2O_CMD_UTIL_PARAMS_GET : I2O_CMD_UTIL_PARAMS_SET);
- if (copy_from_user(&kcmd, cmd, sizeof(struct i2o_cmd_psetget)))
- return -EFAULT;
+ if (copy_from_user(&kcmd, cmd, sizeof(struct i2o_cmd_psetget))) {
+ rc = -EFAULT;
+ goto exit;
+ }
- if (get_user(reslen, kcmd.reslen))
- return -EFAULT;
+ if (get_user(reslen, kcmd.reslen)) {
+ rc = -EFAULT;
+ goto exit;
+ }
c = i2o_find_iop(kcmd.iop);
- if (!c)
- return -ENXIO;
+ if (!c) {
+ rc = -ENXIO;
+ goto exit;
+ }
dev = i2o_iop_find_device(c, kcmd.tid);
- if (!dev)
- return -ENXIO;
+ if (!dev) {
+ rc = -ENXIO;
+ goto exit;
+ }
ops = (u8 *) kmalloc(kcmd.oplen, GFP_KERNEL);
- if (!ops)
- return -ENOMEM;
+ if (!ops) {
+ rc = -ENOMEM;
+ goto exit;
+ }
if (copy_from_user(ops, kcmd.opbuf, kcmd.oplen)) {
- kfree(ops);
- return -EFAULT;
+ rc = -EFAULT;
+ goto cleanup_ops;
}
/*
@@ -309,27 +342,33 @@
*/
res = (u8 *) kmalloc(65536, GFP_KERNEL);
if (!res) {
- kfree(ops);
- return -ENOMEM;
+ rc = -ENOMEM;
+ goto cleanup_ops;
}
len = i2o_parm_issue(dev, i2o_cmd, ops, kcmd.oplen, res, 65536);
- kfree(ops);
-
if (len < 0) {
- kfree(res);
- return -EAGAIN;
+ rc = -EAGAIN;
+ goto cleanup_res;
}
put_user(len, kcmd.reslen);
- if (len > reslen)
- ret = -ENOBUFS;
- else if (copy_to_user(kcmd.resbuf, res, len))
- ret = -EFAULT;
+ if (len > reslen) {
+ rc = -ENOBUFS;
+ goto cleanup_res;
+ }
+
+ if (copy_to_user(kcmd.resbuf, res, len))
+ rc = -EFAULT;
+cleanup_res:
kfree(res);
- return ret;
+cleanup_ops:
+ kfree(ops);
+
+exit:
+ return rc;
};
static int i2o_cfg_swdl(unsigned long arg)
@@ -342,39 +381,57 @@
u32 m;
unsigned int status = 0, swlen = 0, fragsize = 8192;
struct i2o_controller *c;
+ int rc = 0;
- if (copy_from_user(&kxfer, pxfer, sizeof(struct i2o_sw_xfer)))
- return -EFAULT;
+ if (copy_from_user(&kxfer, pxfer, sizeof(struct i2o_sw_xfer))) {
+ rc = -EFAULT;
+ goto exit;
+ }
- if (get_user(swlen, kxfer.swlen) < 0)
- return -EFAULT;
+ if (get_user(swlen, kxfer.swlen) < 0) {
+ rc = -EFAULT;
+ goto exit;
+ }
- if (get_user(maxfrag, kxfer.maxfrag) < 0)
- return -EFAULT;
+ if (get_user(maxfrag, kxfer.maxfrag) < 0) {
+ rc = -EFAULT;
+ goto exit;
+ }
- if (get_user(curfrag, kxfer.curfrag) < 0)
- return -EFAULT;
+ if (get_user(curfrag, kxfer.curfrag) < 0) {
+ rc = -EFAULT;
+ goto exit;
+ }
if (curfrag == maxfrag)
fragsize = swlen - (maxfrag - 1) * 8192;
- if (!kxfer.buf || !access_ok(VERIFY_READ, kxfer.buf, fragsize))
- return -EFAULT;
+ if (!kxfer.buf) {
+ rc = -EFAULT;
+ goto exit;
+ }
c = i2o_find_iop(kxfer.iop);
- if (!c)
- return -ENXIO;
+ if (!c) {
+ rc = -ENXIO;
+ goto exit;
+ }
m = i2o_msg_get_wait(c, &msg, I2O_TIMEOUT_MESSAGE_GET);
- if (m == I2O_QUEUE_EMPTY)
- return -EBUSY;
+ if (m == I2O_QUEUE_EMPTY) {
+ rc = -EBUSY;
+ goto exit;
+ }
if (i2o_dma_alloc(&c->pdev->dev, &buffer, fragsize, GFP_KERNEL)) {
- i2o_msg_nop(c, m);
- return -ENOMEM;
+ rc = -ENOMEM;
+ goto nop_msg;
}
- __copy_from_user(buffer.virt, kxfer.buf, fragsize);
+ if(copy_from_user(buffer.virt, kxfer.buf, fragsize)) {
+ rc = -EFAULT;
+ goto cleanup_buffer;
+ }
writel(NINE_WORD_MSG_SIZE | SGL_OFFSET_7, &msg->u.head[0]);
writel(I2O_CMD_SW_DOWNLOAD << 24 | HOST_TID << 12 | ADAPTER_TID,
@@ -400,10 +457,19 @@
printk(KERN_INFO
"i2o_config: swdl failed, DetailedStatus = %d\n",
status);
- return status;
+ rc = status;
}
- return 0;
+exit:
+ return rc;
+
+cleanup_buffer:
+ i2o_dma_free(&c->pdev->dev, &buffer);
+
+nop_msg:
+ i2o_msg_nop(c, m);
+
+ return rc;
};
static int i2o_cfg_swul(unsigned long arg)
@@ -416,36 +482,51 @@
u32 m;
unsigned int status = 0, swlen = 0, fragsize = 8192;
struct i2o_controller *c;
+ int rc = 0;
- if (copy_from_user(&kxfer, pxfer, sizeof(struct i2o_sw_xfer)))
- return -EFAULT;
+ if (copy_from_user(&kxfer, pxfer, sizeof(struct i2o_sw_xfer))) {
+ rc = -EFAULT;
+ goto exit;
+ }
- if (get_user(swlen, kxfer.swlen) < 0)
- return -EFAULT;
+ if (get_user(swlen, kxfer.swlen) < 0) {
+ rc = -EFAULT;
+ goto exit;
+ }
- if (get_user(maxfrag, kxfer.maxfrag) < 0)
- return -EFAULT;
+ if (get_user(maxfrag, kxfer.maxfrag) < 0) {
+ rc = -EFAULT;
+ goto exit;
+ }
- if (get_user(curfrag, kxfer.curfrag) < 0)
- return -EFAULT;
+ if (get_user(curfrag, kxfer.curfrag) < 0) {
+ rc = -EFAULT;
+ goto exit;
+ }
if (curfrag == maxfrag)
fragsize = swlen - (maxfrag - 1) * 8192;
- if (!kxfer.buf || !access_ok(VERIFY_WRITE, kxfer.buf, fragsize))
- return -EFAULT;
+ if (!kxfer.buf) {
+ rc = -EFAULT;
+ goto exit;
+ }
c = i2o_find_iop(kxfer.iop);
- if (!c)
- return -ENXIO;
+ if (!c) {
+ rc = -ENXIO;
+ goto exit;
+ }
m = i2o_msg_get_wait(c, &msg, I2O_TIMEOUT_MESSAGE_GET);
- if (m == I2O_QUEUE_EMPTY)
- return -EBUSY;
+ if (m == I2O_QUEUE_EMPTY) {
+ rc = -EBUSY;
+ goto exit;
+ }
if (i2o_dma_alloc(&c->pdev->dev, &buffer, fragsize, GFP_KERNEL)) {
- i2o_msg_nop(c, m);
- return -ENOMEM;
+ rc = -ENOMEM;
+ goto nop_msg;
}
writel(NINE_WORD_MSG_SIZE | SGL_OFFSET_7, &msg->u.head[0]);
@@ -471,13 +552,22 @@
printk(KERN_INFO
"i2o_config: swul failed, DetailedStatus = %d\n",
status);
- return status;
+ rc = status;
+ goto exit;
}
- __copy_to_user(kxfer.buf, buffer.virt, fragsize);
+ if(copy_to_user(kxfer.buf, buffer.virt, fragsize))
+ rc = -EFAULT;
+
i2o_dma_free(&c->pdev->dev, &buffer);
- return 0;
+exit:
+ return rc;
+
+nop_msg:
+ i2o_msg_nop(c, m);
+
+ return rc;
};
static int i2o_cfg_swdel(unsigned long arg)
@@ -489,20 +579,29 @@
u32 m;
unsigned int swlen;
int token;
+ int rc = 0;
- if (copy_from_user(&kxfer, pxfer, sizeof(struct i2o_sw_xfer)))
- return -EFAULT;
+ if (copy_from_user(&kxfer, pxfer, sizeof(struct i2o_sw_xfer))) {
+ rc = -EFAULT;
+ goto exit;
+ }
- if (get_user(swlen, kxfer.swlen) < 0)
- return -EFAULT;
+ if (get_user(swlen, kxfer.swlen) < 0) {
+ rc = -EFAULT;
+ goto exit;
+ }
c = i2o_find_iop(kxfer.iop);
- if (!c)
- return -ENXIO;
+ if (!c) {
+ rc = -ENXIO;
+ goto exit;
+ }
m = i2o_msg_get_wait(c, &msg, I2O_TIMEOUT_MESSAGE_GET);
- if (m == I2O_QUEUE_EMPTY)
- return -EBUSY;
+ if (m == I2O_QUEUE_EMPTY) {
+ rc = -EBUSY;
+ goto exit;
+ }
writel(SEVEN_WORD_MSG_SIZE | SGL_OFFSET_0, &msg->u.head[0]);
writel(I2O_CMD_SW_REMOVE << 24 | HOST_TID << 12 | ADAPTER_TID,
@@ -520,10 +619,11 @@
printk(KERN_INFO
"i2o_config: swdel failed, DetailedStatus = %d\n",
token);
- return -ETIMEDOUT;
+ rc = -ETIMEDOUT;
}
- return 0;
+exit:
+ return rc;
};
static int i2o_cfg_validate(unsigned long arg)
@@ -533,14 +633,19 @@
struct i2o_message *msg;
u32 m;
struct i2o_controller *c;
+ int rc = 0;
c = i2o_find_iop(iop);
- if (!c)
- return -ENXIO;
+ if (!c) {
+ rc = -ENXIO;
+ goto exit;
+ }
m = i2o_msg_get_wait(c, &msg, I2O_TIMEOUT_MESSAGE_GET);
- if (m == I2O_QUEUE_EMPTY)
- return -EBUSY;
+ if (m == I2O_QUEUE_EMPTY) {
+ rc = -EBUSY;
+ goto exit;
+ }
writel(FOUR_WORD_MSG_SIZE | SGL_OFFSET_0, &msg->u.head[0]);
writel(I2O_CMD_CONFIG_VALIDATE << 24 | HOST_TID << 12 | iop,
@@ -553,10 +658,11 @@
if (token != I2O_POST_WAIT_OK) {
printk(KERN_INFO "Can't validate configuration, ErrorStatus = "
"%d\n", token);
- return -ETIMEDOUT;
+ rc = -ETIMEDOUT;
}
- return 0;
+exit:
+ return rc;
};
static int i2o_cfg_evt_reg(unsigned long arg, struct file *fp)
@@ -567,23 +673,32 @@
struct i2o_evt_id kdesc;
struct i2o_controller *c;
struct i2o_device *d;
+ int rc = 0;
- if (copy_from_user(&kdesc, pdesc, sizeof(struct i2o_evt_id)))
- return -EFAULT;
+ if (copy_from_user(&kdesc, pdesc, sizeof(struct i2o_evt_id))) {
+ rc = -EFAULT;
+ goto exit;
+ }
/* IOP exists? */
c = i2o_find_iop(kdesc.iop);
- if (!c)
- return -ENXIO;
+ if (!c) {
+ rc = -ENXIO;
+ goto exit;
+ }
/* Device exists? */
d = i2o_iop_find_device(c, kdesc.tid);
- if (!d)
- return -ENODEV;
+ if (!d) {
+ rc = -ENODEV;
+ goto exit;
+ }
m = i2o_msg_get_wait(c, &msg, I2O_TIMEOUT_MESSAGE_GET);
- if (m == I2O_QUEUE_EMPTY)
- return -EBUSY;
+ if (m == I2O_QUEUE_EMPTY) {
+ rc = -EBUSY;
+ goto exit;
+ }
writel(FOUR_WORD_MSG_SIZE | SGL_OFFSET_0, &msg->u.head[0]);
writel(I2O_CMD_UTIL_EVT_REGISTER << 24 | HOST_TID << 12 | kdesc.tid,
@@ -594,7 +709,8 @@
i2o_msg_post(c, m);
- return 0;
+exit:
+ return rc;
}
static int i2o_cfg_evt_get(unsigned long arg, struct file *fp)
@@ -603,13 +719,16 @@
struct i2o_evt_get __user *uget = (struct i2o_evt_get __user *)arg;
struct i2o_evt_get kget;
unsigned long flags;
+ int rc = 0;
for (p = open_files; p; p = p->next)
if (p->q_id == (ulong) fp->private_data)
break;
- if (!p->q_len)
- return -ENOENT;
+ if (!p->q_len) {
+ rc = -ENOENT;
+ goto exit;
+ }
memcpy(&kget.info, &p->event_q[p->q_out], sizeof(struct i2o_evt_info));
MODINC(p->q_out, I2O_EVT_Q_LEN);
@@ -620,8 +739,10 @@
spin_unlock_irqrestore(&i2o_config_lock, flags);
if (copy_to_user(uget, &kget, sizeof(struct i2o_evt_get)))
- return -EFAULT;
- return 0;
+ rc = -EFAULT;
+
+exit:
+ return rc;
}
#if BITS_PER_LONG == 64
next prev parent reply other threads:[~2004-10-07 21:21 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2004-10-05 21:43 [PATCH] add missing checks of __copy_to_user return value in i2o_config.c Jesper Juhl
2004-10-05 22:21 ` Andrew Morton
2004-10-05 22:34 ` Jesper Juhl
2004-10-05 22:43 ` Andrew Morton
2004-10-05 23:11 ` Jesper Juhl
2004-10-06 10:41 ` Christoph Hellwig
2004-10-06 20:11 ` Jesper Juhl
2004-10-07 21:01 ` Markus Lidel [this message]
2004-10-08 0:23 ` Jesper Juhl
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4165AEB9.8040608@shadowconnect.com \
--to=markus.lidel@shadowconnect.com \
--cc=akpm@osdl.org \
--cc=alan@lxorguk.ukuu.org.uk \
--cc=hch@infradead.org \
--cc=juhl-lkml@dif.dk \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.