On Sun, Jul 10, 2011 at 6:18 PM, Valient Gough <vgo...@pobox.com> wrote:
> Add tunnel key support to CAPWAP vport. Uses the optional WSI field in a
> CAPWAP header to store a 64bit key. It can also be used without keys, in
> which
> case it is backward compatible with the old code. Documentation about the
> WSI field format is in CAPWAP.txt.
Thanks for keeping at this. I think it is very close, I just have a
few remaining concerns.
I got one further whitespace error:
Applying: datapath: add key support to CAPWAP tunnel
/home/jesse/openvswitch/.git/rebase-apply/patch:16: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
(This is from the blank lines at the bottom of CAPWAP.txt.)
> index f0bb327..c338685 100644
> --- a/datapath/vport-capwap.c
> +++ b/datapath/vport-capwap.c
> +static int process_capwap_wsi(struct sk_buff *skb, __be64 *key)
> {
> struct capwaphdr *cwh = capwap_hdr(skb);
> + struct capwaphdr_wsi *wsi;
> + int min_wsi_len = sizeof(struct capwaphdr_wsi);
> + int wsi_len;
> +
> + /* ensure we have at least a minimal wsi header */
> + if (unlikely(!pskb_may_pull(skb, CAPWAP_MIN_HLEN + min_wsi_len +
> ETH_HLEN)))
> + return 1;
Can you use real error codes in this function instead of 1, even if
they don't actually get interpreted? For example, here I would use
-ENOMEM, same for the one below, and then finally -EINVAL.
> +static inline struct sk_buff *process_capwap_proto(struct sk_buff *skb,
> + __be64 *key)
> +{
> + struct capwaphdr *cwh = capwap_hdr(skb);
> + int hdr_len = CAPWAP_MIN_HLEN;
> +
> + if ((cwh->begin & CAPWAP_WBID_MASK) != CAPWAP_WBID_30) {
> + if (unlikely((cwh->begin & CAPWAP_COMPAT_MASK) !=
> CAPWAP_WO_WSI))
> + goto error;
I think the parsing here is still a little bit too accepting of bad
packets. For example, it doesn't check the version number. Currently
the masks will ignore any field that is zero. However, some of these
(like the version number) we want to enforce being zero.
> + } else if (cwh->begin & CAPWAP_FLAG_WSI) {
> + struct capwaphdr_wsi *wsi = (struct capwaphdr_wsi *)(cwh + 1);
> + hdr_len += 1 + (unsigned int)wsi->wsi_len;
> +
> + if (unlikely(process_capwap_wsi(skb, key)))
> + goto error;
> }
This will use the key from the last fragment that arrives, which I
think is pretty non-deterministic. While a good packet should have
the same key in all fragments, a maliciously crafted packet may have
different keys, for example, to fool an IDS. This is pretty similar
to the issues encountered with IP reassembly.
To get around this, I would fix the behavior in one of two ways:
* Always use the key from the first fragment in the reassembled packet.
* Enforce that the key must be the same in all fragments.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
