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]

Date:	Sun, 21 Oct 2007 16:02:15 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	drzeus@...eus.cx
Cc:	netdev@...r.kernel.org
Subject: Re: net: alignment problem in icmp code

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.

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

void foo(unsigned long long *a,
	 unsigned long long *b)
{
	memcpy(a, b, sizeof(*a));
}

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

foo:
	ldx	[%o1], %g1
	jmp	%o7+8
	 stx	%g1, [%o0]

Structure assignment is also essentially just another kind of
inline memcpy().

> Still, just give it the char* from the skb and it cannot make any
> assumption on alignment.

Yes, that would make it more likely to work.

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

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

commit 6471862b69e9272125f6a01916e6587523bf91f3
Author: David S. Miller <davem@...set.davemloft.net>
Date:   Sun Oct 21 16:01:49 2007 -0700

    [IPV4]: Handle potentially unaligned icmp headers in skb.
    
    Signed-off-by: David S. Miller <davem@...emloft.net>

diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 272c69e..2b654b1 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -92,6 +92,7 @@
 #include <asm/system.h>
 #include <asm/uaccess.h>
 #include <net/checksum.h>
+#include <asm/unaligned.h>
 
 /*
  *	Build xmit assembly blocks
@@ -766,6 +767,17 @@ out_err:
 	goto out;
 }
 
+static void fill_icmp_param_data(struct icmp_bxm *param,
+				 struct icmphdr *src,
+				 u8 type, u8 code)
+{
+	struct icmphdr *dst = &param->data.icmph;
+
+	dst->type = type;
+	dst->code = code;
+	dst->un.gateway = get_unaligned(&src->un.gateway);
+}
+
 /*
  *	Handle ICMP_ECHO ("ping") requests.
  *
@@ -783,8 +795,10 @@ static void icmp_echo(struct sk_buff *skb)
 	if (!sysctl_icmp_echo_ignore_all) {
 		struct icmp_bxm icmp_param;
 
-		icmp_param.data.icmph	   = *icmp_hdr(skb);
-		icmp_param.data.icmph.type = ICMP_ECHOREPLY;
+		fill_icmp_param_data(&icmp_param,
+				     icmp_hdr(skb),
+				     ICMP_ECHOREPLY,
+				     icmp_hdr(skb)->code);
 		icmp_param.skb		   = skb;
 		icmp_param.offset	   = 0;
 		icmp_param.data_len	   = skb->len;
@@ -819,9 +833,8 @@ static void icmp_timestamp(struct sk_buff *skb)
 	icmp_param.data.times[2] = icmp_param.data.times[1];
 	if (skb_copy_bits(skb, 0, &icmp_param.data.times[0], 4))
 		BUG();
-	icmp_param.data.icmph	   = *icmp_hdr(skb);
-	icmp_param.data.icmph.type = ICMP_TIMESTAMPREPLY;
-	icmp_param.data.icmph.code = 0;
+	fill_icmp_param_data(&icmp_param, icmp_hdr(skb),
+			     ICMP_TIMESTAMPREPLY, 0);
 	icmp_param.skb		   = skb;
 	icmp_param.offset	   = 0;
 	icmp_param.data_len	   = 0;
-
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