| Message ID | 1525714086-2264-1-git-send-email-anju.thomas@ericsson.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v1] Fix crash when processing malformed Bundle Add message in OVS | expand |
On Mon, May 07, 2018 at 10:58:06PM +0530, Anju Thomas wrote: > When an OpenFlow Bundle Add message is received, a bundle entry is > created and the OpenFlow message embedded in the bundle add message is > processed. If any error is encountered while processing the embedded > message, the bundle entry is freed. The bundle entry free function > assumes that the entry has been populated with a properly formatted > OpenFlow message and performs some message specific cleanup actions . > This assumption does not hold true in the error case and OVS crashes > when performing the cleanup. > > The fix is in case of errors, simply free the bundle entry without > attempting to perform any embedded message cleanup > > Signed-off-by: Anju Thomas <anju.thomas@ericsson.com> Thanks for the fix. It's not really nice to have multiple levels of initialized-ness. What do you think of this fix instead? Thanks, Ben. --8<--------------------------cut here-------------------------->8-- From: Anju Thomas <anju.thomas@ericsson.com> Date: Mon, 7 May 2018 22:58:06 +0530 Subject: [PATCH] Fix crash when processing malformed Bundle Add message in OVS When an OpenFlow Bundle Add message is received, a bundle entry is created and the OpenFlow message embedded in the bundle add message is processed. If any error is encountered while processing the embedded message, the bundle entry is freed. The bundle entry free function assumes that the entry has been populated with a properly formatted OpenFlow message and performs some message specific cleanup actions . This assumption does not hold true in the error case and OVS crashes when performing the cleanup. The fix is in case of errors, simply free the bundle entry without attempting to perform any embedded message cleanup Signed-off-by: Anju Thomas <anju.thomas@ericsson.com> Signed-off-by: Ben Pfaff <blp@ovn.org> --- ofproto/bundles.h | 13 ------------- ofproto/ofproto.c | 21 +++++++++++++-------- 2 files changed, 13 insertions(+), 21 deletions(-) diff --git a/ofproto/bundles.h b/ofproto/bundles.h index 806e853d6883..1164fddd8702 100644 --- a/ofproto/bundles.h +++ b/ofproto/bundles.h @@ -58,8 +58,6 @@ struct ofp_bundle { struct ovs_list msg_list; /* List of 'struct bundle_message's */ }; -static inline struct ofp_bundle_entry *ofp_bundle_entry_alloc( - enum ofptype type, const struct ofp_header *oh); static inline void ofp_bundle_entry_free(struct ofp_bundle_entry *); enum ofperr ofp_bundle_open(struct ofconn *, uint32_t id, uint16_t flags, @@ -72,17 +70,6 @@ enum ofperr ofp_bundle_add_message(struct ofconn *, uint32_t id, void ofp_bundle_remove__(struct ofconn *, struct ofp_bundle *); -static inline struct ofp_bundle_entry * -ofp_bundle_entry_alloc(enum ofptype type, const struct ofp_header *oh) -{ - struct ofp_bundle_entry *entry = xmalloc(sizeof *entry); - - entry->type = type; - entry->msg = xmemdup(oh, ntohs(oh->length)); - - return entry; -} - static inline void ofp_bundle_entry_free(struct ofp_bundle_entry *entry) { diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 36f4c0b16d48..565a45db1507 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -7899,7 +7899,6 @@ handle_bundle_add(struct ofconn *ofconn, const struct ofp_header *oh) struct ofproto *ofproto = ofconn_get_ofproto(ofconn); enum ofperr error; struct ofputil_bundle_add_msg badd; - struct ofp_bundle_entry *bmsg; enum ofptype type; error = reject_slave_controller(ofconn); @@ -7912,7 +7911,8 @@ handle_bundle_add(struct ofconn *ofconn, const struct ofp_header *oh) return error; } - bmsg = ofp_bundle_entry_alloc(type, badd.msg); + /* Allocate bundle entry and decode the embedded message. */ + struct ofp_bundle_entry *bmsg = xmalloc(sizeof *bmsg); struct ofpbuf ofpacts; uint64_t ofpacts_stub[1024 / 8]; @@ -7951,18 +7951,23 @@ handle_bundle_add(struct ofconn *ofconn, const struct ofp_header *oh) } else { OVS_NOT_REACHED(); } - ofpbuf_uninit(&ofpacts); - - if (!error) { - error = ofp_bundle_add_message(ofconn, badd.bundle_id, badd.flags, - bmsg, oh); + if (error) { + free(bmsg); + return error; } + /* Now that the embedded message has been successfully decoded, finish up + * initializing the bundle entry. */ + bmsg->type = type; + bmsg->msg = xmemdup(oh, ntohs(oh->length)); + + /* Add bundle entry to bundle. */ + error = ofp_bundle_add_message(ofconn, badd.bundle_id, badd.flags, + bmsg, oh); if (error) { ofp_bundle_entry_free(bmsg); } - return error; }
Hi Ben, Yes. This patch certainly looks more cleaner and better to me . Thanks Anju -----Original Message----- From: Ben Pfaff [mailto:blp@ovn.org] Sent: Thursday, May 10, 2018 2:33 AM To: Anju Thomas <anju.thomas@ericsson.com> Cc: dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH v1] Fix crash when processing malformed Bundle Add message in OVS On Mon, May 07, 2018 at 10:58:06PM +0530, Anju Thomas wrote: > When an OpenFlow Bundle Add message is received, a bundle entry is > created and the OpenFlow message embedded in the bundle add message is > processed. If any error is encountered while processing the embedded > message, the bundle entry is freed. The bundle entry free function > assumes that the entry has been populated with a properly formatted > OpenFlow message and performs some message specific cleanup actions . > This assumption does not hold true in the error case and OVS crashes > when performing the cleanup. > > The fix is in case of errors, simply free the bundle entry without > attempting to perform any embedded message cleanup > > Signed-off-by: Anju Thomas <anju.thomas@ericsson.com> Thanks for the fix. It's not really nice to have multiple levels of initialized-ness. What do you think of this fix instead? Thanks, Ben. --8<--------------------------cut here-------------------------->8-- From: Anju Thomas <anju.thomas@ericsson.com> Date: Mon, 7 May 2018 22:58:06 +0530 Subject: [PATCH] Fix crash when processing malformed Bundle Add message in OVS When an OpenFlow Bundle Add message is received, a bundle entry is created and the OpenFlow message embedded in the bundle add message is processed. If any error is encountered while processing the embedded message, the bundle entry is freed. The bundle entry free function assumes that the entry has been populated with a properly formatted OpenFlow message and performs some message specific cleanup actions . This assumption does not hold true in the error case and OVS crashes when performing the cleanup. The fix is in case of errors, simply free the bundle entry without attempting to perform any embedded message cleanup Signed-off-by: Anju Thomas <anju.thomas@ericsson.com> Signed-off-by: Ben Pfaff <blp@ovn.org> --- ofproto/bundles.h | 13 ------------- ofproto/ofproto.c | 21 +++++++++++++-------- 2 files changed, 13 insertions(+), 21 deletions(-) diff --git a/ofproto/bundles.h b/ofproto/bundles.h index 806e853d6883..1164fddd8702 100644 --- a/ofproto/bundles.h +++ b/ofproto/bundles.h @@ -58,8 +58,6 @@ struct ofp_bundle { struct ovs_list msg_list; /* List of 'struct bundle_message's */ }; -static inline struct ofp_bundle_entry *ofp_bundle_entry_alloc( - enum ofptype type, const struct ofp_header *oh); static inline void ofp_bundle_entry_free(struct ofp_bundle_entry *); enum ofperr ofp_bundle_open(struct ofconn *, uint32_t id, uint16_t flags, @@ -72,17 +70,6 @@ enum ofperr ofp_bundle_add_message(struct ofconn *, uint32_t id, void ofp_bundle_remove__(struct ofconn *, struct ofp_bundle *); -static inline struct ofp_bundle_entry * -ofp_bundle_entry_alloc(enum ofptype type, const struct ofp_header *oh) -{ - struct ofp_bundle_entry *entry = xmalloc(sizeof *entry); - - entry->type = type; - entry->msg = xmemdup(oh, ntohs(oh->length)); - - return entry; -} - static inline void ofp_bundle_entry_free(struct ofp_bundle_entry *entry) { diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 36f4c0b16d48..565a45db1507 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -7899,7 +7899,6 @@ handle_bundle_add(struct ofconn *ofconn, const struct ofp_header *oh) struct ofproto *ofproto = ofconn_get_ofproto(ofconn); enum ofperr error; struct ofputil_bundle_add_msg badd; - struct ofp_bundle_entry *bmsg; enum ofptype type; error = reject_slave_controller(ofconn); @@ -7912,7 +7911,8 @@ handle_bundle_add(struct ofconn *ofconn, const struct ofp_header *oh) return error; } - bmsg = ofp_bundle_entry_alloc(type, badd.msg); + /* Allocate bundle entry and decode the embedded message. */ + struct ofp_bundle_entry *bmsg = xmalloc(sizeof *bmsg); struct ofpbuf ofpacts; uint64_t ofpacts_stub[1024 / 8]; @@ -7951,18 +7951,23 @@ handle_bundle_add(struct ofconn *ofconn, const struct ofp_header *oh) } else { OVS_NOT_REACHED(); } - ofpbuf_uninit(&ofpacts); - - if (!error) { - error = ofp_bundle_add_message(ofconn, badd.bundle_id, badd.flags, - bmsg, oh); + if (error) { + free(bmsg); + return error; } + /* Now that the embedded message has been successfully decoded, finish up + * initializing the bundle entry. */ + bmsg->type = type; + bmsg->msg = xmemdup(oh, ntohs(oh->length)); + + /* Add bundle entry to bundle. */ + error = ofp_bundle_add_message(ofconn, badd.bundle_id, badd.flags, + bmsg, oh); if (error) { ofp_bundle_entry_free(bmsg); } - return error; } -- 2.16.1
Thanks. I applied this to master and branch-2.9. On Thu, May 10, 2018 at 09:13:41AM +0000, Anju Thomas wrote: > Hi Ben, > > Yes. This patch certainly looks more cleaner and better to me . > > Thanks > Anju > -----Original Message----- > From: Ben Pfaff [mailto:blp@ovn.org] > Sent: Thursday, May 10, 2018 2:33 AM > To: Anju Thomas <anju.thomas@ericsson.com> > Cc: dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH v1] Fix crash when processing malformed Bundle Add message in OVS > > On Mon, May 07, 2018 at 10:58:06PM +0530, Anju Thomas wrote: > > When an OpenFlow Bundle Add message is received, a bundle entry is > > created and the OpenFlow message embedded in the bundle add message is > > processed. If any error is encountered while processing the embedded > > message, the bundle entry is freed. The bundle entry free function > > assumes that the entry has been populated with a properly formatted > > OpenFlow message and performs some message specific cleanup actions . > > This assumption does not hold true in the error case and OVS crashes > > when performing the cleanup. > > > > The fix is in case of errors, simply free the bundle entry without > > attempting to perform any embedded message cleanup > > > > Signed-off-by: Anju Thomas <anju.thomas@ericsson.com> > > Thanks for the fix. > > It's not really nice to have multiple levels of initialized-ness. What do you think of this fix instead? > > Thanks, > > Ben. > > --8<--------------------------cut here-------------------------->8-- > > From: Anju Thomas <anju.thomas@ericsson.com> > Date: Mon, 7 May 2018 22:58:06 +0530 > Subject: [PATCH] Fix crash when processing malformed Bundle Add message in OVS > > When an OpenFlow Bundle Add message is received, a bundle entry is created and the OpenFlow message embedded in the bundle add message is processed. If any error is encountered while processing the embedded message, the bundle entry is freed. The bundle entry free function assumes that the entry has been populated with a properly formatted OpenFlow message and performs some message specific cleanup actions . > This assumption does not hold true in the error case and OVS crashes when performing the cleanup. > > The fix is in case of errors, simply free the bundle entry without attempting to perform any embedded message cleanup > > Signed-off-by: Anju Thomas <anju.thomas@ericsson.com> > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- > ofproto/bundles.h | 13 ------------- > ofproto/ofproto.c | 21 +++++++++++++-------- > 2 files changed, 13 insertions(+), 21 deletions(-) > > diff --git a/ofproto/bundles.h b/ofproto/bundles.h index 806e853d6883..1164fddd8702 100644 > --- a/ofproto/bundles.h > +++ b/ofproto/bundles.h > @@ -58,8 +58,6 @@ struct ofp_bundle { > struct ovs_list msg_list; /* List of 'struct bundle_message's */ > }; > > -static inline struct ofp_bundle_entry *ofp_bundle_entry_alloc( > - enum ofptype type, const struct ofp_header *oh); > static inline void ofp_bundle_entry_free(struct ofp_bundle_entry *); > > enum ofperr ofp_bundle_open(struct ofconn *, uint32_t id, uint16_t flags, @@ -72,17 +70,6 @@ enum ofperr ofp_bundle_add_message(struct ofconn *, uint32_t id, > > void ofp_bundle_remove__(struct ofconn *, struct ofp_bundle *); > > > -static inline struct ofp_bundle_entry * -ofp_bundle_entry_alloc(enum ofptype type, const struct ofp_header *oh) -{ > - struct ofp_bundle_entry *entry = xmalloc(sizeof *entry); > - > - entry->type = type; > - entry->msg = xmemdup(oh, ntohs(oh->length)); > - > - return entry; > -} > - > static inline void > ofp_bundle_entry_free(struct ofp_bundle_entry *entry) { diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 36f4c0b16d48..565a45db1507 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -7899,7 +7899,6 @@ handle_bundle_add(struct ofconn *ofconn, const struct ofp_header *oh) > struct ofproto *ofproto = ofconn_get_ofproto(ofconn); > enum ofperr error; > struct ofputil_bundle_add_msg badd; > - struct ofp_bundle_entry *bmsg; > enum ofptype type; > > error = reject_slave_controller(ofconn); @@ -7912,7 +7911,8 @@ handle_bundle_add(struct ofconn *ofconn, const struct ofp_header *oh) > return error; > } > > - bmsg = ofp_bundle_entry_alloc(type, badd.msg); > + /* Allocate bundle entry and decode the embedded message. */ > + struct ofp_bundle_entry *bmsg = xmalloc(sizeof *bmsg); > > struct ofpbuf ofpacts; > uint64_t ofpacts_stub[1024 / 8]; > @@ -7951,18 +7951,23 @@ handle_bundle_add(struct ofconn *ofconn, const struct ofp_header *oh) > } else { > OVS_NOT_REACHED(); > } > - > ofpbuf_uninit(&ofpacts); > - > - if (!error) { > - error = ofp_bundle_add_message(ofconn, badd.bundle_id, badd.flags, > - bmsg, oh); > + if (error) { > + free(bmsg); > + return error; > } > > + /* Now that the embedded message has been successfully decoded, finish up > + * initializing the bundle entry. */ > + bmsg->type = type; > + bmsg->msg = xmemdup(oh, ntohs(oh->length)); > + > + /* Add bundle entry to bundle. */ > + error = ofp_bundle_add_message(ofconn, badd.bundle_id, badd.flags, > + bmsg, oh); > if (error) { > ofp_bundle_entry_free(bmsg); > } > - > return error; > } > > -- > 2.16.1 >
diff --git a/ofproto/bundles.c b/ofproto/bundles.c index 405ec8c..628ba0a 100644 --- a/ofproto/bundles.c +++ b/ofproto/bundles.c @@ -63,7 +63,7 @@ ofp_bundle_remove__(struct ofconn *ofconn, struct ofp_bundle *bundle) struct ofp_bundle_entry *msg; LIST_FOR_EACH_POP (msg, node, &bundle->msg_list) { - ofp_bundle_entry_free(msg); + ofp_bundle_entry_free(msg, true); } ofconn_remove_bundle(ofconn, bundle); diff --git a/ofproto/bundles.h b/ofproto/bundles.h index 806e853..7fc731c 100644 --- a/ofproto/bundles.h +++ b/ofproto/bundles.h @@ -60,7 +60,7 @@ struct ofp_bundle { static inline struct ofp_bundle_entry *ofp_bundle_entry_alloc( enum ofptype type, const struct ofp_header *oh); -static inline void ofp_bundle_entry_free(struct ofp_bundle_entry *); +static inline void ofp_bundle_entry_free(struct ofp_bundle_entry *, bool); enum ofperr ofp_bundle_open(struct ofconn *, uint32_t id, uint16_t flags, const struct ofp_header *); @@ -84,15 +84,17 @@ ofp_bundle_entry_alloc(enum ofptype type, const struct ofp_header *oh) } static inline void -ofp_bundle_entry_free(struct ofp_bundle_entry *entry) +ofp_bundle_entry_free(struct ofp_bundle_entry *entry, bool cleanup) { if (entry) { - if (entry->type == OFPTYPE_FLOW_MOD) { - ofproto_flow_mod_uninit(&entry->ofm); - } else if (entry->type == OFPTYPE_GROUP_MOD) { - ofputil_uninit_group_mod(&entry->ogm.gm); - } else if (entry->type == OFPTYPE_PACKET_OUT) { - ofproto_packet_out_uninit(&entry->opo); + if (cleanup) { + if (entry->type == OFPTYPE_FLOW_MOD) { + ofproto_flow_mod_uninit(&entry->ofm); + } else if (entry->type == OFPTYPE_GROUP_MOD) { + ofputil_uninit_group_mod(&entry->ogm.gm); + } else if (entry->type == OFPTYPE_PACKET_OUT) { + ofproto_packet_out_uninit(&entry->opo); + } } free(entry->msg); free(entry); diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 36f4c0b..bfbc043 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -7901,6 +7901,7 @@ handle_bundle_add(struct ofconn *ofconn, const struct ofp_header *oh) struct ofputil_bundle_add_msg badd; struct ofp_bundle_entry *bmsg; enum ofptype type; + bool cleanup=true; error = reject_slave_controller(ofconn); if (error) { @@ -7929,7 +7930,13 @@ handle_bundle_add(struct ofconn *ofconn, const struct ofp_header *oh) &ofproto->vl_mff_map, &ofpacts, u16_to_ofp(ofproto->max_ports), ofproto->n_tables); - if (!error) { + if (error) { + VLOG_WARN("Unable to parse OFPTYPE_FLOW_MOD message in " + "OFPTYPE_BUNDLE_ADD_MESSAGE(bundle idx = %x)" + "successfully", + badd.bundle_id); + cleanup=false; + } else { error = ofproto_flow_mod_init(ofproto, &bmsg->ofm, &fm, NULL); minimatch_destroy(&fm.match); } @@ -7944,7 +7951,13 @@ handle_bundle_add(struct ofconn *ofconn, const struct ofp_header *oh) error = ofputil_decode_packet_out(&po, badd.msg, ofproto_get_tun_tab(ofproto), &ofpacts); - if (!error) { + if (error) { + VLOG_WARN("Unable to parse OFPTYPE_PACKET_OUT message in " + "OFPTYPE_BUNDLE_ADD_MESSAGE(bundle idx = %x)" + "successfully", + badd.bundle_id); + cleanup=false; + } else { po.ofpacts = ofpbuf_steal_data(&ofpacts); /* Move to heap. */ error = ofproto_packet_out_init(ofproto, ofconn, &bmsg->opo, &po); } @@ -7960,7 +7973,7 @@ handle_bundle_add(struct ofconn *ofconn, const struct ofp_header *oh) } if (error) { - ofp_bundle_entry_free(bmsg); + ofp_bundle_entry_free(bmsg, cleanup); } return error;
When an OpenFlow Bundle Add message is received, a bundle entry is created and the OpenFlow message embedded in the bundle add message is processed. If any error is encountered while processing the embedded message, the bundle entry is freed. The bundle entry free function assumes that the entry has been populated with a properly formatted OpenFlow message and performs some message specific cleanup actions . This assumption does not hold true in the error case and OVS crashes when performing the cleanup. The fix is in case of errors, simply free the bundle entry without attempting to perform any embedded message cleanup Signed-off-by: Anju Thomas <anju.thomas@ericsson.com> --- ofproto/bundles.c | 2 +- ofproto/bundles.h | 18 ++++++++++-------- ofproto/ofproto.c | 19 ++++++++++++++++--- 3 files changed, 27 insertions(+), 12 deletions(-)