[ovs-dev,2/2] dpif-netdev: Prevent unsafe access when retrieving meter stats.

Message ID 20180829004614.129668-2-jpettit@ovn.org
State New
Headers show
Series
  • [ovs-dev,1/2] dpif-netdev: Don't check if xcalloc() failed when creating meter.
Related show

Commit Message

Justin Pettit Aug. 29, 2018, 12:46 a.m.
dpif_netdev_meter_get() retrieved a pointer to a meter entry without
holding a lock.  It's possible that another thread could have deleted
that entry between retrieving the pointer and dereferencing the pointer.
This makes the function hold the lock the entire time the meter entry is
needed.

Found by inspection.

Signed-off-by: Justin Pettit <jpettit@ovn.org>
---
 lib/dpif-netdev.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

Comments

Flavio Leitner Aug. 30, 2018, 2:25 p.m. | #1
On Tue, Aug 28, 2018 at 05:46:14PM -0700, Justin Pettit wrote:
> dpif_netdev_meter_get() retrieved a pointer to a meter entry without
> holding a lock.  It's possible that another thread could have deleted
> that entry between retrieving the pointer and dereferencing the pointer.
> This makes the function hold the lock the entire time the meter entry is
> needed.
> 
> Found by inspection.
> 
> Signed-off-by: Justin Pettit <jpettit@ovn.org>
> ---

Acked-by: Flavio Leitner <fbl@sysclose.org>
Justin Pettit Sept. 5, 2018, 6:44 p.m. | #2
> On Aug 30, 2018, at 7:25 AM, Flavio Leitner <fbl@sysclose.org> wrote:
> 
> On Tue, Aug 28, 2018 at 05:46:14PM -0700, Justin Pettit wrote:
>> dpif_netdev_meter_get() retrieved a pointer to a meter entry without
>> holding a lock.  It's possible that another thread could have deleted
>> that entry between retrieving the pointer and dereferencing the pointer.
>> This makes the function hold the lock the entire time the meter entry is
>> needed.
>> 
>> Found by inspection.
>> 
>> Signed-off-by: Justin Pettit <jpettit@ovn.org>
>> ---
> 
> Acked-by: Flavio Leitner <fbl@sysclose.org>

Thank you.  I pushed this to master and all the way back to branch-2.8.

--Justin

Patch

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 8b0b3745860b..7c0300cc554a 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -5243,20 +5243,22 @@  dpif_netdev_meter_get(const struct dpif *dpif,
                       struct ofputil_meter_stats *stats, uint16_t n_bands)
 {
     const struct dp_netdev *dp = get_dp_netdev(dpif);
-    const struct dp_meter *meter;
     uint32_t meter_id = meter_id_.uint32;
+    int retval = 0;
 
     if (meter_id >= MAX_METERS) {
         return EFBIG;
     }
-    meter = dp->meters[meter_id];
+
+    meter_lock(dp, meter_id);
+    const struct dp_meter *meter = dp->meters[meter_id];
     if (!meter) {
-        return ENOENT;
+        retval = ENOENT;
+        goto done;
     }
     if (stats) {
         int i = 0;
 
-        meter_lock(dp, meter_id);
         stats->packet_in_count = meter->packet_count;
         stats->byte_in_count = meter->byte_count;
 
@@ -5264,11 +5266,13 @@  dpif_netdev_meter_get(const struct dpif *dpif,
             stats->bands[i].packet_count = meter->bands[i].packet_count;
             stats->bands[i].byte_count = meter->bands[i].byte_count;
         }
-        meter_unlock(dp, meter_id);
 
         stats->n_bands = i;
     }
-    return 0;
+
+done:
+    meter_unlock(dp, meter_id);
+    return retval;
 }
 
 static int