Patchwork Bug in limit_mt_check()

login
register
mail settings
Submitter Jan Engelhardt
Date Sept. 22, 2012, 8:03 a.m.
Message ID <alpine.LNX.2.01.1209220957510.3512@nerf07.vanv.qr>
Download mbox | patch
Permalink /patch/186111/
State Not Applicable
Headers show

Comments

Jan Engelhardt - Sept. 22, 2012, 8:03 a.m.
On Saturday 2012-09-22 00:26, Jim Robinson wrote:
>
>The bug is that under reproducible conditions, it is possible for
>limit_mt_check() to execute and kmalloc() a structure which does not
>get initialized. Specifically, if 'r->cost' is non-zero, 'priv' is
>kmalloc()ed but not initialized. The result of this is that
>'priv->prev' and 'priv->credit' have bad values in them
>
>--8<-- (Jim's patch)
Without this patch the allocated 'priv' structure could be used without initializing. This
was causing premature rate-limit matches (i.e. limit_mt() returning 'true' when it shouldn't
have). This patch is a best guess as to how to initialize 'priv', and seems to work.

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Engelhardt - Sept. 22, 2012, 8:13 a.m.
>On Saturday 2012-09-22 00:26, Jim Robinson wrote:
>>
>>The bug is that under reproducible conditions, it is possible for
>>limit_mt_check() to execute and kmalloc() a structure which does not
>>get initialized. Specifically, if 'r->cost' is non-zero, 'priv' is
>>kmalloc()ed but not initialized. The result of this is that
>>'priv->prev' and 'priv->credit' have bad values in them
>>
>>--8<-- (Jim's patch)
>Without this patch the allocated 'priv' structure could be used without initializing. This
>was causing premature rate-limit matches (i.e. limit_mt() returning 'true' when it shouldn't
>have). This patch is a best guess as to how to initialize 'priv', and seems to work.
>
>diff -Nur linux-2.6.32.8/net/netfilter/xt_limit.c.ORIG linux-2.6.32.8/net/netfilter/xt_limit.c
>--- linux-2.6.32.8/net/netfilter/xt_limit.c.ORIG	2012-09-12 17:14:19.758371942 -0700
>+++ linux-2.6.32.8/net/netfilter/xt_limit.c	2012-09-19 12:09:00.238416282 -0700
>@@ -114,6 +114,32 @@
> 	if (priv == NULL)
> 		return false;
> 
>+    // Start of Infoblox patch.
>+    // Zero out 'priv' for good measure.
>+    memset(priv, 0, sizeof(*priv));
>+    // This seems to fix the problem of priv not being initialized when
>+    // r->cost is non-zero.
>+    if (r->cost != 0) {
>+        // Note that non-zero r->cost seems to imply non-null
>+        // r->master, however, better safe than sorry, so check r->master.
>+        if (r->master != NULL) {

r->master does not have any sensical value when it's coming from userspace.

>+            spin_lock_bh(&limit_lock);
>+            priv->prev = r->master->prev;
>+            priv->credit = r->master->credit;
>+            spin_unlock_bh(&limit_lock);
>+        } else {
>+            // Not sure if this can ever happen. And definitely not sure
>+            // if this is the right thing to do, however, priv needs to be
>+            // initialized to something.
>+            printk("limit_mt_check: unexpected non-zero cost and null master\n");
>+            priv->prev = jiffies;
>+            priv->credit = user2credits(r->avg * r->burst); /* Credits full. */
>+        }
>+        r->master = priv;
>+        return true;
>+    }
>+    // End of Infoblox patch.
>+
> 	/* For SMP, we only want to use one set of state. */
> 	r->master = priv;
> 	if (r->cost == 0) {

A patch I think is better comes as reply to this message.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff -Nur linux-2.6.32.8/net/netfilter/xt_limit.c.ORIG linux-2.6.32.8/net/netfilter/xt_limit.c
--- linux-2.6.32.8/net/netfilter/xt_limit.c.ORIG	2012-09-12 17:14:19.758371942 -0700
+++ linux-2.6.32.8/net/netfilter/xt_limit.c	2012-09-19 12:09:00.238416282 -0700
@@ -114,6 +114,32 @@ 
 	if (priv == NULL)
 		return false;
 
+    // Start of Infoblox patch.
+    // Zero out 'priv' for good measure.
+    memset(priv, 0, sizeof(*priv));
+    // This seems to fix the problem of priv not being initialized when
+    // r->cost is non-zero.
+    if (r->cost != 0) {
+        // Note that non-zero r->cost seems to imply non-null
+        // r->master, however, better safe than sorry, so check r->master.
+        if (r->master != NULL) {
+            spin_lock_bh(&limit_lock);
+            priv->prev = r->master->prev;
+            priv->credit = r->master->credit;
+            spin_unlock_bh(&limit_lock);
+        } else {
+            // Not sure if this can ever happen. And definitely not sure
+            // if this is the right thing to do, however, priv needs to be
+            // initialized to something.
+            printk("limit_mt_check: unexpected non-zero cost and null master\n");
+            priv->prev = jiffies;
+            priv->credit = user2credits(r->avg * r->burst); /* Credits full. */
+        }
+        r->master = priv;
+        return true;
+    }
+    // End of Infoblox patch.
+
 	/* For SMP, we only want to use one set of state. */
 	r->master = priv;
 	if (r->cost == 0) {