[ovs-dev] [runt-flows 8/8] datapath: Avoid accesses past the end of skbuff data in actions.
Ben Pfaff
blp at nicira.com
Fri Aug 13 17:55:47 UTC 2010
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 at nicira.com>
---
datapath/actions.c | 112 +++++++++++++++++++++++++++++++++------------------
datapath/flow.c | 15 -------
lib/dpif-netdev.c | 20 ++++++---
lib/flow.c | 32 ++++++++-------
4 files changed, 102 insertions(+), 77 deletions(-)
diff --git a/datapath/actions.c b/datapath/actions.c
index a6771b6..484e3e3 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -52,7 +52,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)
@@ -90,8 +90,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);
@@ -237,31 +243,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;
}
@@ -270,7 +296,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);
@@ -282,8 +308,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;
@@ -293,28 +319,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 bfef8b9..07867d3 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -307,22 +307,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)) {
@@ -332,11 +322,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 c17b525..b79daa2 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 d545661..1fab02f 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 {
--
1.7.1
More information about the dev
mailing list