datapath: Avoid accesses past the end of skbuff data in actions.

Some of the flow actions that modify skbuff data did not check that the
skbuff was long enough before doing so.  This commit fixes that problem.

Previously, the strategy for avoiding this was to only indicate the layer-3
nw_proto field in the flow if the corresponding layer-4 header was fully
present, so that if, for example, nw_proto was IPPROTO_TCP, this meant
that a TCP header was present.  The original motivation for this patch was
to add corresponding code to only indicate a layer-2 dl_type if the
corresponding layer-3 header was fully present.  But I'm now convinced that
this approach is conceptually wrong, because the meaning of a layer-N
header should not be affected by the meaning of a layer-(N+1) header.

This commit switches to a new approach.  Now, when a header is missing, its
fields in the flow are simply zeroed and have no effect on the "type" field
for the outer header.  Responsibility for ensuring that a header is fully
present is now shifted to the actions that wish to modify that header.

Signed-off-by: Ben Pfaff <blp@nicira.com>

diff --git a/datapath/actions.c b/datapath/actions.c
index 943d7570569b6c876fef650b0d38a88160402c05..53e05872cd1efeb756797f3d27f39d0e13c53135 100644 (file)
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -53,7 +53,7 @@ static struct sk_buff *vlan_pull_tag(struct sk_buff *skb)
        struct ethhdr *eh;
 
        /* Verify we were given a vlan packet */
-       if (vh->h_vlan_proto != htons(ETH_P_8021Q))
+       if (vh->h_vlan_proto != htons(ETH_P_8021Q) || skb->len < VLAN_ETH_HLEN)
                return skb;
 
        if (OVS_CB(skb)->ip_summed == OVS_CSUM_COMPLETE)
@@ -91,8 +91,14 @@ static struct sk_buff *modify_vlan_tci(struct datapath *dp, struct sk_buff *skb,
 
        if (skb->protocol == htons(ETH_P_8021Q)) {
                /* Modify vlan id, but maintain other TCI values */
-               struct vlan_ethhdr *vh = vlan_eth_hdr(skb);
-               __be16 old_tci = vh->h_vlan_TCI;
+               struct vlan_ethhdr *vh;
+               __be16 old_tci;
+
+               if (skb->len < VLAN_ETH_HLEN)
+                       return skb;
+
+               vh = vlan_eth_hdr(skb);
+               old_tci = vh->h_vlan_TCI;
 
                vh->h_vlan_TCI = htons((ntohs(vh->h_vlan_TCI) & ~mask) | tci);
 
@@ -238,31 +244,51 @@ static void update_csum(__sum16 *sum, struct sk_buff *skb,
                                csum_unfold(*sum)));
 }
 
+static bool is_ip(struct sk_buff *skb, const struct odp_flow_key *key)
+{
+       return (key->dl_type == htons(ETH_P_IP) &&
+               skb->transport_header > skb->network_header);
+}
+
+static __sum16 *get_l4_checksum(struct sk_buff *skb, const struct odp_flow_key *key)
+{
+       int transport_len = skb->len - skb_transport_offset(skb);
+       if (key->nw_proto == IPPROTO_TCP) {
+               if (likely(transport_len >= sizeof(struct tcphdr)))
+                       return &tcp_hdr(skb)->check;
+       } else if (key->nw_proto == IPPROTO_UDP) {
+               if (likely(transport_len >= sizeof(struct udphdr)))
+                       return &udp_hdr(skb)->check;
+       }
+       return NULL;
+}
+
 static struct sk_buff *set_nw_addr(struct sk_buff *skb,
                                   const struct odp_flow_key *key,
                                   const struct odp_action_nw_addr *a,
                                   gfp_t gfp)
 {
-       if (key->dl_type != htons(ETH_P_IP))
+       struct iphdr *nh;
+       __sum16 *check;
+       __be32 *nwaddr;
+
+       if (unlikely(!is_ip(skb, key)))
                return skb;
 
        skb = make_writable(skb, 0, gfp);
-       if (skb) {
-               struct iphdr *nh = ip_hdr(skb);
-               u32 *f = a->type == ODPAT_SET_NW_SRC ? &nh->saddr : &nh->daddr;
-               u32 old = *f;
-               u32 new = a->nw_addr;
-
-               if (key->nw_proto == IPPROTO_TCP) {
-                       struct tcphdr *th = tcp_hdr(skb);
-                       update_csum(&th->check, skb, old, new, 1);
-               } else if (key->nw_proto == IPPROTO_UDP) {
-                       struct udphdr *th = udp_hdr(skb);
-                       update_csum(&th->check, skb, old, new, 1);
-               }
-               update_csum(&nh->check, skb, old, new, 0);
-               *f = new;
-       }
+       if (unlikely(!skb))
+               return NULL;
+
+       nh = ip_hdr(skb);
+       nwaddr = a->type == ODPAT_SET_NW_SRC ? &nh->saddr : &nh->daddr;
+
+       check = get_l4_checksum(skb, key);
+       if (likely(check))
+               update_csum(check, skb, *nwaddr, a->nw_addr, 1);
+       update_csum(&nh->check, skb, *nwaddr, a->nw_addr, 0);
+
+       *nwaddr = a->nw_addr;
+
        return skb;
 }
 
@@ -271,7 +297,7 @@ static struct sk_buff *set_nw_tos(struct sk_buff *skb,
                                   const struct odp_action_nw_tos *a,
                                   gfp_t gfp)
 {
-       if (key->dl_type != htons(ETH_P_IP))
+       if (unlikely(!is_ip(skb, key)))
                return skb;
 
        skb = make_writable(skb, 0, gfp);
@@ -283,8 +309,8 @@ static struct sk_buff *set_nw_tos(struct sk_buff *skb,
 
                /* Set the DSCP bits and preserve the ECN bits. */
                new = a->nw_tos | (nh->tos & INET_ECN_MASK);
-               update_csum(&nh->check, skb, htons((uint16_t)old),
-                               htons((uint16_t)new), 0);
+               update_csum(&nh->check, skb, htons((u16)old),
+                           htons((u16)new), 0);
                *f = new;
        }
        return skb;
@@ -294,28 +320,34 @@ static struct sk_buff *set_tp_port(struct sk_buff *skb,
                                   const struct odp_flow_key *key,
                                   const struct odp_action_tp_port *a, gfp_t gfp)
 {
-       int check_ofs;
+       struct udphdr *th;
+       __sum16 *check;
+       __be16 *port;
 
-       if (key->dl_type != htons(ETH_P_IP))
+       if (unlikely(!is_ip(skb, key)))
                return skb;
 
-       if (key->nw_proto == IPPROTO_TCP)
-               check_ofs = offsetof(struct tcphdr, check);
-       else if (key->nw_proto == IPPROTO_UDP)
-               check_ofs = offsetof(struct udphdr, check);
-       else
+       skb = make_writable(skb, 0, gfp);
+       if (unlikely(!skb))
+               return NULL;
+
+       /* Must follow make_writable() since that can move the skb data. */
+       check = get_l4_checksum(skb, key);
+       if (unlikely(!check))
                return skb;
 
-       skb = make_writable(skb, 0, gfp);
-       if (skb) {
-               struct udphdr *th = udp_hdr(skb);
-               u16 *f = a->type == ODPAT_SET_TP_SRC ? &th->source : &th->dest;
-               u16 old = *f;
-               u16 new = a->tp_port;
-               update_csum((u16*)(skb_transport_header(skb) + check_ofs), 
-                               skb, old, new, 0);
-               *f = new;
-       }
+       /*
+        * Update port and checksum.
+        *
+        * This is OK because source and destination port numbers are at the
+        * same offsets in both UDP and TCP headers, and get_l4_checksum() only
+        * supports those protocols.
+        */
+       th = udp_hdr(skb);
+       port = a->type == ODPAT_SET_TP_SRC ? &th->source : &th->dest;
+       update_csum(check, skb, *port, a->tp_port, 0);
+       *port = a->tp_port;
+
        return skb;
 }
 

diff --git a/datapath/flow.c b/datapath/flow.c
index 7d3bc04cda6826ca21b3d85bbea3c2537a034987..b80e0e295082b148085815d75e07d9c19b141491 100644 (file)
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -311,22 +311,12 @@ int flow_extract(struct sk_buff *skb, u16 in_port, struct odp_flow_key *key)
                                        struct tcphdr *tcp = tcp_hdr(skb);
                                        key->tp_src = tcp->source;
                                        key->tp_dst = tcp->dest;
-                               } else {
-                                       /* Avoid tricking other code into
-                                        * thinking that this packet has an L4
-                                        * header. */
-                                       key->nw_proto = 0;
                                }
                        } else if (key->nw_proto == IPPROTO_UDP) {
                                if (udphdr_ok(skb)) {
                                        struct udphdr *udp = udp_hdr(skb);
                                        key->tp_src = udp->source;
                                        key->tp_dst = udp->dest;
-                               } else {
-                                       /* Avoid tricking other code into
-                                        * thinking that this packet has an L4
-                                        * header. */
-                                       key->nw_proto = 0;
                                }
                        } else if (key->nw_proto == IPPROTO_ICMP) {
                                if (icmphdr_ok(skb)) {
@@ -336,11 +326,6 @@ int flow_extract(struct sk_buff *skb, u16 in_port, struct odp_flow_key *key)
                                         * in 16-bit network byte order. */
                                        key->tp_src = htons(icmp->type);
                                        key->tp_dst = htons(icmp->code);
-                               } else {
-                                       /* Avoid tricking other code into
-                                        * thinking that this packet has an L4
-                                        * header. */
-                                       key->nw_proto = 0;
                                }
                        }
                } else {

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 4a5b2a7a2dd22b13f34bba701852e1344ac8a608..ade658d8d7403c10118c27911bee7cfaefa9dd51 100644 (file)
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -1151,19 +1151,25 @@ dp_netdev_set_dl_dst(struct ofpbuf *packet, const uint8_t dl_addr[ETH_ADDR_LEN])
     memcpy(eh->eth_dst, dl_addr, sizeof eh->eth_dst);
 }
 
+static bool
+is_ip(const struct ofpbuf *packet, const flow_t *key)
+{
+    return key->dl_type == htons(ETH_TYPE_IP) && packet->l4;
+}
+
 static void
 dp_netdev_set_nw_addr(struct ofpbuf *packet, const flow_t *key,
                       const struct odp_action_nw_addr *a)
 {
-    if (key->dl_type == htons(ETH_TYPE_IP)) {
+    if (is_ip(packet, key)) {
         struct ip_header *nh = packet->l3;
         uint32_t *field;
 
         field = a->type == ODPAT_SET_NW_SRC ? &nh->ip_src : &nh->ip_dst;
-        if (key->nw_proto == IP_TYPE_TCP) {
+        if (key->nw_proto == IP_TYPE_TCP && packet->l7) {
             struct tcp_header *th = packet->l4;
             th->tcp_csum = recalc_csum32(th->tcp_csum, *field, a->nw_addr);
-        } else if (key->nw_proto == IP_TYPE_UDP) {
+        } else if (key->nw_proto == IP_TYPE_UDP && packet->l7) {
             struct udp_header *uh = packet->l4;
             if (uh->udp_csum) {
                 uh->udp_csum = recalc_csum32(uh->udp_csum, *field, a->nw_addr);
@@ -1181,7 +1187,7 @@ static void
 dp_netdev_set_nw_tos(struct ofpbuf *packet, const flow_t *key,
                      const struct odp_action_nw_tos *a)
 {
-    if (key->dl_type == htons(ETH_TYPE_IP)) {
+    if (is_ip(packet, key)) {
         struct ip_header *nh = packet->l3;
         uint8_t *field = &nh->ip_tos;
 
@@ -1198,14 +1204,14 @@ static void
 dp_netdev_set_tp_port(struct ofpbuf *packet, const flow_t *key,
                       const struct odp_action_tp_port *a)
 {
-    if (key->dl_type == htons(ETH_TYPE_IP)) {
+       if (is_ip(packet, key)) {
         uint16_t *field;
-        if (key->nw_proto == IPPROTO_TCP) {
+        if (key->nw_proto == IPPROTO_TCP && packet->l7) {
             struct tcp_header *th = packet->l4;
             field = a->type == ODPAT_SET_TP_SRC ? &th->tcp_src : &th->tcp_dst;
             th->tcp_csum = recalc_csum16(th->tcp_csum, *field, a->tp_port);
             *field = a->tp_port;
-        } else if (key->nw_proto == IPPROTO_UDP) {
+        } else if (key->nw_proto == IPPROTO_UDP && packet->l7) {
             struct udp_header *uh = packet->l4;
             field = a->type == ODPAT_SET_TP_SRC ? &uh->udp_src : &uh->udp_dst;
             uh->udp_csum = recalc_csum16(uh->udp_csum, *field, a->tp_port);

diff --git a/lib/flow.c b/lib/flow.c
index d545661ab0b27fed2f21e0ccf320f17fc17e1f4c..1fab02fd6e5beb924fcd02ab6a7df75c9c9db1d5 100644 (file)
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -120,9 +120,23 @@ parse_ethertype(struct ofpbuf *b)
     return llc->snap.snap_type;
 }
 
-/* Returns 1 if 'packet' is an IP fragment, 0 otherwise.
- * 'tun_id' is in network byte order, while 'in_port' is in host byte order.
- * These byte orders are the same as they are in struct odp_flow_key. */
+/* 'tun_id' is in network byte order, while 'in_port' is in host byte order.
+ * These byte orders are the same as they are in struct odp_flow_key.
+ *
+ * Initializes packet header pointers as follows:
+ *
+ *    - packet->l2 to the start of the Ethernet header.
+ *
+ *    - packet->l3 to just past the Ethernet header, or just past the
+ *      vlan_header if one is present, to the first byte of the payload of the
+ *      Ethernet frame.
+ *
+ *    - packet->l4 to just past the IPv4 header, if one is present and has a
+ *      correct length, and otherwise NULL.
+ *
+ *    - packet->l7 to just past the TCP or UDP or ICMP header, if one is
+ *      present and has a correct length, and otherwise NULL.
+ */
 int
 flow_extract(struct ofpbuf *packet, uint32_t tun_id, uint16_t in_port,
              flow_t *flow)
@@ -176,10 +190,6 @@ flow_extract(struct ofpbuf *packet, uint32_t tun_id, uint16_t in_port,
                         flow->tp_src = tcp->tcp_src;
                         flow->tp_dst = tcp->tcp_dst;
                         packet->l7 = b.data;
-                    } else {
-                        /* Avoid tricking other code into thinking that
-                         * this packet has an L4 header. */
-                        flow->nw_proto = 0;
                     }
                 } else if (flow->nw_proto == IP_TYPE_UDP) {
                     const struct udp_header *udp = pull_udp(&b);
@@ -187,10 +197,6 @@ flow_extract(struct ofpbuf *packet, uint32_t tun_id, uint16_t in_port,
                         flow->tp_src = udp->udp_src;
                         flow->tp_dst = udp->udp_dst;
                         packet->l7 = b.data;
-                    } else {
-                        /* Avoid tricking other code into thinking that
-                         * this packet has an L4 header. */
-                        flow->nw_proto = 0;
                     }
                 } else if (flow->nw_proto == IP_TYPE_ICMP) {
                     const struct icmp_header *icmp = pull_icmp(&b);
@@ -198,10 +204,6 @@ flow_extract(struct ofpbuf *packet, uint32_t tun_id, uint16_t in_port,
                         flow->icmp_type = htons(icmp->icmp_type);
                         flow->icmp_code = htons(icmp->icmp_code);
                         packet->l7 = b.data;
-                    } else {
-                        /* Avoid tricking other code into thinking that
-                         * this packet has an L4 header. */
-                        flow->nw_proto = 0;
                     }
                 }
             } else {