| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <49A8E036.5090802@cosmosbay.com>
Date: Sat, 28 Feb 2009 07:56:54 +0100
From: Eric Dumazet <dada1@...mosbay.com>
To: Jan Engelhardt <jengelh@...ozas.de>
CC: Patrick McHardy <kaber@...sh.net>,
Stephen Hemminger <shemminger@...tta.com>,
David Miller <davem@...emloft.net>,
Rick Jones <rick.jones2@...com>, netdev@...r.kernel.org,
netfilter-devel@...r.kernel.org
Subject: Re: [PATCH] iptables: xt_hashlimit fix
Jan Engelhardt a écrit :
> On Friday 2009-02-20 19:33, Jan Engelhardt wrote:
>> On Friday 2009-02-20 19:10, Eric Dumazet wrote:
>>> Damned this broke xt_hashlimit, version=0
>>> Look file "net/netfilter/xt_hashlimit.c" line 706
>>>
>>> /* Ugly hack: For SMP, we only want to use one set */
>>> r->u.master = r;
>>>
>>> So, it appears some modules are using pointers to themselves, what a hack :(
>>> We probably need an audit of other modules.
>> xt_limit and xt_statistic are affected; I'll happily fix that up.
>
> Please have a look!
>
> ---8<---
> parent b2bd9ab764d65237232c4aad5ab8d5d8b5714f72 (v2.6.29-rc6-31-gb2bd9ab)
> commit 3e7ee1dcc808b8eec82eddfa0436f78e31d2004a
> Author: Jan Engelhardt <jengelh@...ozas.de>
> Date: Sat Feb 28 02:49:28 2009 +0100
>
> netfilter: xtables: avoid pointer to self
>
> Signed-off-by: Jan Engelhardt <jengelh@...ozas.de>
Seems good to me ! Thanks Jan !
Reviewed-by: Eric Dumazet <dada1@...mosbay.com>
Are you sure xt_quota doesnt need some tweak, or should we not care of changes
done in quota while temporary tables are installed (iptables -L) ?
> ---
> include/linux/netfilter/xt_limit.h | 9 +++--
> include/linux/netfilter/xt_statistic.h | 7 ++--
> net/netfilter/xt_limit.c | 40 +++++++++++++++++------
> net/netfilter/xt_statistic.c | 28 +++++++++++++---
> 4 files changed, 61 insertions(+), 23 deletions(-)
>
> diff --git a/include/linux/netfilter/xt_limit.h b/include/linux/netfilter/xt_limit.h
> index b3ce653..fda222c 100644
> --- a/include/linux/netfilter/xt_limit.h
> +++ b/include/linux/netfilter/xt_limit.h
> @@ -4,6 +4,8 @@
> /* timings are in milliseconds. */
> #define XT_LIMIT_SCALE 10000
>
> +struct xt_limit_priv;
> +
> /* 1/10,000 sec period => max of 10,000/sec. Min rate is then 429490
> seconds, or one every 59 hours. */
> struct xt_rateinfo {
> @@ -11,11 +13,10 @@ struct xt_rateinfo {
> u_int32_t burst; /* Period multiplier for upper limit. */
>
> /* Used internally by the kernel */
> - unsigned long prev;
> - u_int32_t credit;
> + unsigned long prev; /* moved to xt_limit_priv */
> + u_int32_t credit; /* moved to xt_limit_priv */
> u_int32_t credit_cap, cost;
>
> - /* Ugly, ugly fucker. */
> - struct xt_rateinfo *master;
> + struct xt_limit_priv *master;
> };
> #endif /*_XT_RATE_H*/
> diff --git a/include/linux/netfilter/xt_statistic.h b/include/linux/netfilter/xt_statistic.h
> index 3d38bc9..8f521ab 100644
> --- a/include/linux/netfilter/xt_statistic.h
> +++ b/include/linux/netfilter/xt_statistic.h
> @@ -13,6 +13,8 @@ enum xt_statistic_flags {
> };
> #define XT_STATISTIC_MASK 0x1
>
> +struct xt_statistic_priv;
> +
> struct xt_statistic_info {
> u_int16_t mode;
> u_int16_t flags;
> @@ -23,11 +25,10 @@ struct xt_statistic_info {
> struct {
> u_int32_t every;
> u_int32_t packet;
> - /* Used internally by the kernel */
> - u_int32_t count;
> + u_int32_t count; /* unused */
> } nth;
> } u;
> - struct xt_statistic_info *master __attribute__((aligned(8)));
> + struct xt_statistic_priv *master __attribute__((aligned(8)));
> };
>
> #endif /* _XT_STATISTIC_H */
> diff --git a/net/netfilter/xt_limit.c b/net/netfilter/xt_limit.c
> index c908d69..2e8089e 100644
> --- a/net/netfilter/xt_limit.c
> +++ b/net/netfilter/xt_limit.c
> @@ -14,6 +14,11 @@
> #include <linux/netfilter/x_tables.h>
> #include <linux/netfilter/xt_limit.h>
>
> +struct xt_limit_priv {
> + unsigned long prev;
> + uint32_t credit;
> +};
> +
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("Herve Eychenne <rv@...lfire.org>");
> MODULE_DESCRIPTION("Xtables: rate-limit match");
> @@ -60,18 +65,18 @@ static DEFINE_SPINLOCK(limit_lock);
> static bool
> limit_mt(const struct sk_buff *skb, const struct xt_match_param *par)
> {
> - struct xt_rateinfo *r =
> - ((const struct xt_rateinfo *)par->matchinfo)->master;
> + const struct xt_rateinfo *r = par->matchinfo;
> + struct xt_limit_priv *priv = r->master;
> unsigned long now = jiffies;
>
> spin_lock_bh(&limit_lock);
> - r->credit += (now - xchg(&r->prev, now)) * CREDITS_PER_JIFFY;
> - if (r->credit > r->credit_cap)
> - r->credit = r->credit_cap;
> + priv->credit += (now - xchg(&priv->prev, now)) * CREDITS_PER_JIFFY;
> + if (priv->credit > r->credit_cap)
> + priv->credit = r->credit_cap;
>
> - if (r->credit >= r->cost) {
> + if (priv->credit >= r->cost) {
> /* We're not limited. */
> - r->credit -= r->cost;
> + priv->credit -= r->cost;
> spin_unlock_bh(&limit_lock);
> return true;
> }
> @@ -95,6 +100,7 @@ user2credits(u_int32_t user)
> static bool limit_mt_check(const struct xt_mtchk_param *par)
> {
> struct xt_rateinfo *r = par->matchinfo;
> + struct xt_limit_priv *priv;
>
> /* Check for overflow. */
> if (r->burst == 0
> @@ -104,19 +110,30 @@ static bool limit_mt_check(const struct xt_mtchk_param *par)
> return false;
> }
>
> - /* For SMP, we only want to use one set of counters. */
> - r->master = r;
> + priv = kmalloc(sizeof(*priv), GFP_KERNEL);
> + if (priv == NULL)
> + return -ENOMEM;
> +
> + /* For SMP, we only want to use one set of state. */
> + r->master = priv;
> if (r->cost == 0) {
> /* User avg in seconds * XT_LIMIT_SCALE: convert to jiffies *
> 128. */
> - r->prev = jiffies;
> - r->credit = user2credits(r->avg * r->burst); /* Credits full. */
> + priv->prev = jiffies;
> + priv->credit = user2credits(r->avg * r->burst); /* Credits full. */
> r->credit_cap = user2credits(r->avg * r->burst); /* Credits full. */
> r->cost = user2credits(r->avg);
> }
> return true;
> }
>
> +static void limit_mt_destroy(const struct xt_mtdtor_param *par)
> +{
> + const struct xt_rateinfo *info = par->matchinfo;
> +
> + kfree(info->master);
> +}
> +
> #ifdef CONFIG_COMPAT
> struct compat_xt_rateinfo {
> u_int32_t avg;
> @@ -167,6 +184,7 @@ static struct xt_match limit_mt_reg __read_mostly = {
> .family = NFPROTO_UNSPEC,
> .match = limit_mt,
> .checkentry = limit_mt_check,
> + .destroy = limit_mt_destroy,
> .matchsize = sizeof(struct xt_rateinfo),
> #ifdef CONFIG_COMPAT
> .compatsize = sizeof(struct compat_xt_rateinfo),
> diff --git a/net/netfilter/xt_statistic.c b/net/netfilter/xt_statistic.c
> index 0d75141..d8c0f8f 100644
> --- a/net/netfilter/xt_statistic.c
> +++ b/net/netfilter/xt_statistic.c
> @@ -16,6 +16,10 @@
> #include <linux/netfilter/xt_statistic.h>
> #include <linux/netfilter/x_tables.h>
>
> +struct xt_statistic_priv {
> + uint32_t count;
> +};
> +
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("Patrick McHardy <kaber@...sh.net>");
> MODULE_DESCRIPTION("Xtables: statistics-based matching (\"Nth\", random)");
> @@ -27,7 +31,7 @@ static DEFINE_SPINLOCK(nth_lock);
> static bool
> statistic_mt(const struct sk_buff *skb, const struct xt_match_param *par)
> {
> - struct xt_statistic_info *info = (void *)par->matchinfo;
> + const struct xt_statistic_info *info = par->matchinfo;
> bool ret = info->flags & XT_STATISTIC_INVERT;
>
> switch (info->mode) {
> @@ -36,10 +40,9 @@ statistic_mt(const struct sk_buff *skb, const struct xt_match_param *par)
> ret = !ret;
> break;
> case XT_STATISTIC_MODE_NTH:
> - info = info->master;
> spin_lock_bh(&nth_lock);
> - if (info->u.nth.count++ == info->u.nth.every) {
> - info->u.nth.count = 0;
> + if (info->master->count++ == info->u.nth.every) {
> + info->master->count = 0;
> ret = !ret;
> }
> spin_unlock_bh(&nth_lock);
> @@ -56,16 +59,31 @@ static bool statistic_mt_check(const struct xt_mtchk_param *par)
> if (info->mode > XT_STATISTIC_MODE_MAX ||
> info->flags & ~XT_STATISTIC_MASK)
> return false;
> - info->master = info;
> +
> + info->master = kzalloc(sizeof(*info->master), GFP_KERNEL);
> + if (info->master == NULL) {
> + printk(KERN_ERR KBUILD_MODNAME ": Out of memory\n");
> + return false;
> + }
> + info->master->count = info->u.nth.count;
> +
> return true;
> }
>
> +static void statistic_mt_destroy(const struct xt_mtdtor_param *par)
> +{
> + const struct xt_statistic_info *info = par->matchinfo;
> +
> + kfree(info->master);
> +}
> +
> static struct xt_match xt_statistic_mt_reg __read_mostly = {
> .name = "statistic",
> .revision = 0,
> .family = NFPROTO_UNSPEC,
> .match = statistic_mt,
> .checkentry = statistic_mt_check,
> + .destroy = statistic_mt_destroy,
> .matchsize = sizeof(struct xt_statistic_info),
> .me = THIS_MODULE,
> };
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists