* Re: [KJ] [patch] potential data-corruption fix in md.c
2005-12-02 9:39 [KJ] [patch] potential data-corruption fix in md.c Jaco Kroon
@ 2005-12-02 17:38 ` Jesper Juhl
2005-12-02 18:57 ` Alexey Dobriyan
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Jesper Juhl @ 2005-12-02 17:38 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [KJ] [patch] potential data-corruption fix in md.c
2005-12-02 9:39 [KJ] [patch] potential data-corruption fix in md.c Jaco Kroon
2005-12-02 17:38 ` Jesper Juhl
@ 2005-12-02 18:57 ` Alexey Dobriyan
2005-12-02 19:19 ` Jaco Kroon
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Alexey Dobriyan @ 2005-12-02 18:57 UTC (permalink / raw)
To: kernel-janitors
[-- 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [KJ] [patch] potential data-corruption fix in md.c
2005-12-02 9:39 [KJ] [patch] potential data-corruption fix in md.c Jaco Kroon
2005-12-02 17:38 ` Jesper Juhl
2005-12-02 18:57 ` Alexey Dobriyan
@ 2005-12-02 19:19 ` Jaco Kroon
2005-12-02 19:45 ` Håkon Løvdal
2005-12-02 20:48 ` Jaco Kroon
4 siblings, 0 replies; 6+ messages in thread
From: Jaco Kroon @ 2005-12-02 19:19 UTC (permalink / raw)
To: kernel-janitors
[-- 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [KJ] [patch] potential data-corruption fix in md.c
2005-12-02 9:39 [KJ] [patch] potential data-corruption fix in md.c Jaco Kroon
` (2 preceding siblings ...)
2005-12-02 19:19 ` Jaco Kroon
@ 2005-12-02 19:45 ` Håkon Løvdal
2005-12-02 20:48 ` Jaco Kroon
4 siblings, 0 replies; 6+ messages in thread
From: Håkon Løvdal @ 2005-12-02 19:45 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [KJ] [patch] potential data-corruption fix in md.c
2005-12-02 9:39 [KJ] [patch] potential data-corruption fix in md.c Jaco Kroon
` (3 preceding siblings ...)
2005-12-02 19:45 ` Håkon Løvdal
@ 2005-12-02 20:48 ` Jaco Kroon
4 siblings, 0 replies; 6+ messages in thread
From: Jaco Kroon @ 2005-12-02 20:48 UTC (permalink / raw)
To: kernel-janitors
[-- 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
^ permalink raw reply [flat|nested] 6+ messages in thread