[PATCH] [MTD] mtdchar.c: Fix regression in MEMGETREGIONINFO ioctl()

[PATCH] [MTD] mtdchar.c: Fix regression in MEMGETREGIONINFO ioctl()

[Zev Weiss]

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;
  	}

Re: [PATCH] [MTD] mtdchar.c: Fix regression in MEMGETREGIONINFO ioctl()

[Andrew Morton]

On Wed, 20 Aug 2008 00:47:23 -0700
Zev Weiss <zevweiss@gmail.com> wrote:

> 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;
>   	}

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.

Re: [PATCH] [MTD] mtdchar.c: Fix regression in MEMGETREGIONINFO ioctl()

[Andrew Morton]

On Wed, 20 Aug 2008 00:47:23 -0700
Zev Weiss <zevweiss@gmail.com> wrote:

> 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;
>   	}

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.

Re: [PATCH] [MTD] mtdchar.c: Fix regression in MEMGETREGIONINFO ioctl()

[Zev Weiss]

Andrew Morton wrote:
> On Wed, 20 Aug 2008 00:47:23 -0700
> Zev Weiss <zevweiss@gmail.com> wrote:
> 
>> 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;
>>   	}
> 
> 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.
> 
> 

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

Re: [PATCH] [MTD] mtdchar.c: Fix regression in MEMGETREGIONINFO ioctl()

[Zev Weiss]

Andrew Morton wrote:
> On Wed, 20 Aug 2008 00:47:23 -0700
> Zev Weiss <zevweiss@gmail.com> wrote:
> 
>> 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;
>>   	}
> 
> 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.
> 
> 

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

Re: [PATCH] [MTD] mtdchar.c: Fix regression in MEMGETREGIONINFO ioctl()

[Andrew Morton]

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

> Andrew Morton wrote:
> > On Wed, 20 Aug 2008 00:47:23 -0700
> > Zev Weiss <zevweiss@gmail.com> wrote:
> > 
> >> 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;
> >>   	}
> > 
> > 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.
> > 
> > 
> 
> 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.

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

"good enough" is never good enough ;)

What is the ideal implementation?  Let's implement that.

Re: [PATCH] [MTD] mtdchar.c: Fix regression in MEMGETREGIONINFO ioctl()

[Andrew Morton]

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

> Andrew Morton wrote:
> > On Wed, 20 Aug 2008 00:47:23 -0700
> > Zev Weiss <zevweiss@gmail.com> wrote:
> > 
> >> 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;
> >>   	}
> > 
> > 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.
> > 
> > 
> 
> 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.

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

"good enough" is never good enough ;)

What is the ideal implementation?  Let's implement that.

Re: [PATCH] [MTD] mtdchar.c: Fix regression in MEMGETREGIONINFO ioctl()

[Zev Weiss]

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]),
-                               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;
         }


[Note: this is not even so much as compile-tested, and will remain that way
until Monday, but I think you get the picture.]

 >>  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.
 >
 > "good enough" is never good enough ;)
 >
 > What is the ideal implementation?  Let's implement that.
 >
 >

Well that, I'm afraid, is a question that I think would require somewhat
greater depth of understanding than I possess (and I very much doubt the above
patch is it).  Given what you said earlier, it seems like the ideal solution
would involve a reworking of the patch that added the `lockmap' member to
struct mtd_erase_region_info, but I'm probably not the person to be messing
around with that.  So...anyone-who-knows-mtd-better-than-I, care to comment?

Re: [PATCH] [MTD] mtdchar.c: Fix regression in MEMGETREGIONINFO ioctl()

[Zev Weiss]

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]),
-                               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;
         }


[Note: this is not even so much as compile-tested, and will remain that way
until Monday, but I think you get the picture.]

 >>  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.
 >
 > "good enough" is never good enough ;)
 >
 > What is the ideal implementation?  Let's implement that.
 >
 >

Well that, I'm afraid, is a question that I think would require somewhat
greater depth of understanding than I possess (and I very much doubt the above
patch is it).  Given what you said earlier, it seems like the ideal solution
would involve a reworking of the patch that added the `lockmap' member to
struct mtd_erase_region_info, but I'm probably not the person to be messing
around with that.  So...anyone-who-knows-mtd-better-than-I, care to comment?

Re: [PATCH] [MTD] mtdchar.c: Fix regression in MEMGETREGIONINFO ioctl()

[Zev Weiss]

Zev Weiss wrote:
> 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]),
> -                               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;
>         }
> 
> 
> [Note: this is not even so much as compile-tested, and will remain that way
> until Monday, but I think you get the picture.]
> 

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

Re: [PATCH] [MTD] mtdchar.c: Fix regression in MEMGETREGIONINFO ioctl()

[Zev Weiss]

Zev Weiss wrote:
> 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]),
> -                               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;
>         }
> 
> 
> [Note: this is not even so much as compile-tested, and will remain that way
> until Monday, but I think you get the picture.]
> 

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

Re: [PATCH] [MTD] mtdchar.c: Fix regression in MEMGETREGIONINFO ioctl()

[David Woodhouse]

On Tue, 2008-08-26 at 21:31 -0700, Zev Weiss wrote:
> 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.

Please. With a signed-off-by.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH] [MTD] mtdchar.c: Fix regression in MEMGETREGIONINFO ioctl()

[David Woodhouse]

On Tue, 2008-08-26 at 21:31 -0700, Zev Weiss wrote:
> 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.

Please. With a signed-off-by.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH] [MTD] mtdchar.c: Fix regression in MEMGETREGIONINFO ioctl()

[Zev Weiss]

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

Re: [PATCH] [MTD] mtdchar.c: Fix regression in MEMGETREGIONINFO ioctl()

[Zev Weiss]

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

Re: [PATCH] [MTD] mtdchar.c: Fix regression in MEMGETREGIONINFO ioctl()

[David Woodhouse]

On Mon, 2008-09-01 at 05:19 -0700, Zev Weiss wrote:
> 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>

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

Re: [PATCH] [MTD] mtdchar.c: Fix regression in MEMGETREGIONINFO ioctl()

[David Woodhouse]

On Mon, 2008-09-01 at 05:19 -0700, Zev Weiss wrote:
> 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>

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

Re: [PATCH] [MTD] mtdchar.c: Fix regression in MEMGETREGIONINFO ioctl()

[David Woodhouse]

On Sat, 2008-08-23 at 22:27 -0700, Andrew Morton wrote:
> 
> > On the other hand, if I'm missing something completely, please let me know,
> > and perhaps I can prepare a more suitable fix.
> 
> "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

Re: [PATCH] [MTD] mtdchar.c: Fix regression in MEMGETREGIONINFO ioctl()

[David Woodhouse]

On Sat, 2008-08-23 at 22:27 -0700, Andrew Morton wrote:
> 
> > On the other hand, if I'm missing something completely, please let me know,
> > and perhaps I can prepare a more suitable fix.
> 
> "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

Re: [PATCH] [MTD] mtdchar.c: Fix regression in MEMGETREGIONINFO ioctl()

[Jamie Lokier]

Andrew Morton wrote:
> struct mtd_erase_region_info {
> 	struct {
> 		u_int32_t offset;
> 		u_int32_t erasesize;
> 		u_int32_t numblocks;
> 	} user_part;
> 	unsigned long *lockmap;
> };

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

Re: [PATCH] [MTD] mtdchar.c: Fix regression in MEMGETREGIONINFO ioctl()

[Jamie Lokier]

Andrew Morton wrote:
> struct mtd_erase_region_info {
> 	struct {
> 		u_int32_t offset;
> 		u_int32_t erasesize;
> 		u_int32_t numblocks;
> 	} user_part;
> 	unsigned long *lockmap;
> };

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