Re: [net-next v2] vxlan: fix ND proxy when skb doesn't have transport header offset

[Vincent Bernat]

 ❦ 30 mars 2017 06:36 -0700, Eric Dumazet  :

>>> Parsing of neighbor discovery options is done earlier to ignore the
>>> whole packet in case of a malformed option. Moreover, the assumption the
>>> skb was linear is removed and options are extracted with
>>> skb_header_pointer() as well. The check on the source link-layer address
>>> option is also more strict (for Ethernet, we expect the length field to
>>> be 1).
>>
>> There is some parsing implemented in net/ipv6/ndisc.c, notably
>> ndisc_parse_options(). I don't know if this is a good idea to reuse
>> that: it may have the expectation that some IP processing has already
>> been done (for example, the IPv6 length has already been checked, the
>> SKB is expected to be linear).
>
> Forcing ICMP being linear is probably fine.
>
> Prior to 91269e390d062b52 ("vxlan: using pskb_may_pull as early as possible")
> this was happening (at the wrong place) in neigh_reduce() doing a :
> if (!pskb_may_pull(skb, skb->len))
>  goto out;

OK, I'll simplify the patch then.

> So the following would be ok (while incomplete of course)
>
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index 
> bdb6ae16d4a85bf9539199e189011bce104ba51a..cd032819d4a36d5ca94739f20f947f3f5a31aba3
> 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -2243,12 +2243,13 @@ static netdev_tx_t vxlan_xmit(struct sk_buff
> *skb, struct net_device *dev)
> return arp_reduce(dev, skb, vni);
>  #if IS_ENABLED(CONFIG_IPV6)
> else if (ntohs(eth->h_proto) == ETH_P_IPV6 &&
> -pskb_may_pull(skb, sizeof(struct ipv6hdr)
> -  + sizeof(struct nd_msg)) &&
> +skb->len >= sizeof(struct ipv6hdr) +
> +sizeof(struct nd_msg) &&
> +pskb_may_pull(skb, skb->len) &&
>  ipv6_hdr(skb)->nexthdr == IPPROTO_ICMPV6) {
> struct nd_msg *msg;
>
> -   msg = (struct nd_msg
> *)skb_transport_header(skb);
> +   msg = (struct nd_msg *)(ipv6_hdr(skb) + 1);
> if (msg->icmph.icmp6_code == 0 &&
> msg->icmph.icmp6_type ==
> NDISC_NEIGHBOUR_SOLICITATION)
> return neigh_reduce(dev, skb, vni);

pskb_may_pull() is called while we only know this is an IPv6 packet, not
an ICMPv6 one. I'll keep skb_header_pointer to handle IPv6 header, then
I'll pull the whole ICMP packet (unless I am mistaken, of course).
-- 
The devil can cite Scripture for his purpose.
-- William Shakespeare, "The Merchant of Venice"

Re: [net-next v2] vxlan: fix ND proxy when skb doesn't have transport header offset

[Eric Dumazet]

On Wed, Mar 29, 2017 at 11:41 PM, Vincent Bernat  wrote:
>  ❦ 29 mars 2017 22:47 +0200, Vincent Bernat  :
>
>> Parsing of neighbor discovery options is done earlier to ignore the
>> whole packet in case of a malformed option. Moreover, the assumption the
>> skb was linear is removed and options are extracted with
>> skb_header_pointer() as well. The check on the source link-layer address
>> option is also more strict (for Ethernet, we expect the length field to
>> be 1).
>
> There is some parsing implemented in net/ipv6/ndisc.c, notably
> ndisc_parse_options(). I don't know if this is a good idea to reuse
> that: it may have the expectation that some IP processing has already
> been done (for example, the IPv6 length has already been checked, the
> SKB is expected to be linear).

Forcing ICMP being linear is probably fine.

Prior to 91269e390d062b52 ("vxlan: using pskb_may_pull as early as possible")
this was happening (at the wrong place) in neigh_reduce() doing a :
if (!pskb_may_pull(skb, skb->len))
 goto out;

So the following would be ok (while incomplete of course)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 
bdb6ae16d4a85bf9539199e189011bce104ba51a..cd032819d4a36d5ca94739f20f947f3f5a31aba3
100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -2243,12 +2243,13 @@ static netdev_tx_t vxlan_xmit(struct sk_buff
*skb, struct net_device *dev)
return arp_reduce(dev, skb, vni);
 #if IS_ENABLED(CONFIG_IPV6)
else if (ntohs(eth->h_proto) == ETH_P_IPV6 &&
-pskb_may_pull(skb, sizeof(struct ipv6hdr)
-  + sizeof(struct nd_msg)) &&
+skb->len >= sizeof(struct ipv6hdr) +
+sizeof(struct nd_msg) &&
+pskb_may_pull(skb, skb->len) &&
 ipv6_hdr(skb)->nexthdr == IPPROTO_ICMPV6) {
struct nd_msg *msg;

-   msg = (struct nd_msg
*)skb_transport_header(skb);
+   msg = (struct nd_msg *)(ipv6_hdr(skb) + 1);
if (msg->icmph.icmp6_code == 0 &&
msg->icmph.icmp6_type ==
NDISC_NEIGHBOUR_SOLICITATION)
return neigh_reduce(dev, skb, vni);

Re: [net-next v2] vxlan: fix ND proxy when skb doesn't have transport header offset

[Vincent Bernat]

 ❦ 29 mars 2017 22:47 +0200, Vincent Bernat  :

> Parsing of neighbor discovery options is done earlier to ignore the
> whole packet in case of a malformed option. Moreover, the assumption the
> skb was linear is removed and options are extracted with
> skb_header_pointer() as well. The check on the source link-layer address
> option is also more strict (for Ethernet, we expect the length field to
> be 1).

There is some parsing implemented in net/ipv6/ndisc.c, notably
ndisc_parse_options(). I don't know if this is a good idea to reuse
that: it may have the expectation that some IP processing has already
been done (for example, the IPv6 length has already been checked, the
SKB is expected to be linear).
-- 
Watch out for off-by-one errors.
- The Elements of Programming Style (Kernighan & Plauger)

[net-next v2] vxlan: fix ND proxy when skb doesn't have transport header offset

[Vincent Bernat]

When an incoming frame is tagged or when GRO is disabled, the skb
handled to vxlan_xmit() doesn't contain a valid transport header
offset. This makes ND proxying fail.

Do not rely on skb_transport_offset(). Instead, use offsets from
skb_network_offset() with skb_header_pointer() to extract appropriate
information to detect an ICMPv6 neighbor solicitation and to extract the
information needed to build the answer.

Duplicate checks at the beginning of neigh_reduce() are removed.

Parsing of neighbor discovery options is done earlier to ignore the
whole packet in case of a malformed option. Moreover, the assumption the
skb was linear is removed and options are extracted with
skb_header_pointer() as well. The check on the source link-layer address
option is also more strict (for Ethernet, we expect the length field to
be 1).

After this change, the vxlan module is correctly able to answer to
VLAN-encapsulated frames:

22:02:46.332573 50:54:33:00:00:02 > 33:33:00:00:00:02, ethertype 802.1Q 
(0x8100), length 74: vlan 100, p 0,ethertype IPv6, (hlim 255, next-header 
ICMPv6 (58) payload length: 16) fe80::5254:33ff:fe00:2 > ff02::2: [icmp6 sum 
ok] ICMP6, router solicitation, length 16
  source link-address option (1), length 8 (1): 50:54:33:00:00:02
0x:  5054 3300 0002
22:02:47.846631 50:54:33:00:00:08 > 33:33:00:00:00:02, ethertype 802.1Q 
(0x8100), length 74: vlan 100, p 0,ethertype IPv6, (hlim 255, next-header 
ICMPv6 (58) payload length: 16) fe80::5254:33ff:fe00:8 > ff02::2: [icmp6 sum 
ok] ICMP6, router solicitation, length 16
  source link-address option (1), length 8 (1): 50:54:33:00:00:08
0x:  5054 3300 0008

Signed-off-by: Vincent Bernat 
---
 drivers/net/vxlan.c | 85 ++---
 1 file changed, 48 insertions(+), 37 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 1e54fb5c883a..f40272785aa2 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -1504,20 +1504,43 @@ static int arp_reduce(struct net_device *dev, struct 
sk_buff *skb, __be32 vni)
 
 #if IS_ENABLED(CONFIG_IPV6)
 static struct sk_buff *vxlan_na_create(struct sk_buff *request,
-   struct neighbour *n, bool isrouter)
+  struct ipv6hdr *ip6h, struct nd_msg *ns,
+  struct neighbour *n, bool isrouter)
 {
struct net_device *dev = request->dev;
struct sk_buff *reply;
-   struct nd_msg *ns, *na;
+   struct nd_msg *na;
struct ipv6hdr *pip6;
-   u8 *daddr;
+   u8 *daddr, _daddr[ETH_ALEN];
int na_olen = 8; /* opt hdr + ETH_ALEN for target */
-   int ns_olen;
-   int i, len;
+   int len, remaining, offset;
 
if (dev == NULL)
return NULL;
 
+   /* Destination address is the "source link-layer address"
+* option if present and valid or the source Ethernet
+* address */
+   daddr = eth_hdr(request)->h_source;
+   remaining = htons(ip6h->payload_len) - sizeof(*ns);
+   offset = skb_network_offset(request) + sizeof(*ip6h) + sizeof(*ns);
+   while (remaining > sizeof(struct nd_opt_hdr)) {
+   struct nd_opt_hdr *ohdr, _ohdr;
+   ohdr = skb_header_pointer(request, offset, sizeof(_ohdr), 
&_ohdr);
+   if (!ohdr || !(len = ohdr->nd_opt_len<<3) || len > remaining)
+   return NULL;
+   if (ohdr->nd_opt_type == ND_OPT_SOURCE_LL_ADDR &&
+   len == na_olen) {
+   daddr = skb_header_pointer(request, offset + 
sizeof(_ohdr),
+  sizeof(_daddr), _daddr);
+   if (!daddr)
+   return NULL;
+   break;
+   }
+   remaining -= len;
+   offset += len;
+   }
+
len = LL_RESERVED_SPACE(dev) + sizeof(struct ipv6hdr) +
sizeof(*na) + na_olen + dev->needed_tailroom;
reply = alloc_skb(len, GFP_ATOMIC);
@@ -1530,16 +1553,6 @@ static struct sk_buff *vxlan_na_create(struct sk_buff 
*request,
skb_push(reply, sizeof(struct ethhdr));
skb_reset_mac_header(reply);
 
-   ns = (struct nd_msg *)skb_transport_header(request);
-
-   daddr = eth_hdr(request)->h_source;
-   ns_olen = request->len - skb_transport_offset(request) - sizeof(*ns);
-   for (i = 0; i < ns_olen-1; i += (ns->opt[i+1]<<3)) {
-   if (ns->opt[i] == ND_OPT_SOURCE_LL_ADDR) {
-   daddr = ns->opt + i + sizeof(struct nd_opt_hdr);
-   break;
-   }
-   }
 
/* Ethernet header */
ether_addr_copy(eth_hdr(reply)->h_dest, daddr);
@@ -1556,10 +1569,10 @@ static struct sk_buff *vxlan_na_create(struct sk_buff 
*request,
pip6 = ipv6_hdr(reply);
memset(pip6, 0, sizeof(struct ipv6hdr));