netdev - Re: [PATCH] sctp: Clean up type-punning in sctp_cmd

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: <20121029185932.GC10177@hmsreliant.think-freely.org>
Date:	Mon, 29 Oct 2012 14:59:32 -0400
From:	Neil Horman <nhorman@...driver.com>
To:	David Laight <David.Laight@...LAB.COM>
Cc:	David Miller <davem@...emloft.net>, vyasevich@...il.com,
	netdev@...r.kernel.org, linux-sctp@...r.kernel.org
Subject: Re: [PATCH] sctp: Clean up type-punning in sctp_cmd_t union

On Mon, Oct 29, 2012 at 03:07:07PM -0000, David Laight wrote:
> > > On Fri, Oct 26, 2012 at 03:12:11PM -0400, Vlad Yasevich wrote:
> > >> Look at commit 19c7e9ee that introduced this.  I don't remember all
> > >> the details any more, but the problem only occurred on ia64
> > >> (probably due its speculative load handling).
> > >>
> > >> -vlad
> > >>
> > > Thanks Vlad, I'll have a look see.
> > 
> > Ok, so this IA64 issue is all about accesses to uninitialized memory.
> > 
> > I think Neil's change is thus the most desirable thing to do.  Simple
> > memset the object to zero.
> 
> I'd guess it an artifact of the C memory aliasing rules and unions.
> If you write to a location as a one type but read it as another type
> (and neither type is char) then the compiler is generally allowed to
> schedule the read before the write (and may even optimise the write away).
> This shouldn't happen if the fields are both members of the same union,
> but casting some other address to a 'union *' separately for the
> write and read isn't enough.
> 
I don't think thats whats going on at all.  The problem, as commit
19c7e9eef503dc1ae926f3d26c56f88bee568d7b describes it, is that ia64 was
speculatively loading a value from memory of type sctp_arg_t.  If the
initialization step set the value of one of the smaller members of the union
(say a short), then the load stage may still load a larger amount of data (given
that the union is larger than its smallest members).  That results in part of
the load being uninitalized, and ia64 declaring it Not A Thing, and consequently
trapping out.

The fix previously was to just initazlie the unsigned long member zero in that
union before setting the actuall type member that was requried so as to ensure
all the data was initalized.  That works well enough, but it presumes that
unsigned long is the largest member of the union, which is risky.  Its better to
memset the union to 0 for sizeof bytes to ensure future proofing.

I've sent an updated version of the patch to do the memset. Its marked as V3,
but I fat fingered the in-reply-to so it didn't show up in this thread.
Apologies.

> This probably means that this entire type-punning argument passing
> scheme is actually broken.
> 
No, It works, as long as you read the same member as you write, and initalize
the entire union, which is what this patch does.
Neil

> 	David
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html