Date:   Mon, 21 Aug 2017 14:11:10 +0800
From:   "Yang, Yi" <yi.y.yang@...el.com>
To:     Jiri Benc <jbenc@...hat.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "dev@...nvswitch.org" <dev@...nvswitch.org>,
        "blp@....org" <blp@....org>, "e@...g.me" <e@...g.me>,
        "jan.scheurich@...csson.com" <jan.scheurich@...csson.com>
Subject: Re: [PATCH net-next v4] openvswitch: enable NSH support

On Fri, Aug 18, 2017 at 09:26:01PM +0800, Jiri Benc wrote:
> On Fri, 18 Aug 2017 15:24:31 +0800, Yi Yang wrote:
> > +struct nsh_md2_tlv {
> > +	__be16 md_class;
> > +	u8 type;
> > +	u8 length;
> > +	/* Followed by variable-length data. */
> > +};
> 
> What was wrong with the u8[] field that was present at the end of the
> struct in the previous version of the patch?

In OVS code, it has been removed because of Microsoft compiler issue.

> 
> > +#define NSH_M_TYPE2_MAX_LEN 256
> 
> This is defined twice, please delete this define and keep the one lower
> in the file.

Removed duplicate one in v5.

> 
> > +#define NSH_DST_PORT    4790     /* UDP Port for NSH on VXLAN. */
> 
> This is a VXLAN-GPE port, it has nothing to do with NSH (except that
> VXLAN-GPE can contain a NSH packet). It's also unused. Please remove it.
> 

Removed in v5.

> > +/* NSH Metadata Length. */
> > +#define NSH_M_TYPE1_MDLEN 16
> 
> This is unused and it seems it's not much useful anyway,
> sizeof(struct nsh_md1_ctx) provides the same value. Please remove this
> define.

Removed in v5.

> 
> > +#define NSH_MD1_CTX(nsh_hdr_ptr) (&(nsh_hdr_ptr)->md1)
> > +
> > +#define NSH_MD2_CTX(nsh_hdr_ptr) (&(nsh_hdr_ptr)->md2)
> 
> Please remove these two. They are unused and would just obscure things
> anyway.
> 
> > +static inline struct nsh_md1_ctx *nsh_md1_ctx(struct nsh_hdr *nsh)
> > +{
> > +	return &nsh->md1;
> > +}
> > +
> > +static inline struct nsh_md2_tlv *nsh_md2_ctx(struct nsh_hdr *nsh)
> > +{
> > +	return &nsh->md2;
> > +}
> 
> And remove these too, for the same reason. Just use nsh->md1 when you
> need the metadata, there's no reason for these helper functions. They
> just obscure things.
> 

Removed them in v5

> > +static inline void nsh_set_flags_and_ttl(struct nsh_hdr *nsh, u8 flags, u8 ttl)
> > +{
> > +	nsh->ver_flags_ttl_len
> > +		= htons((ntohs(nsh->ver_flags_ttl_len)
> > +			& ~(NSH_FLAGS_MASK | NSH_TTL_MASK))
> > +			| ((flags << NSH_FLAGS_SHIFT) & NSH_FLAGS_MASK)
> > +			| ((ttl << NSH_TTL_SHIFT) & NSH_TTL_MASK));
> > +}
> > +
> > +static inline void nsh_set_flags_ttl_len(struct nsh_hdr *nsh, u8 flags,
> > +					 u8 ttl, u8 len)
> > +{
> > +	nsh->ver_flags_ttl_len
> > +		= htons((ntohs(nsh->ver_flags_ttl_len)
> > +			& ~(NSH_FLAGS_MASK | NSH_TTL_MASK | NSH_LEN_MASK))
> > +			| ((flags << NSH_FLAGS_SHIFT) & NSH_FLAGS_MASK)
> > +			| ((ttl << NSH_TTL_SHIFT) & NSH_TTL_MASK)
> > +			| ((len << NSH_LEN_SHIFT) & NSH_LEN_MASK));
> > +}
> 
> Okay. Could those two perhaps use a common function?
> 
> static inline void __nsh_set_flags(struct nsh_hdr *nsh, u16 value, u16 mask)
> {
> 	nsh->ver_flags_ttl_len = nsh->ver_flags_ttl_len & ~htons(mask)
> 							| htons(value);
> }
> 
> static inline void nsh_set_flags_and_ttl(struct nsh_hdr *nsh, u8 flags, u8 ttl)
> {
> 	__nsh_set_flags(nsh, flags << NSH_FLAGS_SHIFT | ttl << NSH_TTL_SHIFT,
> 			NSH_FLAGS_MASK | NSH_TTL_MASK);
> }
> 
> etc.

Thanks for this good suggestion, applied in v5 with small change.

> 
> > +static int push_nsh(struct sk_buff *skb, struct sw_flow_key *key,
> > +		    const struct nsh_hdr *nsh_src)
> > +{
> [...]
> > +	if (!skb->inner_protocol)
> > +		skb_set_inner_protocol(skb, skb->protocol);
> 
> I was wondering about this during the reviews of the previous versions.
> Now I've given this more thought but I still don't see it - why is the
> inner_protocol set here?

I saw push_mpls has it, so also set it.

> 
> > +	case OVS_KEY_ATTR_NSH: {
> > +		struct ovs_key_nsh nsh;
> > +		struct ovs_key_nsh nsh_mask;
> > +		size_t size = nla_len(a) / 2;
> > +		struct nlattr attr[1 + size / sizeof(struct nlattr) + 1];
> > +		struct nlattr mask[1 + size / sizeof(struct nlattr) + 1];
> > +
> > +		attr->nla_type = nla_type(a);
> > +		mask->nla_type = attr->nla_type;
> > +		attr->nla_len = NLA_HDRLEN + size;
> > +		mask->nla_len = attr->nla_len;
> > +		memcpy(attr + 1, (char *)(a + 1), size);
> > +		memcpy(mask + 1, (char *)(a + 1) + size, size);
> 
> No, please. See my reply to the previous version for how to do this in
> a less hacky way.

I have used your proposal in previous comments and have it in v5.

> 
> > +		case OVS_ACTION_ATTR_PUSH_NSH: {
> > +			u8 buffer[256];
> > +			struct nsh_hdr *nsh_hdr = (struct nsh_hdr *)buffer;
> > +			const struct nsh_hdr *nsh_src = nsh_hdr;
> > +
> > +			nsh_hdr_from_nlattr(nla_data(a), nsh_hdr);
> 
> This is very dangerous security wise. You have to protect against
> buffer overflow, one way or other. The current code may not overflow
> (I have not checked that, though) but a future addition may break the
> assumption without being obvious it's a problem.
> 
> Note that the previous version had exactly the same problem but it was
> hidden and I didn't notice it. Which means that getting rid of that
> push_nsh_para struct was a very good thing, the code is more clean and
> more obvious now.

I have added a size parameter for nsh_hdr_from_nlattr in which there is
size check code in order to make sure there will not buffer overflow
happening. please chech v5 for details.

> 
> > +static int parse_nsh(struct sk_buff *skb, struct sw_flow_key *key)
> > +{
> > +	struct nsh_hdr *nsh = (struct nsh_hdr *)skb_network_header(skb);
> > +	u8 version, length;
> > +	int err;
> > +
> > +	err = check_header(skb, NSH_BASE_HDR_LEN);
> > +	if (unlikely(err))
> > +		return err;
> > +
> > +	memset(&key->nsh, 0, sizeof(struct ovs_key_nsh));
> 
> This is unnecessary and expensive. We're initializing all the fields
> below.

Removed in v5.

> 
> > +	version = nsh_get_ver(nsh);
> > +	length = nsh_hdr_len(nsh);
> 
> You have to reload nsh after pskb_may_pull (which is called by
> check_header).

I have removed check_header and use skb->len to check in v5.

> 
> > +	if (version != 0)
> > +		return -EINVAL;
> > +
> > +	if (nsh->md_type == NSH_M_TYPE1 && length != NSH_M_TYPE1_LEN)
> > +		return -EINVAL;
> > +
> > +	if (nsh->md_type == NSH_M_TYPE2 && length < NSH_BASE_HDR_LEN)
> > +		return -EINVAL;
> 
> This might better be merged to the switch below. Or are you concerned
> about potentially expensive pskb_may_pull with unchecked length? In
> that case, it would be better to convert to switch and reject on
> unknown md_types.

Good point, I have moved them to switch in v5.

> 
> > +	err = check_header(skb, length);
> > +	if (unlikely(err))
> > +		return err;
> > +
> > +	key->nsh.flags = nsh_get_flags(nsh);
> 
> Again, need to reload nsh.

I used skb->len in v5, so we can't avoid such issue.

> 
> > +	key->nsh.ttl = nsh_get_ttl(nsh);
> > +	key->nsh.mdtype = nsh->md_type;
> > +	key->nsh.np = nsh->next_proto;
> > +	key->nsh.path_hdr = nsh->path_hdr;
> > +	switch (key->nsh.mdtype) {
> > +	case NSH_M_TYPE1:
> > +		memcpy(key->nsh.context, nsh->md1.context,
> > +		       sizeof(nsh->md1));
> > +		break;
> > +	case NSH_M_TYPE2:
> > +		/* Don't support MD type 2 metedata parsing yet */
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> 
> This is the switch I mentioned above.

Yes, done in v5.

> 
> > +struct ovs_key_nsh {
> > +	__u8 flags;
> > +	__u8 ttl;
> > +	__u8 mdtype;
> > +	__u8 np;
> 
> Just u8, please, this is kernel internal.

Changed to u8 in v5.

> 
> > +size_t ovs_nsh_key_attr_size(void)
> > +{
> > +	/* Whenever adding new OVS_NSH_KEY_ FIELDS, we should consider
> > +	 * updating this function.
> > +	 */
> > +	return  nla_total_size(8)      /* OVS_NSH_KEY_ATTR_BASE */
> 
> NSH_BASE_HDR_LEN, perhaps? Not that much important, though.

Replaced 8 with NSH_BASE_HDR_LEN in v5.

> 
> > +		switch (type) {
> > +		case OVS_NSH_KEY_ATTR_BASE: {
> > +			const struct ovs_nsh_key_base *base =
> > +				(struct ovs_nsh_key_base *)nla_data(a);
> > +			flags = base->flags;
> > +			ttl = base->ttl;
> > +			nsh->next_proto = base->np;
> > +			nsh->md_type = base->mdtype;
> > +			nsh->path_hdr = base->path_hdr;
> 
> Wouldn't it be nicer if the fields of struct ovs_nsh_key_base and of
> struct nsh_hdr had the same names?

Such change also will impact on OVS code, so I prefer not to change
them.

For struct nsh_hdr, we need more self-descriptive fields, but for struct
ovs_nsh_key_base, because we need to use keys in OVS OpenFlow, so np is
obviously better than next_proto, we also try our best to make sure the
old NSH implementation has same match fields as the new one does.

> 
> > +		case OVS_NSH_KEY_ATTR_MD1: {
> > +			const struct ovs_nsh_key_md1 *md1 =
> > +				(struct ovs_nsh_key_md1 *)nla_data(a);
> > +			struct nsh_md1_ctx *md1_dst = nsh_md1_ctx(nsh);
> > +
> > +			has_md1 = true;
> > +			mdlen = nla_len(a);
> > +			memcpy(md1_dst, md1, mdlen);
> 
> How can we be sure there's enough room in the nsh buffer? See also my
> previous remark.

I have added a size parameter for nsh_hdr_from_nlattr and also added
check code here in v5.

> 
> > +			break;
> > +		}
> > +		case OVS_NSH_KEY_ATTR_MD2: {
> > +			const struct u8 *md2 = nla_data(a);
> > +			struct nsh_md2_tlv *md2_dst = nsh_md2_ctx(nsh);
> > +
> > +			has_md2 = true;
> > +			mdlen = nla_len(a);
> > +			if ((mdlen > NSH_M_TYPE2_MD_MAX_LEN) ||
> > +			    (mdlen == 0)) {
> > +				OVS_NLERR(
> > +				    1,
> > +				    "length %d of nsh attr %d is invalid",
> > +				    mdlen,
> > +				    type
> > +				);
> > +				return -EINVAL;
> > +			}
> > +			memcpy(md2_dst, md2, mdlen);
> 
> And, more importantly, here. It seems that it's currently capped at
> 256 bytes by the mdlen check yet it's too fragile. Either add a
> parameter with the nsh buffer size or find other way to make this more
> robust. Otherwise we're going to hunt a buffer overflow in a year.

Done in v5.

> 
> > +	if ((has_md1 && nsh->md_type != NSH_M_TYPE1) ||
> > +	    (has_md2 && nsh->md_type != NSH_M_TYPE2)) {
> > +		OVS_NLERR(1,
> > +			  "nsh attribute has unmatched MD type %d.",
> > +			  nsh->md_type);
> > +		return -EINVAL;
> > +	}
> 
> What if both type 1 and type 2 attributes were specified? Or neither?
> This condition does not catch that.

I have added these checks in the function, but for set action, we may
only have OVS_NSH_KEY_BASE without OVS_NSH_KEY_MD1 and OVS_NSH_KEY_MD2,
so these checks will be different in different use case.

> 
> > +	/* nsh header length  = NSH_BASE_HDR_LEN + mdlen */
> > +	nsh_set_flags_ttl_len(nsh, flags, ttl,
> > +			      (NSH_BASE_HDR_LEN + mdlen) >> 2);
> 
> Just specify the len. It's the job of the helper function to convert it
> to whatever format is needed in the header. (I'm talking about the
> ">> 2". That should not be done by the caller but by the helper
> function.)

Changed nsh_set_flags_ttl_len for this in v5.

> 
> Out of time for today, will continue the review next week. Again, feel
> free to send a new version meanwhile or wait for the rest of the
> review, whatever works better for you.

I have sent out v5, please continue to review that version, thanks a
lot.
> 
>  Jiri

Powered by blists - more mailing lists