unaligned ip header due to ip_route_me_harder()->pskb_expand_head()

Hi,

Recently on our chip (arch/tile), a panic caused by unaligned data access was observed. The stack looks like this:

frame 0: 0xfd247ef0 ipt_do_table+0x240/0x850 (sp 0xf6a6fb40) frame 1: 0xfd4a44a8 nf_iterate+0xe8/0x188 (sp 0xf6a6fbd0) frame 2: 0xfd4ab850 nf_hook_slow+0xa0/0x180 (sp 0xf6a6fbf8) frame 3: 0xfd0c02a8 ip_local_out+0x28/0x88 (sp 0xf6a6fc30) frame 4: 0xfd25b778 ip_push_pending_frames+0x4f8/0x798 (sp 0xf6a6fc40) frame 5: 0xfd272298 icmp_send+0x670/0x700 (sp 0xf6a6fc60) frame 6: 0xfd593d28 ipv4_link_failure+0x28/0xa8 (sp 0xf6a6fd50) frame 7: 0xfd5aae90 arp_error_report+0x70/0x90 (sp 0xf6a6fd60) frame 8: 0xfd483f28 neigh_invalidate+0x128/0x1b0 (sp 0xf6a6fd70) frame 9: 0xfd3b5410 neigh_timer_handler.cold+0x2a0/0x2e0 (sp 0xf6a6fd98) frame 10: 0xfd028cd8 run_timer_softirq+0x2d8/0x4a8 (sp 0xf6a6fdb0) frame 11: 0xfd02bfe0 __do_softirq+0x1e0/0x330 (sp 0xf6a6fe08) frame 12: 0xfd034808 do_softirq+0xc8/0x160 (sp 0xf6a6fe50) frame 13: 0xfd034658 irq_exit+0x98/0x130 (sp 0xf6a6fe60) frame 14: 0xfd031cc0 do_timer_interrupt+0xc0/0xf8 (sp 0xf6a6fe68) frame 15: 0xfd3b1da8 handle_interrupt+0x2d8/0x2e0 (sp 0xf6a6fe80) <interrupt 25 while in kernel mode>
frame 16: 0xfd5cb350 _cpu_idle_nap+0x0/0x10 (sp 0xf6a6ffd0) frame 17: 0xfd06b020 cpu_idle+0x1a0/0x3b8 (sp 0xf6a6ffd0)

int ip_route_me_harder(struct sk_buff *skb, unsigned addr_type)

{
        ......
	/* Change in oif may mean change in hh_len. */
	hh_len = skb_dst(skb)->dev->hard_header_len;
	if (skb_headroom(skb) < hh_len &&
	    pskb_expand_head(skb, hh_len - skb_headroom(skb), 0, GFP_ATOMIC))
		return -1;
}

During testing, our chip acts as NAT. the interface for internal hosts are configured with vlan. When we try to ping a unknown external host from internal hosts, it panic immediately due to unaligned data access. In this case, hh_len is equal to 18 (vlan mac header length), headroom (skb_headroom(skb)) for this packet is 16 so after calling pskb_expand_head() the ip header becomes to be unaligned. This seems to be a bug in netfilter. Modifying pskb_expand_head() like this can resolve this issue.

+ pskb_expand_head(skb, LL_RESERVED_SPACE(skb_dst(skb)->dev) - skb_headroom(skb), 0, GFP_ATOMIC))
- pskb_expand_head(skb, hh_len - skb_headroom(skb), 0, GFP_ATOMIC))

Quoted 36 lines

Hi,

Recently on our chip (arch/tile), a panic caused by unaligned data access was observed. The stack looks like this:

frame 0: 0xfd247ef0 ipt_do_table+0x240/0x850 (sp 0xf6a6fb40) frame 1: 0xfd4a44a8 nf_iterate+0xe8/0x188 (sp 0xf6a6fbd0) frame 2: 0xfd4ab850 nf_hook_slow+0xa0/0x180 (sp 0xf6a6fbf8) frame 3: 0xfd0c02a8 ip_local_out+0x28/0x88 (sp 0xf6a6fc30) frame 4: 0xfd25b778 ip_push_pending_frames+0x4f8/0x798 (sp 0xf6a6fc40) frame 5: 0xfd272298 icmp_send+0x670/0x700 (sp 0xf6a6fc60) frame 6: 0xfd593d28 ipv4_link_failure+0x28/0xa8 (sp 0xf6a6fd50) frame 7: 0xfd5aae90 arp_error_report+0x70/0x90 (sp 0xf6a6fd60) frame 8: 0xfd483f28 neigh_invalidate+0x128/0x1b0 (sp 0xf6a6fd70) frame 9: 0xfd3b5410 neigh_timer_handler.cold+0x2a0/0x2e0 (sp 0xf6a6fd98) frame 10: 0xfd028cd8 run_timer_softirq+0x2d8/0x4a8 (sp 0xf6a6fdb0) frame 11: 0xfd02bfe0 __do_softirq+0x1e0/0x330 (sp 0xf6a6fe08) frame 12: 0xfd034808 do_softirq+0xc8/0x160 (sp 0xf6a6fe50) frame 13: 0xfd034658 irq_exit+0x98/0x130 (sp 0xf6a6fe60) frame 14: 0xfd031cc0 do_timer_interrupt+0xc0/0xf8 (sp 0xf6a6fe68) frame 15: 0xfd3b1da8 handle_interrupt+0x2d8/0x2e0 (sp 0xf6a6fe80) <interrupt 25 while in kernel mode>
frame 16: 0xfd5cb350 _cpu_idle_nap+0x0/0x10 (sp 0xf6a6ffd0) frame 17: 0xfd06b020 cpu_idle+0x1a0/0x3b8 (sp 0xf6a6ffd0)

int ip_route_me_harder(struct sk_buff *skb, unsigned addr_type)

{
        ......
	/* Change in oif may mean change in hh_len. */
	hh_len = skb_dst(skb)->dev->hard_header_len;
	if (skb_headroom(skb) < hh_len &&
	    pskb_expand_head(skb, hh_len - skb_headroom(skb), 0, GFP_ATOMIC))
		return -1;
}

During testing, our chip acts as NAT. the interface for internal hosts are configured with vlan. When we try to ping a unknown external host from internal hosts, it panic immediately due to unaligned data access. In this case, hh_len is equal to 18 (vlan mac header length), headroom (skb_headroom(skb)) for this packet is 16 so after calling pskb_expand_head() the ip header becomes to be unaligned. This seems to be a bug in netfilter. Modifying pskb_expand_head() like this can resolve this issue.

Quoted 3 lines

+ pskb_expand_head(skb, LL_RESERVED_SPACE(skb_dst(skb)->dev) - skb_headroom(skb), 0, GFP_ATOMIC))
- pskb_expand_head(skb, hh_len - skb_headroom(skb), 0, GFP_ATOMIC))

or

pskb_expand_head(skb, HH_DATA_ALIGN(hh_len - skb_headroom(skb)), 0, GFP_ATOMIC))

Quoted 3 lines

or

pskb_expand_head(skb, HH_DATA_ALIGN(hh_len - skb_headroom(skb)), 0, GFP_ATOMIC))

Thanks. This looks better.

Quoted 6 lines

Quoted 4 lines

or

pskb_expand_head(skb, HH_DATA_ALIGN(hh_len - skb_headroom(skb)), 0, GFP_ATOMIC))

Thanks. This looks better.

Could you submit an official patch ?

Thanks !

Quoted 9 lines · Paul Guo

Quoted 6 lines

Quoted 4 lines

or

pskb_expand_head(skb, HH_DATA_ALIGN(hh_len - skb_headroom(skb)), 0, GFP_ATOMIC))

Thanks. This looks better.

Quoted 3 lines

Could you submit an official patch ?

Thanks !

OK. I tried to clone a netfilter branch firstly, but failed. Cloning the network branch is OK. It looks there is something wrong with the netfilter git branch, or am I doing something wrong? Thanks.

git://1984.lsi.us.es/net/.git

Quoted 1 line

git://1984.lsi.us.es/net/.git

OK. Here is the patch based on above git tree.

Signed-off-by Paul Guo <[email protected]>

---
 net/ipv4/netfilter.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

net/ipv4/netfilter.c +0 -0

index 929b27b..0dcb0fa 100644

Quoted 23 lines

Quoted 1 line

git://1984.lsi.us.es/net/.git

OK. Here is the patch based on above git tree.

Signed-off-by Paul Guo <[email protected]>

---
 net/ipv4/netfilter.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

net/ipv4/netfilter.c +2 -1

index 929b27b..0dcb0fa 100644
@@ -63,7 +63,8 @@ int ip_route_me_harder(struct sk_buff *skb, unsigned addr_type)
 	/* Change in oif may mean change in hh_len. */
 	hh_len = skb_dst(skb)->dev->hard_header_len;
 	if (skb_headroom(skb) < hh_len &&
-	    pskb_expand_head(skb, hh_len - skb_headroom(skb), 0, GFP_ATOMIC))
+	    pskb_expand_head(skb, HH_DATA_ALIGN(hh_len - skb_headroom(skb)),
+	    		     0, GFP_ATOMIC))
 		return -1;
 

net/ipv4/netfilter.c int ip_route_me_harder(struct sk_buff *skb, unsigned addr_type) · line 70

Quoted 1 line

net/ipv4/netfilter.c +0 -0

 	return 0;

OK, but could you please add a changelog as well ?

Your initial message contained a lot of useful information that could help future bug hunting ;)

Quoted 23 lines · Paul Guo

Quoted 1 line

git://1984.lsi.us.es/net/.git

OK. Here is the patch based on above git tree.

Signed-off-by Paul Guo <[email protected]>

---
 net/ipv4/netfilter.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

net/ipv4/netfilter.c +2 -1

index 929b27b..0dcb0fa 100644
@@ -63,7 +63,8 @@ int ip_route_me_harder(struct sk_buff *skb, unsigned addr_type)
 	/* Change in oif may mean change in hh_len. */
 	hh_len = skb_dst(skb)->dev->hard_header_len;
 	if (skb_headroom(skb) < hh_len &&
-	    pskb_expand_head(skb, hh_len - skb_headroom(skb), 0, GFP_ATOMIC))
+	    pskb_expand_head(skb, HH_DATA_ALIGN(hh_len - skb_headroom(skb)),
+	    		     0, GFP_ATOMIC))
 		return -1;
 

net/ipv4/netfilter.c int ip_route_me_harder(struct sk_buff *skb, unsigned addr_type) · line 70

Quoted 1 line

net/ipv4/netfilter.c +0 -0

 	return 0;

OK, but could you please add a changelog as well ?

Your initial message contained a lot of useful information that could help future bug hunting ;)

Hi Eric,

Quoted 28 lines · Eric Dumazet

Quoted 15 lines · Paul Guo

Quoted 1 line

git://1984.lsi.us.es/net/.git

OK. Here is the patch based on above git tree.

Signed-off-by Paul Guo <[email protected]>

---
 net/ipv4/netfilter.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

net/ipv4/netfilter.c +0 -0

index 929b27b..0dcb0fa 100644

net/ipv4/netfilter.c int ip_route_me_harder(struct sk_buff *skb, unsigned addr_type) · line 70

Quoted 9 lines

net/ipv4/netfilter.c +2 -1

@@ -63,7 +63,8 @@ int ip_route_me_harder(struct sk_buff *skb, unsigned addr_type)
 	/* Change in oif may mean change in hh_len. */
 	hh_len = skb_dst(skb)->dev->hard_header_len;
 	if (skb_headroom(skb) < hh_len &&
-	    pskb_expand_head(skb, hh_len - skb_headroom(skb), 0, GFP_ATOMIC))
+	    pskb_expand_head(skb, HH_DATA_ALIGN(hh_len - skb_headroom(skb)),
+	    		     0, GFP_ATOMIC))
 		return -1;
 
 	return 0;

OK, but could you please add a changelog as well ?

Quoted 2 lines

Your initial message contained a lot of useful information that could help future bug hunting ;)

BTW, did you acked this patch?

Let me know, to include it in the changelog.

This patch tries to fix the following issue in netfilter:
ip_route_me_harder()->pskb_expand_head() rellocates new header with additional head room which can break the alignment of the original packet header.

In one of my NAT test case, the NIC port for internal hosts is configured with vlan and the port for external hosts is with general configuration. If we ping an external "unknown" hosts from an internal host, an icmp packet will be sent. We find that in icmp_send()->...->ip_route_me_harder()->pskb_expand_head(), hh_len=18 and current headroom (skb_headroom(skb)) of the packet is 16. After calling pskb_expand_head() the packet header becomes to be unaligned and then our system (arch/tile) panics immediately.

Here is the patch:

Signed-off-by Paul Guo <[email protected]>