diff mbox

[nf,1/3] netfilter: nft_dynset: fix panic if NFT_SET_HASH is not enabled

Message ID CAML_gOeZgqToS0_V==or4drPPDg1FPYz3dT_+FXEmfdZp5W0Vw@mail.gmail.com
State RFC
Delegated to: Pablo Neira
Headers show

Commit Message

Liping Zhang Oct. 25, 2016, 2:25 p.m. UTC 
2016-10-22 18:51 GMT+08:00 Liping Zhang <zlpnobody@163.com>:
> From: Liping Zhang <zlpnobody@gmail.com>
>
> When CONFIG_NFT_SET_HASH is not enabled and I input the following rule:
> "nft add rule filter output flow table test {ip daddr counter }", kernel
> panic happened on my system:
>  BUG: unable to handle kernel NULL pointer dereference at (null)
> ---
>  net/netfilter/nft_dynset.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/net/netfilter/nft_dynset.c b/net/netfilter/nft_dynset.c
> index e3b83c3..6a631cb 100644
> --- a/net/netfilter/nft_dynset.c
> +++ b/net/netfilter/nft_dynset.c
> @@ -139,6 +139,9 @@ static int nft_dynset_init(const struct nft_ctx *ctx,
>                         return PTR_ERR(set);
>         }
>
> +       if (set->ops->update == NULL)
> +               return -EOPNOTSUPP;
> +

Maybe it's better to treat the NFT_SET_EVAL as features, I will send V2 latter:

the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Liping Zhang Oct. 26, 2016, 1:14 p.m. UTC | #1 
2016-10-25 22:25 GMT+08:00 Liping Zhang <zlpnobody@gmail.com>:
> Maybe it's better to treat the NFT_SET_EVAL as features, I will send V2 latter:
>
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index b70d3ea..8a39b2a 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -2350,7 +2350,8 @@ nft_select_set_ops(const struct nlattr * const nla[],
>         features = 0;
>         if (nla[NFTA_SET_FLAGS] != NULL) {
>                 features = ntohl(nla_get_be32(nla[NFTA_SET_FLAGS]));
> -               features &= NFT_SET_INTERVAL | NFT_SET_MAP | NFT_SET_TIMEOUT;
> +               features &= NFT_SET_INTERVAL | NFT_SET_MAP | NFT_SET_TIMEOUT |
> +                           NFT_SET_EVAL;
>         }
>
>         bops       = NULL;
> diff --git a/net/netfilter/nft_set_hash.c b/net/netfilter/nft_set_hash.c
> index 3794cb2..328d23c 100644
> --- a/net/netfilter/nft_set_hash.c
> +++ b/net/netfilter/nft_set_hash.c
> @@ -382,7 +382,7 @@ static struct nft_set_ops nft_hash_ops __read_mostly = {
>         .lookup         = nft_hash_lookup,
>         .update         = nft_hash_update,
>         .walk           = nft_hash_walk,
> -       .features       = NFT_SET_MAP | NFT_SET_TIMEOUT,
> +       .features       = NFT_SET_MAP | NFT_SET_TIMEOUT | NFT_SET_EVAL,
>         .owner          = THIS_MODULE,
>  };

Sorry for this noise, the original patch should be fine. :(

After I have a careful look at the implementation of the dynset expr,
it's not appropriate to treat the NFT_SET_EVAL as the features.
The NFTA_DYNSET_EXPR attr is optional, and when it is not
specified, we will report -EINVAL if (set->flags & NFT_SET_EVAL)
is true:

static int nft_dynset_init()
{
    ...
    if (tb[NFTA_DYNSET_EXPR] != NULL) {
        if (!(set->flags & NFT_SET_EVAL))
            return -EINVAL;
        ...
    } else if (set->flags & NFT_SET_EVAL)
        return -EINVAL;

So for dynset, NFT_SET_EVAL is not a must option, but set->ops->update is.
--
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
diff mbox

Patch

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index b70d3ea..8a39b2a 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2350,7 +2350,8 @@  nft_select_set_ops(const struct nlattr * const nla[],
        features = 0;
        if (nla[NFTA_SET_FLAGS] != NULL) {
                features = ntohl(nla_get_be32(nla[NFTA_SET_FLAGS]));
-               features &= NFT_SET_INTERVAL | NFT_SET_MAP | NFT_SET_TIMEOUT;
+               features &= NFT_SET_INTERVAL | NFT_SET_MAP | NFT_SET_TIMEOUT |
+                           NFT_SET_EVAL;
        }

        bops       = NULL;
diff --git a/net/netfilter/nft_set_hash.c b/net/netfilter/nft_set_hash.c
index 3794cb2..328d23c 100644
--- a/net/netfilter/nft_set_hash.c
+++ b/net/netfilter/nft_set_hash.c
@@ -382,7 +382,7 @@  static struct nft_set_ops nft_hash_ops __read_mostly = {
        .lookup         = nft_hash_lookup,
        .update         = nft_hash_update,
        .walk           = nft_hash_walk,
-       .features       = NFT_SET_MAP | NFT_SET_TIMEOUT,
+       .features       = NFT_SET_MAP | NFT_SET_TIMEOUT | NFT_SET_EVAL,
        .owner          = THIS_MODULE,
 };
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in