linux-kernel - Re: [patch 1/9] Fix HPET init race

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20070206164459.GA7271@dwarf.suse.cz>
Date:	Tue, 6 Feb 2007 17:44:59 +0100
From:	Jiri Bohac <jbohac@...e.cz>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	jbohac@...e.cz, Andi Kleen <ak@...e.de>,
	linux-kernel@...r.kernel.org, Vojtech Pavlik <vojtech@...e.cz>,
	ssouhlal@...ebsd.org, arjan@...radead.org, tglx@...utronix.de,
	johnstul@...ibm.com, zippel@...ux-m68k.org, andrea@...e.de
Subject: Re: [patch 1/9] Fix HPET init race

On Thu, Feb 01, 2007 at 06:34:50PM -0800, Andrew Morton wrote:
> On Thu, 01 Feb 2007 10:59:53 +0100 jbohac@...e.cz wrote:
> 
> > Fix a race in the initialization of HPET, which might result in a 
> > 5 minute lockup on boot.
> > 
> 
> What race?  Please always describe bugs when fixing them.

If the value of the HPET_T0_CMP register is reached and exceeded
by the value of the HPET_COUNTER register after HPET_T0_CMP is
read into trigger, but before the first iteration of the while,
the while loop will iterate "endlessly" until the HPET overlaps
eventually (in as much as 5 minutes).

> > 
> > Index: linux-2.6.20-rc5/arch/x86_64/kernel/apic.c
> > ===================================================================
> > --- linux-2.6.20-rc5.orig/arch/x86_64/kernel/apic.c
> > +++ linux-2.6.20-rc5/arch/x86_64/kernel/apic.c
> > @@ -764,10 +767,12 @@ static void setup_APIC_timer(unsigned in
> >  
> >  	/* wait for irq slice */
> >   	if (vxtime.hpet_address && hpet_use_timer) {
> > - 		int trigger = hpet_readl(HPET_T0_CMP);
> > - 		while (hpet_readl(HPET_COUNTER) >= trigger)
> > - 			/* do nothing */ ;
> > - 		while (hpet_readl(HPET_COUNTER) <  trigger)
> > +		int trigger;
> > +		do
> > +			trigger = hpet_readl(HPET_T0_CMP);
> > +		while (hpet_readl(HPET_COUNTER) >= trigger);
> > +
> 
> Is this signedness-safe and wraparound-safe?  It might be better to make
> `trigger' unsigned and do
> 
> 	while (hpet_readl(HPET_COUNTER) - trigger >= 0)


Yes, making trigger unsigned is a good idea (although having it
signed would probably never cause any problem, because this is
called during boot and it takes ~2 minutes for HPET to overflow
the s32)

But no, looping while the unsigned result is >= 0 does not seem
that good an idea to me ;-)

An updated patch follows. It is still not wraparound safe (a
lockup would still happen if it's called ~5 minutes after boot, but
this should never happen -- it's called early during boot)

--- linux-2.6.20-rc5.orig/arch/x86_64/kernel/apic.c	2007-02-06 16:56:00.000000000 +0100
+++ linux-2.6.20-rc5/arch/x86_64/kernel/apic.c	2007-02-06 17:26:42.000000000 +0100
@@ -764,10 +764,12 @@ static void setup_APIC_timer(unsigned in
 
 	/* wait for irq slice */
  	if (vxtime.hpet_address && hpet_use_timer) {
- 		int trigger = hpet_readl(HPET_T0_CMP);
- 		while (hpet_readl(HPET_COUNTER) >= trigger)
- 			/* do nothing */ ;
- 		while (hpet_readl(HPET_COUNTER) <  trigger)
+		u32 trigger;
+		do
+			trigger = hpet_readl(HPET_T0_CMP);
+		while (hpet_readl(HPET_COUNTER) >= trigger);
+
+		while (hpet_readl(HPET_COUNTER) <  trigger)
  			/* do nothing */ ;
  	} else {
 		int c1, c2;


-- 
Jiri Bohac <jbohac@...e.cz>
SUSE Labs, SUSE CZ

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/