My apologies, let me dig this issue out. Thank you for the review! Yifeng
On Thu, Dec 27, 2018 at 10:55 AM Ben Pfaff <[email protected]> wrote: > 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 <[email protected]> > > --- > > 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. > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
