Messages in this thread Patch in this message | | | Date | Tue, 29 Sep 2009 11:24:10 +0200 | From | Arjan van de Ven <> | Subject | Re: [PATCH 7/9] Simplify bound checks in capabilities for copy_from_user |
| |
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 -- 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/
| |