On Thu, Dec 06 2012 at 4:22pm -0500, Alasdair G Kergon <agk@xxxxxxxxxx> 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@xxxxxxxxxx> > > 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@xxxxxxxxxx> > Signed-off-by: Alasdair G Kergon <agk@xxxxxxxxxx> > Cc: stable@xxxxxxxxxx Looks good. Acked-by: Mike Snitzer <snitzer@xxxxxxxxxx> -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
