From mboxrd@z Thu Jan 1 00:00:00 1970 Message-ID: <4896D6CC.1040103@domain.hid> Date: Mon, 04 Aug 2008 12:15:40 +0200 From: Philippe Gerum MIME-Version: 1.0 References: <001b01c8f600$0c5a5a60$09201fac@domain.hid> <4896B492.4000404@domain.hid> <005b01c8f615$db466930$09201fac@domain.hid> In-Reply-To: <005b01c8f615$db466930$09201fac@domain.hid> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Xenomai-core] [PATCH] Buffer over flow in /proc/xenomai/stat Reply-To: rpm@xenomai.org List-Id: "Xenomai life and development \(bug reports, patches, discussions\)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Atsushi Katagiri Cc: xenomai@xenomai.org 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" > To: "Atsushi Katagiri" > Cc: > 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@domain.hid >> https://mail.gna.org/listinfo/xenomai-core >> > > -- Philippe.