Skip to contentSkip to conversation

PATCH: 2.6.7-rc3 drivers/message/i2o/i2o_config.c: user/kernel pointer bugs

PATCH Robert T. Johnson 3 participants 4 messages Jun 9, 2004 – Jun 11, 2004

lore
Share & tools
lore

Robert T. Johnson

author
#0 lore raw

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

drivers/message/i2o/i2o_config.c renamed from drivers/message/i2o/i2o_config.c.orig +10 -5
@@ -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;
#1 lore raw
Quoted 5 lines · Robert T. Johnson

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.

drivers/message/i2o/i2o_config.c static int ioctl_passthru(unsigned long · line 862

Quoted 2 lines
drivers/message/i2o/i2o_config.c renamed from drivers/message/i2o/i2o_config.c.orig +2 -0
+	if (copy_from_user(&cmd, (void *)arg, sizeof(cmd)))
+	  return -EFAULT;

Fix the indentation, please.

Review

Quoted 2 lines
(quoted diff) +1 -1
-	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.

Review

Quoted 2 lines
(quoted diff) +1 -1
-		memset(&msg, 0, MSG_FRAME_SIZE*4);
+		memset(msg, 0, MSG_FRAME_SIZE*4);

Ditto.

author
#2 lore raw
Quoted 6 lines · On Wed, 2004-06-09 at 18:37, [email protected] wrote:
Quoted 2 lines · Robert T. Johnson

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.

Quoted 4 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+	if (copy_from_user(&cmd, (void *)arg, sizeof(cmd)))
+	  return -EFAULT;

Fix the indentation, please.

Whoops. Thanks. Done.

Quoted 4 lines

Review

Quoted 4 lines
(quoted diff) +2 -2
-	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

drivers/message/i2o/i2o_config.c renamed from drivers/message/i2o/i2o_config.c.orig +8 -3
@@ -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;
#3 lore raw
In reply to Robert T. Johnson

Hello,

Quoted 1 line · Robert T. Johnson

Since arg is a user pointer, accessing values like cmd->iop requires an unsafe user pointer dereference.

Yes, you're right...

Quoted 2 lines

QUESTION: Does ioctl_passthru mean arg is a kernel pointer? If so, then disregard this bug report.

No, arg is a user pointer...

Quoted 1 line

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