On Thu, Jun 22, 2023 at 12:20:40PM -0400, Mike Snitzer wrote:
> On Sat, Jun 03 2023 at 10:52P -0400,
> Demi Marie Obenour <d...@invisiblethingslab.com> wrote:
>
> > The version is fetched once in check_version(), which then does some
> > validation and then overwrites the version in userspace with the API
> > version supported by the kernel. copy_params() then fetches the version
> > from userspace *again*, and this time no validation is done. The result
> > is that the kernel's version number is completely controllable by
> > userspace, provided that userspace can win a race condition.
> >
> > Fix this flaw by not copying the version back to the kernel the second
> > time. This is not exploitable as the version is not further used in the
> > kernel. However, it could become a problem if future patches start
> > relying on the version field.
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Cc: sta...@vger.kernel.org
> > Signed-off-by: Demi Marie Obenour <d...@invisiblethingslab.com>
> > ---
> > drivers/md/dm-ioctl.c | 30 ++++++++++++++++++------------
> > 1 file changed, 18 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> > index
> > da6ca26b51d0953df380582bb3a51c2ec22c27cb..7510afe237d979a5ee71afe87a20d49f631de1aa
> > 100644
> > --- a/drivers/md/dm-ioctl.c
> > +++ b/drivers/md/dm-ioctl.c
> > @@ -1873,30 +1873,33 @@ static ioctl_fn lookup_ioctl(unsigned int cmd, int
> > *ioctl_flags)
> > * As well as checking the version compatibility this always
> > * copies the kernel interface version out.
> > */
> > -static int check_version(unsigned int cmd, struct dm_ioctl __user *user)
> > +static int check_version(unsigned int cmd, struct dm_ioctl __user *user,
> > + struct dm_ioctl *kernel_params)
> > {
> > - uint32_t version[3];
> > int r = 0;
> >
> > - if (copy_from_user(version, user->version, sizeof(version)))
> > + if (copy_from_user(kernel_params->version, user->version,
> > sizeof(kernel_params->version)))
> > return -EFAULT;
> >
> > - if ((version[0] != DM_VERSION_MAJOR) ||
> > - (version[1] > DM_VERSION_MINOR)) {
> > + if ((kernel_params->version[0] != DM_VERSION_MAJOR) ||
> > + (kernel_params->version[1] > DM_VERSION_MINOR)) {
> > DMERR("ioctl interface mismatch: kernel(%u.%u.%u),
> > user(%u.%u.%u), cmd(%d)",
> > DM_VERSION_MAJOR, DM_VERSION_MINOR,
> > DM_VERSION_PATCHLEVEL,
> > - version[0], version[1], version[2], cmd);
> > + kernel_params->version[0],
> > + kernel_params->version[1],
> > + kernel_params->version[2],
> > + cmd);
> > r = -EINVAL;
> > }
> >
> > /*
> > * Fill in the kernel version.
> > */
> > - version[0] = DM_VERSION_MAJOR;
> > - version[1] = DM_VERSION_MINOR;
> > - version[2] = DM_VERSION_PATCHLEVEL;
> > - if (copy_to_user(user->version, version, sizeof(version)))
> > + kernel_params->version[0] = DM_VERSION_MAJOR;
> > + kernel_params->version[1] = DM_VERSION_MINOR;
> > + kernel_params->version[2] = DM_VERSION_PATCHLEVEL;
> > + if (copy_to_user(user->version, kernel_params->version,
> > sizeof(kernel_params->version)))
> > return -EFAULT;
> >
> > return r;
> > @@ -1922,7 +1925,10 @@ static int copy_params(struct dm_ioctl __user *user,
> > struct dm_ioctl *param_kern
> > const size_t minimum_data_size = offsetof(struct dm_ioctl, data);
> > unsigned int noio_flag;
> >
> > - if (copy_from_user(param_kernel, user, minimum_data_size))
> > + /* Version has been copied from userspace already, avoid TOCTOU */
> > + if (copy_from_user((char *)param_kernel + sizeof(param_kernel->version),
> > + (char __user *)user + sizeof(param_kernel->version),
> > + minimum_data_size - sizeof(param_kernel->version)))
> > return -EFAULT;
> >
> > if (param_kernel->data_size < minimum_data_size) {
> > @@ -2034,7 +2040,7 @@ static int ctl_ioctl(struct file *file, uint command,
> > struct dm_ioctl __user *us
> > * Check the interface version passed in. This also
> > * writes out the kernel's interface version.
> > */
> > - r = check_version(cmd, user);
> > + r = check_version(cmd, user, ¶m_kernel);
> > if (r)
> > return r;
> >
>
> I picked this patch up for 6.5, please see:
> https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-6.5&id=c71878e9982075eab2e9f6dc5a09ba7b60ac1e24
That’s great, thanks!
> But FYI, I folded these changes in:
>
> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> index 526464904fc1..b58a15e212a3 100644
> --- a/drivers/md/dm-ioctl.c
> +++ b/drivers/md/dm-ioctl.c
> @@ -1838,6 +1838,9 @@ static int check_version(unsigned int cmd, struct
> dm_ioctl __user *user,
> {
> int r = 0;
>
> + /* Make certain version is first member of dm_ioctl struct */
> + BUILD_BUG_ON(offsetof(struct dm_ioctl, version) != 0);
> +
> if (copy_from_user(kernel_params->version, user->version,
> sizeof(kernel_params->version)))
> return -EFAULT;
>
> @@ -1885,7 +1888,7 @@ static int copy_params(struct dm_ioctl __user *user,
> struct dm_ioctl *param_kern
> const size_t minimum_data_size = offsetof(struct dm_ioctl, data);
> unsigned int noio_flag;
>
> - /* Version has been copied from userspace already, avoid TOCTOU */
> + /* check_version() already copied version from userspace, avoid TOCTOU
> */
> if (copy_from_user((char *)param_kernel + sizeof(param_kernel->version),
> (char __user *)user + sizeof(param_kernel->version),
> minimum_data_size - sizeof(param_kernel->version)))
Those changes indeed make the code better, thanks for including them!
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
signature.asc
Description: PGP signature
-- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
