linux-kernel - Re: [patch] x86, UV: integer wrap bug in uv_hub_ipi

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]

Date:	Tue, 20 Nov 2012 11:07:25 -0600
From:	Russ Anderson <rja@....com>
To:	Dan Carpenter <dan.carpenter@...cle.com>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	Dimitri Sivanich <sivanich@....com>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
	linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
	Russ Anderson <rja@....com>
Subject: Re: [patch] x86, UV: integer wrap bug in uv_hub_ipi_value()

On Tue, Nov 20, 2012 at 07:28:56AM +0300, Dan Carpenter wrote:
> On Mon, Nov 19, 2012 at 06:48:34PM -0600, Russ Anderson wrote:
> > On Sat, Nov 17, 2012 at 06:16:11PM +0300, Dan Carpenter wrote:
> > > This is a static checker fix.  The problem is that we store the bits
> > > from "uv_apicid_hibits" into "apicid" (the high 16 bits) but then we
> > > shift it 16 bit to the left.  "apicid" is an int so it wraps and we lose
> > > them.
> > 
> > Is this the complete patch?  phys_apicid is an int, but gets
> > cast as unsigned long.  Doesn't phys_apicid also have to be
> > changed to unsigned long?  And why ulong instead of uint (on x86_64)?
> 
> Uint is 32bit across all arches in linux and unix, according to
> wikipedia.

But long isn't 32bit across all arches.

http://software.intel.com/en-us/articles/size-of-long-integer-type-on-different-architecture-and-os

>             The wakeup_secondary_cpu() function pointer takes an int
> so I couldn't change the parameter.

Yes.  The real problem is much of the apicid code is based 
on signed int (ie parameters in struct apic).  Not sure why
they chose to make it signed, but since they did that decision
ripples through the rest of the code.  Changing it to unsigned
means changing struct apic, which likewise will ripple through
the rest of the code.  That is a much bigger change than
your patch deals with.

> > I agree with changing signed to unsigned where appropriate, but
> > this looks like a partial fix.  Am I missing something?
> > 
> 
> I do feel a little embarrassed that I didn't use "unsigned long"
> consistently.  I just used ulong to make the line a bit shorter, but
> I could redo it with "unsigned long" if you want.

The issue isn't "ulong" vs "unsigned long".  The issue
is int is 32 bit and long is 64 bit on x86_64.  Your 
patch is casting the value as an "unsigned long" (64 bit
on x86_64) into an int (32 bit).  I don't think that
was your intent.

This does highlight the problematic nature of "long" being
different size on different architectures.

-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          rja@....com
--
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/