[PATCH net v2] ipvlan: fix crash when master is set in loopback mode

[PATCH net v2] ipvlan: fix crash when master is set in loopback mode

[Mahesh Bandewar]

From: Mahesh Bandewar <maheshb@google.com>

In an IPvlan setup when master is set in loopback mode e.g.

  ethtool -K eth0 set loopback on

  where eth0 is master device for IPvlan setup.

The failure actually happens while processing mulitcast packets
but that's a result of unconditionally queueing packets without
ensuring ether-header is part of the linear part of skb.

This patch forces this check at the reception and drops packets
which fail this check before queuing them.

------------[ cut here ]------------
kernel BUG at include/linux/skbuff.h:1737!
Call Trace:
 [<ffffffff921fbbc2>] dev_forward_skb+0x92/0xd0
 [<ffffffffc031ac65>] ipvlan_process_multicast+0x395/0x4c0 [ipvlan]
 [<ffffffffc031a9a7>] ? ipvlan_process_multicast+0xd7/0x4c0 [ipvlan]
 [<ffffffff91cdfea7>] ? process_one_work+0x147/0x660
 [<ffffffff91cdff09>] process_one_work+0x1a9/0x660
 [<ffffffff91cdfea7>] ? process_one_work+0x147/0x660
 [<ffffffff91ce086d>] worker_thread+0x11d/0x360
 [<ffffffff91ce0750>] ? rescuer_thread+0x350/0x350
 [<ffffffff91ce960b>] kthread+0xdb/0xe0
 [<ffffffff91c05c70>] ? _raw_spin_unlock_irq+0x30/0x50
 [<ffffffff91ce9530>] ? flush_kthread_worker+0xc0/0xc0
 [<ffffffff92348b7a>] ret_from_fork+0x9a/0xd0
 [<ffffffff91ce9530>] ? flush_kthread_worker+0xc0/0xc0

Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
v1->v2: commit log update

 drivers/net/ipvlan/ipvlan_core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ipvlan/ipvlan_core.c b/drivers/net/ipvlan/ipvlan_core.c
index b4e990743e1d..4294fc1f5564 100644
--- a/drivers/net/ipvlan/ipvlan_core.c
+++ b/drivers/net/ipvlan/ipvlan_core.c
@@ -660,6 +660,9 @@ rx_handler_result_t ipvlan_handle_frame(struct sk_buff **pskb)
 	if (!port)
 		return RX_HANDLER_PASS;
 
+	if (unlikely(!pskb_may_pull(skb, sizeof(struct ethhdr))))
+		goto out;
+
 	switch (port->mode) {
 	case IPVLAN_MODE_L2:
 		return ipvlan_handle_mode_l2(pskb, port);
@@ -672,6 +675,8 @@ rx_handler_result_t ipvlan_handle_frame(struct sk_buff **pskb)
 	/* Should not reach here */
 	WARN_ONCE(true, "ipvlan_handle_frame() called for mode = [%hx]\n",
 			  port->mode);
+
+out:
 	kfree_skb(skb);
 	return RX_HANDLER_CONSUMED;
 }
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH net v2] ipvlan: fix crash when master is set in loopback mode

[Eric Dumazet]

On Sun, 2016-12-18 at 11:00 -0800, Mahesh Bandewar wrote:
> From: Mahesh Bandewar <maheshb@google.com>
> 
> In an IPvlan setup when master is set in loopback mode e.g.
> 
>   ethtool -K eth0 set loopback on
> 
>   where eth0 is master device for IPvlan setup.
> 
> The failure actually happens while processing mulitcast packets
> but that's a result of unconditionally queueing packets without
> ensuring ether-header is part of the linear part of skb.
> 
> This patch forces this check at the reception and drops packets
> which fail this check before queuing them.
> 
> ------------[ cut here ]------------
> kernel BUG at include/linux/skbuff.h:1737!
> Call Trace:
>  [<ffffffff921fbbc2>] dev_forward_skb+0x92/0xd0
>  [<ffffffffc031ac65>] ipvlan_process_multicast+0x395/0x4c0 [ipvlan]
>  [<ffffffffc031a9a7>] ? ipvlan_process_multicast+0xd7/0x4c0 [ipvlan]
>  [<ffffffff91cdfea7>] ? process_one_work+0x147/0x660
>  [<ffffffff91cdff09>] process_one_work+0x1a9/0x660
>  [<ffffffff91cdfea7>] ? process_one_work+0x147/0x660
>  [<ffffffff91ce086d>] worker_thread+0x11d/0x360
>  [<ffffffff91ce0750>] ? rescuer_thread+0x350/0x350
>  [<ffffffff91ce960b>] kthread+0xdb/0xe0
>  [<ffffffff91c05c70>] ? _raw_spin_unlock_irq+0x30/0x50
>  [<ffffffff91ce9530>] ? flush_kthread_worker+0xc0/0xc0
>  [<ffffffff92348b7a>] ret_from_fork+0x9a/0xd0
>  [<ffffffff91ce9530>] ? flush_kthread_worker+0xc0/0xc0
> 
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
> ---
> v1->v2: commit log update
> 
>  drivers/net/ipvlan/ipvlan_core.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/ipvlan/ipvlan_core.c b/drivers/net/ipvlan/ipvlan_core.c
> index b4e990743e1d..4294fc1f5564 100644
> --- a/drivers/net/ipvlan/ipvlan_core.c
> +++ b/drivers/net/ipvlan/ipvlan_core.c
> @@ -660,6 +660,9 @@ rx_handler_result_t ipvlan_handle_frame(struct sk_buff **pskb)
>  	if (!port)
>  		return RX_HANDLER_PASS;
>  
> +	if (unlikely(!pskb_may_pull(skb, sizeof(struct ethhdr))))
> +		goto out;
> +
>  	switch (port->mode) {
>  	case IPVLAN_MODE_L2:
>  		return ipvlan_handle_mode_l2(pskb, port);
> @@ -672,6 +675,8 @@ rx_handler_result_t ipvlan_handle_frame(struct sk_buff **pskb)
>  	/* Should not reach here */
>  	WARN_ONCE(true, "ipvlan_handle_frame() called for mode = [%hx]\n",
>  			  port->mode);
> +
> +out:
>  	kfree_skb(skb);
>  	return RX_HANDLER_CONSUMED;
>  }


There is something funky here.

When rx_handler() is called, we must have pulled Ethernet header
already.

Is this because RX_HANDLER_ANOTHER is returned in some cases, while it
should not ?

It looks like you added a pskb_may_pull() at a wrong place. Sure it
might fix the crash, but it looks weird here.

Re: [PATCH net v2] ipvlan: fix crash when master is set in loopback mode

[David Miller]

From: Mahesh Bandewar <mahesh@bandewar.net>
Date: Sun, 18 Dec 2016 11:00:25 -0800

> From: Mahesh Bandewar <maheshb@google.com>
> 
> In an IPvlan setup when master is set in loopback mode e.g.
> 
>   ethtool -K eth0 set loopback on
> 
>   where eth0 is master device for IPvlan setup.
> 
> The failure actually happens while processing mulitcast packets
> but that's a result of unconditionally queueing packets without
> ensuring ether-header is part of the linear part of skb.
> 
> This patch forces this check at the reception and drops packets
> which fail this check before queuing them.
...
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
> ---
> v1->v2: commit log update

Like Eric, I still do not like this change nor the explanation.

Whether in loopback mode or not, your explanation makes no sense at
all.

If the packet comes from the ethernet device, the freakin' ethernet
header is there in the linear SKB area.  No pulling should be needed
whatsoever.

Something creates this bad situation where the ethernet header is not
there, but you have not explained that sufficiently yet.

Thank you.