* [PATCH 1/3] dm-ioctl: don't use PF_MEMALLOC
@ 2012-11-23 0:19 Mikulas Patocka
2012-11-23 0:20 ` [PATCH 2/3] dm-ioctl: fix unsafe use of dm_ioctl.data_size Mikulas Patocka
0 siblings, 1 reply; 6+ messages in thread
From: Mikulas Patocka @ 2012-11-23 0:19 UTC (permalink / raw)
To: Alasdair G. Kergon; +Cc: dm-devel
dm-ioctl: don't use PF_MEMALLOC
PF_MEMALLOC is a hack. Remove it and use documented GFP flags instead.
Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
---
drivers/md/dm-ioctl.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
Index: linux-3.7-rc6/drivers/md/dm-ioctl.c
===================================================================
--- linux-3.7-rc6.orig/drivers/md/dm-ioctl.c 2012-11-20 22:44:00.000000000 +0100
+++ linux-3.7-rc6/drivers/md/dm-ioctl.c 2012-11-21 23:49:55.000000000 +0100
@@ -1598,7 +1598,7 @@ static int copy_params(struct dm_ioctl _
secure_data = tmp.flags & DM_SECURE_DATA_FLAG;
- dmi = vmalloc(tmp.data_size);
+ dmi = __vmalloc(tmp.data_size, GFP_NOIO | __GFP_REPEAT | __GFP_HIGH, PAGE_KERNEL);
if (!dmi) {
if (secure_data && clear_user(user, tmp.data_size))
return -EFAULT;
@@ -1691,18 +1691,10 @@ static int ctl_ioctl(uint command, struc
}
/*
- * Trying to avoid low memory issues when a device is
- * suspended.
- */
- current->flags |= PF_MEMALLOC;
-
- /*
* Copy the parameters into kernel space.
*/
r = copy_params(user, ¶m);
- current->flags &= ~PF_MEMALLOC;
-
if (r)
return r;
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 2/3] dm-ioctl: fix unsafe use of dm_ioctl.data_size 2012-11-23 0:19 [PATCH 1/3] dm-ioctl: don't use PF_MEMALLOC Mikulas Patocka @ 2012-11-23 0:20 ` Mikulas Patocka 2012-11-23 0:21 ` [PATCH 3/3] dm-ioctl: use kmalloc if possible Mikulas Patocka 2012-12-06 21:22 ` [PATCH 2/3] dm-ioctl: fix unsafe use of dm_ioctl.data_size Alasdair G Kergon 0 siblings, 2 replies; 6+ messages in thread From: Mikulas Patocka @ 2012-11-23 0:20 UTC (permalink / raw) To: Alasdair G. Kergon; +Cc: dm-devel dm-ioctl: fix unsafe use of dm_ioctl.data_size 1. ctl_ioctl calls copy_params 2. copy_params makes a first copy of the beginning of userspace parameters into the local variable "tmp" 3. copy_params then validates tmp.data_size and allocates a new structure and copies the whole userspace buffer there 4. ctl_ioctl reads userspace data the second time and copies the whole buffer into the pointer "param" 5. ctl_ioctl reads param->data_size without any validation and stores it in the variable "input_param_size" 6. "input_param_size" is further used as the authoritative size of the kernel buffer The problem is that the userspace code can change the content of user memory between steps 2. and 4. The userspace can set valid data_size, call the ioctl, the kernel code concludes that tmp.data_size is valid, the userspace changes the value to something invalid, the kernel reads the userspace buffer at step 4. The kernel then uses the invalid value in param->data_size without any checks. Thus, userspace can force the kernel to access invalid kernel memory. This patch fixes it so that "input_param_size" is read in copy_params, so the validated value is used. Hopefully, this doesn't have security impact because only root can access /dev/mapper/control device. But it should be fixed anyway. Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> Cc: stable@kernel.org --- drivers/md/dm-ioctl.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) Index: linux-3.7-rc6/drivers/md/dm-ioctl.c =================================================================== --- linux-3.7-rc6.orig/drivers/md/dm-ioctl.c 2012-11-22 03:06:49.000000000 +0100 +++ linux-3.7-rc6/drivers/md/dm-ioctl.c 2012-11-22 03:23:47.000000000 +0100 @@ -1585,7 +1585,8 @@ static int check_version(unsigned int cm return r; } -static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl **param) +static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl **param, + size_t *param_size) { struct dm_ioctl tmp, *dmi; int secure_data; @@ -1596,6 +1597,8 @@ static int copy_params(struct dm_ioctl _ if (tmp.data_size < (sizeof(tmp) - sizeof(tmp.data))) return -EINVAL; + *param_size = tmp.data_size; + secure_data = tmp.flags & DM_SECURE_DATA_FLAG; dmi = __vmalloc(tmp.data_size, GFP_NOIO | __GFP_REPEAT | __GFP_HIGH, PAGE_KERNEL); @@ -1693,12 +1696,11 @@ static int ctl_ioctl(uint command, struc /* * Copy the parameters into kernel space. */ - r = copy_params(user, ¶m); + r = copy_params(user, ¶m, &input_param_size); if (r) return r; - input_param_size = param->data_size; wipe_buffer = param->flags & DM_SECURE_DATA_FLAG; r = validate_params(cmd, param); ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 3/3] dm-ioctl: use kmalloc if possible 2012-11-23 0:20 ` [PATCH 2/3] dm-ioctl: fix unsafe use of dm_ioctl.data_size Mikulas Patocka @ 2012-11-23 0:21 ` Mikulas Patocka 2012-12-06 21:22 ` [PATCH 2/3] dm-ioctl: fix unsafe use of dm_ioctl.data_size Alasdair G Kergon 1 sibling, 0 replies; 6+ messages in thread From: Mikulas Patocka @ 2012-11-23 0:21 UTC (permalink / raw) To: Alasdair G. Kergon; +Cc: dm-devel dm-ioctl: use kmalloc if possible Use kmalloc if possible to allocate the parameters and only fallback to vmalloc if kmalloc fails. vmalloc is noticeable slower than kmalloc because it has to manipulate page tables. On PA-RISC this patch speeds up activation 13 times. On Opteron this patch speeds up activation by 5%. This patch introduces a new variable param_flags that holds flags for parameter allocation. The variables "int secure_data" and "int wipe_buffer" were moved to param_flags too (as flag PARAM_SECURE) to simplify the code. Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> --- drivers/md/dm-ioctl.c | 47 +++++++++++++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 18 deletions(-) Index: linux-3.7-rc6/drivers/md/dm-ioctl.c =================================================================== --- linux-3.7-rc6.orig/drivers/md/dm-ioctl.c 2012-11-22 03:30:59.000000000 +0100 +++ linux-3.7-rc6/drivers/md/dm-ioctl.c 2012-11-22 03:44:01.000000000 +0100 @@ -1585,25 +1585,43 @@ static int check_version(unsigned int cm return r; } +#define PARAM_VMALLOC 1 +#define PARAM_SECURE 2 + +static void free_params(struct dm_ioctl *param, size_t param_size, + int param_flags) +{ + if (param_flags & PARAM_SECURE) + memset(param, 0, param_size); + if (param_flags & PARAM_VMALLOC) + vfree(param); + else + kfree(param); +} + static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl **param, - size_t *param_size) + size_t *param_size, int *param_flags) { struct dm_ioctl tmp, *dmi; - int secure_data; if (copy_from_user(&tmp, user, sizeof(tmp) - sizeof(tmp.data))) return -EFAULT; *param_size = tmp.data_size; + *param_flags = tmp.flags & DM_SECURE_DATA_FLAG ? PARAM_SECURE : 0; if (*param_size < (sizeof(tmp) - sizeof(tmp.data))) return -EINVAL; - secure_data = tmp.flags & DM_SECURE_DATA_FLAG; - - dmi = __vmalloc(*param_size, GFP_NOIO | __GFP_REPEAT | __GFP_HIGH, PAGE_KERNEL); + dmi = NULL; + if (*param_size <= KMALLOC_MAX_SIZE) + dmi = kmalloc(*param_size, GFP_NOIO | __GFP_NORETRY | __GFP_NOMEMALLOC | __GFP_NOWARN); if (!dmi) { - if (secure_data && clear_user(user, *param_size)) + dmi = __vmalloc(*param_size, GFP_NOIO | __GFP_REPEAT | __GFP_HIGH, PAGE_KERNEL); + *param_flags |= PARAM_VMALLOC; + } + if (!dmi) { + if (*param_flags & PARAM_SECURE && clear_user(user, *param_size)) return -EFAULT; return -ENOMEM; } @@ -1612,16 +1630,14 @@ static int copy_params(struct dm_ioctl _ goto bad; /* Wipe the user buffer so we do not return it to userspace */ - if (secure_data && clear_user(user, *param_size)) + if (*param_flags & PARAM_SECURE && clear_user(user, *param_size)) goto bad; *param = dmi; return 0; bad: - if (secure_data) - memset(dmi, 0, *param_size); - vfree(dmi); + free_params(dmi, *param_size, *param_flags); return -EFAULT; } @@ -1658,11 +1674,11 @@ static int validate_params(uint cmd, str static int ctl_ioctl(uint command, struct dm_ioctl __user *user) { int r = 0; - int wipe_buffer; unsigned int cmd; struct dm_ioctl *uninitialized_var(param); ioctl_fn fn = NULL; size_t input_param_size; + int input_param_flags; /* only root can play with this */ if (!capable(CAP_SYS_ADMIN)) @@ -1696,13 +1712,11 @@ static int ctl_ioctl(uint command, struc /* * Copy the parameters into kernel space. */ - r = copy_params(user, ¶m, &input_param_size); + r = copy_params(user, ¶m, &input_param_size, &input_param_flags); if (r) return r; - wipe_buffer = param->flags & DM_SECURE_DATA_FLAG; - r = validate_params(cmd, param); if (r) goto out; @@ -1717,10 +1731,7 @@ static int ctl_ioctl(uint command, struc r = -EFAULT; out: - if (wipe_buffer) - memset(param, 0, input_param_size); - - vfree(param); + free_params(param, input_param_size, input_param_flags); return r; } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] dm-ioctl: fix unsafe use of dm_ioctl.data_size 2012-11-23 0:20 ` [PATCH 2/3] dm-ioctl: fix unsafe use of dm_ioctl.data_size Mikulas Patocka 2012-11-23 0:21 ` [PATCH 3/3] dm-ioctl: use kmalloc if possible Mikulas Patocka @ 2012-12-06 21:22 ` Alasdair G Kergon 2012-12-06 23:30 ` Mike Snitzer 2012-12-07 2:53 ` Mikulas Patocka 1 sibling, 2 replies; 6+ messages in thread From: Alasdair G Kergon @ 2012-12-06 21:22 UTC (permalink / raw) To: Mikulas Patocka; +Cc: dm-devel I don't think we need to support the situation when the value changes during the copying of the parameters, so how about something more like this instead? Alasdair From: Alasdair G Kergon <agk@redhat.com> Abort dm ioctl processing if userspace changes the data_size parameter after we validated it but before we finished copying the data buffer from userspace. The dm ioctl parameters are processed in the following sequence: 1. ctl_ioctl() calls copy_params(); 2. copy_params() makes a first copy of the fixed-sized portion of the userspace parameters into the local variable "tmp"; 3. copy_params() then validates tmp.data_size and allocates a new structure big enough to hold the complete data and copies the whole userspace buffer there; 4. ctl_ioctl() reads userspace data the second time and copies the whole buffer into the pointer "param"; 5. ctl_ioctl() reads param->data_size without any validation and stores it in the variable "input_param_size"; 6. "input_param_size" is further used as the authoritative size of the kernel buffer. The problem is that userspace code could change the contents of user memory between steps 2 and 4. In particular, the data_size parameter can be changed to an invalid value after the kernel has validated it. This lets userspace force the kernel to access invalid kernel memory. The fix is to ensure that the size has not changed at step 4. This patch shouldn't have a security impact because CAP_SYS_ADMIN is required to run this code, but it should be fixed anyway. Reported-by: Mikulas Patocka <mpatocka@redhat.com> Signed-off-by: Alasdair G Kergon <agk@redhat.com> Cc: stable@kernel.org --- drivers/md/dm-ioctl.c | 8 ++++++++ 1 file changed, 8 insertions(+) Index: linux/drivers/md/dm-ioctl.c =================================================================== --- linux.orig/drivers/md/dm-ioctl.c +++ linux/drivers/md/dm-ioctl.c @@ -1566,6 +1566,14 @@ static int copy_params(struct dm_ioctl _ if (copy_from_user(dmi, user, tmp.data_size)) goto bad; + /* + * Abort if something changed the ioctl data while it was being copied. + */ + if (dmi->data_size != tmp.data_size) { + DMERR("rejecting ioctl: data size modified while processing parameters"); + goto bad; + } + /* Wipe the user buffer so we do not return it to userspace */ if (secure_data && clear_user(user, tmp.data_size)) goto bad; ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] dm-ioctl: fix unsafe use of dm_ioctl.data_size 2012-12-06 21:22 ` [PATCH 2/3] dm-ioctl: fix unsafe use of dm_ioctl.data_size Alasdair G Kergon @ 2012-12-06 23:30 ` Mike Snitzer 2012-12-07 2:53 ` Mikulas Patocka 1 sibling, 0 replies; 6+ messages in thread From: Mike Snitzer @ 2012-12-06 23:30 UTC (permalink / raw) To: Alasdair G Kergon; +Cc: dm-devel, Mikulas Patocka On Thu, Dec 06 2012 at 4:22pm -0500, Alasdair G Kergon <agk@redhat.com> wrote: > I don't think we need to support the situation when the value changes during > the copying of the parameters, so how about something more like this instead? > > Alasdair > > > From: Alasdair G Kergon <agk@redhat.com> > > Abort dm ioctl processing if userspace changes the data_size parameter > after we validated it but before we finished copying the data buffer > from userspace. > > The dm ioctl parameters are processed in the following sequence: > 1. ctl_ioctl() calls copy_params(); > 2. copy_params() makes a first copy of the fixed-sized portion of the > userspace parameters into the local variable "tmp"; > 3. copy_params() then validates tmp.data_size and allocates a new > structure big enough to hold the complete data and copies the whole > userspace buffer there; > 4. ctl_ioctl() reads userspace data the second time and copies the whole > buffer into the pointer "param"; > 5. ctl_ioctl() reads param->data_size without any validation and stores it > in the variable "input_param_size"; > 6. "input_param_size" is further used as the authoritative size of the > kernel buffer. > > The problem is that userspace code could change the contents of user > memory between steps 2 and 4. In particular, the data_size parameter > can be changed to an invalid value after the kernel has validated it. > This lets userspace force the kernel to access invalid kernel memory. > > The fix is to ensure that the size has not changed at step 4. > > This patch shouldn't have a security impact because CAP_SYS_ADMIN is > required to run this code, but it should be fixed anyway. > > Reported-by: Mikulas Patocka <mpatocka@redhat.com> > Signed-off-by: Alasdair G Kergon <agk@redhat.com> > Cc: stable@kernel.org Looks good. Acked-by: Mike Snitzer <snitzer@redhat.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] dm-ioctl: fix unsafe use of dm_ioctl.data_size 2012-12-06 21:22 ` [PATCH 2/3] dm-ioctl: fix unsafe use of dm_ioctl.data_size Alasdair G Kergon 2012-12-06 23:30 ` Mike Snitzer @ 2012-12-07 2:53 ` Mikulas Patocka 1 sibling, 0 replies; 6+ messages in thread From: Mikulas Patocka @ 2012-12-07 2:53 UTC (permalink / raw) To: Alasdair G Kergon; +Cc: dm-devel Yes, it's ok. Acked-by: Mikulas Patocka <mpatocka@redhat.com> On Thu, 6 Dec 2012, Alasdair G Kergon wrote: > I don't think we need to support the situation when the value changes during > the copying of the parameters, so how about something more like this instead? > > Alasdair > > > From: Alasdair G Kergon <agk@redhat.com> > > Abort dm ioctl processing if userspace changes the data_size parameter > after we validated it but before we finished copying the data buffer > from userspace. > > The dm ioctl parameters are processed in the following sequence: > 1. ctl_ioctl() calls copy_params(); > 2. copy_params() makes a first copy of the fixed-sized portion of the > userspace parameters into the local variable "tmp"; > 3. copy_params() then validates tmp.data_size and allocates a new > structure big enough to hold the complete data and copies the whole > userspace buffer there; > 4. ctl_ioctl() reads userspace data the second time and copies the whole > buffer into the pointer "param"; > 5. ctl_ioctl() reads param->data_size without any validation and stores it > in the variable "input_param_size"; > 6. "input_param_size" is further used as the authoritative size of the > kernel buffer. > > The problem is that userspace code could change the contents of user > memory between steps 2 and 4. In particular, the data_size parameter > can be changed to an invalid value after the kernel has validated it. > This lets userspace force the kernel to access invalid kernel memory. > > The fix is to ensure that the size has not changed at step 4. > > This patch shouldn't have a security impact because CAP_SYS_ADMIN is > required to run this code, but it should be fixed anyway. > > Reported-by: Mikulas Patocka <mpatocka@redhat.com> > Signed-off-by: Alasdair G Kergon <agk@redhat.com> > Cc: stable@kernel.org > > --- > drivers/md/dm-ioctl.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > Index: linux/drivers/md/dm-ioctl.c > =================================================================== > --- linux.orig/drivers/md/dm-ioctl.c > +++ linux/drivers/md/dm-ioctl.c > @@ -1566,6 +1566,14 @@ static int copy_params(struct dm_ioctl _ > if (copy_from_user(dmi, user, tmp.data_size)) > goto bad; > > + /* > + * Abort if something changed the ioctl data while it was being copied. > + */ > + if (dmi->data_size != tmp.data_size) { > + DMERR("rejecting ioctl: data size modified while processing parameters"); > + goto bad; > + } > + > /* Wipe the user buffer so we do not return it to userspace */ > if (secure_data && clear_user(user, tmp.data_size)) > goto bad; > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-12-07 2:53 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-11-23 0:19 [PATCH 1/3] dm-ioctl: don't use PF_MEMALLOC Mikulas Patocka 2012-11-23 0:20 ` [PATCH 2/3] dm-ioctl: fix unsafe use of dm_ioctl.data_size Mikulas Patocka 2012-11-23 0:21 ` [PATCH 3/3] dm-ioctl: use kmalloc if possible Mikulas Patocka 2012-12-06 21:22 ` [PATCH 2/3] dm-ioctl: fix unsafe use of dm_ioctl.data_size Alasdair G Kergon 2012-12-06 23:30 ` Mike Snitzer 2012-12-07 2:53 ` Mikulas Patocka
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.