Re: [Xenomai-core] [PATCH] Buffer over flow in /proc/xenomai/stat

[Philippe Gerum <rpm@xenomai.org>]

Jan Kiszka wrote:
> Philippe Gerum wrote:
>> Philippe Gerum wrote:
>>> Atsushi-san,
>>>
>>> Atsushi Katagiri wrote:
>>>> Yes, I actually encountered this bug and my Linux was crashed by NULL pointer dereference.
>>>>
>>>> I think this is a very simple bug.
>>>> It happens "everytime" we open /proc/xenomai/stat,
>>>> because the last iter->nentries++; (line 466) surely reaches the value of the count,
>>>> and the next iteration, line 449, surely overwrites zero on out of the kmalloced area.
>>>>
>>> Please try this fix instead:
>>>
>> Actually, this one is better:
>>
>> --- ksrc/nucleus/module.c	(revision 4074)
>> +++ ksrc/nucleus/module.c	(working copy)
>> @@ -440,13 +440,13 @@
>>  	/* Iterate over all IRQ numbers, ... */
>>  	for (irq = 0; irq < XNARCH_NR_IRQS; irq++) {
>>  		xnintr_t *prev = NULL;
>> -		int cpu = 0;
>> +		int cpu = 0, _cpu;
>>  		int err;
>>
>>  		/* ...over all shared IRQs on all CPUs */
>>  		while (1) {
>>  			stat_info = &iter->stat_info[iter->nentries];
>> -			stat_info->cpu = cpu;
>> +			_cpu = cpu;
>>
>>  			err = xnintr_query(irq, &cpu, &prev, intr_rev,
>>  					   stat_info->name,
>> @@ -458,6 +458,7 @@
>>  			if (err)
>>  				break; /* line unused or end of chain */
>>
>> +			stat_info->cpu = _cpu;
>>  			stat_info->pid = 0;
>>  			stat_info->state =  0;
>>  			stat_info->ssw = 0;
>>
> 
> Good catch. But for the sake of telling variable names I would suggest:
> 
> Index: ksrc/nucleus/module.c
> ===================================================================
> --- ksrc/nucleus/module.c	(Revision 4094)
> +++ ksrc/nucleus/module.c	(Arbeitskopie)
> @@ -444,15 +444,15 @@ static int stat_seq_open(struct inode *i
>  	/* Iterate over all IRQ numbers, ... */
>  	for (irq = 0; irq < XNARCH_NR_IRQS; irq++) {
>  		xnintr_t *prev = NULL;
> -		int cpu = 0, _cpu;
> -		int err;
> +		int next_cpu = 0;
> +		int cpu, err;
>  
>  		/* ...over all shared IRQs on all CPUs */
>  		while (1) {
>  			stat_info = &iter->stat_info[iter->nentries];
> -			_cpu = cpu;
> +			cpu = next_cpu;
>  
> -			err = xnintr_query(irq, &cpu, &prev, intr_rev,
> +			err = xnintr_query(irq, &next_cpu, &prev, intr_rev,
>  					   stat_info->name,
>  					   &stat_info->csw,
>  					   &stat_info->exectime,
> @@ -462,7 +462,7 @@ static int stat_seq_open(struct inode *i
>  			if (err)
>  				break; /* line unused or end of chain */
>  
> -			stat_info->cpu = _cpu;
> +			stat_info->cpu = cpu;
>  			stat_info->pid = 0;
>  			stat_info->state =  0;
>  			stat_info->ssw = 0;
> 

This would be even better with a proper iterator construct.

-- 
Philippe.