From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mike Snitzer Subject: Re: [PATCH 2/3] dm-ioctl: fix unsafe use of dm_ioctl.data_size Date: Thu, 6 Dec 2012 18:30:28 -0500 Message-ID: <20121206233027.GA781@redhat.com> References: <20121206212216.GJ6834@agk.fab.redhat.com> Reply-To: device-mapper development Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20121206212216.GJ6834@agk.fab.redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dm-devel-bounces@redhat.com Errors-To: dm-devel-bounces@redhat.com To: Alasdair G Kergon Cc: dm-devel@redhat.com, Mikulas Patocka List-Id: dm-devel.ids On Thu, Dec 06 2012 at 4:22pm -0500, 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 > > 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 > Signed-off-by: Alasdair G Kergon > Cc: stable@kernel.org Looks good. Acked-by: Mike Snitzer