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

[Jan Kiszka <jan.kiszka@domain.hid>]

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

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;


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]