Skip to content Skip to conversation

Improve 'register_handler_proc()' and 'register_irq_proc()'

KJ Christophe Jaillet 3 participants 3 messages Feb 17, 2007 – Feb 23, 2007 Kernel janitors

Share & tools

Christophe Jaillet

author

2007-02-17 21:04 UTC #0 lore raw

In file /irq/proc.c, function 'register_handler_proc()' and 'register_irq_proc()' memset a local variable 'name[MAX_NAMELEN]'

I don't really see the point of this memset.

In 'register_handler_proc()' :

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

char name [MAX_NAMELEN];
[...]
memset(name, 0, MAX_NAMELEN);
snprintf(name, MAX_NAMELEN, "%s", action->name);
[...] proc_mkdir(name, ...);

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

In 'register_handler_proc()' :

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

char name [MAX_NAMELEN];
[...]
memset(name, 0, MAX_NAMELEN);
sprintf(name, "%d", irq);
[...] proc_mkdir(name, ...);

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

Here are my remarks :

1) I think that both function should use 'snprintf', just to be sure. Actualy only 'register_handler_proc()' does

2) I think that the memset are completely useless and could make s.o. think that the 'name' variables are correctly cleanned and NULL terminated. But,
- In 'register_handler_proc()', the use of snprintf can lead to name beeing not NULL terminated !
- In 'register_irq_proc()', the use of sprintf can lead to access arbitrary data in the stack (well if the variable irq is 2^30 = 2147483648, then 11 chars are used and MAX_NAMELEN is only 10 !

3) So I think that code like above should be better in both cases :

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

char name [MAX_NAMELEN + 1];
[...]
snprintf(name, MAX_NAMELEN, "%d", irq);
name[MAX_NAMELEN] = '\0';
[...] proc_mkdir(name, ...);

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

Any comments ?

Christophe Jaillet

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Alexey Dobriyan

2007-02-22 23:55 UTC #1 lore raw

Re: [KJ] Improve 'register_handler_proc()' and

Quoted 33 lines · Christophe Jaillet

In file /irq/proc.c, function 'register_handler_proc()' and 'register_irq_proc()' memset a local variable 'name[MAX_NAMELEN]'
I don't really see the point of this memset.
In 'register_handler_proc()' :
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
char name [MAX_NAMELEN];
[...]
memset(name, 0, MAX_NAMELEN);
snprintf(name, MAX_NAMELEN, "%s", action->name);
[...] proc_mkdir(name, ...);
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
In 'register_handler_proc()' :
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
char name [MAX_NAMELEN];
[...]
memset(name, 0, MAX_NAMELEN);
sprintf(name, "%d", irq);
[...] proc_mkdir(name, ...);
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Here are my remarks :
1) I think that both function should use 'snprintf', just to be sure. Actualy only 'register_handler_proc()' does
2) I think that the memset are completely useless and could make s.o. think that the 'name' variables are correctly cleanned and NULL terminated. But,
- In 'register_handler_proc()', the use of snprintf can lead to name beeing not NULL terminated !
- In 'register_irq_proc()', the use of sprintf can lead to access arbitrary data in the stack (well if the variable irq is 2^30 = 2147483648, then 11 chars are used and MAX_NAMELEN is only 10 !

Quoted 8 lines

3) So I think that code like above should be better in both cases :
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
char name [MAX_NAMELEN + 1];
[...]
snprintf(name, MAX_NAMELEN, "%d", irq);
name[MAX_NAMELEN] = '\0';
[...] proc_mkdir(name, ...);
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

Yes, the code is overly paranoid. Note that in one case strlcpy() should be used and in another snprintf(). After that memset() and putting NUL become truly useless.

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Jaco Kroon

2007-02-23 05:39 UTC #2 lore raw

Re: [KJ] Improve 'register_handler_proc()' and

In reply to Christophe Jaillet

Quoted 26 lines · Christophe Jaillet

Quoted 25 lines
In file /irq/proc.c, function 'register_handler_proc()' and 'register_irq_proc()' memset a local variable 'name[MAX_NAMELEN]'
I don't really see the point of this memset.
In 'register_handler_proc()' :
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
char name [MAX_NAMELEN];
[...]
memset(name, 0, MAX_NAMELEN);
snprintf(name, MAX_NAMELEN, "%s", action->name);
[...] proc_mkdir(name, ...);
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
In 'register_handler_proc()' :
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
char name [MAX_NAMELEN];
[...]
memset(name, 0, MAX_NAMELEN);
sprintf(name, "%d", irq);
[...] proc_mkdir(name, ...);
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Here are my remarks :

Quoted 8 lines

Quoted 8 lines
1) I think that both function should use 'snprintf', just to be sure. Actualy only 'register_handler_proc()' does
2) I think that the memset are completely useless and could make s.o. think that the 'name' variables are correctly cleanned and NULL terminated. But,
- In 'register_handler_proc()', the use of snprintf can lead to
name beeing not NULL terminated !

Hold up right there. snprintf should _always_ NULL-terminate the resulting string, strncpy, however, does not always NULL-terminate. (Note that this assumes that the kernel implementation of snprintf exhibits the same behaviour as that described in the manpage of snprintf.)

Jaco
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors