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:

--- ksrc/nucleus/module.c       (revision 4074)
+++ ksrc/nucleus/module.c       (working copy)
@@ -446,7 +446,6 @@
                /* ...over all shared IRQs on all CPUs */
                while (1) {
                        stat_info = &iter->stat_info[iter->nentries];
-                       stat_info->cpu = cpu;

                        err = xnintr_query(irq, &cpu, &prev, intr_rev,
                                           stat_info->name,
@@ -458,6 +457,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;


> If you doubt of it, please try the next debug code.
> 
> Regards,
> Atsushi KATAGIRI
> 
> --- xenomai-2.4.4-org/ksrc/nucleus/module.c 2008-06-02 00:44:48.000000000 
> +0900
> +++ xenomai-2.4.4/ksrc/nucleus/module.c 2008-08-04 17:54:12.000000000 +0900
> @@ -382,6 +382,8 @@
>   iter = kmalloc(sizeof(*iter)
>           + (count - 1) * sizeof(struct stat_seq_info),
>           GFP_KERNEL);
> + printk("iter=%x size=%d count=%d\n",iter,(int)(sizeof(*iter)
> +         + (count - 1) * sizeof(struct stat_seq_info)),count); //+++++debug
>   if (!iter)
>    return -ENOMEM;
>  
> @@ -437,17 +439,29 @@
>    xnlock_put_irqrestore(&nklock, s);
>   }
>  
> + int nentries_old = -1; //+++++debug
> +
>   /* Iterate over all IRQ numbers, ... */
>   for (irq = 0; irq < XNARCH_NR_IRQS; irq++) {
>    xnintr_t *prev = NULL;
>    int cpu = 0;
>    int err;
>  
> +//  if (iter->nentries >= count)
> +//   break;
> +
>    /* ...over all shared IRQs on all CPUs */
>    while (1) {
>     stat_info = &iter->stat_info[iter->nentries];
>     stat_info->cpu = cpu;
>  
> +//+++++debug----->
> +   if(nentries_old != iter->nentries){
> +    printk("stat_info=%x nentries=%d\n",stat_info,(int)(iter->nentries));
> +    nentries_old = iter->nentries;
> +   }
> +//+++++debug<-----
> +
>     err = xnintr_query(irq, &cpu, &prev, intr_rev,
>          stat_info->name,
>          &stat_info->csw,
> @@ -464,7 +478,9 @@
>     stat_info->pf = 0;
>  
>     iter->nentries++;
> -  };
> +//   if (iter->nentries >= count)
> +//    break;
> +  }
>   }
>  
>   seq = file->private_data;
> 
> ----- Original Message ----- 
> From: "Philippe Gerum" <[EMAIL PROTECTED]>
> To: "Atsushi Katagiri" <[EMAIL PROTECTED]>
> Cc: <xenomai-core@gna.org>
> Sent: Monday, August 04, 2008 4:49 PM
> Subject: Re: [Xenomai-core] [PATCH] Buffer over flow in /proc/xenomai/stat
> 
> 
> Atsushi Katagiri wrote:
>> Hello all.
>>
>> This is a small patch that fixes a serious bug.
>>
>> When we open /proc/xenomai/stat, function stat_seq_open kmalloc the area, 
>> write the data and increment iter->nentries.
>> The last increment of this value reaches "count",
>> and at the next iteration "stat_info->cpu = cpu;"  overwrites zero on 
>> illegal address!
>>
> 
> Did you actually see this bug happen?
> 
> This code takes a snapshot of the IRQ and thread lists, that are identified by
> two fingerprint values (intr_rev, thrq_rev). The only way to have 
> iter->nentries
> greater than count at some point, would be to see more IRQ/thread descriptors
> being linked to their respective lists on-the-fly while we scan them. But in
> that case, the current fingerprint value would stop matching the snapshot 
> value
> as well, causing the loops to restart, re-allocating sufficient space to hold
> all the data.
> 
> If you did see this bug happen, please tell us a bit more about your setup.
> 
>> Here is my proposal of the fix..
>>
>> =====patch start=====>
>> diff -Nur xenomai-2.4.4-org/ksrc/nucleus/module.c 
>> xenomai-2.4.4/ksrc/nucleus/module.c
>> --- xenomai-2.4.4-org/ksrc/nucleus/module.c 2008-06-02 00:44:48.000000000 
>> +0900
>> +++ xenomai-2.4.4/ksrc/nucleus/module.c 2008-07-29 09:46:45.000000000 +0900
>> @@ -443,6 +443,9 @@
>>    int cpu = 0;
>>    int err;
>>  
>> +  if (iter->nentries >= count)
>> +   break;
>> +
>>    /* ...over all shared IRQs on all CPUs */
>>    while (1) {
>>     stat_info = &iter->stat_info[iter->nentries];
>> @@ -464,7 +467,9 @@
>>     stat_info->pf = 0;
>>  
>>     iter->nentries++;
>> -  };
>> +   if (iter->nentries >= count)
>> +    break;
>> +  }
>>   }
>>  
>>   seq = file->private_data;
>> <=====patch end=====
>>
>> I hope someone who knows this function well will solve the problem.
>>
>> Regards,
>>
>> Atsushi KATAGIRI
>> Software Engineer
>> A&D Company, Limited
>> Tokyo, Japan
>>
>>
>> _______________________________________________
>> Xenomai-core mailing list
>> Xenomai-core@gna.org
>> https://mail.gna.org/listinfo/xenomai-core
>>
> 
> 


-- 
Philippe.

_______________________________________________
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Reply via email to