Re: [PATCH 2/3] dm-ioctl: fix unsafe use of dm_ioctl.data_size

[Mike Snitzer <snitzer@redhat.com>]

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>