| Message ID | 20121206212216.GJ6834@agk.fab.redhat.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
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> -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/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; > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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;