[KJ] [patch] potential data-corruption fix in md.c

[KJ] [patch] potential data-corruption fix in md.c

[Jaco Kroon]

Fix a check whereby a negative personality number passed to either
register_md_personality or unregister_md_personality could potentially
cause data corruption.

Signed off:  Jaco Kroon <jaco@kroon.co.za>

--- linux-2.6.14/drivers/md/md.c.orig	2005-12-02 11:31:59.000000000 +0200
+++ linux-2.6.14/drivers/md/md.c	2005-12-02 11:32:37.000000000 +0200
@@ -3425,7 +3425,7 @@

 int register_md_personality(int pnum, mdk_personality_t *p)
 {
-	if (pnum >= MAX_PERSONALITY) {
+	if (pnum < 0 || pnum >= MAX_PERSONALITY) {
 		printk(KERN_ERR
 		       "md: tried to install personality %s as nr %d, but max is %lu\n",
 		       p->name, pnum, MAX_PERSONALITY-1);
@@ -3446,7 +3446,7 @@

 int unregister_md_personality(int pnum)
 {
-	if (pnum >= MAX_PERSONALITY)
+	if (pnum < 0 || pnum >= MAX_PERSONALITY)
 		return -EINVAL;

 	printk(KERN_INFO "md: %s personality unregistered\n", pers[pnum]->name);

-- 
There are 10 kinds of people in the world, those who understand binary,
and those who don't.
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [patch] potential data-corruption fix in md.c

[Jesper Juhl]

On 12/2/05, Jaco Kroon <jaco@kroon.co.za> wrote:
> Fix a check whereby a negative personality number passed to either
> register_md_personality or unregister_md_personality could potentially
> cause data corruption.
>
> Signed off:  Jaco Kroon <jaco@kroon.co.za>
>
> --- linux-2.6.14/drivers/md/md.c.orig   2005-12-02 11:31:59.000000000 +0200
> +++ linux-2.6.14/drivers/md/md.c        2005-12-02 11:32:37.000000000 +0200
> @@ -3425,7 +3425,7 @@
>
>  int register_md_personality(int pnum, mdk_personality_t *p)
>  {
> -       if (pnum >= MAX_PERSONALITY) {
> +       if (pnum < 0 || pnum >= MAX_PERSONALITY) {
>                 printk(KERN_ERR
>                        "md: tried to install personality %s as nr %d, but max is %lu\n",
>                        p->name, pnum, MAX_PERSONALITY-1);
> @@ -3446,7 +3446,7 @@
>
>  int unregister_md_personality(int pnum)
>  {
> -       if (pnum >= MAX_PERSONALITY)
> +       if (pnum < 0 || pnum >= MAX_PERSONALITY)
>                 return -EINVAL;
>

MAX_PERSONALITY that pnum is compared to is defined as

#define MAX_PERSONALITY   11UL

so might it make more sense to just switch pnum to be an "unsigned
long" instead of adding <0 checks?

I haven't read the code in question though, so I may be way off - just
a thought...


--
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [patch] potential data-corruption fix in md.c

[Alexey Dobriyan]

[-- Attachment #1: Type: text/plain, Size: 1272 bytes --]

On Fri, Dec 02, 2005 at 06:38:53PM +0100, Jesper Juhl wrote:
> On 12/2/05, Jaco Kroon <jaco@kroon.co.za> wrote:
> > Fix a check whereby a negative personality number passed to either
> > register_md_personality or unregister_md_personality could potentially
> > cause data corruption.

> > --- linux-2.6.14/drivers/md/md.c.orig
> > +++ linux-2.6.14/drivers/md/md.c
> > @@ -3425,7 +3425,7 @@
> >
> >  int register_md_personality(int pnum, mdk_personality_t *p)
> >  {
> > -       if (pnum >= MAX_PERSONALITY) {
> > +       if (pnum < 0 || pnum >= MAX_PERSONALITY) {
> >                 printk(KERN_ERR
> >                        "md: tried to install personality %s as nr %d, but max is %lu\n",
> >                        p->name, pnum, MAX_PERSONALITY-1);
> > @@ -3446,7 +3446,7 @@
> >
> >  int unregister_md_personality(int pnum)
> >  {
> > -       if (pnum >= MAX_PERSONALITY)
> > +       if (pnum < 0 || pnum >= MAX_PERSONALITY)
> >                 return -EINVAL;
> >
>
> MAX_PERSONALITY that pnum is compared to is defined as
>
> #define MAX_PERSONALITY   11UL
>
> so might it make more sense to just switch pnum to be an "unsigned
> long" instead of adding <0 checks?

It is int in other md files. So, one may want to change int to bitwised
enum. Or add the check.


[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [patch] potential data-corruption fix in md.c

[Jaco Kroon]

[-- Attachment #1.1: Type: text/plain, Size: 1580 bytes --]

Alexey Dobriyan wrote:
> On Fri, Dec 02, 2005 at 06:38:53PM +0100, Jesper Juhl wrote:
> 
>>On 12/2/05, Jaco Kroon <jaco@kroon.co.za> wrote:
>>
>>>Fix a check whereby a negative personality number passed to either
>>>register_md_personality or unregister_md_personality could potentially
>>>cause data corruption.
> 
> 
>>>--- linux-2.6.14/drivers/md/md.c.orig
>>>+++ linux-2.6.14/drivers/md/md.c
>>>@@ -3425,7 +3425,7 @@
>>>
>>> int register_md_personality(int pnum, mdk_personality_t *p)
>>> {
>>>-       if (pnum >= MAX_PERSONALITY) {
>>>+       if (pnum < 0 || pnum >= MAX_PERSONALITY) {
>>>                printk(KERN_ERR
>>>                       "md: tried to install personality %s as nr %d, but max is %lu\n",
>>>                       p->name, pnum, MAX_PERSONALITY-1);
>>>@@ -3446,7 +3446,7 @@
>>>
>>> int unregister_md_personality(int pnum)
>>> {
>>>-       if (pnum >= MAX_PERSONALITY)
>>>+       if (pnum < 0 || pnum >= MAX_PERSONALITY)
>>>                return -EINVAL;
>>>
>>
>>MAX_PERSONALITY that pnum is compared to is defined as
>>
>>#define MAX_PERSONALITY   11UL
>>
>>so might it make more sense to just switch pnum to be an "unsigned
>>long" instead of adding <0 checks?
> 
> 
> It is int in other md files. So, one may want to change int to bitwised
> enum. Or add the check.

Or just cast to (unsigned int) for the check?
Perhaps that happens implicitly?
Would that be compiler dependant?
Do we care about anything but gcc?

-- 
There are only 10 kinds of people in this world,
  those that understand binary and those that don't.
http://www.kroon.co.za/

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/x-pkcs7-signature, Size: 3166 bytes --]

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [patch] potential data-corruption fix in md.c

[Håkon Løvdal]

On 12/2/05, Jaco Kroon <jaco@kroon.co.za> wrote:
> Or just cast to (unsigned int) for the check?
> Perhaps that happens implicitly?
> Would that be compiler dependant?

The conversion does indeed happen implicit, and the C standard
(ISO/IEC 9899:1990) does very specific specify how this should be done,
see "6.2.1.5 Usual arithmetic conversions" for details.

The short version is that a smallest type is promoted to the biggest type
as well that unsigned "wins" over signed. The last point can result in
some unexpected results if you are not careful.
Example: What does the following program print?

#include <stdio.h>
int main(int argc, char *argv[])
{
        unsigned int i = 12345;
        if (i < -1)
                printf("(%d < -1)\n", i);
        else
                printf("! (%d < -1)\n", i);
        return 0;
}

BR Håkon Løvdal

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [patch] potential data-corruption fix in md.c

[Jaco Kroon]

[-- Attachment #1.1: Type: text/plain, Size: 1994 bytes --]

Håkon Løvdal wrote:
> On 12/2/05, Jaco Kroon <jaco@kroon.co.za> wrote:
> 
>>Or just cast to (unsigned int) for the check?
>>Perhaps that happens implicitly?
>>Would that be compiler dependant?
> 
> 
> The conversion does indeed happen implicit, and the C standard
> (ISO/IEC 9899:1990) does very specific specify how this should be done,
> see "6.2.1.5 Usual arithmetic conversions" for details.
> 
> The short version is that a smallest type is promoted to the biggest type
> as well that unsigned "wins" over signed. The last point can result in
> some unexpected results if you are not careful.
> Example: What does the following program print?
> 
> #include <stdio.h>
> int main(int argc, char *argv[])
> {
>         unsigned int i = 12345;
>         if (i < -1)
>                 printf("(%d < -1)\n", i);
>         else
>                 printf("! (%d < -1)\n", i);
>         return 0;
> }

jkroon@pug tmp $ gcc -o signed_unsigned signed_unsigned.c
jkroon@pug tmp $ gcc -o signed_unsigned signed_unsigned.c -Wall -W
signed_unsigned.c: In function `main':
signed_unsigned.c:5: warning: comparison between signed and unsigned
signed_unsigned.c: At top level:
signed_unsigned.c:2: warning: unused parameter 'argc'
signed_unsigned.c:2: warning: unused parameter 'argv'
jkroon@pug tmp $ ./signed_unsigned
(12345 < -1)
jkroon@pug tmp $

Which means that gcc is smart enough to recognise that you are doing
something unusual, and 12345 is indeed < -1 due to the fact that -1 is
in fact 2 ^ 32 - 1.  As such the test in register_md_personality with
pnum >= MAX_PERSONALITY is in fact sufficient since MAX_PERSONALITY is
unsigned, so pnum gets converted to unsigned (negative values becomes
extremely large) and as such the <0 is done "by accident".

Very, very nice, but extremely unclear from the code :).

Jaco
-- 
There are only 10 kinds of people in this world,
  those that understand binary and those that don't.
http://www.kroon.co.za/

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/x-pkcs7-signature, Size: 3166 bytes --]

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors