netdev - Re: net: alignment problem in icmp code

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: <20071022065443.2f3c1b8a@poseidon.drzeus.cx>
Date:	Mon, 22 Oct 2007 06:54:43 +0200
From:	Pierre Ossman <drzeus@...eus.cx>
To:	David Miller <davem@...emloft.net>
Cc:	netdev@...r.kernel.org
Subject: Re: net: alignment problem in icmp code

On Sun, 21 Oct 2007 16:02:15 -0700 (PDT)
David Miller <davem@...emloft.net> wrote:

> From: Pierre Ossman <drzeus@...eus.cx>
> Date: Sun, 21 Oct 2007 23:21:13 +0200
> 
> > Not sure that would be valid. memcpy() is defined as having void*
> > arguments, and the compiler cannot just ignore that if it chooses to
> > inline it.
> 
> Yes it can, there are C language rules about the alignment of types
> that the compiler completely can take advantage of in those kinds of
> situations.
> 

I'm not debating that. What I'm saying is that calling memcpy() casts your pointers to void* with the included semantical changes. It can't just ignore that because it decides to inline the function. It would be the same thing as when gcc decided to ignore the volatile qualifier on a pointer just because it could optimize away to the real object and discover it wasn't marked with volatile. Something that was considered a bug and was fixed.

> If you don't believe me, compile something like the following
> with optimizations enabled:

gcc has had bugs in the past.

> You will get a 64-bit load and a 64-bit store emitted by
> the compiler.  Here is what we get on sparc64:
> 

I assume those ops cause a bus error on unaligned addresses?

> 
> However, instead of relying upon magic like this, let's just tell the
> compiler explicitly what it going on by using get_unaligned().
> 

It wouldn't be magic:

memcpy(&icmp_param.data.icmph, skb_transport_header(skb), sizeof(struct icmphdr));

I believe platforms without alignment requirements could optimize this better than the series of assignments. Not that I think this will be a potential bottle neck, but still.

> Next, there are redundant stores being done here since the code and
> type are explicitly overwritten in various ways.

Indeed.

Rgds
Pierre

Download attachment "signature.asc" of type "application/pgp-signature" (190 bytes)