diff mbox series

[ovs-dev,v4,2/3] ofp-util: Fix memory leaks on error cases in ofputil_decode_group_mod().

Message ID 20170921165958.3218-3-blp@ovn.org
State Accepted
Headers show
Series Fix memory leaks and overreads in ofp-util | expand

Commit Message

Ben Pfaff Sept. 21, 2017, 4:59 p.m. UTC
Found by libFuzzer.

Reported-by: Bhargava Shastry <bshastry@sec.t-labs.tu-berlin.de>
Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 lib/ofp-util.c | 82 ++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 48 insertions(+), 34 deletions(-)

Comments

Justin Pettit Sept. 22, 2017, 9:26 p.m. UTC | #1
> On Sep 21, 2017, at 9:59 AM, Ben Pfaff <blp@ovn.org> wrote:
> 
> Found by libFuzzer.
> 
> Reported-by: Bhargava Shastry <bshastry@sec.t-labs.tu-berlin.de>
> Signed-off-by: Ben Pfaff <blp@ovn.org>

Acked-by: Justin Pettit <jpettit@ovn.org>

--Justin
Ben Pfaff Sept. 22, 2017, 9:57 p.m. UTC | #2
On Fri, Sep 22, 2017 at 02:26:36PM -0700, Justin Pettit wrote:
> 
> > On Sep 21, 2017, at 9:59 AM, Ben Pfaff <blp@ovn.org> wrote:
> > 
> > Found by libFuzzer.
> > 
> > Reported-by: Bhargava Shastry <bshastry@sec.t-labs.tu-berlin.de>
> > Signed-off-by: Ben Pfaff <blp@ovn.org>
> 
> Acked-by: Justin Pettit <jpettit@ovn.org>

Thanks, applied to master, backported as far as 2.6.
Markos Chandras Nov. 29, 2017, 10 a.m. UTC | #3
On 22/09/17 22:57, Ben Pfaff wrote:
> On Fri, Sep 22, 2017 at 02:26:36PM -0700, Justin Pettit wrote:
>>
>>> On Sep 21, 2017, at 9:59 AM, Ben Pfaff <blp@ovn.org> wrote:
>>>
>>> Found by libFuzzer.
>>>
>>> Reported-by: Bhargava Shastry <bshastry@sec.t-labs.tu-berlin.de>
>>> Signed-off-by: Ben Pfaff <blp@ovn.org>
>>
>> Acked-by: Justin Pettit <jpettit@ovn.org>
> 
> Thanks, applied to master, backported as far as 2.6.

Hello,

Should this patch be in 2.5 too given it's an LTS release?
Ben Pfaff Nov. 29, 2017, 4:51 p.m. UTC | #4
On Wed, Nov 29, 2017 at 10:00:33AM +0000, Markos Chandras wrote:
> On 22/09/17 22:57, Ben Pfaff wrote:
> > On Fri, Sep 22, 2017 at 02:26:36PM -0700, Justin Pettit wrote:
> >>
> >>> On Sep 21, 2017, at 9:59 AM, Ben Pfaff <blp@ovn.org> wrote:
> >>>
> >>> Found by libFuzzer.
> >>>
> >>> Reported-by: Bhargava Shastry <bshastry@sec.t-labs.tu-berlin.de>
> >>> Signed-off-by: Ben Pfaff <blp@ovn.org>
> >>
> >> Acked-by: Justin Pettit <jpettit@ovn.org>
> > 
> > Thanks, applied to master, backported as far as 2.6.
> 
> Hello,
> 
> Should this patch be in 2.5 too given it's an LTS release?

I normally backport as far as a patch applies, so probably it didn't
apply to 2.5.  If you can see that the memory leak is still present in
2.5, please do feel free to fix up the patch to apply to 2.5 and submit
it.
diff mbox series

Patch

diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index e915cb2ab2d7..2309a2ad2515 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -9174,6 +9174,7 @@  ofputil_pull_ofp11_buckets(struct ofpbuf *msg, size_t buckets_length,
         if (!ob) {
             VLOG_WARN_RL(&bad_ofmsg_rl, "buckets end with %"PRIuSIZE" leftover bytes",
                          buckets_length);
+            ofputil_bucket_list_destroy(buckets);
             return OFPERR_OFPGMFC_BAD_BUCKET;
         }
 
@@ -9181,11 +9182,13 @@  ofputil_pull_ofp11_buckets(struct ofpbuf *msg, size_t buckets_length,
         if (ob_len < sizeof *ob) {
             VLOG_WARN_RL(&bad_ofmsg_rl, "OpenFlow message bucket length "
                          "%"PRIuSIZE" is not valid", ob_len);
+            ofputil_bucket_list_destroy(buckets);
             return OFPERR_OFPGMFC_BAD_BUCKET;
         } else if (ob_len > buckets_length) {
             VLOG_WARN_RL(&bad_ofmsg_rl, "OpenFlow message bucket length "
                          "%"PRIuSIZE" exceeds remaining buckets data size %"PRIuSIZE,
                          ob_len, buckets_length);
+            ofputil_bucket_list_destroy(buckets);
             return OFPERR_OFPGMFC_BAD_BUCKET;
         }
         buckets_length -= ob_len;
@@ -9817,6 +9820,7 @@  ofputil_pull_ofp11_group_mod(struct ofpbuf *msg, enum ofp_version ofp_version,
         && gm->command == OFPGC11_DELETE
         && !ovs_list_is_empty(&gm->buckets)) {
         error = OFPERR_OFPGMFC_INVALID_GROUP;
+        ofputil_bucket_list_destroy(&gm->buckets);
     }
 
     return error;
@@ -9881,41 +9885,9 @@  ofputil_pull_ofp15_group_mod(struct ofpbuf *msg, enum ofp_version ofp_version,
                                         msg->size);
 }
 
-/* Converts OpenFlow group mod message 'oh' into an abstract group mod in
- * 'gm'.  Returns 0 if successful, otherwise an OpenFlow error code. */
-enum ofperr
-ofputil_decode_group_mod(const struct ofp_header *oh,
-                         struct ofputil_group_mod *gm)
+static enum ofperr
+ofputil_check_group_mod(const struct ofputil_group_mod *gm)
 {
-    ofputil_init_group_properties(&gm->props);
-
-    enum ofp_version ofp_version = oh->version;
-    struct ofpbuf msg = ofpbuf_const_initializer(oh, ntohs(oh->length));
-    ofpraw_pull_assert(&msg);
-
-    enum ofperr err;
-    switch (ofp_version)
-    {
-    case OFP11_VERSION:
-    case OFP12_VERSION:
-    case OFP13_VERSION:
-    case OFP14_VERSION:
-        err = ofputil_pull_ofp11_group_mod(&msg, ofp_version, gm);
-        break;
-
-    case OFP15_VERSION:
-    case OFP16_VERSION:
-        err = ofputil_pull_ofp15_group_mod(&msg, ofp_version, gm);
-        break;
-
-    case OFP10_VERSION:
-    default:
-        OVS_NOT_REACHED();
-    }
-    if (err) {
-        return err;
-    }
-
     switch (gm->type) {
     case OFPGT11_INDIRECT:
         if (gm->command != OFPGC11_DELETE
@@ -9977,6 +9949,48 @@  ofputil_decode_group_mod(const struct ofp_header *oh,
     return 0;
 }
 
+/* Converts OpenFlow group mod message 'oh' into an abstract group mod in
+ * 'gm'.  Returns 0 if successful, otherwise an OpenFlow error code. */
+enum ofperr
+ofputil_decode_group_mod(const struct ofp_header *oh,
+                         struct ofputil_group_mod *gm)
+{
+    ofputil_init_group_properties(&gm->props);
+
+    enum ofp_version ofp_version = oh->version;
+    struct ofpbuf msg = ofpbuf_const_initializer(oh, ntohs(oh->length));
+    ofpraw_pull_assert(&msg);
+
+    enum ofperr err;
+    switch (ofp_version)
+    {
+    case OFP11_VERSION:
+    case OFP12_VERSION:
+    case OFP13_VERSION:
+    case OFP14_VERSION:
+        err = ofputil_pull_ofp11_group_mod(&msg, ofp_version, gm);
+        break;
+
+    case OFP15_VERSION:
+    case OFP16_VERSION:
+        err = ofputil_pull_ofp15_group_mod(&msg, ofp_version, gm);
+        break;
+
+    case OFP10_VERSION:
+    default:
+        OVS_NOT_REACHED();
+    }
+    if (err) {
+        return err;
+    }
+
+    err = ofputil_check_group_mod(gm);
+    if (err) {
+        ofputil_uninit_group_mod(gm);
+    }
+    return err;
+}
+
 /* Destroys 'bms'. */
 void
 ofputil_free_bundle_msgs(struct ofputil_bundle_msg *bms, size_t n_bms)