From: Ben Pfaff Date: Fri, 13 Aug 2010 17:46:12 +0000 (-0700) Subject: datapath: Avoid accesses past the end of skbuff data in actions. X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ca78c6b69c01508713b8a8e50065843fcaf53936;p=openvswitch 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 --- diff --git a/datapath/actions.c b/datapath/actions.c index 943d7570..53e05872 100644 --- 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 7d3bc04c..b80e0e29 100644 --- 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 4a5b2a7a..ade658d8 100644 --- 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 d545661a..1fab02fd 100644 --- 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 {