[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
From: Arjan van de Ven <[email protected]>
Subject: [PATCH 1/9] Fix bound checks for copy_from_user in the acpi /proc code
CC: Len Brown <[email protected]>
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 <[email protected]>
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
From: Arjan van de Ven <[email protected]>
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 <[email protected]>
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
From: Arjan van de Ven <[email protected]>
Subject: [PATCH 3/9] Add bound checks in wext for copy_from_user
CC: [email protected]
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 <[email protected]>
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
From: Arjan van de Ven <[email protected]>
Subject: [PATCH 4/9] Simplify bound checks in the MTRR code
CC: [email protected]
CC: [email protected]
CC: [email protected]
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 <[email protected]>
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
From: Arjan van de Ven <[email protected]>
Subject: [PATCH 5/9] Add bound checks in acpi/video for copy_from_user
CC: Len Brown <[email protected]>
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 <[email protected]>
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
From: Arjan van de Ven <[email protected]>
Subject: [PATCH 6/9] Simplify bound checks in cifs for copy_from_user
CC: Steve French <[email protected]>
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 <[email protected]>
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
From: Arjan van de Ven <[email protected]>
Subject: [PATCH 7/9] Simplify bound checks in capabilities for copy_from_user
CC: James Morris <[email protected]>
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 <[email protected]>
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
From: Arjan van de Ven <[email protected]>
Subject: [PATCH 8/9] Add explicit bound checks in mm/migrate.c
CC: [email protected]
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 <[email protected]>
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
From: Arjan van de Ven <[email protected]>
Subject: [PATCH 9/9] Add explicit bound checks in net/socket.c
CC: [email protected]
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 <[email protected]>
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
[Arjan van de Ven - Sat, Sep 26, 2009 at 08:54:32PM +0200]
| From: Arjan van de Ven <[email protected]>
| Subject: [PATCH 9/9] Add explicit bound checks in net/socket.c
| CC: [email protected]
|
| 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 <[email protected]>
|
|
| 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
On Sat, 26 Sep 2009 23:01:03 +0400
Cyrill Gorcunov <[email protected]> wrote:
> [Arjan van de Ven - Sat, Sep 26, 2009 at 08:54:32PM +0200]
> | From: Arjan van de Ven <[email protected]>
> | Subject: [PATCH 9/9] Add explicit bound checks in net/socket.c
> | CC: [email protected]
> |
> | 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 <[email protected]>
> |
> |
> | 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 <[email protected]>
Subject: [PATCH 9/9] Add explicit bound checks in net/socket.c
CC: [email protected]
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 <[email protected]>
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
On Sat, 26 Sep 2009 23:01:03 +0400
Cyrill Gorcunov <[email protected]> wrote:
> [Arjan van de Ven - Sat, Sep 26, 2009 at 08:54:32PM +0200]
> | From: Arjan van de Ven <[email protected]>
> | Subject: [PATCH 9/9] Add explicit bound checks in net/socket.c
> | CC: [email protected]
> |
> | 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 <[email protected]>
> |
> |
> | 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 <[email protected]>
Subject: [PATCH 9/9] Add explicit bound checks in net/socket.c
CC: [email protected]
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 <[email protected]>
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
[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
From: Arjan van de Ven <[email protected]>
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 <[email protected]>
Applied, thanks.
On Sun, Sep 27, 2009 at 4:53 AM, Arjan van de Ven <[email protected]> wrote:
> From: Arjan van de Ven <[email protected]>
> Subject: [PATCH 7/9] Simplify bound checks in capabilities for copy_from_user
> CC: James Morris <[email protected]>
>
> 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 <[email protected]>
>
> 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 [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
>
On Tue, 29 Sep 2009 15:55:49 +1000
Dave Airlie <[email protected]> wrote:
> On Sun, Sep 27, 2009 at 4:53 AM, Arjan van de Ven
> <[email protected]> wrote:
> > From: Arjan van de Ven <[email protected]>
> > Subject: [PATCH 7/9] Simplify bound checks in capabilities for
> > copy_from_user CC: James Morris <[email protected]>
> >
> > 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 <[email protected]>
> >
> > 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 <[email protected]>
Subject: [PATCH 7/9] Simplify bound checks in capabilities for copy_from_user
CC: James Morris <[email protected]>
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 <[email protected]>
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
On Sat, 26 Sep 2009 20:54:06 +0200
Arjan van de Ven <[email protected]> wrote:
> From: Arjan van de Ven <[email protected]>
> Subject: [PATCH 8/9] Add explicit bound checks in mm/migrate.c
> CC: [email protected]
>
> 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;
Hi
> On Sat, 26 Sep 2009 20:54:06 +0200
> Arjan van de Ven <[email protected]> wrote:
>
> > From: Arjan van de Ven <[email protected]>
> > Subject: [PATCH 8/9] Add explicit bound checks in mm/migrate.c
> > CC: [email protected]
> >
> > 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;
>
>
On Tue, 29 Sep 2009, Arjan van de Ven wrote:
> From: Arjan van de Ven <[email protected]>
> Subject: [PATCH 7/9] Simplify bound checks in capabilities for copy_from_user
> CC: James Morris <[email protected]>
>
> 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 <[email protected]>
Acked-by: James Morris <[email protected]>
> 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
<[email protected]>
Commit-ID: 11879ba5d9ab8174af9b9cefbb2396a54dfbf8c1
Gitweb: http://git.kernel.org/tip/11879ba5d9ab8174af9b9cefbb2396a54dfbf8c1
Author: Arjan van de Ven <[email protected]>
AuthorDate: Sat, 26 Sep 2009 20:51:50 +0200
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Cc: Yinghai Lu <[email protected]>
Cc: Linus Torvalds <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
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);