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

2008-08-13 Thread Philippe Gerum 
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.

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

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

2008-08-04 Thread Philippe Gerum 
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.0 
 +0900
 +++ xenomai-2.4.4/ksrc/nucleus/module.c 2008-07-29 09:46:45.0 +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
 AD 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

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

2008-08-04 Thread Atsushi Katagiri 
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.

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.0 +0900
+++ xenomai-2.4.4/ksrc/nucleus/module.c 2008-08-04 17:54:12.0 +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.0 
 +0900
 +++ xenomai-2.4.4/ksrc/nucleus/module.c 2008-07-29 09:46:45.0 +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
 AD 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

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

2008-08-04 Thread Philippe Gerum 
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;

-- 
Philippe.

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