Using possibly corrupted structure in atm/he.c

PATCH In review Felipe W Damasio 2 participants 2 messages Sep 21, 2003 – Sep 22, 2003 ATM

Share & tools

Felipe W Damasio

author

2003-09-21 04:20 UTC #0 lore raw

PATCH 1/1 Using possibly corrupted structure in atm/he.c

	Hi Chas,

	Patch against 2.6-test5.

	If copy_from_user returns != 0, it means the the regs structure wasn't

filled correctly, and since its fields are used to determine which ioctl the user is requesting the kernel could oops.

And as long as we're covering the subject, the patch also audits
copy_to_user on the same function to check a possible failure to copy the result back to userspace.

These bugs were found by smatch.

Please consider applying.

Cheers,

Felipe

Signature · 2 lines

It's most certainly GNU/Linux, not Linux. Read more at
http://www.gnu.org/gnu/why-gnu-linux.html

drivers/atm/he.c +7 -4

@@ -2860,8 +2860,10 @@
 			if (!capable(CAP_NET_ADMIN))
 				return -EPERM;
 
-			copy_from_user(&reg, (struct he_ioctl_reg *) arg,
-						sizeof(struct he_ioctl_reg));
+			if (copy_from_user(&reg, (struct he_ioctl_reg *) arg,
+						sizeof(struct he_ioctl_reg)))
+				return -EFAULT;
+			
 			spin_lock_irqsave(&he_dev->global_lock, flags);
 			switch (reg.type) {
 				case HE_REGTYPE_PCI:

@@ -2885,8 +2887,9 @@
 			}
 			spin_unlock_irqrestore(&he_dev->global_lock, flags);
 			if (err == 0)
-				copy_to_user((struct he_ioctl_reg *) arg, &reg,
-							sizeof(struct he_ioctl_reg));
+				if (copy_to_user((struct he_ioctl_reg *) arg, &reg,
+							sizeof(struct he_ioctl_reg)))
+					return -EFAULT;
 			break;
 		default:
 #ifdef CONFIG_ATM_HE_USE_SUNI

chas williams

2003-09-22 14:42 UTC #1 lore raw

Quoted 7 lines · In message <[email protected]>,Felipe W Damasio writes:

	If copy_from_user returns != 0, it means the the regs structure wasn't 
filled correctly, and since its fields are used to determine which ioctl the user is requesting the kernel could oops.
And as long as we're covering the subject, the patch also audits
copy_to_user on the same function to check a possible failure to copy the result back to userspace.

indeed these are broken as written. i will get this sent up the chain.