[PATCH 0/9] Series to make copy_from_user to a stack slot provable right

[PATCH 0/9] Series to make copy_from_user to a stack slot provable right

[Arjan van de Ven]

[PATCH 0/9] Series to make copy_from_user to a stack slot provable right

This series contains a series of patches that, when applied, make every
copy_from_user() in a make allyesconfig to a (direct) stack slot
provable-by-gcc to have a correct size.

This is useful because if we fix all of these, we can make the non-provable
case an error, as an indication of a possible security hole.

Now the series has 4 types of patches
1) changes where the original code really was missing checks
2) changes where the checks were coded so complex and games were played with
   types, that I (and the compiler) couldn't be sure if it was correct or
   not
3) changes where we're hitting a small gcc missing optimization, but where
   a simplification of the code allows gcc to prove things anyway.
   (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41477 is filed for this)
4) a case in sys_socketcall where Dave Miller and co were very smart in
   optimizing the code to the point where it's not reasonable for gcc
   to realize the result is ok.



-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

[PATCH 1/9] Fix bound checks for copy_from_user in the acpi /proc code

[Arjan van de Ven]

From: Arjan van de Ven <arjan@linux.intel.com>
Subject: [PATCH 1/9] Fix bound checks for copy_from_user in the acpi /proc code
CC: Len Brown <lenb@kernel.org>

The ACPI /proc write() code takes an unsigned length argument like any write()
function, but then assigned it to a *signed* integer called "len".
Only after this is a sanity check for len done to make it not larger than 4.

Due to the type change a len < 0 is in principle also possible; this patch
adds a check for this.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>

diff --git a/drivers/acpi/proc.c b/drivers/acpi/proc.c
index d0d550d..f8b6f55 100644
--- a/drivers/acpi/proc.c
+++ b/drivers/acpi/proc.c
@@ -398,6 +398,8 @@ acpi_system_write_wakeup_device(struct file *file,
 
 	if (len > 4)
 		len = 4;
+	if (len < 0)
+		return -EFAULT;
 
 	if (copy_from_user(strbuf, buffer, len))
 		return -EFAULT;


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

[PATCH 2/9] Simplify bound checks in nvram for copy_from_user

[Arjan van de Ven]

From: Arjan van de Ven <arjan@linux.intel.com>
Subject: [PATCH 2/9] Simplify bound checks in nvram for copy_from_user

The nvram driver's write() function has an interesting bound check.
Not only does it use the always-hard-to-read ? C operator, it also
has a magic "i" in there, which comes from the file position of
the file.

On first sight the check looks sane, however the value of "i" is not 
checked at all and I as human don't know if the C type rules guarantee
that the result is always within bounds.. and neither does gcc seem to
know.

This patch simplifies the checks and guarantees that the copy will not
overflow the destination buffer.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>


diff --git a/drivers/char/nvram.c b/drivers/char/nvram.c
index 88cee40..b2a7eaf 100644
--- a/drivers/char/nvram.c
+++ b/drivers/char/nvram.c
@@ -267,7 +267,15 @@ static ssize_t nvram_write(struct file *file, const char __user *buf,
 	unsigned char *tmp;
 	int len;
 
-	len = (NVRAM_BYTES - i) < count ? (NVRAM_BYTES - i) : count;
+	len = count;
+	if (count > NVRAM_BYTES - i)
+		len = NVRAM_BYTES - i;
+
+	if (len > NVRAM_BYTES)
+		len = NVRAM_BYTES;
+	if (len < 0)
+		return -EINVAL;
+
 	if (copy_from_user(contents, buf, len))
 		return -EFAULT;
 


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

[PATCH 3/9] Add bound checks in wext for copy_from_user

[Arjan van de Ven]

From: Arjan van de Ven <arjan@linux.intel.com>
Subject: [PATCH 3/9] Add bound checks in wext for copy_from_user
CC: linux-wireless@vger.kernel.org

The wireless extensions have a copy_from_user to a local stack
array "essid", but both me and gcc have failed to find where
the bounds for this copy are located in the code.

This patch adds some basic sanity checks for the copy length
to make sure that we don't overflow the stack buffer.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>


diff --git a/net/wireless/wext.c b/net/wireless/wext.c
index 5b4a0ce..34beae6 100644
--- a/net/wireless/wext.c
+++ b/net/wireless/wext.c
@@ -773,10 +773,13 @@ static int ioctl_standard_iw_point(struct iw_point *iwp, unsigned int cmd,
 			essid_compat = 1;
 		else if (IW_IS_SET(cmd) && (iwp->length != 0)) {
 			char essid[IW_ESSID_MAX_SIZE + 1];
+			unsigned int len;
+			len = iwp->length * descr->token_size;
 
-			err = copy_from_user(essid, iwp->pointer,
-					     iwp->length *
-					     descr->token_size);
+			if (len > IW_ESSID_MAX_SIZE)
+				return -EFAULT;
+
+			err = copy_from_user(essid, iwp->pointer, len);
 			if (err)
 				return -EFAULT;
 



-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

[PATCH 4/9] Simplify bound checks in the MTRR code

[Arjan van de Ven]

From: Arjan van de Ven <arjan@linux.intel.com>
Subject: [PATCH 4/9] Simplify bound checks in the MTRR code
CC: mingo@elte.hu
CC: hpa@zytor.com
CC: tglx@tglx.de

The current bound checks for copy_from_user in the MTRR driver
are not as obvious as they could be, and gcc agrees with that.

This patch simplifies the boundary checks to the point that gcc
can now prove to itself that the copy_from_user() is never going
past its bounds.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>

diff --git a/arch/x86/kernel/cpu/mtrr/if.c b/arch/x86/kernel/cpu/mtrr/if.c
index f04e725..3c1b12d 100644
--- a/arch/x86/kernel/cpu/mtrr/if.c
+++ b/arch/x86/kernel/cpu/mtrr/if.c
@@ -96,17 +96,24 @@ mtrr_write(struct file *file, const char __user *buf, size_t len, loff_t * ppos)
 	unsigned long long base, size;
 	char *ptr;
 	char line[LINE_SIZE];
+	int length;
 	size_t linelen;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
-	if (!len)
-		return -EINVAL;
 
 	memset(line, 0, LINE_SIZE);
-	if (len > LINE_SIZE)
-		len = LINE_SIZE;
-	if (copy_from_user(line, buf, len - 1))
+
+	length = len;
+	length--;
+
+	if (length > LINE_SIZE - 1)
+		length = LINE_SIZE - 1;
+
+	if (length < 0)
+		return -EINVAL;
+
+	if (copy_from_user(line, buf, length))
 		return -EFAULT;
 
 	linelen = strlen(line);



-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

[PATCH 5/9] Add bound checks in acpi/video for copy_from_user

[Arjan van de Ven]

From: Arjan van de Ven <arjan@linux.intel.com>
Subject: [PATCH 5/9] Add bound checks in acpi/video for copy_from_user
CC: Len Brown <lenb@kernel.org>

The ACPI video driver has a few boundary checks for copy_from_user
that unfortunately confuse the GCC optimizer.

This patch simplifies these boundary checks to the point that
gcc knows they copy_from_user() is always within bounds.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>


diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 94b1a4c..0dd2cc8 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -1218,7 +1218,9 @@ acpi_video_device_write_state(struct file *file,
 	u32 state = 0;
 
 
-	if (!dev || count + 1 > sizeof str)
+	if (!dev)
+		return -EINVAL;
+	if (count >= sizeof(str))
 		return -EINVAL;
 
 	if (copy_from_user(str, buffer, count))
@@ -1275,7 +1277,10 @@ acpi_video_device_write_brightness(struct file *file,
 	int i;
 
 
-	if (!dev || !dev->brightness || count + 1 > sizeof str)
+	if (!dev || !dev->brightness)
+		return -EINVAL;
+
+	if (count >= sizeof(str))
 		return -EINVAL;
 
 	if (copy_from_user(str, buffer, count))
@@ -1557,7 +1562,10 @@ acpi_video_bus_write_POST(struct file *file,
 	unsigned long long opt, options;
 
 
-	if (!video || count + 1 > sizeof str)
+	if (!video)
+		return -EINVAL;
+
+	if (count >= sizeof(str))
 		return -EINVAL;
 
 	status = acpi_video_bus_POST_options(video, &options);
@@ -1597,7 +1605,9 @@ acpi_video_bus_write_DOS(struct file *file,
 	unsigned long opt;
 
 
-	if (!video || count + 1 > sizeof str)
+	if (!video)
+		return -EINVAL;
+	if (count >= sizeof(str))
 		return -EINVAL;
 
 	if (copy_from_user(str, buffer, count))



-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

[PATCH 6/9] Simplify bound checks in cifs for copy_from_user

[Arjan van de Ven]

From: Arjan van de Ven <arjan@linux.intel.com>
Subject: [PATCH 6/9] Simplify bound checks in cifs for copy_from_user
CC: Steve French <sfrench@samba.org>

The CIFS code unfortunately hits a missed optimization in gcc
(http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41477)
where gcc can't prove to itself that count will not be larger than 11.

This patch simplifies the expression so that GCC does realize this,
giving slightly better code soon when copy_from_user() grows some checks.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>

diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
index 42cec2a..94b86da 100644
--- a/fs/cifs/cifs_debug.c
+++ b/fs/cifs/cifs_debug.c
@@ -732,11 +732,13 @@ static ssize_t cifs_security_flags_proc_write(struct file *file,
 	char flags_string[12];
 	char c;
 
-	if ((count < 1) || (count > 11))
-		return -EINVAL;
-
 	memset(flags_string, 0, 12);
 
+	if (count < 1)
+		return -EINVAL;
+	if (count > 11)
+		return -EINVAL;
+
 	if (copy_from_user(flags_string, buffer, count))
 		return -EFAULT;
 



-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

[PATCH 7/9] Simplify bound checks in capabilities for copy_from_user

[Arjan van de Ven]

From: Arjan van de Ven <arjan@linux.intel.com>
Subject: [PATCH 7/9] Simplify bound checks in capabilities for copy_from_user
CC: James Morris <jmorris@namei.org>

The capabilities syscall has a copy_from_user() call where gcc currently
cannot prove to itself that the copy is always within bounds.

This patch adds a very explicity bound check to prove to gcc that 
this copy_from_user cannot overflow its destination buffer.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>

diff --git a/kernel/capability.c b/kernel/capability.c
index 4e17041..204f11f 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -238,7 +241,7 @@ SYSCALL_DEFINE2(capget, cap_user_header_t, header, cap_user_data_t, dataptr)
 SYSCALL_DEFINE2(capset, cap_user_header_t, header, const cap_user_data_t, data)
 {
 	struct __user_cap_data_struct kdata[_KERNEL_CAPABILITY_U32S];
-	unsigned i, tocopy;
+	unsigned i, tocopy, copybytes;
 	kernel_cap_t inheritable, permitted, effective;
 	struct cred *new;
 	int ret;
@@ -255,8 +258,11 @@ SYSCALL_DEFINE2(capset, cap_user_header_t, header, const cap_user_data_t, data)
 	if (pid != 0 && pid != task_pid_vnr(current))
 		return -EPERM;
 
-	if (copy_from_user(&kdata, data,
-			   tocopy * sizeof(struct __user_cap_data_struct)))
+	copybytes = tocopy * sizeof(struct __user_cap_data_struct);
+	if (copybytes > _KERNEL_CAPABILITY_U32S)
+		return -EFAULT;
+
+	if (copy_from_user(&kdata, data, copybytes))
 		return -EFAULT;
 
 	for (i = 0; i < tocopy; i++) {



-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

[PATCH 8/9] Add explicit bound checks in mm/migrate.c

[Arjan van de Ven]

From: Arjan van de Ven <arjan@linux.intel.com>
Subject: [PATCH 8/9] Add explicit bound checks in mm/migrate.c
CC: akpm@linux-foundation.org

The memory migration code has some curious copy_from_user bounds,
that are likely ok, but are not immediately obvious to me or to GCC.

This patch adds a simple explicit bound check; this allows GCC
and me to be more assured that the copy_from_user will never overwrite
its destination buffer.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>


diff --git a/mm/migrate.c b/mm/migrate.c
index 1a4bf48..5b9ebc5 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1044,11 +1044,15 @@ static int do_pages_stat(struct mm_struct *mm, unsigned long nr_pages,
 	int err;
 
 	for (i = 0; i < nr_pages; i += chunk_nr) {
+		unsigned int copy;
 		if (chunk_nr + i > nr_pages)
 			chunk_nr = nr_pages - i;
 
-		err = copy_from_user(chunk_pages, &pages[i],
-				     chunk_nr * sizeof(*chunk_pages));
+		copy = chunk_nr * sizeof(*chunk_pages);
+		if (copy > DO_PAGES_STAT_CHUNK_NR)
+			return -EFAULT;
+
+		err = copy_from_user(chunk_pages, &pages[i], copy);
 		if (err) {
 			err = -EFAULT;
 			goto out;


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

[PATCH 9/9] Add explicit bound checks in net/socket.c

[Arjan van de Ven]

From: Arjan van de Ven <arjan@linux.intel.com>
Subject: [PATCH 9/9] Add explicit bound checks in net/socket.c
CC: netdev@vger.kernel.org

The sys_socketcall() function has a very clever system for the copy
size of its arguments. Unfortunately, gcc cannot deal with this in
terms of proving that the copy_from_user() is then always in bounds.
This is the last (well 9th of this series, but last in the kernel) such
case around.

With this patch, we can turn on code to make having the boundary provably
right for the whole kernel, and detect introduction of new security
accidents of this type early on.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>


diff --git a/net/socket.c b/net/socket.c
index 49917a1..13a8d67 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -2098,12 +2098,17 @@ SYSCALL_DEFINE2(socketcall, int, call, unsigned long __user *, args)
 	unsigned long a[6];
 	unsigned long a0, a1;
 	int err;
+	unsigned int len;
 
 	if (call < 1 || call > SYS_ACCEPT4)
 		return -EINVAL;
 
+	len = nargs[call];
+	if (len > 6)
+		return -EINVAL;
+
 	/* copy_from_user should be SMP safe. */
-	if (copy_from_user(a, args, nargs[call]))
+	if (copy_from_user(a, args, len))
 		return -EFAULT;
 
 	audit_socketcall(nargs[call] / sizeof(unsigned long), a);


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH 9/9] Add explicit bound checks in net/socket.c

[Cyrill Gorcunov]

[Arjan van de Ven - Sat, Sep 26, 2009 at 08:54:32PM +0200]
| From: Arjan van de Ven <arjan@linux.intel.com>
| Subject: [PATCH 9/9] Add explicit bound checks in net/socket.c
| CC: netdev@vger.kernel.org
| 
| The sys_socketcall() function has a very clever system for the copy
| size of its arguments. Unfortunately, gcc cannot deal with this in
| terms of proving that the copy_from_user() is then always in bounds.
| This is the last (well 9th of this series, but last in the kernel) such
| case around.
| 
| With this patch, we can turn on code to make having the boundary provably
| right for the whole kernel, and detect introduction of new security
| accidents of this type early on.
| 
| Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
| 
| 
| diff --git a/net/socket.c b/net/socket.c
| index 49917a1..13a8d67 100644
| --- a/net/socket.c
| +++ b/net/socket.c
| @@ -2098,12 +2098,17 @@ SYSCALL_DEFINE2(socketcall, int, call, unsigned long __user *, args)
|  	unsigned long a[6];
|  	unsigned long a0, a1;
|  	int err;
| +	unsigned int len;
|  
|  	if (call < 1 || call > SYS_ACCEPT4)
|  		return -EINVAL;
|  
| +	len = nargs[call];
| +	if (len > 6)

Hi Arjan, wouldn't ARRAY_SIZE suffice beter there?
Or I miss something?

| +		return -EINVAL;
| +
|  	/* copy_from_user should be SMP safe. */
| -	if (copy_from_user(a, args, nargs[call]))
| +	if (copy_from_user(a, args, len))
|  		return -EFAULT;
|  
|  	audit_socketcall(nargs[call] / sizeof(unsigned long), a);
| 
| 
| -- 
| Arjan van de Ven 	Intel Open Source Technology Centre
| For development, discussion and tips for power savings, 
| visit http://www.lesswatts.org
|

	-- Cyrill

Re: [PATCH 9/9] Add explicit bound checks in net/socket.c

[Arjan van de Ven]

On Sat, 26 Sep 2009 23:01:03 +0400
Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> [Arjan van de Ven - Sat, Sep 26, 2009 at 08:54:32PM +0200]
> | From: Arjan van de Ven <arjan@linux.intel.com>
> | Subject: [PATCH 9/9] Add explicit bound checks in net/socket.c
> | CC: netdev@vger.kernel.org
> | 
> | The sys_socketcall() function has a very clever system for the copy
> | size of its arguments. Unfortunately, gcc cannot deal with this in
> | terms of proving that the copy_from_user() is then always in bounds.
> | This is the last (well 9th of this series, but last in the kernel)
> such | case around.
> | 
> | With this patch, we can turn on code to make having the boundary
> provably | right for the whole kernel, and detect introduction of new
> security | accidents of this type early on.
> | 
> | Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
> | 
> | 
> | diff --git a/net/socket.c b/net/socket.c
> | index 49917a1..13a8d67 100644
> | --- a/net/socket.c
> | +++ b/net/socket.c
> | @@ -2098,12 +2098,17 @@ SYSCALL_DEFINE2(socketcall, int, call,
> unsigned long __user *, args) |  	unsigned long a[6];
> |  	unsigned long a0, a1;
> |  	int err;
> | +	unsigned int len;
> |  
> |  	if (call < 1 || call > SYS_ACCEPT4)
> |  		return -EINVAL;
> |  
> | +	len = nargs[call];
> | +	if (len > 6)
> 
> Hi Arjan, wouldn't ARRAY_SIZE suffice beter there?
> Or I miss something?

yeah you missed that I screwed up ;(

From: Arjan van de Ven <arjan@linux.intel.com>
Subject: [PATCH 9/9] Add explicit bound checks in net/socket.c
CC: netdev@vger.kernel.org

The sys_socketcall() function has a very clever system for the copy
size of its arguments. Unfortunately, gcc cannot deal with this in
terms of proving that the copy_from_user() is then always in bounds.
This is the last (well 9th of this series, but last in the kernel) such
case around.

With this patch, we can turn on code to make having the boundary provably
right for the whole kernel, and detect introduction of new security
accidents of this type early on.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>


diff --git a/net/socket.c b/net/socket.c
index 49917a1..13a8d67 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -2098,12 +2098,17 @@ SYSCALL_DEFINE2(socketcall, int, call, unsigned long __user *, args)
 	unsigned long a[6];
 	unsigned long a0, a1;
 	int err;
+	unsigned int len;
 
 	if (call < 1 || call > SYS_ACCEPT4)
 		return -EINVAL;
 
+	len = nargs[call];
+	if (len > 6 * sizeof(unsiged long))
+		return -EINVAL;
+
 	/* copy_from_user should be SMP safe. */
-	if (copy_from_user(a, args, nargs[call]))
+	if (copy_from_user(a, args, len))
 		return -EFAULT;
 
 	audit_socketcall(nargs[call] / sizeof(unsigned long), a);



-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH 9/9] Add explicit bound checks in net/socket.c

[Arjan van de Ven]

On Sat, 26 Sep 2009 23:01:03 +0400
Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> [Arjan van de Ven - Sat, Sep 26, 2009 at 08:54:32PM +0200]
> | From: Arjan van de Ven <arjan@linux.intel.com>
> | Subject: [PATCH 9/9] Add explicit bound checks in net/socket.c
> | CC: netdev@vger.kernel.org
> | 
> | The sys_socketcall() function has a very clever system for the copy
> | size of its arguments. Unfortunately, gcc cannot deal with this in
> | terms of proving that the copy_from_user() is then always in bounds.
> | This is the last (well 9th of this series, but last in the kernel)
> such | case around.
> | 
> | With this patch, we can turn on code to make having the boundary
> provably | right for the whole kernel, and detect introduction of new
> security | accidents of this type early on.
> | 
> | Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
> | 
> | 
> | diff --git a/net/socket.c b/net/socket.c
> | index 49917a1..13a8d67 100644
> | --- a/net/socket.c
> | +++ b/net/socket.c
> | @@ -2098,12 +2098,17 @@ SYSCALL_DEFINE2(socketcall, int, call,
> unsigned long __user *, args) |  	unsigned long a[6];
> |  	unsigned long a0, a1;
> |  	int err;
> | +	unsigned int len;
> |  
> |  	if (call < 1 || call > SYS_ACCEPT4)
> |  		return -EINVAL;
> |  
> | +	len = nargs[call];
> | +	if (len > 6)
> 
> Hi Arjan, wouldn't ARRAY_SIZE suffice beter there?
> Or I miss something?
> 

goof once goof twice, make it sizeof.. that's nicer.

From: Arjan van de Ven <arjan@linux.intel.com>
Subject: [PATCH 9/9] Add explicit bound checks in net/socket.c
CC: netdev@vger.kernel.org

The sys_socketcall() function has a very clever system for the copy
size of its arguments. Unfortunately, gcc cannot deal with this in
terms of proving that the copy_from_user() is then always in bounds.
This is the last (well 9th of this series, but last in the kernel) such
case around.

With this patch, we can turn on code to make having the boundary provably
right for the whole kernel, and detect introduction of new security
accidents of this type early on.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>


diff --git a/net/socket.c b/net/socket.c
index 49917a1..13a8d67 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -2098,12 +2098,17 @@ SYSCALL_DEFINE2(socketcall, int, call, unsigned long __user *, args)
 	unsigned long a[6];
 	unsigned long a0, a1;
 	int err;
+	unsigned int len;
 
 	if (call < 1 || call > SYS_ACCEPT4)
 		return -EINVAL;
 
+	len = nargs[call];
+	if (len > sizeof(a))
+		return -EINVAL;
+
 	/* copy_from_user should be SMP safe. */
-	if (copy_from_user(a, args, nargs[call]))
+	if (copy_from_user(a, args, len))
 		return -EFAULT;
 
 	audit_socketcall(nargs[call] / sizeof(unsigned long), a);


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH 9/9] Add explicit bound checks in net/socket.c

[Cyrill Gorcunov]

[Arjan van de Ven - Sat, Sep 26, 2009 at 09:23:02PM +0200]
...
| 
| goof once goof twice, make it sizeof.. that's nicer.
| 

yeah, I was about to propose the same :)

...
	- Cyrill

Re: [PATCH 9/9] Add explicit bound checks in net/socket.c

[David Miller]

From: Arjan van de Ven <arjan@infradead.org>
Date: Sat, 26 Sep 2009 21:23:02 +0200

> The sys_socketcall() function has a very clever system for the copy
> size of its arguments. Unfortunately, gcc cannot deal with this in
> terms of proving that the copy_from_user() is then always in bounds.
> This is the last (well 9th of this series, but last in the kernel) such
> case around.
> 
> With this patch, we can turn on code to make having the boundary provably
> right for the whole kernel, and detect introduction of new security
> accidents of this type early on.
> 
> Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>

Applied, thanks.

Re: [PATCH 7/9] Simplify bound checks in capabilities for copy_from_user

[Dave Airlie]

On Sun, Sep 27, 2009 at 4:53 AM, Arjan van de Ven <arjan@infradead.org> wrote:
> From: Arjan van de Ven <arjan@linux.intel.com>
> Subject: [PATCH 7/9] Simplify bound checks in capabilities for copy_from_user
> CC: James Morris <jmorris@namei.org>
>
> The capabilities syscall has a copy_from_user() call where gcc currently
> cannot prove to itself that the copy is always within bounds.
>
> This patch adds a very explicity bound check to prove to gcc that
> this copy_from_user cannot overflow its destination buffer.
>
> Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
>
> diff --git a/kernel/capability.c b/kernel/capability.c
> index 4e17041..204f11f 100644
> --- a/kernel/capability.c
> +++ b/kernel/capability.c
> @@ -238,7 +241,7 @@ SYSCALL_DEFINE2(capget, cap_user_header_t, header, cap_user_data_t, dataptr)
>  SYSCALL_DEFINE2(capset, cap_user_header_t, header, const cap_user_data_t, data)
>  {
>        struct __user_cap_data_struct kdata[_KERNEL_CAPABILITY_U32S];
> -       unsigned i, tocopy;
> +       unsigned i, tocopy, copybytes;
>        kernel_cap_t inheritable, permitted, effective;
>        struct cred *new;
>        int ret;
> @@ -255,8 +258,11 @@ SYSCALL_DEFINE2(capset, cap_user_header_t, header, const cap_user_data_t, data)
>        if (pid != 0 && pid != task_pid_vnr(current))
>                return -EPERM;
>
> -       if (copy_from_user(&kdata, data,
> -                          tocopy * sizeof(struct __user_cap_data_struct)))
> +       copybytes = tocopy * sizeof(struct __user_cap_data_struct);
> +       if (copybytes > _KERNEL_CAPABILITY_U32S)
> +               return -EFAULT;

This is broken, it breaks dbus at least for me. you compare bytes
to u32s wrongly.

Dave.

> +
> +       if (copy_from_user(&kdata, data, copybytes))
>                return -EFAULT;
>
>        for (i = 0; i < tocopy; i++) {
>
>
>
> --
> Arjan van de Ven        Intel Open Source Technology Centre
> For development, discussion and tips for power savings,
> visit http://www.lesswatts.org
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH 7/9] Simplify bound checks in capabilities for copy_from_user

[Arjan van de Ven]

On Tue, 29 Sep 2009 15:55:49 +1000
Dave Airlie <airlied@gmail.com> wrote:

> On Sun, Sep 27, 2009 at 4:53 AM, Arjan van de Ven
> <arjan@infradead.org> wrote:
> > From: Arjan van de Ven <arjan@linux.intel.com>
> > Subject: [PATCH 7/9] Simplify bound checks in capabilities for
> > copy_from_user CC: James Morris <jmorris@namei.org>
> >
> > The capabilities syscall has a copy_from_user() call where gcc
> > currently cannot prove to itself that the copy is always within
> > bounds.
> >
> > This patch adds a very explicity bound check to prove to gcc that
> > this copy_from_user cannot overflow its destination buffer.
> >
> > Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
> >
> > diff --git a/kernel/capability.c b/kernel/capability.c
> > index 4e17041..204f11f 100644
> > --- a/kernel/capability.c
> > +++ b/kernel/capability.c
> > @@ -238,7 +241,7 @@ SYSCALL_DEFINE2(capget, cap_user_header_t,
> > header, cap_user_data_t, dataptr) SYSCALL_DEFINE2(capset,
> > cap_user_header_t, header, const cap_user_data_t, data) {
> >        struct __user_cap_data_struct kdata[_KERNEL_CAPABILITY_U32S];
> > -       unsigned i, tocopy;
> > +       unsigned i, tocopy, copybytes;
> >        kernel_cap_t inheritable, permitted, effective;
> >        struct cred *new;
> >        int ret;
> > @@ -255,8 +258,11 @@ SYSCALL_DEFINE2(capset, cap_user_header_t,
> > header, const cap_user_data_t, data) if (pid != 0 && pid !=
> > task_pid_vnr(current)) return -EPERM;
> >
> > -       if (copy_from_user(&kdata, data,
> > -                          tocopy * sizeof(struct
> > __user_cap_data_struct)))
> > +       copybytes = tocopy * sizeof(struct __user_cap_data_struct);
> > +       if (copybytes > _KERNEL_CAPABILITY_U32S)
> > +               return -EFAULT;
> 
> This is broken, it breaks dbus at least for me. you compare bytes
> to u32s wrongly.
> 
> Dave.

good point

From: Arjan van de Ven <arjan@linux.intel.com>
Subject: [PATCH 7/9] Simplify bound checks in capabilities for copy_from_user
CC: James Morris <jmorris@namei.org>

The capabilities syscall has a copy_from_user() call where gcc currently
cannot prove to itself that the copy is always within bounds.

This patch adds a very explicity bound check to prove to gcc that 
this copy_from_user cannot overflow its destination buffer.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>

diff --git a/kernel/capability.c b/kernel/capability.c
index 4e17041..204f11f 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -238,7 +241,7 @@ SYSCALL_DEFINE2(capget, cap_user_header_t, header, cap_user_data_t, dataptr)
 SYSCALL_DEFINE2(capset, cap_user_header_t, header, const cap_user_data_t, data)
 {
 	struct __user_cap_data_struct kdata[_KERNEL_CAPABILITY_U32S];
-	unsigned i, tocopy;
+	unsigned i, tocopy, copybytes;
 	kernel_cap_t inheritable, permitted, effective;
 	struct cred *new;
 	int ret;
@@ -255,8 +258,11 @@ SYSCALL_DEFINE2(capset, cap_user_header_t, header, const cap_user_data_t, data)
 	if (pid != 0 && pid != task_pid_vnr(current))
 		return -EPERM;
 
-	if (copy_from_user(&kdata, data,
-			   tocopy * sizeof(struct __user_cap_data_struct)))
+	copybytes = tocopy * sizeof(struct __user_cap_data_struct);
+	if (copybytes > sizeof(kdata))
+		return -EFAULT;
+
+	if (copy_from_user(&kdata, data, copybytes))
 		return -EFAULT;
 
 	for (i = 0; i < tocopy; i++) {



-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH 8/9] Add explicit bound checks in mm/migrate.c

[Andrew Morton]

On Sat, 26 Sep 2009 20:54:06 +0200
Arjan van de Ven <arjan@infradead.org> wrote:

> From: Arjan van de Ven <arjan@linux.intel.com>
> Subject: [PATCH 8/9] Add explicit bound checks in mm/migrate.c
> CC: akpm@linux-foundation.org
> 
> The memory migration code has some curious copy_from_user bounds,
> that are likely ok, but are not immediately obvious to me or to GCC.
> 
> This patch adds a simple explicit bound check; this allows GCC
> and me to be more assured that the copy_from_user will never overwrite
> its destination buffer.

I don't really see what's being fixed here.  The original code seems
straightforward and safe enough?

The identifier `chunk_nr' is a bit ambiguous.  Is it "number of chunks" or
is it "index of this chunk"?

> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 1a4bf48..5b9ebc5 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1044,11 +1044,15 @@ static int do_pages_stat(struct mm_struct *mm, unsigned long nr_pages,
>  	int err;
>  
>  	for (i = 0; i < nr_pages; i += chunk_nr) {
> +		unsigned int copy;
>  		if (chunk_nr + i > nr_pages)
>  			chunk_nr = nr_pages - i;

A newline after end-of-locals is conventional.

`i' and `chunk_nr' have type `unsigned long' and you're mixing that up
with `unsigned int'.

> -		err = copy_from_user(chunk_pages, &pages[i],
> -				     chunk_nr * sizeof(*chunk_pages));

And we mix it up with size_t as well.

The type choices are a bit confused and sloppy.  Converting it all to
`unsigned int' should be OK.

> +		copy = chunk_nr * sizeof(*chunk_pages);
> +		if (copy > DO_PAGES_STAT_CHUNK_NR)
> +			return -EFAULT;
> +
> +		err = copy_from_user(chunk_pages, &pages[i], copy);
>  		if (err) {
>  			err = -EFAULT;
>  			goto out;

Re: [PATCH 8/9] Add explicit bound checks in mm/migrate.c

[KOSAKI Motohiro]

Hi

> On Sat, 26 Sep 2009 20:54:06 +0200
> Arjan van de Ven <arjan@infradead.org> wrote:
> 
> > From: Arjan van de Ven <arjan@linux.intel.com>
> > Subject: [PATCH 8/9] Add explicit bound checks in mm/migrate.c
> > CC: akpm@linux-foundation.org
> > 
> > The memory migration code has some curious copy_from_user bounds,
> > that are likely ok, but are not immediately obvious to me or to GCC.
> > 
> > This patch adds a simple explicit bound check; this allows GCC
> > and me to be more assured that the copy_from_user will never overwrite
> > its destination buffer.
> 
> I don't really see what's being fixed here.  The original code seems
> straightforward and safe enough?

I think original code is safe too.

> The identifier `chunk_nr' is a bit ambiguous.  Is it "number of chunks" or
> is it "index of this chunk"?

chunk_nr is batch size. (ie it's number of chunks)


Plus, I have a review comment.

> 
> > 
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index 1a4bf48..5b9ebc5 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -1044,11 +1044,15 @@ static int do_pages_stat(struct mm_struct *mm, unsigned long nr_pages,
> >  	int err;
> >  
> >  	for (i = 0; i < nr_pages; i += chunk_nr) {
> > +		unsigned int copy;
> >  		if (chunk_nr + i > nr_pages)
> >  			chunk_nr = nr_pages - i;
> 
> A newline after end-of-locals is conventional.
> 
> `i' and `chunk_nr' have type `unsigned long' and you're mixing that up
> with `unsigned int'.
> 
> > -		err = copy_from_user(chunk_pages, &pages[i],
> > -				     chunk_nr * sizeof(*chunk_pages));
> 
> And we mix it up with size_t as well.
> 
> The type choices are a bit confused and sloppy.  Converting it all to
> `unsigned int' should be OK.
> 
> > +		copy = chunk_nr * sizeof(*chunk_pages);
> > +		if (copy > DO_PAGES_STAT_CHUNK_NR)
> > +			return -EFAULT;

this seems a bit strange. 
the unit of copy is byte. but the unit of DO_PAGES_STAT_CHUNK_NR is
not byte.



> > +
> > +		err = copy_from_user(chunk_pages, &pages[i], copy);
> >  		if (err) {
> >  			err = -EFAULT;
> >  			goto out;
> 
> 

Re: [PATCH 7/9] Simplify bound checks in capabilities for copy_from_user

[James Morris]

On Tue, 29 Sep 2009, Arjan van de Ven wrote:

> From: Arjan van de Ven <arjan@linux.intel.com>
> Subject: [PATCH 7/9] Simplify bound checks in capabilities for copy_from_user
> CC: James Morris <jmorris@namei.org>
> 
> The capabilities syscall has a copy_from_user() call where gcc currently
> cannot prove to itself that the copy is always within bounds.
> 
> This patch adds a very explicity bound check to prove to gcc that 
> this copy_from_user cannot overflow its destination buffer.
> 
> Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>

Acked-by: James Morris <jmorris@namei.org>


> diff --git a/kernel/capability.c b/kernel/capability.c
> index 4e17041..204f11f 100644
> --- a/kernel/capability.c
> +++ b/kernel/capability.c
> @@ -238,7 +241,7 @@ SYSCALL_DEFINE2(capget, cap_user_header_t, header, cap_user_data_t, dataptr)
>  SYSCALL_DEFINE2(capset, cap_user_header_t, header, const cap_user_data_t, data)
>  {
>  	struct __user_cap_data_struct kdata[_KERNEL_CAPABILITY_U32S];
> -	unsigned i, tocopy;
> +	unsigned i, tocopy, copybytes;
>  	kernel_cap_t inheritable, permitted, effective;
>  	struct cred *new;
>  	int ret;
> @@ -255,8 +258,11 @@ SYSCALL_DEFINE2(capset, cap_user_header_t, header, const cap_user_data_t, data)
>  	if (pid != 0 && pid != task_pid_vnr(current))
>  		return -EPERM;
>  
> -	if (copy_from_user(&kdata, data,
> -			   tocopy * sizeof(struct __user_cap_data_struct)))
> +	copybytes = tocopy * sizeof(struct __user_cap_data_struct);
> +	if (copybytes > sizeof(kdata))
> +		return -EFAULT;
> +
> +	if (copy_from_user(&kdata, data, copybytes))
>  		return -EFAULT;
>  
>  	for (i = 0; i < tocopy; i++) {
> 
> 
> 
> -- 
> Arjan van de Ven 	Intel Open Source Technology Centre
> For development, discussion and tips for power savings, 
> visit http://www.lesswatts.org
> 

-- 
James Morris
<jmorris@namei.org>

[tip:x86/urgent] x86: Simplify bound checks in the MTRR code

[tip-bot for Arjan van de Ven]

Commit-ID:  11879ba5d9ab8174af9b9cefbb2396a54dfbf8c1
Gitweb:     http://git.kernel.org/tip/11879ba5d9ab8174af9b9cefbb2396a54dfbf8c1
Author:     Arjan van de Ven <arjan@infradead.org>
AuthorDate: Sat, 26 Sep 2009 20:51:50 +0200
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 2 Oct 2009 19:51:56 +0200

x86: Simplify bound checks in the MTRR code

The current bound checks for copy_from_user in the MTRR driver are
not as obvious as they could be, and gcc agrees with that.

This patch simplifies the boundary checks to the point that gcc can
now prove to itself that the copy_from_user() is never going past
its bounds.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
LKML-Reference: <20090926205150.30797709@infradead.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 arch/x86/kernel/cpu/mtrr/if.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/mtrr/if.c b/arch/x86/kernel/cpu/mtrr/if.c
index f04e725..3c1b12d 100644
--- a/arch/x86/kernel/cpu/mtrr/if.c
+++ b/arch/x86/kernel/cpu/mtrr/if.c
@@ -96,17 +96,24 @@ mtrr_write(struct file *file, const char __user *buf, size_t len, loff_t * ppos)
 	unsigned long long base, size;
 	char *ptr;
 	char line[LINE_SIZE];
+	int length;
 	size_t linelen;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
-	if (!len)
-		return -EINVAL;
 
 	memset(line, 0, LINE_SIZE);
-	if (len > LINE_SIZE)
-		len = LINE_SIZE;
-	if (copy_from_user(line, buf, len - 1))
+
+	length = len;
+	length--;
+
+	if (length > LINE_SIZE - 1)
+		length = LINE_SIZE - 1;
+
+	if (length < 0)
+		return -EINVAL;
+
+	if (copy_from_user(line, buf, length))
 		return -EFAULT;
 
 	linelen = strlen(line);

Re: [tip:x86/urgent] mm: Adjust do_pages_stat() so gcc can see copy_from_user() is safe

[KOSAKI Motohiro]

> Commit-ID:  b925585039cf39275c2e0e57512e5df27fa73aad
> Gitweb:     http://git.kernel.org/tip/b925585039cf39275c2e0e57512e5df27fa73aad
> Author:     H. Peter Anvin <hpa@zytor.com>
> AuthorDate: Tue, 8 Dec 2009 14:01:32 -0800
> Committer:  H. Peter Anvin <hpa@zytor.com>
> CommitDate: Fri, 11 Dec 2009 15:27:47 -0800
> 
> mm: Adjust do_pages_stat() so gcc can see copy_from_user() is safe
> 
> Slightly adjust the logic for determining the size of the
> copy_form_user() in do_pages_stat(); with this change, gcc can see
> that the copying is safe.
> 
> Without this, we get a build error for i386 allyesconfig:
> 
> /home/hpa/kernel/linux-2.6-tip.urgent/arch/x86/include/asm/uaccess_32.h:213:
> error: call to ‘copy_from_user_overflow’ declared with attribute
> error: copy_from_user() buffer size is not provably correct
> 
> Unlike an earlier patch from Arjan, this doesn't introduce new
> variables; merely reshuffles the compare so that gcc can see that an
> overflow cannot happen.
> 
> Signed-off-by: H. Peter Anvin <hpa@zytor.com>
> Cc: Brice Goglin <Brice.Goglin@inria.fr>
> Cc: Arjan van de Ven <arjan@infradead.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> LKML-Reference: <20090926205406.30d55b08@infradead.org>
> ---
>  mm/migrate.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 7dbcb22..0bc640f 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1044,7 +1044,7 @@ static int do_pages_stat(struct mm_struct *mm, unsigned long nr_pages,
>  	int err;
>  
>  	for (i = 0; i < nr_pages; i += chunk_nr) {
> -		if (chunk_nr + i > nr_pages)
> +		if (chunk_nr > nr_pages - i)
>  			chunk_nr = nr_pages - i;

Ah, good. Thank you.
	Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>