Re: [PATCH] dm ioctl: prevent stack leak in dm ioctl call

Re: [PATCH] dm ioctl: prevent stack leak in dm ioctl call

[Alasdair G Kergon]

On Tue, Apr 25, 2017 at 04:31:29PM -0700, Adrian Salido wrote:
> Struct dm_ioctl has some padding/data that is not explicitly cleared
> before copying to user. This can cause kernel stack contents to be
> leaked to user space.

Please be more precise here, explaining which part of the buffer 
and under exactly what circumstances you have found that uninitialised 
content gets returned to userspace.

Alasdair

Re: [PATCH] dm ioctl: prevent stack leak in dm ioctl call

[Alasdair G Kergon]

On Tue, Apr 25, 2017 at 05:33:19PM -0700, Adrian Salido wrote:
> it's actually the data portion of the struct under a custom user ioctl
> where (param_kernel->data_size - minimum_data_size) <
> sizeof(param_kernel->data)
> Will update the patch to be clear

Yes - but before updating the patch, we need to be clearer about the
requirements of the ioctl here.

Why are two different minimum data sizes used?

If we let userspace send a truncated dm_ioctl struct, why are we not
returning the same truncated one?

Is this the bug?
  param->data_size = sizeof(*param);

Alasdair

Re: [PATCH] dm ioctl: prevent stack leak in dm ioctl call

[Alasdair G Kergon]

On Tue, Apr 25, 2017 at 05:57:41PM -0700, Adrian Salido wrote:
> 1. param_kernel is allocated from stack and passed to copy_params
> 2. copy_params only copies up to param_kernel->data from user
> (param_kernel->data still contains stack contents)
> 3. in copy_params, since there are no params it will skip through and
> return param = dmi = param_kernel

after setting
  dmi->data_size = minimum_data_size;

and then         
  input_param_size = param->data_size;

> 4. that stale data is copied back to user
because it is incorrectly extending the buffer?
  param->data_size = sizeof(*param);
instead of continuing to use input_param_size?

Alasdair

Re: [PATCH] dm ioctl: prevent stack leak in dm ioctl call

[Alasdair G Kergon]

On Tue, Apr 25, 2017 at 05:33:19PM -0700, Adrian Salido wrote:
> Will update the patch to be clear
 
So at the moment, we're leaning towards just:

  param->data_size = offsetof(struct dm_ioctl, data)

to replace

  param->data_size = sizeof(*param);

in the caller.

Alasdair