[PATCH] kernel/irq/proc.c: set MAX_NAMELEN 11 instead of 10 which express the maximize size of "%d" or "%u".

[PATCH] kernel/irq/proc.c: set MAX_NAMELEN 11 instead of 10 which express the maximize size of "%d" or "%u".

[Chen Gang]

Theoretically, the maximize size of "%d" or "%u" is 11 (10 + '\0'), so
need set MAX_NAMELEN 11 instead of 10.

In real world, it may not cause issue, but still recommend to fix this
'theoretical' bug (especially original definition already considered
about 'theoretical' using).

Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 kernel/irq/proc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
index 36f6ee1..4e9a9ee 100644
--- a/kernel/irq/proc.c
+++ b/kernel/irq/proc.c
@@ -305,7 +305,7 @@ void register_handler_proc(unsigned int irq, struct irqaction *action)
 
 #undef MAX_NAMELEN
 
-#define MAX_NAMELEN 10
+#define MAX_NAMELEN 11
 
 void register_irq_proc(unsigned int irq, struct irq_desc *desc)
 {
-- 
1.7.7.

Re: [PATCH] kernel/irq/proc.c: set MAX_NAMELEN 11 instead of 10 which express the maximize size of "%d" or "%u".

[Chen Gang]

Hello Al Viro:

Is this patch correct?  ;-)

Thanks.

On 10/05/2013 11:19 PM, Chen Gang wrote:
> Theoretically, the maximize size of "%d" or "%u" is 11 (10 + '\0'), so
> need set MAX_NAMELEN 11 instead of 10.
> 
> In real world, it may not cause issue, but still recommend to fix this
> 'theoretical' bug (especially original definition already considered
> about 'theoretical' using).
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  kernel/irq/proc.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
> index 36f6ee1..4e9a9ee 100644
> --- a/kernel/irq/proc.c
> +++ b/kernel/irq/proc.c
> @@ -305,7 +305,7 @@ void register_handler_proc(unsigned int irq, struct irqaction *action)
>  
>  #undef MAX_NAMELEN
>  
> -#define MAX_NAMELEN 10
> +#define MAX_NAMELEN 11
>  
>  void register_irq_proc(unsigned int irq, struct irq_desc *desc)
>  {
> 


-- 
Chen Gang

Re: [PATCH] kernel/irq/proc.c: set MAX_NAMELEN 11 instead of 10 which express the maximize size of "%d" or "%u".

[Joe Perches]

On Sat, 2013-10-05 at 23:19 +0800, Chen Gang wrote:
> Theoretically, the maximize size of "%d" or "%u" is 11 (10 + '\0'), so
> need set MAX_NAMELEN 11 instead of 10.

%d can be negative.

Re: [PATCH] kernel/irq/proc.c: set MAX_NAMELEN 11 instead of 10 which express the maximize size of "%d" or "%u".

[Chen Gang]

On 10/05/2013 11:41 PM, Joe Perches wrote:
> On Sat, 2013-10-05 at 23:19 +0800, Chen Gang wrote:
>> Theoretically, the maximize size of "%d" or "%u" is 11 (10 + '\0'), so
>> need set MAX_NAMELEN 11 instead of 10.
> 
> %d can be negative.
> 
> 
> 
> 

Oh, really, it is incorrect. Al Viro succeeds once.  :-(

And I should send patch v2 for it.

Thanks.
-- 
Chen Gang

Re: [PATCH] kernel/irq/proc.c: set MAX_NAMELEN 11 instead of 10 which express the maximize size of "%d" or "%u".

[Richard Weinberger]

On Sat, Oct 5, 2013 at 5:46 PM, Chen Gang <gang.chen@asianux.com> wrote:
> On 10/05/2013 11:41 PM, Joe Perches wrote:
>> On Sat, 2013-10-05 at 23:19 +0800, Chen Gang wrote:
>>> Theoretically, the maximize size of "%d" or "%u" is 11 (10 + '\0'), so
>>> need set MAX_NAMELEN 11 instead of 10.
>>
>> %d can be negative.
>>
>>
>>
>>
>
> Oh, really, it is incorrect. Al Viro succeeds once.  :-(
>
> And I should send patch v2 for it.

irq is in both register_irq_proc() and unregister_irq_proc() an unsigned int.
Therefore %d makes not really sense. Both should use %u.
IMHO sprintf() should also get replaced by snprintf() but that's a
matter of taste.

-- 
Thanks,
//richard

Re: [PATCH] kernel/irq/proc.c: set MAX_NAMELEN 11 instead of 10 which express the maximize size of "%d" or "%u".

[Chen Gang]

On 10/06/2013 12:08 AM, Richard Weinberger wrote:
> On Sat, Oct 5, 2013 at 5:46 PM, Chen Gang <gang.chen@asianux.com> wrote:
>> On 10/05/2013 11:41 PM, Joe Perches wrote:
>>> On Sat, 2013-10-05 at 23:19 +0800, Chen Gang wrote:
>>>> Theoretically, the maximize size of "%d" or "%u" is 11 (10 + '\0'), so
>>>> need set MAX_NAMELEN 11 instead of 10.
>>>
>>> %d can be negative.
>>>
>>>
>>>
>>>
>>
>> Oh, really, it is incorrect. Al Viro succeeds once.  :-(
>>
>> And I should send patch v2 for it.
> 
> irq is in both register_irq_proc() and unregister_irq_proc() an unsigned int.
> Therefore %d makes not really sense. Both should use %u.
> IMHO sprintf() should also get replaced by snprintf() but that's a
> matter of taste.
> 

OK, thanks. your opinions sounds more reasonable to me.

After 1 day (if no additional reply), I should send patch v2 for it.


Thanks
-- 
Chen Gang

Re: [PATCH] kernel/irq/proc.c: set MAX_NAMELEN 11 instead of 10 which express the maximize size of "%d" or "%u".

[Chen Gang]

On 10/06/2013 12:50 AM, Chen Gang wrote:
> On 10/06/2013 12:08 AM, Richard Weinberger wrote:
>> On Sat, Oct 5, 2013 at 5:46 PM, Chen Gang <gang.chen@asianux.com> wrote:
>>> On 10/05/2013 11:41 PM, Joe Perches wrote:
>>>> On Sat, 2013-10-05 at 23:19 +0800, Chen Gang wrote:
>>>>> Theoretically, the maximize size of "%d" or "%u" is 11 (10 + '\0'), so
>>>>> need set MAX_NAMELEN 11 instead of 10.
>>>>
>>>> %d can be negative.
>>>>
>>>>
>>>>
>>>>
>>>
>>> Oh, really, it is incorrect. Al Viro succeeds once.  :-(
>>>
>>> And I should send patch v2 for it.
>>
>> irq is in both register_irq_proc() and unregister_irq_proc() an unsigned int.
>> Therefore %d makes not really sense. Both should use %u.
>> IMHO sprintf() should also get replaced by snprintf() but that's a
>> matter of taste.
>>

Oh, commonly, snprintf() are used for the string which can be truncated,
and can not be used for the string which contents must not be truncated.

In our case, the name string must be not truncated (or may not unique,
theoretically), so we have to still use sprintf().


Thanks.
> 
> OK, thanks. your opinions sounds more reasonable to me.
> 
> After 1 day (if no additional reply), I should send patch v2 for it.
> 
> 
> Thanks
> 


-- 
Chen Gang

Re: [PATCH] kernel/irq/proc.c: set MAX_NAMELEN 11 instead of 10 which express the maximize size of "%d" or "%u".

[Richard Weinberger]

Am 05.10.2013 19:06, schrieb Chen Gang:
> On 10/06/2013 12:50 AM, Chen Gang wrote:
>> On 10/06/2013 12:08 AM, Richard Weinberger wrote:
>>> On Sat, Oct 5, 2013 at 5:46 PM, Chen Gang <gang.chen@asianux.com> wrote:
>>>> On 10/05/2013 11:41 PM, Joe Perches wrote:
>>>>> On Sat, 2013-10-05 at 23:19 +0800, Chen Gang wrote:
>>>>>> Theoretically, the maximize size of "%d" or "%u" is 11 (10 + '\0'), so
>>>>>> need set MAX_NAMELEN 11 instead of 10.
>>>>>
>>>>> %d can be negative.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>> Oh, really, it is incorrect. Al Viro succeeds once.  :-(
>>>>
>>>> And I should send patch v2 for it.
>>>
>>> irq is in both register_irq_proc() and unregister_irq_proc() an unsigned int.
>>> Therefore %d makes not really sense. Both should use %u.
>>> IMHO sprintf() should also get replaced by snprintf() but that's a
>>> matter of taste.
>>>
> 
> Oh, commonly, snprintf() are used for the string which can be truncated,
> and can not be used for the string which contents must not be truncated.
> 
> In our case, the name string must be not truncated (or may not unique,
> theoretically), so we have to still use sprintf().

Of course you would have to check the return value of snprintf() to detect
a truncation and abort...

Thanks,
//richard

Re: [PATCH] kernel/irq/proc.c: set MAX_NAMELEN 11 instead of 10 which express the maximize size of "%d" or "%u".

[Chen Gang]

On 10/06/2013 01:45 AM, Richard Weinberger wrote:
> Am 05.10.2013 19:06, schrieb Chen Gang:
>> On 10/06/2013 12:50 AM, Chen Gang wrote:
>>> On 10/06/2013 12:08 AM, Richard Weinberger wrote:
>>>> On Sat, Oct 5, 2013 at 5:46 PM, Chen Gang <gang.chen@asianux.com> wrote:
>>>>> On 10/05/2013 11:41 PM, Joe Perches wrote:
>>>>>> On Sat, 2013-10-05 at 23:19 +0800, Chen Gang wrote:
>>>>>>> Theoretically, the maximize size of "%d" or "%u" is 11 (10 + '\0'), so
>>>>>>> need set MAX_NAMELEN 11 instead of 10.
>>>>>>
>>>>>> %d can be negative.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> Oh, really, it is incorrect. Al Viro succeeds once.  :-(
>>>>>
>>>>> And I should send patch v2 for it.
>>>>
>>>> irq is in both register_irq_proc() and unregister_irq_proc() an unsigned int.
>>>> Therefore %d makes not really sense. Both should use %u.
>>>> IMHO sprintf() should also get replaced by snprintf() but that's a
>>>> matter of taste.
>>>>
>>
>> Oh, commonly, snprintf() are used for the string which can be truncated,
>> and can not be used for the string which contents must not be truncated.
>>
>> In our case, the name string must be not truncated (or may not unique,
>> theoretically), so we have to still use sprintf().
> 
> Of course you would have to check the return value of snprintf() to detect
> a truncation and abort...
> 

OK, thanks, that sounds reasonable to me, so I feel that's not a matter
of taste.

In my opinion, when we know the maximized length, we need always use
s(c)nprintf instead of sprintf, if the string can be truncated, use
scnprintf, else use snprintf and also check the return value.

sprintf is 'dangrous', need try to use s(c)nprintf instead of. We can
scan whole kernel, I guess quite a few of sprintf may be related with
memory overflow 'theoretically' (welcome any members to give a check).


Thanks.

> Thanks,
> //richard
> 
> 
> 

-- 
Chen Gang

Re: [PATCH] kernel/irq/proc.c: set MAX_NAMELEN 11 instead of 10 which express the maximize size of "%d" or "%u".

[Chen Gang]

On 10/06/2013 08:37 AM, Chen Gang wrote:
> On 10/06/2013 01:45 AM, Richard Weinberger wrote:
>> Am 05.10.2013 19:06, schrieb Chen Gang:
>>> On 10/06/2013 12:50 AM, Chen Gang wrote:
>>>> On 10/06/2013 12:08 AM, Richard Weinberger wrote:
>>>>> On Sat, Oct 5, 2013 at 5:46 PM, Chen Gang <gang.chen@asianux.com> wrote:
>>>>>> On 10/05/2013 11:41 PM, Joe Perches wrote:
>>>>>>> On Sat, 2013-10-05 at 23:19 +0800, Chen Gang wrote:
>>>>>>>> Theoretically, the maximize size of "%d" or "%u" is 11 (10 + '\0'), so
>>>>>>>> need set MAX_NAMELEN 11 instead of 10.
>>>>>>>
>>>>>>> %d can be negative.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> Oh, really, it is incorrect. Al Viro succeeds once.  :-(
>>>>>>
>>>>>> And I should send patch v2 for it.
>>>>>
>>>>> irq is in both register_irq_proc() and unregister_irq_proc() an unsigned int.
>>>>> Therefore %d makes not really sense. Both should use %u.
>>>>> IMHO sprintf() should also get replaced by snprintf() but that's a
>>>>> matter of taste.
>>>>>
>>>
>>> Oh, commonly, snprintf() are used for the string which can be truncated,
>>> and can not be used for the string which contents must not be truncated.
>>>
>>> In our case, the name string must be not truncated (or may not unique,
>>> theoretically), so we have to still use sprintf().
>>
>> Of course you would have to check the return value of snprintf() to detect
>> a truncation and abort...
>>
> 
> OK, thanks, that sounds reasonable to me, so I feel that's not a matter
> of taste.
> 

Hmm... does 'taste' means: "for string which can not be truncated, if
maximized length is obvious (e.g. our case 11 for "%u"), sprintf() is
more clearer than snprintf() for both readers and writers"?


BTW: when MAX_NAMELEN is defined as 128, is it suitable to check the
return value of snprintf() in register_handler_proc(), and WARN_ON() if
it is not less than 128?


Thanks.

> In my opinion, when we know the maximized length, we need always use
> s(c)nprintf instead of sprintf, if the string can be truncated, use
> scnprintf, else use snprintf and also check the return value.
> 
> sprintf is 'dangrous', need try to use s(c)nprintf instead of. We can
> scan whole kernel, I guess quite a few of sprintf may be related with
> memory overflow 'theoretically' (welcome any members to give a check).
> 
> 
> Thanks.
> 
>> Thanks,
>> //richard
>>
>>
>>
> 


-- 
Chen Gang

[PATCH v2] kernel/irq/proc.c: set MAX_NAMELEN 11 instead of 10 and use "%u" instead of "%d" for printing 'irq'

[Chen Gang]

Theoretically, the maximize size of "%u" is 11 (10 + '\0'), so need set
MAX_NAMELEN 11 instead of 10, and also 'irq' is unsigned int, so need
use "%u" instead of "%d".

In real world, it may not cause issue, but still recommend to fix the
'theoretical' bugs (especially original definition already considered
about 'theoretical' using).


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 kernel/irq/proc.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
index 36f6ee1..efda701 100644
--- a/kernel/irq/proc.c
+++ b/kernel/irq/proc.c
@@ -305,7 +305,7 @@ void register_handler_proc(unsigned int irq, struct irqaction *action)
 
 #undef MAX_NAMELEN
 
-#define MAX_NAMELEN 10
+#define MAX_NAMELEN 11
 
 void register_irq_proc(unsigned int irq, struct irq_desc *desc)
 {
@@ -315,7 +315,7 @@ void register_irq_proc(unsigned int irq, struct irq_desc *desc)
 		return;
 
 	memset(name, 0, MAX_NAMELEN);
-	sprintf(name, "%d", irq);
+	sprintf(name, "%u", irq);
 
 	/* create /proc/irq/1234 */
 	desc->dir = proc_mkdir(name, root_irq_dir);
-- 
1.7.7.6