Hi,
in an effort to move things forward I decided to take the liberty
of posting a fresh version of this patch which addresses each
of the issues that Jesse raises below.
On Fri, Jul 22, 2011 at 02:18:47PM -0700, Jesse Gross wrote:
> On Wed, Jul 13, 2011 at 10:05 PM, Valient Gough <vgo...@pobox.com> wrote:
> > diff --git a/datapath/vport-capwap.c b/datapath/vport-capwap.c
> > index f0bb327..161c6d4 100644
> > --- a/datapath/vport-capwap.c
> > +++ b/datapath/vport-capwap.c
> > +/*
> > + * Should be sized as multiple of u32, for hash functions.
> > + */
> > struct frag_match {
> > __be32 saddr;
> > __be32 daddr;
> > __be16 id;
> > + __be16 reserved;
> > + __be64 key;
> > };
>
> I think this violates the CAPWAP spec because it changes the
> parameters that reassembly is based on. A device that understands
> keys can now put together packets in a different way than one that
> does not if there are overlapping IDs. If you go down the route of
> checking that all of the fragments have the same ID, I think you need
> to do reassembly as before and check to see whether all segments have
> the same key and drop if not. Probably the simpler implementation
> would be to just declare that only the key in the first fragment
> matters.
My reading is that can be achieved by simply not
adding reserved and key to struct frag_match, not
setting those values in defrag() and not comparing key
in capwap_frag_match(). Is that correct?
> > +static int process_capwap_wsi(struct sk_buff *skb, __be64 *key, int
> > *hdr_len)
> > {
> > struct capwaphdr *cwh = capwap_hdr(skb);
> > + struct capwaphdr_wsi *wsi = (struct capwaphdr_wsi *)(cwh + 1);
> > + int min_wsi_len = sizeof(struct capwaphdr_wsi);
> > + int wsi_len;
>
> You can't use these points after the calls to pskb_may_pull() below as
> they may reallocate memory and cause the pointers to be invalid.
>
> > - if (likely(cwh->begin == NO_FRAG_HDR))
> > - return skb;
> > - else if (cwh->begin == FRAG_HDR)
> > - return defrag(skb, false);
> > - else if (cwh->begin == FRAG_LAST_HDR)
> > - return defrag(skb, true);
> > - else {
> > - if (net_ratelimit())
> > - pr_warn("unparsable packet receive on capwap
> > socket\n");
> > + if (unlikely(!pskb_may_pull(skb, CAPWAP_MIN_HLEN + min_wsi_len +
> > ETH_HLEN)))
> > + return -ENOMEM;
> >
> > - kfree_skb(skb);
> > - return NULL;
> > + /* read wsi header to find out how big it really is */
> > + wsi = (struct capwaphdr_wsi *)(cwh + 1);
> > + /* +1 for length byte not included in wsi_len */
> > + wsi_len = 1 + (unsigned int)wsi->wsi_len;
> > + *hdr_len += wsi_len;
> > +
> > + if (unlikely(!pskb_may_pull(skb, CAPWAP_MIN_HLEN + wsi_len +
> > ETH_HLEN)))
> > + return -ENOMEM;
> > +
> > + /* parse wsi field */
> > + if (wsi->flags & CAPWAP_WSI_F_KEY64) {
> > + struct capwaphdr_wsi_key *opt = (struct capwaphdr_wsi_key
> > *)(wsi + 1);
> > + if (unlikely(wsi_len <= sizeof(struct capwaphdr_wsi_key)))
> > + return -EINVAL;
>
> Is this length check correct? struct capwaphdr_wsi_key is just the 8
> byte value but wsi_len includes struct capwaphdr_wsi as well.
I have reworked process_capwap_wsi() as follows which
I believe addresses both the potentially invalid pointer and
incorrect length check issues.
static int process_capwap_wsi(struct sk_buff *skb, __be64 *key, int *hdr_len)
{
struct capwaphdr *cwh;
struct capwaphdr_wsi *wsi;
int min_wsi_len = sizeof(struct capwaphdr_wsi);
int wsi_len;
if (unlikely(!pskb_may_pull(skb, CAPWAP_MIN_HLEN + min_wsi_len +
ETH_HLEN)))
return -ENOMEM;
/* read wsi header to find out how big it really is */
cwh = capwap_hdr(skb);
wsi = (struct capwaphdr_wsi *)(cwh + 1);
/* +1 for length byte not included in wsi_len */
wsi_len = 1 + (unsigned int)wsi->wsi_len;
*hdr_len += wsi_len;
/* parse wsi field */
if (wsi_len > min_wsi_len && wsi->flags & CAPWAP_WSI_F_KEY64) {
struct capwaphdr_wsi_key *opt;
if (unlikely(wsi_len < min_wsi_len + sizeof(struct
capwaphdr_wsi_key)))
return -EINVAL;
if (unlikely(!pskb_may_pull(skb, CAPWAP_MIN_HLEN + wsi_len +
ETH_HLEN)))
return -ENOMEM;
cwh = capwap_hdr(skb);
wsi = (struct capwaphdr_wsi *)(cwh + 1);
opt = (struct capwaphdr_wsi_key *)(wsi + 1);
*key = opt->key;
}
return 0;
}
>
> > +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;
> > + int wbid = cwh->begin & CAPWAP_WBID_MASK;
>
> This triggers sparse errors; it can be fixed by using __be32 instead.
Done.
> > @@ -628,7 +768,8 @@ static int capwap_frag_match(struct inet_frag_queue
> > *ifq, void *a_)
> > struct frag_match *a = a_;
> > struct frag_match *b = &ifq_cast(ifq)->match;
> >
> > - return a->id == b->id && a->saddr == b->saddr && a->daddr ==
> > b->daddr;
> > + return a->id == b->id && a->saddr == b->saddr &&
> > + a->daddr == b->daddr && a->key == b->key;
>
> Might as well use memcmp() here now that the struct won't have any
> extra padding in it.
Done.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
