Buffer over flow in /proc/xenomai/stat

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!

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

ksrc/nucleus/module.c 2008-07-29 09:46:45.000000000 +0900 renamed from ksrc/nucleus/module.c 2008-06-02 00:44:48.000000000 +0900 +6 -1

@@ -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
[email protected]
https://mail.gna.org/listinfo/xenomai-core

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!

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

ksrc/nucleus/module.c 2008-07-29 09:46:45.000000000 +0900 renamed from ksrc/nucleus/module.c 2008-06-02 00:44:48.000000000 +0900 +6 -1

@@ -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
[email protected]
https://mail.gna.org/listinfo/xenomai-core

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

ksrc/nucleus/module.c 2008-08-04 17:54:12.000000000 +0900 renamed from ksrc/nucleus/module.c 2008-06-02 00:44:48.000000000 +0900 +17 -2

@@ -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: <[email protected]>
Sent: Monday, August 04, 2008 4:49 PM
Subject: Re: [Xenomai-core] [PATCH] Buffer over flow in /proc/xenomai/stat

Quoted 8 lines · Atsushi Katagiri

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.

Quoted 44 lines

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

ksrc/nucleus/module.c 2008-07-29 09:46:45.000000000 +0900 renamed from ksrc/nucleus/module.c 2008-06-02 00:44:48.000000000 +0900 +6 -1

@@ -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
[email protected]
https://mail.gna.org/listinfo/xenomai-core

Atsushi-san,

Quoted 2 lines · Atsushi Katagiri

Yes, I actually encountered this bug and my Linux was crashed by NULL pointer dereference.

Quoted 5 lines

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:

Atsushi-san,

Quoted 2 lines · Atsushi Katagiri

Yes, I actually encountered this bug and my Linux was crashed by NULL pointer dereference.

Quoted 5 lines

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:

Quoted 12 lines · Philippe Gerum

Atsushi-san,

Quoted 7 lines · 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.

Please try this fix instead:

Actually, this one is better:

nucleus/module.c +2 -2

@@ -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,

Quoted 35 lines · Philippe Gerum

Quoted 12 lines · Philippe Gerum

Atsushi-san,

Quoted 7 lines · 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.

Please try this fix instead:

Actually, this one is better:

nucleus/module.c +2 -2

@@ -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,

nucleus/module.c line 464

Quoted 7 lines

nucleus/module.c +1 -0

@@ -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
===================================================================

nucleus/module.c +4 -4

@@ -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,

...
This would be even better with a proper iterator construct.

Quoted 3 lines · Philippe Gerum

...
This would be even better with a proper iterator construct.

Something like this? I think I tested most cases successfully (e.g. 4 CPUs with a shared IRQ), but the code should also be better readable, thus better reviewable. Comments/feedback welcome.

Jan

---
 include/nucleus/intr.h |   25 +++++++++++-----
 ksrc/nucleus/intr.c    |   76 ++++++++++++++++++++++++++++++++-----------------
 ksrc/nucleus/module.c  |   46 ++++++++++-------------------
 3 files changed, 84 insertions(+), 63 deletions(-)

Index: b/include/nucleus/intr.h
===================================================================

include/nucleus/intr.h +16 -4

@@ -69,11 +69,23 @@ typedef struct xnintr {
 
 } xnintr_t;
 
+typedef struct xnintr_iterator {
+
+    int cpu;		/* !< Current CPU in iteration. */
+
+    unsigned long hits;	/* !< Current hit counter. */
+
+    xnticks_t exectime;	/* !< Used CPU time in current accounting period. */
+
+    xnticks_t account_period; /* !< Length of accounting period. */
+
+    int list_rev;	/* !< System-wide xnintr list revision (internal use). */
+
+    xnintr_t *prev;	/* !< Previously visited xnintr object (internal use). */
+
+} xnintr_iterator_t;
+
 extern xnintr_t nkclock;
-#ifdef CONFIG_XENO_OPT_STATS
-extern int xnintr_count;
-extern int xnintr_list_rev;
-#endif
 
 #ifdef __cplusplus
 extern "C" {

include/nucleus/intr.h +0 -0

 #ifdef __cplusplus
 }

Index: b/ksrc/nucleus/intr.c
===================================================================

ksrc/nucleus/intr.c +38 -14

@@ -42,9 +42,9 @@
 DEFINE_PRIVATE_XNLOCK(intrlock);
 
 #ifdef CONFIG_XENO_OPT_STATS
-xnintr_t nkclock;	/* Only for statistics */
-int xnintr_count = 1;	/* Number of attached xnintr objects + nkclock */
-int xnintr_list_rev;	/* Modification counter of xnintr list */
+xnintr_t nkclock;	     /* Only for statistics */
+static int xnintr_count = 1; /* Number of attached xnintr objects + nkclock */
+static int xnintr_list_rev;  /* Modification counter of xnintr list */
 
 /* Both functions update xnintr_list_rev at the very end.
  * This guarantees that module.c::stat_seq_open() won't get

@@ -899,55 +899,79 @@ int xnintr_irq_proc(unsigned int irq, ch
 #endif /* CONFIG_PROC_FS */
 
 #ifdef CONFIG_XENO_OPT_STATS
-int xnintr_query(int irq, int *cpu, xnintr_t **prev, int revision, char *name,
-		 unsigned long *hits, xnticks_t *exectime,
-		 xnticks_t *account_period)
+int xnintr_init_query(xnintr_iterator_t *iterator)
+{
+	iterator->cpu = -1;
+	iterator->prev = NULL;
+
+	/* The order is important here: first xnintr_list_rev then
+	 * xnintr_count.  On the other hand, xnintr_attach/detach()
+	 * update xnintr_count first and then xnintr_list_rev.  This
+	 * should guarantee that we can't get an up-to-date
+	 * xnintr_list_rev and old xnintr_count here. The other way
+	 * around is not a problem as xnintr_query() will notice this
+	 * fact later.  Should xnintr_list_rev change later,
+	 * xnintr_query() will trigger an appropriate error below. */
+
+	iterator->list_rev = xnintr_list_rev;
+	xnarch_memory_barrier();
+
+	return xnintr_count;
+}
+
+int xnintr_query(int irq, xnintr_iterator_t *iterator, char *name_buf)
 {
 	xnintr_t *intr;
 	xnticks_t last_switch;
 	int head;
-	int cpu_no = *cpu;
+	int cpu_no = iterator->cpu + 1;
 	int err = 0;
 	spl_t s;
 
+	if (cpu_no == xnarch_num_online_cpus())
+		cpu_no = 0;
+	iterator->cpu = cpu_no;
+
 	xnlock_get_irqsave(&intrlock, s);
 
-	if (revision != xnintr_list_rev) {
+	if (iterator->list_rev != xnintr_list_rev) {
 		err = -EAGAIN;
 		goto unlock_and_exit;
 	}
 
-	if (*prev)
-		intr = xnintr_shirq_next(*prev);
-	else if (irq == XNARCH_TIMER_IRQ)
-		intr = &nkclock;
-	else
-		intr = xnintr_shirq_first(irq);
+	if (!iterator->prev) {
+		if (irq == XNARCH_TIMER_IRQ)
+			intr = &nkclock;
+		else
+			intr = xnintr_shirq_first(irq);
+	} else
+		intr = xnintr_shirq_next(iterator->prev);
 
 	if (!intr) {
+		cpu_no = -1;

ksrc/nucleus/intr.c +9 -9

-	*hits = xnstat_counter_get(&intr->stat[cpu_no].hits);
+	iterator->hits = xnstat_counter_get(&intr->stat[cpu_no].hits);
 
 	last_switch = xnpod_sched_slot(cpu_no)->last_account_switch;
 
-	*exectime       = intr->stat[cpu_no].account.total;
-	*account_period = last_switch - intr->stat[cpu_no].account.start;
+	iterator->exectime = intr->stat[cpu_no].account.total;
+	iterator->account_period =
+		last_switch - intr->stat[cpu_no].account.start;
 
-	intr->stat[cpu_no].account.total  = 0;
+	intr->stat[cpu_no].account.total = 0;
 	intr->stat[cpu_no].account.start = last_switch;
 
-	if (++cpu_no == xnarch_num_online_cpus()) {
-		cpu_no = 0;
-		*prev  = intr;
-	}
-	*cpu = cpu_no;
+	/* Proceed to next entry in shared IRQ chain when all CPUs
+	 * have been visited for this one. */
+	if (cpu_no + 1 == xnarch_num_online_cpus())
+		iterator->prev = intr;
 
      unlock_and_exit:
 	xnlock_put_irqrestore(&intrlock, s);

Index: b/ksrc/nucleus/module.c
===================================================================

ksrc/nucleus/module.c +16 -30

@@ -352,7 +352,9 @@ static int stat_seq_open(struct inode *i
 	struct seq_file *seq;
 	xnholder_t *holder;
 	struct stat_seq_info *stat_info;
-	int err, count, thrq_rev, intr_rev, irq;
+	xnintr_iterator_t intr_iter;
+	int err, count, thrq_rev, irq;
+	int intr_count;
 	spl_t s;
 
 	if (!xnpod_active_p())

@@ -368,18 +370,8 @@ static int stat_seq_open(struct inode *i
 
 	xnlock_put_irqrestore(&nklock, s);
 
-	/* The order is important here: first xnintr_list_rev then
-	 * xnintr_count.  On the other hand, xnintr_attach/detach()
-	 * update xnintr_count first and then xnintr_list_rev.  This
-	 * should guarantee that we can't get an up-to-date
-	 * xnintr_list_rev and old xnintr_count here. The other way
-	 * around is not a problem as xnintr_query() will notice this
-	 * fact later.  Should xnintr_list_rev change later,
-	 * xnintr_query() will trigger an appropriate error below. */
-
-	intr_rev = xnintr_list_rev;
-	xnarch_memory_barrier();
-	count += xnintr_count * RTHAL_NR_CPUS;
+	intr_count = xnintr_init_query(&intr_iter);
+	count += intr_count * RTHAL_NR_CPUS;
 
 	if (iter)
 		kfree(iter);

@@ -442,35 +434,29 @@ 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;
-
+	for (irq = 0; irq < XNARCH_NR_IRQS; irq++)
 		/* ...over all shared IRQs on all CPUs */
 		while (1) {
 			stat_info = &iter->stat_info[iter->nentries];
-			_cpu = cpu;
 
-			err = xnintr_query(irq, &cpu, &prev, intr_rev,
-					   stat_info->name,
-					   &stat_info->csw,
-					   &stat_info->exectime,
-					   &stat_info->account_period);
-			if (err == -EAGAIN)
-				goto restart_unlocked;
-			if (err)
+			err = xnintr_query(irq, &intr_iter, stat_info->name);
+			if (err) {
+				if (err == -EAGAIN)
+					goto restart_unlocked;
 				break; /* line unused or end of chain */
+			}
 
-			stat_info->cpu = _cpu;
+			stat_info->cpu = intr_iter.cpu;
+			stat_info->csw = intr_iter.hits;
+			stat_info->exectime = intr_iter.exectime;
+			stat_info->account_period = intr_iter.account_period;
 			stat_info->pid = 0;
 			stat_info->state =  0;
 			stat_info->ssw = 0;
 			stat_info->pf = 0;
 
 			iter->nentries++;
-		};
-	}
+		}
 
 	seq = file->private_data;
 	seq->private = iter;

Quoted 5 lines

ksrc/nucleus/intr.c +2 -3

-	head = snprintf(name, XNOBJECT_NAME_LEN, "IRQ%d: ", irq);
-	name += head;
-	strncpy(name, intr->name, XNOBJECT_NAME_LEN-head);
+	head = snprintf(name_buf, XNOBJECT_NAME_LEN, "IRQ%d: ", irq);
+	strncpy(name_buf + head, intr->name, XNOBJECT_NAME_LEN-head);

(quoted diff) +2 -1

-	strncpy(name_buf + head, intr->name, XNOBJECT_NAME_LEN-head);
+	strncpy(name_buf + head, intr->name, XNOBJECT_NAME_LEN-1-head);
+	name_buf[XNOBJECT_NAME_LEN-1] = '\0';

Object names should always be null-terminated.

Quoted 9 lines · Philippe Gerum

Quoted 5 lines

(quoted diff) +2 -3

-	head = snprintf(name, XNOBJECT_NAME_LEN, "IRQ%d: ", irq);
-	name += head;
-	strncpy(name, intr->name, XNOBJECT_NAME_LEN-head);
+	head = snprintf(name_buf, XNOBJECT_NAME_LEN, "IRQ%d: ", irq);
+	strncpy(name_buf + head, intr->name, XNOBJECT_NAME_LEN-head);

(quoted diff) +2 -1

-	strncpy(name_buf + head, intr->name, XNOBJECT_NAME_LEN-head);
+	strncpy(name_buf + head, intr->name, XNOBJECT_NAME_LEN-1-head);
+	name_buf[XNOBJECT_NAME_LEN-1] = '\0';

Quoted 1 line

Object names should always be null-terminated.

Yes, and what is the point of not doing everything with only one snprintf ? strncpy uselessly burns CPU cycles by filling the destination buffers with zeros if source is smaller than destination.