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

[Ben Pfaff]

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.