Since arg is a user pointer, accessing values like cmd->iop requires an
unsafe user pointer dereference.
QUESTION: Does ioctl_passthru mean arg is a kernel pointer? If so, then
disregard this bug report.
Let me know if you have any questions, and thanks for looking into this.
Best,
Rob
--- linux-2.6.7-rc3-full/drivers/message/i2o/i2o_config.c.orig Wed Jun 9 12:14:08 2004
+++ linux-2.6.7-rc3-full/drivers/message/i2o/i2o_config.c Wed Jun 9 12:13:33 2004
@@ -842,10 +842,10 @@ static int ioctl_evt_get(unsigned long a
static int ioctl_passthru(unsigned long arg)
{
- struct i2o_cmd_passthru *cmd = (struct i2o_cmd_passthru *) arg;
+ struct i2o_cmd_passthru cmd;
struct i2o_controller *c;
u32 msg[MSG_FRAME_SIZE];
- u32 *user_msg = (u32*)cmd->msg;
+ u32 *user_msg;
u32 *reply = NULL;
u32 *user_reply = NULL;
u32 size = 0;
@@ -858,11 +858,16 @@ static int ioctl_passthru(unsigned long
u32 i = 0;
ulong p = 0;
- c = i2o_find_controller(cmd->iop);
+ if (copy_from_user(&cmd, (void *)arg, sizeof(cmd)))
+ return -EFAULT;
+
+ user_msg = cmd.msg;
+
+ c = i2o_find_controller(cmd.iop);
if(!c)
return -ENXIO;
- memset(&msg, 0, MSG_FRAME_SIZE*4);
+ memset(msg, 0, MSG_FRAME_SIZE*4);
if(get_user(size, &user_msg[0]))
return -EFAULT;
size = size>>16;
@@ -949,7 +954,7 @@ static int ioctl_passthru(unsigned long
int sg_size;
// re-acquire the original message to handle correctly the sg copy operation
- memset(&msg, 0, MSG_FRAME_SIZE*4);
+ memset(msg, 0, MSG_FRAME_SIZE*4);
// get user msg size in u32s
if (get_user(size, &user_msg[0])) {
rcode = -EFAULT;
On Wed, Jun 09, 2004 at 04:01:02PM -0700, Robert T. Johnson wrote:
> Since arg is a user pointer, accessing values like cmd->iop requires an
> unsafe user pointer dereference.
>
> QUESTION: Does ioctl_passthru mean arg is a kernel pointer? If so, then
> disregard this bug report.
No - we have ->ioctl [== cfg_ioctl()] -> ioctl_passthru() chain in there,
so at the very least it can get arbitrary pointers straight from userland.
And yes, these cmd->iop and cmd->msg dereferences are broken.
> + if (copy_from_user(&cmd, (void *)arg, sizeof(cmd)))
> + return -EFAULT;
Fix the indentation, please.
> - memset(&msg, 0, MSG_FRAME_SIZE*4);
> + memset(msg, 0, MSG_FRAME_SIZE*4);
Not needed; they are equivalent and both legitimate. So that just clutters
the patch.
> - memset(&msg, 0, MSG_FRAME_SIZE*4);
> + memset(msg, 0, MSG_FRAME_SIZE*4);
Ditto.
On Wed, 2004-06-09 at 18:37, [email protected]
wrote:
> On Wed, Jun 09, 2004 at 04:01:02PM -0700, Robert T. Johnson wrote:
> > QUESTION: Does ioctl_passthru mean arg is a kernel pointer? If so, then
> > disregard this bug report.
>
> No - we have ->ioctl [== cfg_ioctl()] -> ioctl_passthru() chain in there,
> so at the very least it can get arbitrary pointers straight from userland.
Thanks.
> > + if (copy_from_user(&cmd, (void *)arg, sizeof(cmd)))
> > + return -EFAULT;
>
> Fix the indentation, please.
Whoops. Thanks. Done.
> > - memset(&msg, 0, MSG_FRAME_SIZE*4);
> > + memset(msg, 0, MSG_FRAME_SIZE*4);
> > - memset(&msg, 0, MSG_FRAME_SIZE*4);
> > + memset(msg, 0, MSG_FRAME_SIZE*4);
These fix false positives in cqual -- it's picky about conversions
between arrays and pointers. I left them out of the revised patch, but
if I submitted a patch for a bunch of little things like this, would it
be accepted? I described some scenarios in an old email where &array
can cause problems:
http://lkml.org/lkml/2004/2/23/233
The bug I just found in ipmi_devintf.c is probably an example of exactly
this kind of problem.
Best,
Rob
--- linux-2.6.7-rc3-full/drivers/message/i2o/i2o_config.c.orig Wed Jun 9 12:14:08 2004
+++ linux-2.6.7-rc3-full/drivers/message/i2o/i2o_config.c Wed Jun 9 20:53:01 2004
@@ -842,10 +842,10 @@ static int ioctl_evt_get(unsigned long a
static int ioctl_passthru(unsigned long arg)
{
- struct i2o_cmd_passthru *cmd = (struct i2o_cmd_passthru *) arg;
+ struct i2o_cmd_passthru cmd;
struct i2o_controller *c;
u32 msg[MSG_FRAME_SIZE];
- u32 *user_msg = (u32*)cmd->msg;
+ u32 *user_msg;
u32 *reply = NULL;
u32 *user_reply = NULL;
u32 size = 0;
@@ -858,7 +858,12 @@ static int ioctl_passthru(unsigned long
u32 i = 0;
ulong p = 0;
- c = i2o_find_controller(cmd->iop);
+ if (copy_from_user(&cmd, (void *)arg, sizeof(cmd)))
+ return -EFAULT;
+
+ user_msg = cmd.msg;
+
+ c = i2o_find_controller(cmd.iop);
if(!c)
return -ENXIO;
Hello,
Robert T. Johnson wrote:
> Since arg is a user pointer, accessing values like cmd->iop requires an
> unsafe user pointer dereference.
Yes, you're right...
> QUESTION: Does ioctl_passthru mean arg is a kernel pointer? If so, then
> disregard this bug report.
No, arg is a user pointer...
> Let me know if you have any questions, and thanks for looking into this.
No, thank you for taking your time, and also making the patch :-)
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: [email protected]
URL: http://www.shadowconnect.com