diff mbox series

[v2,4/6] device-mapper: Avoid double-fetch of version

Message ID	20230603145244.1538-5-demi@invisiblethingslab.com
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Feedback-ID: iac594737:Fastmail From: Demi Marie Obenour <demi@invisiblethingslab.com> To: Alasdair Kergon <agk@redhat.com>, Mike Snitzer <snitzer@kernel.org>, dm-devel@redhat.com Cc: Demi Marie Obenour <demi@invisiblethingslab.com>, linux-kernel@vger.kernel.org, stable@vger.kernel.org Subject: [PATCH v2 4/6] device-mapper: Avoid double-fetch of version Date: Sat, 3 Jun 2023 10:52:42 -0400 Message-Id: <20230603145244.1538-5-demi@invisiblethingslab.com> In-Reply-To: <20230603145244.1538-1-demi@invisiblethingslab.com> References: <20230601212456.1533-1-demi@invisiblethingslab.com> <20230603145244.1538-1-demi@invisiblethingslab.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	Several device-mapper fixes \| [v2,0/6] Several device-mapper fixes [v2,1/6] device-mapper: Check that target specs are sufficiently aligned [v2,2/6] device-mapper: Avoid pointer arithmetic overflow [v2,3/6] device-mapper: structs and parameter strings must not overlap [v2,4/6] device-mapper: Avoid double-fetch of version [v2,5/6] device-mapper: Refuse to create device named "control" [v2,6/6] device-mapper: "." and ".." are not valid symlink names

Commit Message

Demi Marie Obenour June 3, 2023, 2:52 p.m. UTC

  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: stable@vger.kernel.org
Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 drivers/md/dm-ioctl.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

Comments

Mike Snitzer June 22, 2023, 4:20 p.m. UTC | #1

On Sat, Jun 03 2023 at 10:52P -0400,
Demi Marie Obenour <demi@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: stable@vger.kernel.org
> Signed-off-by: Demi Marie Obenour <demi@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, &param_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

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)))

Demi Marie Obenour June 22, 2023, 6:43 p.m. UTC | #2

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 <demi@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: stable@vger.kernel.org
> > Signed-off-by: Demi Marie Obenour <demi@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, &param_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!

diff mbox series

Patch

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, &param_kernel);
 	if (r)
 		return r;