Fix improper boundary checks and reference count leaks

Boundary checks allow for one extra label than the maximum, causing memory corruption. Also, when a label is changed, reference to old nexthop has to be released. Two harmless boundary checks in nexthop subysystem is also addressed. Change-Id: I7c7e8cd39797d8d203cac8087d5e31cf02438452 Closes-BUG: #1446550
Juniper · Apr 21, 2015 · f004b0c · f004b0c
1 parent 3dbe5b5
commit f004b0c
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 18 deletions.
diff --git a/dp-core/vr_mpls.c b/dp-core/vr_mpls.c
@@ -11,7 +11,7 @@
 struct vr_nexthop *
 __vrouter_get_label(struct vrouter *router, unsigned int label)
 {
-    if (!router || label > router->vr_max_labels)
+    if (!router || label >= router->vr_max_labels)
         return NULL;
 
     return router->vr_ilm[label];
@@ -25,6 +25,17 @@ vrouter_get_label(unsigned int rid, unsigned int label)
     return __vrouter_get_label(router, label);
 }
 
+static void
+__vr_mpls_del(struct vrouter *router, unsigned int label)
+{
+    if (router->vr_ilm[label])
+        vrouter_put_nexthop(router->vr_ilm[label]);
+
+    router->vr_ilm[label] = NULL;
+
+    return;
+}
+
 int
 vr_mpls_del(vr_mpls_req *req)
 {
@@ -37,16 +48,12 @@ vr_mpls_del(vr_mpls_req *req)
         goto generate_resp;
     }
 
-    if (req->mr_label > (int)router->vr_max_labels) {
+    if ((unsigned int)req->mr_label >= router->vr_max_labels) {
         ret = -EINVAL;
         goto generate_resp;
     }
 
-    if (router->vr_ilm[req->mr_label])
-        vrouter_put_nexthop(router->vr_ilm[req->mr_label]);
-
-    router->vr_ilm[req->mr_label] = NULL;
-
+    __vr_mpls_del(router, (unsigned int)req->mr_label);
 generate_resp:
     vr_send_response(ret);
 
@@ -66,11 +73,13 @@ vr_mpls_add(vr_mpls_req *req)
         goto generate_resp;
     }
 
-    if ((unsigned int)req->mr_label > router->vr_max_labels) {
+    if ((unsigned int)req->mr_label >= router->vr_max_labels) {
         ret = -EINVAL;
         goto generate_resp;
     }
 
+    __vr_mpls_del(router, (unsigned int)req->mr_label);
+
     nh = vrouter_get_nexthop(req->mr_rid, req->mr_nhid);
     if (!nh)  {
         ret = -EINVAL;
@@ -141,17 +150,26 @@ vr_mpls_get(vr_mpls_req *req)
     struct vrouter *router;
 
     router = vrouter_get(req->mr_rid);
-    if (!router || req->mr_label > (int)router->vr_max_labels) {
+    if (!router) {
         ret = -ENODEV;
-    } else {
-        nh = vrouter_get_label(req->mr_rid, req->mr_label);
-        if (!nh)
-            ret = -ENOENT;
+        goto generate_response;
     }
 
-    if (!ret)
-        vr_mpls_make_req(req, nh, req->mr_label);
-    else
+    if ((unsigned int)req->mr_label >= router->vr_max_labels) {
+        ret = -EINVAL;
+        goto generate_response;
+    }
+
+    nh = vrouter_get_label(req->mr_rid, req->mr_label);
+    if (!nh) {
+        ret = -ENOENT;
+        goto generate_response;
+    }
+
+    vr_mpls_make_req(req, nh, req->mr_label);
+
+generate_response:
+    if (ret)
         req = NULL;
 
     vr_message_response(VR_MPLS_OBJECT_ID, req, ret);

diff --git a/dp-core/vr_nexthop.c b/dp-core/vr_nexthop.c
@@ -82,7 +82,7 @@ vrouter_add_nexthop(struct vr_nexthop *nh)
 {
     struct vrouter *router = vrouter_get(nh->nh_rid);
 
-    if (!router || nh->nh_id > router->vr_max_nexthops)
+    if (!router || nh->nh_id >= router->vr_max_nexthops)
         return -EINVAL;
 
     /*
@@ -103,7 +103,7 @@ nh_del(struct vr_nexthop *nh)
 {
     struct vrouter *router = vrouter_get(nh->nh_rid);
 
-    if (!router || nh->nh_id > router->vr_max_nexthops)
+    if (!router || nh->nh_id >= router->vr_max_nexthops)
         return; 
 
     if (router->vr_nexthops[nh->nh_id]) {