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

2008-08-13 Thread Jan Kiszka
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;



signature.asc
Description: OpenPGP digital signature
___
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-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


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

2008-08-04 Thread Philippe Gerum

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.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