From: Zev Weiss <zevweiss@gmail.com>
The MEMGETREGIONINFO ioctl() in mtdchar.c was clobbering user memory by

overwriting more than intended, due to the size of struct

mtd_erase_region_info changing in commit

0ecbc81adfcb9f15f86b05ff576b342ce81bbef8.
Fix uses a member-by-member copy into a local struct region_info_user,

which is then copy_to_user()'d (and matches the size correctly by being

of the same type as the pointer passed in the ioctl() call).
Signed-off-by: Zev Weiss <zevweiss@gmail.com>

Tested-by: Zev Weiss <zevweiss@gmail.com>

---

I had been having some problems with userspace memory corruption, and traced

them to a MEMGETREGIONINFO ioctl() on an MTD device.  I applied this patch and

it seems to fix the problem, though I am not an expert and there may be a more

correct way to go about doing this.  I'm also new at submitting patches, so

hopefully I haven't screwed up the patch-submission etiquette too

horrifically.
  drivers/mtd/mtdchar.c |   11 +++++++++--

  1 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c

index 13cc67a..0acb135 100644

--- a/drivers/mtd/mtdchar.c

+++ b/drivers/mtd/mtdchar.c

@@ -411,14 +411,21 @@ static int mtd_ioctl(struct inode *inode, struct file *file,

  	case MEMGETREGIONINFO:

  	{

  		struct region_info_user ur;

+		struct mtd_erase_region_info *kr;
  		if (copy_from_user(&ur, argp, sizeof(struct region_info_user)))

  			return -EFAULT;
  		if (ur.regionindex >= mtd->numeraseregions)

  			return -EINVAL;

-		if (copy_to_user(argp, &(mtd->eraseregions[ur.regionindex]),

-				sizeof(struct mtd_erase_region_info)))

+

+		kr = &(mtd->eraseregions[ur.regionindex]);

+

+		ur.offset = kr->offset;

+		ur.erasesize = kr->erasesize;

+		ur.numblocks = kr->numblocks;

+

+		if (copy_to_user(argp, &ur, sizeof(struct region_info_user)))

  			return -EFAULT;

  		break;

  	}

--

On Wed, 20 Aug 2008 00:47:23 -0700
ug.
Putting a kernel pointer into a shared-with-userspace data structure

(struct mtd_erase_region_info) was a big mistake.
Copying a `struct region_info_user' back to userspace seems better than

copying a `struct mtd_erase_region_info', but what do I know?
Actually...
Before 0ecbc81adfcb9f15f86b05ff576b342ce81bbef8, `struct

mtd_erase_region_info' had three members, all u32.  We were copying

three u32's out to userspace.
After 0ecbc81adfcb9f15f86b05ff576b342ce81bbef8, `struct

mtd_erase_region_info' has four members: three u32's and one ulong*.

We're copying three u32's and one ulong* out to userspace.
After your change, we're copying _four_ u32's out to userspace, so

there still is potential for scribbling on unsuspecting userspace?
If that reading is right, we need to go back to copying just the three

u32's.  Perhaps via
struct mtd_erase_region_info {

	struct {

		u_int32_t offset;

		u_int32_t erasesize;

		u_int32_t numblocks;

	} user_part;

	unsigned long *lockmap;

};
or similar.
David?  Help?  2.6.25.x anmd 2.6.26.x need fixing as well.
--

Is the name "struct mtd_erase_region_info" something userspace uses?
Nothing wrong with changing the kernel's version, as long as userspace

uses different headers, but it might be confusing.
-- Jamie

--

Hmm.  Well, I may be misunderstanding what you're saying (again, I'm very much

a newbie to kernelspace), but I *think* the "copying four u32's out to

userspace" thing isn't really a problem with my patch.  It does certainly copy

those four u32's, but given that `ur' (struct mtd_region_info_user) is

initialized by copying from userspace, its fourth u32 (the `regionindex'

member) should be identical when copied back out to userspace, given that it's

not touched in the memberwise modification of the struct.  So yes, it is

copying 4 bytes more than is strictly necessary, but it seemed like a

reasonably clean way of going about it (to me, for what that's worth).
In my particular situation it didn't do anything unexpected in my testing (and

restored the normal behavior I had when previously running 2.6.17.7).
On the other hand, if I'm missing something completely, please let me know,

and perhaps I can prepare a more suitable fix.
Thanks,

Zev

--

OK, that's fortuitously bug-free in single-threaded userspace but

fantastically-improbably-buggy if userspace is threaded.
"good enough" is never good enough ;)
What is the ideal implementation?  Let's implement that.
--

It involves ditching the ioctls altogether (well, not adding anything

_more_ to them, at least) and doing it through sysfs.
--

David Woodhouse                            Open Source Technology Centre

David.Woodhouse@intel.com                              Intel Corporation
--

Andrew Morton wrote:

 > On Sat, 23 Aug 2008 01:10:21 -0700 Zev Weiss <zevweiss@gmail.com> wrote:

 >

 >> Hmm.  Well, I may be misunderstanding what you're saying (again, I'm very much

 >> a newbie to kernelspace), but I *think* the "copying four u32's out to

 >> userspace" thing isn't really a problem with my patch.  It does certainly copy

 >> those four u32's, but given that `ur' (struct mtd_region_info_user) is

 >> initialized by copying from userspace, its fourth u32 (the `regionindex'

 >> member) should be identical when copied back out to userspace, given that it's

 >> not touched in the memberwise modification of the struct.

 >

 > OK, that's fortuitously bug-free in single-threaded userspace but

 > fantastically-improbably-buggy if userspace is threaded.

 >

 > But it's something the kernel shouldn't be doing.

 >
Ah, good point -- that hadn't occurred to me at all.  Though it looks pretty

clumsy/simpleminded to me, I guess something like this would avoid copying and

rewriting the fourth u32 ("Well, duh" may be the appropriate response here):
diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c

index 13cc67a..424f318 100644

--- a/drivers/mtd/mtdchar.c

+++ b/drivers/mtd/mtdchar.c

@@ -410,16 +410,20 @@ static int mtd_ioctl(struct inode *inode, struct file *file,
         case MEMGETREGIONINFO:

         {

-               struct region_info_user ur;

+               u32 ur_idx;

+               struct mtd_erase_region_info *kr;

+               struct region_info_user *ur = (struct region_info_user *) argp;
-               if (copy_from_user(&ur, argp, sizeof(struct region_info_user)))

+               if (get_user(ur_idx,&(ur->regionindex)))

                         return -EFAULT;
-               if (ur.regionindex >= mtd->numeraseregions)

-                       return -EINVAL;

-               if (copy_to_user(argp, &(mtd->eraseregions[ur.regionindex]),

-              ...
[ message continues ]

Well, for what little it's probably worth, I've now tested this patch, with no

resulting surprises (seems to work fine for me).  Though I see now that, much to

my chagrin, something in my email chain seems to have spacified my code.  If

there's any need I can resend appropriately.
Zev

--

Please. With a signed-off-by.
--

David Woodhouse                            Open Source Technology Centre

David.Woodhouse@intel.com                              Intel Corporation
--

From: Zev Weiss <zevweiss@gmail.com>

Date: Mon, 1 Sep 2008 05:02:12 -0700

Subject: [PATCH] [MTD] mtdchar.c: Fix regression in MEMGETREGIONINFO ioctl()
The MEMGETREGIONINFO ioctl() in mtdchar.c was clobbering user memory by

overwriting more than intended, due the size of struct mtd_erase_region_info

changing in commit 0ecbc81adfcb9f15f86b05ff576b342ce81bbef8.
Fix avoids this by copying struct members one by one with put_user(), as there

is no longer a convenient struct to use the size of as the length argument to

copy_to_user().
Signed-off-by: Zev Weiss <zevweiss@gmail.com>

---

  drivers/mtd/mtdchar.c |   16 ++++++++++------

  1 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c

index 13cc67a..424f318 100644

--- a/drivers/mtd/mtdchar.c

+++ b/drivers/mtd/mtdchar.c

@@ -410,16 +410,20 @@ static int mtd_ioctl(struct inode *inode, struct file *file,
  	case MEMGETREGIONINFO:

  	{

-		struct region_info_user ur;

+		u32 ur_idx;

+		struct mtd_erase_region_info *kr;

+		struct region_info_user *ur = (struct region_info_user *) argp;
-		if (copy_from_user(&ur, argp, sizeof(struct region_info_user)))

+		if (get_user(ur_idx,&(ur->regionindex)))

  			return -EFAULT;
-		if (ur.regionindex >= mtd->numeraseregions)

-			return -EINVAL;

-		if (copy_to_user(argp, &(mtd->eraseregions[ur.regionindex]),

-				sizeof(struct mtd_erase_region_info)))

+		kr = &(mtd->eraseregions[ur_idx]);

+

+		if (put_user(kr->offset, &(ur->offset))

+		    || put_user(kr->erasesize, &(ur->erasesize))

+		    || put_user(kr->numblocks, &(ur->numblocks)))

  			return -EFAULT;

+

  		break;

  	}
--

1.5.4.1
--

Thanks. Your patch was whitespace-damaged, but I managed to apply it

anyway. Please check for future patches though -- try sending patches to

yourself and then see if they get mangled. I believe gmail is known to

be broken unless you submit mail with SMTP.
I also try to avoid the pointless types like 'u32' in MTD code -- the C

language has perfectly good explicitly sized types; let's use them.
--

David Woodhouse                            Open Source Technology Centre

David.Woodhouse@intel.com                              Intel Corporation
--