misaligned memory access in cmdlinepart.c

misaligned memory access in cmdlinepart.c

[Timofei V. Bondarenko]

Hi,

in mtdpart_setup_real()/newpart() command line parser
'this_mtd' structure can be misaligned,
it may cause exception on some kind of CPU.

That happened because the structure allocated in a variable length area 
and got mixed with partition names.

The patch follows.

Regards.
	Timofei.

--- cmdlinepart.c	2005-04-18 18:06:43.000000000 +0400
+++ linux-2.6.x/drivers/mtd/cmdlinepart.c	2005-04-18 18:40:02.971778640 
+0400
@@ -234,12 +234,14 @@ static int mtdpart_setup_real(char *s)
  		 * parse one mtd. have it reserve memory for the
  		 * struct cmdline_mtd_partition and the mtd-id string.
  		 */
+#define THIS_MTD_ALIGN_CONST (sizeof(void*)-1)
  		parts = newpart(p + 1,		/* cmdline */
  				&s,		/* out: updated cmdline ptr */
  				&num_parts,	/* out: number of parts */
  				0,		/* first partition */
  				(unsigned char**)&this_mtd, /* out: extra mem */
-				mtd_id_len + 1 + sizeof(*this_mtd));
+				mtd_id_len + 1 + sizeof(*this_mtd) +
+                                THIS_MTD_ALIGN_CONST);
  		if(!parts)
  		{
  			/*
@@ -252,7 +254,11 @@ static int mtdpart_setup_real(char *s)
  			 return 0;
  		 }

-		/* enter results */	
+		/* align this_mtd */
+                this_mtd = (struct cmdline_mtd_partition *)
+                  (~THIS_MTD_ALIGN_CONST &
+                    THIS_MTD_ALIGN_CONST + (unsigned long)(char*)this_mtd);
+		/* enter results */
  		this_mtd->parts = parts;
  		this_mtd->num_parts = num_parts;
  		this_mtd->mtd_id = (char*)(this_mtd + 1);

Re: misaligned memory access in cmdlinepart.c

[Timofei V. Bondarenko]

Hi.

27 Apr I've submitted the tiny patch.
No answer since then.

Did the patch rejected?
Or any chance it could be accepted?

--
Regards.
	Timofei.

Re: misaligned memory access in cmdlinepart.c

[Jörn Engel]

Thanks for bugging again.

On Wed, 27 April 2005 15:03:54 +0400, Timofei V. Bondarenko wrote:
> 
> in mtdpart_setup_real()/newpart() command line parser
> 'this_mtd' structure can be misaligned,
> it may cause exception on some kind of CPU.
> 
> That happened because the structure allocated in a variable length area 
> and got mixed with partition names.

Your description appears to make sense (I don't use cmdlinepart).  The
patch, however, should be changed to use ALIGN() from
include/linux/kernel.h instead.

> --- cmdlinepart.c	2005-04-18 18:06:43.000000000 +0400
> +++ linux-2.6.x/drivers/mtd/cmdlinepart.c	2005-04-18 
> 18:40:02.971778640 +0400
> @@ -234,12 +234,14 @@ static int mtdpart_setup_real(char *s)
>  		 * parse one mtd. have it reserve memory for the
>  		 * struct cmdline_mtd_partition and the mtd-id string.
>  		 */
> +#define THIS_MTD_ALIGN_CONST (sizeof(void*)-1)
>  		parts = newpart(p + 1,		/* cmdline */
>  				&s,		/* out: updated cmdline ptr 
>  				*/
>  				&num_parts,	/* out: number of parts */
>  				0,		/* first partition */
>  				(unsigned char**)&this_mtd, /* out: extra 
>  				mem */
> -				mtd_id_len + 1 + sizeof(*this_mtd));
> +				mtd_id_len + 1 + sizeof(*this_mtd) +
> +                                THIS_MTD_ALIGN_CONST);
>  		if(!parts)
>  		{
>  			/*
> @@ -252,7 +254,11 @@ static int mtdpart_setup_real(char *s)
>  			 return 0;
>  		 }
> 
> -		/* enter results */	
> +		/* align this_mtd */
> +                this_mtd = (struct cmdline_mtd_partition *)
> +                  (~THIS_MTD_ALIGN_CONST &
> +                    THIS_MTD_ALIGN_CONST + (unsigned long)(char*)this_mtd);
> +		/* enter results */
>  		this_mtd->parts = parts;
>  		this_mtd->num_parts = num_parts;
>  		this_mtd->mtd_id = (char*)(this_mtd + 1);
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Jörn

-- 
There are two ways of constructing a software design: one way is to make
it so simple that there are obviously no deficiencies, and the other is
to make it so complicated that there are no obvious deficiencies.
-- C. A. R. Hoare

Re: misaligned memory access in cmdlinepart.c

[Timofei V. Bondarenko]

Jörn Engel wrote:
> On Mon, 6 June 2005 18:01:16 +0400, Timofei V. Bondarenko wrote:
> 
>>Index: cmdlinepart.c
>>===================================================================
>>RCS file: /home/cvs/mtd/drivers/mtd/cmdlinepart.c,v
>>retrieving revision 1.17
>>diff -u -p -r1.17 cmdlinepart.c
>>--- cmdlinepart.c	26 Nov 2004 11:18:47 -0000	1.17
>>+++ cmdlinepart.c	6 Jun 2005 13:57:08 -0000
>>@@ -234,12 +234,14 @@ static int mtdpart_setup_real(char *s)
>> 		 * parse one mtd. have it reserve memory for the
>> 		 * struct cmdline_mtd_partition and the mtd-id string.
>> 		 */
>>+#define THIS_MTD_ALIGN_CONST sizeof(void*)
> 
> Not sure if this macro is worth the pain.  If it is, you should
> declare it outside the function.  Just above is what most people do.

Ok.. That's matter of taste. I'm rather like to complicate things.

>>+		/* align this_mtd */
>>+                this_mtd = (struct cmdline_mtd_partition *)
>>+                    ALIGN((unsigned long)this_mtd, THIS_MTD_ALIGN_CONST);
>>+		/* enter results */
> 
> 
> Gcc allows arithmetic with (void*) exactly for code like this:
> 		this_mtd = ALIGN((void*)this_mtd, THIS_MTD_ALIGN_CONST);
> or:
> 		this_mtd = ALIGN((void*)this_mtd, sizeof(void*));
> 
> You should retest it, though.

error: invalid operands to binary &

Then we've to cast it from integer to a pointer type.

--
	Timofei.


Index: cmdlinepart.c
===================================================================
RCS file: /home/cvs/mtd/drivers/mtd/cmdlinepart.c,v
retrieving revision 1.17
diff -u -p -r1.17 cmdlinepart.c
--- cmdlinepart.c	26 Nov 2004 11:18:47 -0000	1.17
+++ cmdlinepart.c	6 Jun 2005 14:33:59 -0000
@@ -239,7 +239,8 @@ static int mtdpart_setup_real(char *s)
  				&num_parts,	/* out: number of parts */
  				0,		/* first partition */
  				(unsigned char**)&this_mtd, /* out: extra mem */
-				mtd_id_len + 1 + sizeof(*this_mtd));
+				mtd_id_len + 1 + sizeof(*this_mtd) +
+                                sizeof(void*)-1 /*alignment*/);
  		if(!parts)
  		{
  			/*
@@ -252,7 +253,10 @@ static int mtdpart_setup_real(char *s)
  			 return 0;
  		 }

-		/* enter results */	
+		/* align this_mtd */
+                this_mtd = (struct cmdline_mtd_partition *)
+                    ALIGN((unsigned long)this_mtd, sizeof(void*));
+		/* enter results */
  		this_mtd->parts = parts;
  		this_mtd->num_parts = num_parts;
  		this_mtd->mtd_id = (char*)(this_mtd + 1);

Re: misaligned memory access in cmdlinepart.c

[Timofei V. Bondarenko]

Jörn Engel wrote:
> 
> Hmm.  Well, I just don't care enough, let's keep it.
> 
> Can you test the current patch and see if it works for you?  Once
> you've confirmed that, I'll commit it.

Yes. I've tested it on the uclinux-bf533.

--
	Timofei.

Re: misaligned memory access in cmdlinepart.c

[Jörn Engel]

On Mon, 6 June 2005 20:49:01 +0400, Timofei V. Bondarenko wrote:
> Jörn Engel wrote:
> >
> >Hmm.  Well, I just don't care enough, let's keep it.
> >
> >Can you test the current patch and see if it works for you?  Once
> >you've confirmed that, I'll commit it.
> 
> Yes. I've tested it on the uclinux-bf533.

Doesn't apply with patch -p1.  Can you respin the patch against mtd
cvs and make sure it does?  If in doubt, read:
http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt

Jörn

-- 
Fools ignore complexity.  Pragmatists suffer it.
Some can avoid it.  Geniuses remove it.
-- Perlis's Programming Proverb #58, SIGPLAN Notices, Sept.  1982

Re: misaligned memory access in cmdlinepart.c

[Timofei V. Bondarenko]

Jörn Engel wrote:
> On Mon, 6 June 2005 20:49:01 +0400, Timofei V. Bondarenko wrote:
> 
>>Jörn Engel wrote:
>>
>>>Hmm.  Well, I just don't care enough, let's keep it.
>>>
>>>Can you test the current patch and see if it works for you?  Once
>>>you've confirmed that, I'll commit it.
>>
>>Yes. I've tested it on the uclinux-bf533.
> 
> 
> Doesn't apply with patch -p1.  Can you respin the patch against mtd
> cvs and make sure it does?  If in doubt, read:
> http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt

Ok, sorry for noise.
Is it good enough?

diff -u -p -r1.17 cmdlinepart.c
--- mtd/drivers/mtd/cmdlinepart.c	26 Nov 2004 11:18:47 -0000	1.17
+++ mtd/drivers/mtd/cmdlinepart.c	7 Jun 2005 08:02:00 -0000
@@ -239,7 +239,8 @@ static int mtdpart_setup_real(char *s)
  				&num_parts,	/* out: number of parts */
  				0,		/* first partition */
  				(unsigned char**)&this_mtd, /* out: extra mem */
-				mtd_id_len + 1 + sizeof(*this_mtd));
+				mtd_id_len + 1 + sizeof(*this_mtd) +
+                                sizeof(void*)-1 /*alignment*/);
  		if(!parts)
  		{
  			/*
@@ -252,7 +253,10 @@ static int mtdpart_setup_real(char *s)
  			 return 0;
  		 }

-		/* enter results */	
+		/* align this_mtd */
+                this_mtd = (struct cmdline_mtd_partition *)
+                    ALIGN((unsigned long)this_mtd, sizeof(void*));
+		/* enter results */
  		this_mtd->parts = parts;
  		this_mtd->num_parts = num_parts;
  		this_mtd->mtd_id = (char*)(this_mtd + 1);

Re: misaligned memory access in cmdlinepart.c

[Vitaly Wool]

Hi Timofei,
I guess you should use a mail client that doesn't expand tabs to spaces, 
like sylpheed or pine.

Best regards,
   Vitaly

Timofei V. Bondarenko wrote:

> JЖrn Engel wrote:
>
>> On Mon, 6 June 2005 20:49:01 +0400, Timofei V. Bondarenko wrote:
>>
>>> JЖrn Engel wrote:
>>>
>>>> Hmm.  Well, I just don't care enough, let's keep it.
>>>>
>>>> Can you test the current patch and see if it works for you?  Once
>>>> you've confirmed that, I'll commit it.
>>>
>>>
>>> Yes. I've tested it on the uclinux-bf533.
>>
>>
>>
>> Doesn't apply with patch -p1.  Can you respin the patch against mtd
>> cvs and make sure it does?  If in doubt, read:
>> http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt
>
>
> Ok, sorry for noise.
> Is it good enough?
>
> diff -u -p -r1.17 cmdlinepart.c
> --- mtd/drivers/mtd/cmdlinepart.c    26 Nov 2004 11:18:47 -0000    1.17
> +++ mtd/drivers/mtd/cmdlinepart.c    7 Jun 2005 08:02:00 -0000
> @@ -239,7 +239,8 @@ static int mtdpart_setup_real(char *s)
>                  &num_parts,    /* out: number of parts */
>                  0,        /* first partition */
>                  (unsigned char**)&this_mtd, /* out: extra mem */
> -                mtd_id_len + 1 + sizeof(*this_mtd));
> +                mtd_id_len + 1 + sizeof(*this_mtd) +
> +                                sizeof(void*)-1 /*alignment*/);
>          if(!parts)
>          {
>              /*
> @@ -252,7 +253,10 @@ static int mtdpart_setup_real(char *s)
>               return 0;
>           }
>
> -        /* enter results */   
> +        /* align this_mtd */
> +                this_mtd = (struct cmdline_mtd_partition *)
> +                    ALIGN((unsigned long)this_mtd, sizeof(void*));
> +        /* enter results */
>          this_mtd->parts = parts;
>          this_mtd->num_parts = num_parts;
>          this_mtd->mtd_id = (char*)(this_mtd + 1);
>
>
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>
>

Re: misaligned memory access in cmdlinepart.c

[Timofei V. Bondarenko]

Vitaly Wool wrote:
> Hi Timofei,
> I guess you should use a mail client that doesn't expand tabs to spaces, 
> like sylpheed or pine.
> 
> Best regards,
>   Vitaly
> 
> Timofei V. Bondarenko wrote:
> 
>> JЖrn Engel wrote:
>>
>>> On Mon, 6 June 2005 20:49:01 +0400, Timofei V. Bondarenko wrote:
>>>
>>>> JЖrn Engel wrote:
>>>>
>>>>> Hmm.  Well, I just don't care enough, let's keep it.
>>>>>
>>>>> Can you test the current patch and see if it works for you?  Once
>>>>> you've confirmed that, I'll commit it.
>>>>
>>>> Yes. I've tested it on the uclinux-bf533.
>>>
>>>
>>> Doesn't apply with patch -p1.  Can you respin the patch against mtd
>>> cvs and make sure it does?  If in doubt, read:
>>> http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt
>>
>> Ok, sorry for noise.
>> Is it good enough?

Ohh. That's not silly mail, that's poor me.


diff -u -p -r1.17 cmdlinepart.c
--- mtd/drivers/mtd/cmdlinepart.c	26 Nov 2004 11:18:47 -0000	1.17
+++ mtd/drivers/mtd/cmdlinepart.c	7 Jun 2005 08:02:00 -0000
@@ -239,7 +239,8 @@ static int mtdpart_setup_real(char *s)
  				&num_parts,	/* out: number of parts */
  				0,		/* first partition */
  				(unsigned char**)&this_mtd, /* out: extra mem */
-				mtd_id_len + 1 + sizeof(*this_mtd));
+				mtd_id_len + 1 + sizeof(*this_mtd) +
+				sizeof(void*)-1 /*alignment*/);
  		if(!parts)
  		{
  			/*
@@ -252,7 +253,10 @@ static int mtdpart_setup_real(char *s)
  			 return 0;
  		 }

-		/* enter results */	
+		/* align this_mtd */
+		this_mtd = (struct cmdline_mtd_partition *)
+		    ALIGN((unsigned long)this_mtd, sizeof(void*));
+		/* enter results */
  		this_mtd->parts = parts;
  		this_mtd->num_parts = num_parts;
  		this_mtd->mtd_id = (char*)(this_mtd + 1);

Re: misaligned memory access in cmdlinepart.c

[Timofei V. Bondarenko]

On Tue, 2005-06-07 at 13:01 +0400, Vitaly Wool wrote:
> Hi Timofei,
> I guess you should use a mail client that doesn't expand tabs to spaces, 
> like sylpheed or pine.
> 
> >> JЖrn Engel wrote:
> >>
> >> Doesn't apply with patch -p1.  Can you respin the patch against mtd
> >> cvs and make sure it does?  If in doubt, read:
> >> http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt

I'm apologize for my unusual inaccuracy.
Now, the tabs are preserved; patch -p1 looks happy.
For cvslog:

---
the this_mtd structure should be aligned
---

diff -u -p -r1.17 cmdlinepart.c
--- mtd/drivers/mtd/cmdlinepart.c	26 Nov 2004 11:18:47 -0000	1.17
+++ mtd/drivers/mtd/cmdlinepart.c	7 Jun 2005 08:02:00 -0000
@@ -239,7 +239,8 @@ static int mtdpart_setup_real(char *s)
 				&num_parts,	/* out: number of parts */
 				0,		/* first partition */
 				(unsigned char**)&this_mtd, /* out: extra mem */
-				mtd_id_len + 1 + sizeof(*this_mtd));
+				mtd_id_len + 1 + sizeof(*this_mtd) + 
+				sizeof(void*)-1 /*alignment*/);
 		if(!parts)
 		{
 			/*
@@ -252,7 +253,10 @@ static int mtdpart_setup_real(char *s)
 			 return 0;
 		 }
 
-		/* enter results */	    
+		/* align this_mtd */
+		this_mtd = (struct cmdline_mtd_partition *) 
+		     ALIGN((unsigned long)this_mtd, sizeof(void*));
+		/* enter results */
 		this_mtd->parts = parts;
 		this_mtd->num_parts = num_parts;
 		this_mtd->mtd_id = (char*)(this_mtd + 1);

Re: misaligned memory access in cmdlinepart.c

[Jörn Engel]

On Tue, 7 June 2005 12:30:13 +0400, Timofei V. Bondarenko wrote:
> 
> Ok, sorry for noise.
> Is it good enough?

Still broken.  You have some whitespace noise in it.

I could easily fix it up, but I feel like a bastard and prefer to
share the pain.  If your mail reader is screwing it up, you can send
the patch as an attachment.  Inline would be preferred, though.

> diff -u -p -r1.17 cmdlinepart.c
> --- mtd/drivers/mtd/cmdlinepart.c	26 Nov 2004 11:18:47 -0000	1.17
> +++ mtd/drivers/mtd/cmdlinepart.c	7 Jun 2005 08:02:00 -0000
> @@ -239,7 +239,8 @@ static int mtdpart_setup_real(char *s)
>  				&num_parts,	/* out: number of parts */
>  				0,		/* first partition */
>  				(unsigned char**)&this_mtd, /* out: extra 
>  				mem */
> -				mtd_id_len + 1 + sizeof(*this_mtd));
> +				mtd_id_len + 1 + sizeof(*this_mtd) +
> +                                sizeof(void*)-1 /*alignment*/);
>  		if(!parts)
>  		{
>  			/*
> @@ -252,7 +253,10 @@ static int mtdpart_setup_real(char *s)
>  			 return 0;
>  		 }
> 
> -		/* enter results */	
> +		/* align this_mtd */
> +                this_mtd = (struct cmdline_mtd_partition *)
> +                    ALIGN((unsigned long)this_mtd, sizeof(void*));
> +		/* enter results */
>  		this_mtd->parts = parts;
>  		this_mtd->num_parts = num_parts;
>  		this_mtd->mtd_id = (char*)(this_mtd + 1);
> 
> 

Jörn

-- 
I've never met a human being who would want to read 17,000 pages of
documentation, and if there was, I'd kill him to get him out of the
gene pool.
-- Joseph Costello