[ovs-dev] [PATCH 1/2] odp-util: Fix a bug in parse_odp_push_nsh_action

[Jan Scheurich]

Hi Ben and Yfeng,

Looking at the current code on master I believe it is correct except that I wrongly used ofpbuf_push_zeros() where it should have been ofpbuf_put_zeros() to append the padding zeroes to the end. The ofp_buf should never be relocated through padding as it is allocated with the allowed maximum size from the start.

The entire MD2 metadata (1 or more TLVs) is treated as a single NLATTR. The reason is that for the purpose of push_nsh the datapath does not care about the internal structure. The whole MD2 complex is added as one binary blob of data. Remember we did not implement the possibility to match on specific MD2 TLV fields in OVS 2.8 as that would have required a generalization of the generic metadata TLV field infrastructure to match fields.

Admittedly, the possibility to specify arbitrary hex-encoded MD2 TLVs in a push_nsh action in ovs-dpctl is a bit raw as it trusts the hex data to be well-formed. As discussed, the datapath code doesn't really care, but the receiver of an NSH packet with malformed MD2 headers might choke. The alternatives would have been to not accept MD2 metadata in dpctl commands or to validate the hex data (at least from a TLV-structural perspective).

I believe for MD2 metadata specified in an OF encap_nsh action we do ensure the generated TLV structure is correct. 

I hope this helps.

BR, Jan

> -----Original Message-----
> From: Ben Pfaff <blp at ovn.org>
> Sent: Thursday, 27 December, 2018 19:55
> To: Yifeng Sun <pkusunyifeng at gmail.com>; Jan Scheurich
> <jan.scheurich at ericsson.com>
> Cc: dev at openvswitch.org
> Subject: Re: [ovs-dev] [PATCH 1/2] odp-util: Fix a bug in
> parse_odp_push_nsh_action
> 
> On Wed, Dec 26, 2018 at 04:52:22PM -0800, Yifeng Sun wrote:
> > In this piece of code, 'struct ofpbuf b' should always point to
> > metadata so that metadata can be filled with values through ofpbuf
> > operations, like ofpbuf_put_hex and ofpbuf_push_zeros. However,
> > ofpbuf_push_zeros may change the data pointer of 'struct ofpbuf b'
> > and therefore, metadata will not contain the expected values. This
> > patch fixes it.
> >
> > Reported-at:
> > https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=10863
> > Signed-off-by: Yifeng Sun <pkusunyifeng at gmail.com>
> > ---
> >  lib/odp-util.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/odp-util.c b/lib/odp-util.c index
> > cb6a5f2047fd..af855873690c 100644
> > --- a/lib/odp-util.c
> > +++ b/lib/odp-util.c
> > @@ -2114,12 +2114,12 @@ parse_odp_push_nsh_action(const char *s,
> struct ofpbuf *actions)
> >              if (ovs_scan_len(s, &n, "md2=0x%511[0-9a-fA-F]", buf)
> >                  && n/2 <= sizeof metadata) {
> >                  ofpbuf_use_stub(&b, metadata, sizeof metadata);
> > -                ofpbuf_put_hex(&b, buf, &mdlen);
> >                  /* Pad metadata to 4 bytes. */
> >                  padding = PAD_SIZE(mdlen, 4);
> >                  if (padding > 0) {
> > -                    ofpbuf_push_zeros(&b, padding);
> > +                    ofpbuf_put_zeros(&b, padding);
> >                  }
> > +                ofpbuf_put_hex(&b, buf, &mdlen);
> >                  md_size = mdlen + padding;
> >                  ofpbuf_uninit(&b);
> >                  continue;
> 
> Yifeng, this fix looks wrong because it uses 'mdlen' in PAD_SIZE before
> initializing it.
> 
> This code is weird.  It adds padding to a 4-byte boundary even though I can't
> find any other code that checks for that or relies on it.
> Furthermore, it puts the padding **BEFORE** the metadata, which just seems
> super wrong.
> 
> When I look at datapath code for md2 I get even more confused.
> nsh_key_put_from_nlattr() seems to assume that an md2 attribute has well-
> formatted data in it, then nsh_hdr_from_nlattr() copies it without checking into
> nh->md2, and then if it's not perfectly formatted then
> nsh->md2.length is going to be invalid.  If I'm reading it right, it
> also assumes there's exactly one TLV.
> 
> Jan, I think this is your code, can you help me understand this code?
> 
> Thanks,
> 
> Ben.