gmane.linux.kernel

Zev Weiss | 20 Aug 09:47

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

From: Zev Weiss <zevweiss <at> 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 <at> gmail.com>
Tested-by: Zev Weiss <zevweiss <at> 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:

(Continue reading)

Andrew Morton | 23 Aug 00:34

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

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

> From: Zev Weiss <zevweiss <at> 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 <at> gmail.com>
> Tested-by: Zev Weiss <zevweiss <at> 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

(Continue reading)

Zev Weiss | 23 Aug 10:10

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

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

(Continue reading)

Andrew Morton | 24 Aug 07:27

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

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

> Andrew Morton wrote:
> > On Wed, 20 Aug 2008 00:47:23 -0700
> > Zev Weiss <zevweiss <at> gmail.com> wrote:
> > 
> >> From: Zev Weiss <zevweiss <at> 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 <at> gmail.com>
> >> Tested-by: Zev Weiss <zevweiss <at> 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(-)
> >>

(Continue reading)

Zev Weiss | 24 Aug 13:01

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

Andrew Morton wrote:
 > On Sat, 23 Aug 2008 01:10:21 -0700 Zev Weiss <zevweiss <at> 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;

(Continue reading)

Zev Weiss | 27 Aug 06:31

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


Zev Weiss wrote:
> Andrew Morton wrote:
>  > On Sat, 23 Aug 2008 01:10:21 -0700 Zev Weiss <zevweiss <at> 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

(Continue reading)

David Woodhouse | 1 Sep 13:01

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

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 <at> intel.com                              Intel Corporation

Zev Weiss | 1 Sep 14:19

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

From: Zev Weiss <zevweiss <at> 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 <at> 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;

(Continue reading)

David Woodhouse | 1 Sep 19:25

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

On Mon, 2008-09-01 at 05:19 -0700, Zev Weiss wrote:
> From: Zev Weiss <zevweiss <at> 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 <at> 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 <at> intel.com                              Intel Corporation

David Woodhouse | 25 Aug 13:37

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

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 <at> intel.com                              Intel Corporation

Jamie Lokier | 24 Aug 14:38

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

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

Return

Return to gmane.linux.kernel.

Advertisement

Search Archive

Language

Change language

Options

Current view: Threads only / Showing only 20 lines / Not hiding cited text.
Change to All messages, whole messages, or hide cited text.

Post a message
NNTP Newsgroup
Classic Gmane web interface
XML

XML

RSS Feed
List Information

About Gmane