dm ioctl: Access user-land memory through safe functions.

PATCH In review Scotty 2 participants 6 messages Dec 1, 2015 – Jan 7, 2016 Device mapper

Share & tools

Scotty

author

2015-12-01 18:11 UTC #0 lore raw

PATCH 1/1 dm ioctl: Access user-land memory through safe functions.

From b26adf880eba03ac6f2b1dd87426bb96fd2a0282 Mon Sep 17 00:00:00 2001
From: Scotty Bauer <[email protected]>
Date: Tue, 1 Dec 2015 10:52:46 -0700
Subject: [PATCH] dm ioctl: Access user-land memory through safe functions.

This patch fixes a user-land dereference. Now we use the safe copy_from_user to access the memory.

Signed-off-by Scotty Bauer <[email protected]>

---
 drivers/md/dm-ioctl.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

drivers/md/dm-ioctl.c +6 -2

index 80a4395..39a9d1a 100644
@@ -1642,9 +1642,13 @@ static ioctl_fn lookup_ioctl(unsigned int cmd, int *ioctl_flags)
 static int check_version(unsigned int cmd, struct dm_ioctl __user *user)
 {
 	uint32_t version[3];
+	uint32_t __user *version_ptr;
 	int r = 0;
 
-	if (copy_from_user(version, user->version, sizeof(version)))
+	if (copy_from_user(&version_ptr, &user->version, sizeof(version_ptr)))
+		return -EFAULT;
+
+	if (copy_from_user(version, version_ptr, sizeof(version)))
 		return -EFAULT;
 
 	if ((DM_VERSION_MAJOR != version[0]) ||

@@ -1663,7 +1667,7 @@ static int check_version(unsigned int cmd, struct dm_ioctl __user *user)
 	version[0] = DM_VERSION_MAJOR;
 	version[1] = DM_VERSION_MINOR;
 	version[2] = DM_VERSION_PATCHLEVEL;
-	if (copy_to_user(user->version, version, sizeof(version)))
+	if (copy_to_user(version_ptr, version, sizeof(version)))
 		return -EFAULT;
 
 	return r;

Signature · 1 line

1.9.1

Scotty Bauer

author

2015-12-08 18:26 UTC #1 lore raw

Quoted 38 lines · Scotty

0001-dm-ioctl-Access-user-land-memory-through-safe-functi.patch
From b26adf880eba03ac6f2b1dd87426bb96fd2a0282 Mon Sep 17 00:00:00 2001
From: Scotty Bauer <[email protected]>
Date: Tue, 1 Dec 2015 10:52:46 -0700
Subject: [PATCH] dm ioctl: Access user-land memory through safe functions.
This patch fixes a user-land dereference. Now we use the safe copy_from_user to access the memory.
Signed-off-by Scotty Bauer <[email protected]>
---
 drivers/md/dm-ioctl.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/md/dm-ioctl.c +5 -1
index 80a4395..39a9d1a 100644
@@ -1642,9 +1642,13 @@ static ioctl_fn lookup_ioctl(unsigned int cmd, int *ioctl_flags)
 static int check_version(unsigned int cmd, struct dm_ioctl __user *user)
 {
 	uint32_t version[3];
+	uint32_t __user *version_ptr;
 	int r = 0;
 
-	if (copy_from_user(version, user->version, sizeof(version)))
+	if (copy_from_user(&version_ptr, &user->version, sizeof(version_ptr)))
+		return -EFAULT;
+
+	if (copy_from_user(version, version_ptr, sizeof(version)))
 		return -EFAULT;
 
 	if ((DM_VERSION_MAJOR != version[0]) ||

drivers/md/dm-ioctl.c static int check_version(unsigned int cmd, struct dm_ioctl __user *user) · line 1673

Quoted 8 lines

drivers/md/dm-ioctl.c +1 -1

@@ -1663,7 +1667,7 @@ static int check_version(unsigned int cmd, struct dm_ioctl __user *user)
 	version[0] = DM_VERSION_MAJOR;
 	version[1] = DM_VERSION_MINOR;
 	version[2] = DM_VERSION_PATCHLEVEL;
-	if (copy_to_user(user->version, version, sizeof(version)))
+	if (copy_to_user(version_ptr, version, sizeof(version)))
 		return -EFAULT;
 
 	return r;

Friendly ping, is anyone interested in this?

Mike Snitzer

2016-01-05 20:16 UTC #2 lore raw

Quoted 46 lines · Scotty Bauer

Quoted 44 lines · Scotty

0001-dm-ioctl-Access-user-land-memory-through-safe-functi.patch

This patch fixes a user-land dereference. Now we use the safe copy_from_user to access the memory.

Signed-off-by Scotty Bauer <[email protected]>

---
 drivers/md/dm-ioctl.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

drivers/md/dm-ioctl.c +6 -2

index 80a4395..39a9d1a 100644
@@ -1642,9 +1642,13 @@ static ioctl_fn lookup_ioctl(unsigned int cmd, int *ioctl_flags)
 static int check_version(unsigned int cmd, struct dm_ioctl __user *user)
 {
 	uint32_t version[3];
+	uint32_t __user *version_ptr;
 	int r = 0;
 
-	if (copy_from_user(version, user->version, sizeof(version)))
+	if (copy_from_user(&version_ptr, &user->version, sizeof(version_ptr)))
+		return -EFAULT;
+
+	if (copy_from_user(version, version_ptr, sizeof(version)))
 		return -EFAULT;
 
 	if ((DM_VERSION_MAJOR != version[0]) ||

@@ -1663,7 +1667,7 @@ static int check_version(unsigned int cmd, struct dm_ioctl __user *user)
 	version[0] = DM_VERSION_MAJOR;
 	version[1] = DM_VERSION_MINOR;
 	version[2] = DM_VERSION_PATCHLEVEL;
-	if (copy_to_user(user->version, version, sizeof(version)))
+	if (copy_to_user(version_ptr, version, sizeof(version)))
 		return -EFAULT;

Quoted 5 lines

drivers/md/dm-ioctl.c static int check_version(unsigned int cmd, struct dm_ioctl __user *user) · line 1673
Quoted 2 lines
drivers/md/dm-ioctl.c +0 -0
 	return r;
Friendly ping, is anyone interested in this?

The passed @user argument is flagged via __user so it can be deferenced directly. It does look like directly deferencing user->version is wrong.

But even if such indirect access is needed (because __user flag is only applicable to @user arg, not the contained version member) we could more easily just do something like this no?:

  uint32_t __user *versionp = (uint32_t __user *)user->version;
  ...
  if (copy_from_user(version, versionp, sizeof(version)))
     return -EFAULT;

I've staged the following, thanks:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.5&id=bffc9e237a0c3176712bcd93fc6a184a61e0df26

Mike Snitzer

2016-01-05 21:13 UTC #3 lore raw

Quoted 13 lines · Mike Snitzer

Quoted 1 line · Scotty Bauer
Friendly ping, is anyone interested in this?
The passed @user argument is flagged via __user so it can be deferenced directly. It does look like directly deferencing user->version is wrong.
But even if such indirect access is needed (because __user flag is only applicable to @user arg, not the contained version member) we could more easily just do something like this no?:

Quoted 7 lines

  uint32_t __user *versionp = (uint32_t __user *)user->version;
  ...
  if (copy_from_user(version, versionp, sizeof(version)))
     return -EFAULT;
I've staged the following, thanks:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.5&id=bffc9e237a0c3176712bcd93fc6a184a61e0df26

Alasdair helped me understand that we do need your original fix. I've staged it for 4.5 (and stable@) here:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.5&id=ead3db62bf10fe143bec99e7b7ff370d7a6d23ef

Thanks again,
Mike

Scotty Bauer

author

2016-01-07 01:22 UTC #4 lore raw

Quoted 24 lines · Mike Snitzer

Quoted 13 lines · Mike Snitzer
Quoted 1 line · Scotty Bauer
Friendly ping, is anyone interested in this?
The passed @user argument is flagged via __user so it can be deferenced directly. It does look like directly deferencing user->version is wrong.
But even if such indirect access is needed (because __user flag is only applicable to @user arg, not the contained version member) we could more easily just do something like this no?:
Quoted 7 lines
  uint32_t __user *versionp = (uint32_t __user *)user->version;
  ...
  if (copy_from_user(version, versionp, sizeof(version)))
     return -EFAULT;
I've staged the following, thanks:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.5&id=bffc9e237a0c3176712bcd93fc6a184a61e0df26

Quoted 7 lines

Alasdair helped me understand that we do need your original fix. I've staged it for 4.5 (and stable@) here:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.5&id=ead3db62bf10fe143bec99e7b7ff370d7a6d23ef
Thanks again,
Mike
--

This broke linux-next because I'm dumb and didn't test it. I thought it was a trivial enough of a patch that I wouldn't screw it up, but I did.

I incorrectly assumed that user->version was essentially a pointer in userland, not a flat chunk of memory. Ie it was a pointer to some malloc'd region, not an inlined version[3].

I thought it was this:
struct dm_ioctl {

uint32_t *version;
...
}

It is really this:

struct dm_ioctl {

uint32_t version[3];

}

I was trying to get the values out of *version, which would have been a pointer, but instead what the code ended up doing was actually getting 8 bytes of the version (think 4,3,1) out and trying to access that version as a memory address, oops.

It turns out that the original code is correct and doesn't actually touch user memory without a copy_from_user(). Gcc is smart enough to see that version[3] is inlined, and it can emit code which simply takes the userland pointer (struct dm_ioctl __user user), and calculates on offset based on the pointer, thus no actual user dereference occurs. Had the struct looked like the first example I believe the patch would work.

I'm wondering now if we should switch the code a bit to make it less ambiguous, so someone like me doesn't come along again thinking the code dereferences userland memory and waste everyones time.

I've attached a patch based off linux-next-20150616 which reverts my broken code but adds an & to the front of user->version so it looks like the code is doing the right thing.

If I should be basing my patch off something other than linux-next let me know and I'll rewrite it, or we can just revert the old patch and ignore this one.

Thanks and very sorry for the confusion and breakage.

From 7dde54b74e4543b6f03ceb57f9479a1d402a3fd1 Mon Sep 17 00:00:00 2001
From: Scotty Bauer <[email protected]>
Date: Wed, 6 Jan 2016 18:17:35 -0700
Subject: [PATCH] dm ioctl: disambiguate the user pointer calculation

This patch adds an & in front of user->version, in hopes of making it clear that user-memory is not being touched.

Signed-off-by Scotty Bauer <[email protected]>

---
 drivers/md/dm-ioctl.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

drivers/md/dm-ioctl.c +2 -6

index fa5bf54..81190df 100644
@@ -1642,13 +1642,9 @@ static ioctl_fn lookup_ioctl(unsigned int cmd, int *ioctl_flags)
 static int check_version(unsigned int cmd, struct dm_ioctl __user *user)
 {
 	uint32_t version[3];
-	uint32_t __user *versionp;
 	int r = 0;
 
-	if (copy_from_user(&versionp, &user->version, sizeof(versionp)))
-		return -EFAULT;
-
-	if (copy_from_user(version, versionp, sizeof(version)))
+	if (copy_from_user(version, &user->version, sizeof(version)))
 		return -EFAULT;
 
 	if ((DM_VERSION_MAJOR != version[0]) ||

@@ -1667,7 +1663,7 @@ static int check_version(unsigned int cmd, struct dm_ioctl __user *user)
 	version[0] = DM_VERSION_MAJOR;
 	version[1] = DM_VERSION_MINOR;
 	version[2] = DM_VERSION_PATCHLEVEL;
-	if (copy_to_user(versionp, version, sizeof(version)))
+	if (copy_to_user(&user->version, version, sizeof(version)))
 		return -EFAULT;
 
 	return r;

Signature · 1 line

1.9.1

Mike Snitzer

2016-01-07 02:07 UTC #5 lore raw

Quoted 58 lines · Scotty Bauer

Quoted 24 lines · Mike Snitzer
Quoted 13 lines · Mike Snitzer
Quoted 1 line · Scotty Bauer
Friendly ping, is anyone interested in this?
The passed @user argument is flagged via __user so it can be deferenced directly. It does look like directly deferencing user->version is wrong.
But even if such indirect access is needed (because __user flag is only applicable to @user arg, not the contained version member) we could more easily just do something like this no?:
Quoted 7 lines
  uint32_t __user *versionp = (uint32_t __user *)user->version;
  ...
  if (copy_from_user(version, versionp, sizeof(version)))
     return -EFAULT;
I've staged the following, thanks:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.5&id=bffc9e237a0c3176712bcd93fc6a184a61e0df26
Quoted 7 lines
Alasdair helped me understand that we do need your original fix. I've staged it for 4.5 (and stable@) here:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.5&id=ead3db62bf10fe143bec99e7b7ff370d7a6d23ef
Thanks again,
Mike
--
This broke linux-next because I'm dumb and didn't test it. I thought it was a trivial enough of a patch that I wouldn't screw it up, but I did.
I incorrectly assumed that user->version was essentially a pointer in userland, not a flat chunk of memory. Ie it was a pointer to some malloc'd region, not an inlined version[3].
I thought it was this:
struct dm_ioctl {
uint32_t *version;
...
}
It is really this:
struct dm_ioctl {
uint32_t version[3];
}
I was trying to get the values out of *version, which would have been a pointer, but instead what the code ended up doing was actually getting 8 bytes of the version (think 4,3,1) out and trying to access that version as a memory address, oops.
It turns out that the original code is correct and doesn't actually touch user memory without a copy_from_user(). Gcc is smart enough to see that version[3] is inlined, and it can emit code which simply takes the userland pointer (struct dm_ioctl __user user), and calculates on offset based on the pointer, thus no actual user dereference occurs. Had the struct looked like the first example I believe the patch would work.

Quoted 7 lines

I'm wondering now if we should switch the code a bit to make it less ambiguous, so someone like me doesn't come along again thinking the code dereferences userland memory and waste everyones time.
I've attached a patch based off linux-next-20150616 which reverts my broken code but adds an & to the front of user->version so it looks like the code is doing the right thing.
If I should be basing my patch off something other than linux-next let me know and I'll rewrite it, or we can just revert the old patch and ignore this one.
Thanks and very sorry for the confusion and breakage.

You're fine, no worries.

But I've just dropped the offending original commit from linux-next and it obviously won't be included in 4.5

I'll revisit whether we need to bother with the extra & change you're suggesting while coming to terms with why I was able to be lulled into thinking your original patch was correct ;)