On Thu, Aug 16, 2018 at 8:51 AM, Ben Pfaff <[email protected]> wrote: > On Wed, Aug 15, 2018 at 04:55:38PM -0700, Darrell Ball wrote: > > On Wed, Aug 15, 2018 at 3:03 PM, Ben Pfaff <[email protected]> wrote: > > > > > decode_ed_prop() accepted encap/decap properties with a reported > length of > > > 0, without consuming any data from the property list, which yielded an > > > infinite loop. > > > > > > Reported-at: https://bugs.chromium.org/p/os > s-fuzz/issues/detail?id=9918 > > > Signed-off-by: Ben Pfaff <[email protected]> > > > --- > > > lib/ofp-ed-props.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/lib/ofp-ed-props.c b/lib/ofp-ed-props.c > > > index 901da2f0dd1b..28382e01235c 100644 > > > --- a/lib/ofp-ed-props.c > > > +++ b/lib/ofp-ed-props.c > > > @@ -35,7 +35,7 @@ decode_ed_prop(const struct ofp_ed_prop_header > > > **ofp_prop, > > > > size_t len = (*ofp_prop)->len; > > > size_t pad_len = ROUND_UP(len, 8); > > > > > > - if (pad_len > *remaining) { > > > + if (len < sizeof **ofp_prop || pad_len > *remaining) { > > > > > > > Is *remaining > pad_len valid ? > > If it is, which is not intuitive, maybe a comment will help ? > > Can you help me understand why it would not be valid? >
I noted that 'len' here is for a single property vs remaining for all properties; I had somehow thought that the caller, decode_NXAST_RAW_ENCAP(), would verify that the remaining length is sufficient before calling decode_ed_prop() since it is already verifying the overall length - remaining, 'remaining' > 0, meaning it handles the list of properties. Below explains better what I had expected, but I still think the existing code and your incremental is better, hence Acked-by: Darrell Ball<[email protected]> diff --git a/include/openvswitch/ofp-ed-props.h b/include/openvswitch/ofp-ed-props.h index 306c6fe..e742d69 100644 --- a/include/openvswitch/ofp-ed-props.h +++ b/include/openvswitch/ofp-ed-props.h @@ -97,7 +97,8 @@ struct ofpact_ed_prop_nsh_tlv { uint8_t data[0]; }; enum ofperr decode_ed_prop(const struct ofp_ed_prop_header **ofp_prop, - struct ofpbuf *out, size_t *remaining); + struct ofpbuf *out, size_t remaining, size_t len, + size_t pad_len); enum ofperr encode_ed_prop(const struct ofpact_ed_prop **prop, struct ofpbuf *out); bool parse_ed_prop_class(const char *str, uint16_t *prop_class); diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index 93e212f..beca500 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -4343,11 +4343,17 @@ decode_NXAST_RAW_ENCAP(const struct nx_action_encap *nae, props_len = ntohs(nae->len) - offsetof(struct nx_action_encap, props); n_props = 0; while (props_len > 0) { - err = decode_ed_prop(&ofp_prop, out, &props_len); + size_t pad_len = ROUND_UP(ofp_prop->len, 8); + if (pad_len > props_len) { + return OFPERR_OFPBAC_BAD_LEN; + } + err = decode_ed_prop(&ofp_prop, out, props_len, ofp_prop->len, + pad_len); if (err) { return err; } n_props++; + props_len -= pad_len; } encap->n_props = n_props; out->header = &encap->ofpact; diff --git a/lib/ofp-ed-props.c b/lib/ofp-ed-props.c index 901da2f..0816659 100644 --- a/lib/ofp-ed-props.c +++ b/lib/ofp-ed-props.c @@ -28,16 +28,10 @@ enum ofperr decode_ed_prop(const struct ofp_ed_prop_header **ofp_prop, struct ofpbuf *out OVS_UNUSED, - size_t *remaining) + size_t remaining, size_t len, size_t pad_len) { uint16_t prop_class = ntohs((*ofp_prop)->prop_class); uint8_t prop_type = (*ofp_prop)->type; - size_t len = (*ofp_prop)->len; - size_t pad_len = ROUND_UP(len, 8); - - if (pad_len > *remaining) { - return OFPERR_OFPBAC_BAD_LEN; - } switch (prop_class) { case OFPPPC_NSH: { @@ -45,7 +39,7 @@ decode_ed_prop(const struct ofp_ed_prop_header **ofp_prop, case OFPPPT_PROP_NSH_MDTYPE: { struct ofp_ed_prop_nsh_md_type *opnmt = ALIGNED_CAST(struct ofp_ed_prop_nsh_md_type *, *ofp_prop); - if (len > sizeof(*opnmt) || len > *remaining) { + if (len > sizeof(*opnmt) || len > remaining) { return OFPERR_NXBAC_BAD_ED_PROP; } struct ofpact_ed_prop_nsh_md_type *pnmt = @@ -60,7 +54,7 @@ decode_ed_prop(const struct ofp_ed_prop_header **ofp_prop, struct ofp_ed_prop_nsh_tlv *opnt = ALIGNED_CAST(struct ofp_ed_prop_nsh_tlv *, *ofp_prop); size_t tlv_pad_len = ROUND_UP(opnt->tlv_len, 8); - if (len != sizeof(*opnt) + tlv_pad_len || len > *remaining) { + if (len != sizeof(*opnt) + tlv_pad_len || len > remaining) { return OFPERR_NXBAC_BAD_ED_PROP; } struct ofpact_ed_prop_nsh_tlv *pnt = @@ -83,7 +77,6 @@ decode_ed_prop(const struct ofp_ed_prop_header **ofp_prop, return OFPERR_NXBAC_UNKNOWN_ED_PROP; } - *remaining -= pad_len; *ofp_prop = ALIGNED_CAST(const struct ofp_ed_prop_header *, ((char *)(*ofp_prop) + pad_len)); return 0; (END) _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
