Looks good to me, thanks. Reviewed-by: Yifeng Sun <pkusunyif...@gmail.com>
On Wed, Mar 20, 2019 at 1:43 PM Toms Atteka <cpp.code...@gmail.com> wrote: > > If enough large input is given ofpact_finish will fail. > Implemented ofpbuf_oversized function to check for oversized > buffer. Checks were added for parse functions and error messages > returned. > > Basic manual testing performed. > > Reported-by: > https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=12972 > Signed-off-by: Toms Atteka <cpp.code...@gmail.com> > > v1->v2: added over sized check as a separate function and added > checks for other parse functions where they might fail. > --- > include/openvswitch/ofpbuf.h | 6 ++++++ > lib/bundle.c | 5 +++++ > lib/learn.c | 5 +++++ > lib/ofp-actions.c | 29 +++++++++++++++++++++++++++++ > 4 files changed, 45 insertions(+) > > diff --git a/include/openvswitch/ofpbuf.h b/include/openvswitch/ofpbuf.h > index e4cf088..1136ba0 100644 > --- a/include/openvswitch/ofpbuf.h > +++ b/include/openvswitch/ofpbuf.h > @@ -162,6 +162,7 @@ char *ofpbuf_to_string(const struct ofpbuf *, size_t > maxbytes); > static inline struct ofpbuf *ofpbuf_from_list(const struct ovs_list *); > void ofpbuf_list_delete(struct ovs_list *); > static inline bool ofpbuf_equal(const struct ofpbuf *, const struct ofpbuf > *); > +static inline bool ofpbuf_oversized(const struct ofpbuf *ofpacts); > > > /* Frees memory that 'b' points to, as well as 'b' itself. */ > @@ -272,6 +273,11 @@ static inline bool ofpbuf_equal(const struct ofpbuf *a, > const struct ofpbuf *b) > memcmp(a->data, b->data, a->size) == 0; > } > > +static inline bool ofpbuf_oversized(const struct ofpbuf *ofpacts) > +{ > + return (char *)ofpbuf_tail(ofpacts) - (char *)ofpacts->header > > UINT16_MAX; > +} > + > #ifdef __cplusplus > } > #endif > diff --git a/lib/bundle.c b/lib/bundle.c > index 558e890..edb11f6 100644 > --- a/lib/bundle.c > +++ b/lib/bundle.c > @@ -183,6 +183,11 @@ bundle_parse__(const char *s, const struct > ofputil_port_map *port_map, > bundle = ofpacts->header; > bundle->n_slaves++; > } > + > + if (ofpbuf_oversized(ofpacts)) { > + return xasprintf("input too big"); > + } > + > ofpact_finish_BUNDLE(ofpacts, &bundle); > bundle->basis = atoi(basis); > > diff --git a/lib/learn.c b/lib/learn.c > index 642ce18..a40209e 100644 > --- a/lib/learn.c > +++ b/lib/learn.c > @@ -455,6 +455,11 @@ learn_parse__(char *orig, char *arg, const struct > ofputil_port_map *port_map, > learn = ofpacts->header; > } > } > + > + if (ofpbuf_oversized(ofpacts)) { > + return xasprintf("input too big"); > + } > + > ofpact_finish_LEARN(ofpacts, &learn); > > return NULL; > diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c > index e8e88ac..1340614 100644 > --- a/lib/ofp-actions.c > +++ b/lib/ofp-actions.c > @@ -989,6 +989,11 @@ parse_CONTROLLER(char *arg, const struct > ofpact_parse_params *pp) > controller = pp->ofpacts->header; > controller->userdata_len = userdata_len; > } > + > + if (ofpbuf_oversized(pp->ofpacts)) { > + return xasprintf("input too big"); > + } > + > ofpact_finish_CONTROLLER(pp->ofpacts, &controller); > } > > @@ -3690,6 +3695,11 @@ parse_DEC_TTL(char *arg, const struct > ofpact_parse_params *pp) > return xstrdup("dec_ttl_cnt_ids: expected at least one > controller " > "id."); > } > + > + if (ofpbuf_oversized(pp->ofpacts)) { > + return xasprintf("input too big"); > + } > + > ofpact_finish_DEC_TTL(pp->ofpacts, &ids); > } > return NULL; > @@ -4443,6 +4453,11 @@ parse_ENCAP(char *arg, const struct > ofpact_parse_params *pp) > /* ofpbuf may have been re-allocated. */ > encap = pp->ofpacts->header; > encap->n_props = n_props; > + > + if (ofpbuf_oversized(pp->ofpacts)) { > + return xasprintf("input too big"); > + } > + > ofpact_finish_ENCAP(pp->ofpacts, &encap); > return NULL; > } > @@ -5772,6 +5787,11 @@ parse_NOTE(const char *arg, const struct > ofpact_parse_params *pp) > struct ofpact_note *note = ofpbuf_at_assert(pp->ofpacts, start_ofs, > sizeof *note); > note->length = pp->ofpacts->size - (start_ofs + sizeof *note); > + > + if (ofpbuf_oversized(pp->ofpacts)) { > + return xasprintf("input too big"); > + } > + > ofpact_finish_NOTE(pp->ofpacts, ¬e); > return NULL; > } > @@ -5929,6 +5949,10 @@ parse_CLONE(char *arg, const struct > ofpact_parse_params *pp) > pp->ofpacts->header = ofpbuf_push_uninit(pp->ofpacts, sizeof *clone); > clone = pp->ofpacts->header; > > + if (ofpbuf_oversized(pp->ofpacts)) { > + return xasprintf("input too big"); > + } > + > ofpact_finish_CLONE(pp->ofpacts, &clone); > ofpbuf_push_uninit(pp->ofpacts, clone_offset); > return error; > @@ -6615,6 +6639,11 @@ parse_CT(char *arg, const struct ofpact_parse_params > *pp) > if (!error && oc->flags & NX_CT_F_FORCE && !(oc->flags & > NX_CT_F_COMMIT)) { > error = xasprintf("\"force\" flag requires \"commit\" flag."); > } > + > + if (ofpbuf_oversized(pp->ofpacts)) { > + return xasprintf("input too big"); > + } > + > ofpact_finish_CT(pp->ofpacts, &oc); > ofpbuf_push_uninit(pp->ofpacts, ct_offset); > return error; > -- > 2.7.4 > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev