netfilter: nf_tables: avoid skb access on nf_stolen

When verdict is NF_STOLEN, the skb might have been freed.

When tracing is enabled, this can result in a use-after-free:
1. access to skb->nf_trace
2. access to skb->mark
3. computation of trace id
4. dump of packet payload

To avoid 1, keep a cached copy of skb->nf_trace in the
trace state struct.
Refresh this copy whenever verdict is != STOLEN.

Avoid 2 by skipping skb->mark access if verdict is STOLEN.

3 is avoided by precomputing the trace id.

Only dump the packet when verdict is not "STOLEN".

Reported-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 279ae0f..5c4e5a9 100644 (file)
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1338,24 +1338,28 @@ void nft_unregister_flowtable_type(struct nf_flowtable_type *type);
 /**
  *     struct nft_traceinfo - nft tracing information and state
  *
+ *     @trace: other struct members are initialised
+ *     @nf_trace: copy of skb->nf_trace before rule evaluation
+ *     @type: event type (enum nft_trace_types)
+ *     @skbid: hash of skb to be used as trace id
+ *     @packet_dumped: packet headers sent in a previous traceinfo message
  *     @pkt: pktinfo currently processed
  *     @basechain: base chain currently processed
  *     @chain: chain currently processed
  *     @rule:  rule that was evaluated
  *     @verdict: verdict given by rule
- *     @type: event type (enum nft_trace_types)
- *     @packet_dumped: packet headers sent in a previous traceinfo message
- *     @trace: other struct members are initialised
  */
 struct nft_traceinfo {
+       bool                            trace;
+       bool                            nf_trace;
+       bool                            packet_dumped;
+       enum nft_trace_types            type:8;
+       u32                             skbid;
        const struct nft_pktinfo        *pkt;
        const struct nft_base_chain     *basechain;
        const struct nft_chain          *chain;
        const struct nft_rule_dp        *rule;
        const struct nft_verdict        *verdict;
-       enum nft_trace_types            type;
-       bool                            packet_dumped;
-       bool                            trace;
 };
 
 void nft_trace_init(struct nft_traceinfo *info, const struct nft_pktinfo *pkt,

diff --git a/net/netfilter/nf_tables_core.c b/net/netfilter/nf_tables_core.c
index 53f40e4..3ddce24 100644 (file)
--- a/net/netfilter/nf_tables_core.c
+++ b/net/netfilter/nf_tables_core.c
@@ -25,9 +25,7 @@ static noinline void __nft_trace_packet(struct nft_traceinfo *info,
                                        const struct nft_chain *chain,
                                        enum nft_trace_types type)
 {
-       const struct nft_pktinfo *pkt = info->pkt;
-
-       if (!info->trace || !pkt->skb->nf_trace)
+       if (!info->trace || !info->nf_trace)
                return;
 
        info->chain = chain;
@@ -42,11 +40,24 @@ static inline void nft_trace_packet(struct nft_traceinfo *info,
                                    enum nft_trace_types type)
 {
        if (static_branch_unlikely(&nft_trace_enabled)) {
+               const struct nft_pktinfo *pkt = info->pkt;
+
+               info->nf_trace = pkt->skb->nf_trace;
                info->rule = rule;
                __nft_trace_packet(info, chain, type);
        }
 }
 
+static inline void nft_trace_copy_nftrace(struct nft_traceinfo *info)
+{
+       if (static_branch_unlikely(&nft_trace_enabled)) {
+               const struct nft_pktinfo *pkt = info->pkt;
+
+               if (info->trace)
+                       info->nf_trace = pkt->skb->nf_trace;
+       }
+}
+
 static void nft_bitwise_fast_eval(const struct nft_expr *expr,
                                  struct nft_regs *regs)
 {
@@ -85,6 +96,7 @@ static noinline void __nft_trace_verdict(struct nft_traceinfo *info,
                                         const struct nft_chain *chain,
                                         const struct nft_regs *regs)
 {
+       const struct nft_pktinfo *pkt = info->pkt;
        enum nft_trace_types type;
 
        switch (regs->verdict.code) {
@@ -92,8 +104,13 @@ static noinline void __nft_trace_verdict(struct nft_traceinfo *info,
        case NFT_RETURN:
                type = NFT_TRACETYPE_RETURN;
                break;
+       case NF_STOLEN:
+               type = NFT_TRACETYPE_RULE;
+               /* can't access skb->nf_trace; use copy */
+               break;
        default:
                type = NFT_TRACETYPE_RULE;
+               info->nf_trace = pkt->skb->nf_trace;
                break;
        }
 
@@ -254,6 +271,7 @@ next_rule:
                switch (regs.verdict.code) {
                case NFT_BREAK:
                        regs.verdict.code = NFT_CONTINUE;
+                       nft_trace_copy_nftrace(&info);
                        continue;
                case NFT_CONTINUE:
                        nft_trace_packet(&info, chain, rule,

diff --git a/net/netfilter/nf_tables_trace.c b/net/netfilter/nf_tables_trace.c
index 5041725..1163ba9 100644 (file)
--- a/net/netfilter/nf_tables_trace.c
+++ b/net/netfilter/nf_tables_trace.c
@@ -7,7 +7,7 @@
 #include <linux/module.h>
 #include <linux/static_key.h>
 #include <linux/hash.h>
-#include <linux/jhash.h>
+#include <linux/siphash.h>
 #include <linux/if_vlan.h>
 #include <linux/init.h>
 #include <linux/skbuff.h>
@@ -25,22 +25,6 @@
 DEFINE_STATIC_KEY_FALSE(nft_trace_enabled);
 EXPORT_SYMBOL_GPL(nft_trace_enabled);
 
-static int trace_fill_id(struct sk_buff *nlskb, struct sk_buff *skb)
-{
-       __be32 id;
-
-       /* using skb address as ID results in a limited number of
-        * values (and quick reuse).
-        *
-        * So we attempt to use as many skb members that will not
-        * change while skb is with netfilter.
-        */
-       id = (__be32)jhash_2words(hash32_ptr(skb), skb_get_hash(skb),
-                                 skb->skb_iif);
-
-       return nla_put_be32(nlskb, NFTA_TRACE_ID, id);
-}
-
 static int trace_fill_header(struct sk_buff *nlskb, u16 type,
                             const struct sk_buff *skb,
                             int off, unsigned int len)
@@ -186,6 +170,7 @@ void nft_trace_notify(struct nft_traceinfo *info)
        struct nlmsghdr *nlh;
        struct sk_buff *skb;
        unsigned int size;
+       u32 mark = 0;
        u16 event;
 
        if (!nfnetlink_has_listeners(nft_net(pkt), NFNLGRP_NFTRACE))
@@ -229,7 +214,7 @@ void nft_trace_notify(struct nft_traceinfo *info)
        if (nla_put_be32(skb, NFTA_TRACE_TYPE, htonl(info->type)))
                goto nla_put_failure;
 
-       if (trace_fill_id(skb, pkt->skb))
+       if (nla_put_u32(skb, NFTA_TRACE_ID, info->skbid))
                goto nla_put_failure;
 
        if (nla_put_string(skb, NFTA_TRACE_CHAIN, info->chain->name))
@@ -249,16 +234,24 @@ void nft_trace_notify(struct nft_traceinfo *info)
        case NFT_TRACETYPE_RULE:
                if (nft_verdict_dump(skb, NFTA_TRACE_VERDICT, info->verdict))
                        goto nla_put_failure;
+
+               /* pkt->skb undefined iff NF_STOLEN, disable dump */
+               if (info->verdict->code == NF_STOLEN)
+                       info->packet_dumped = true;
+               else
+                       mark = pkt->skb->mark;
+
                break;
        case NFT_TRACETYPE_POLICY:
+               mark = pkt->skb->mark;
+
                if (nla_put_be32(skb, NFTA_TRACE_POLICY,
                                 htonl(info->basechain->policy)))
                        goto nla_put_failure;
                break;
        }
 
-       if (pkt->skb->mark &&
-           nla_put_be32(skb, NFTA_TRACE_MARK, htonl(pkt->skb->mark)))
+       if (mark && nla_put_be32(skb, NFTA_TRACE_MARK, htonl(mark)))
                goto nla_put_failure;
 
        if (!info->packet_dumped) {
@@ -283,9 +276,20 @@ void nft_trace_init(struct nft_traceinfo *info, const struct nft_pktinfo *pkt,
                    const struct nft_verdict *verdict,
                    const struct nft_chain *chain)
 {
+       static siphash_key_t trace_key __read_mostly;
+       struct sk_buff *skb = pkt->skb;
+
        info->basechain = nft_base_chain(chain);
        info->trace = true;
+       info->nf_trace = pkt->skb->nf_trace;
        info->packet_dumped = false;
        info->pkt = pkt;
        info->verdict = verdict;
+
+       net_get_random_once(&trace_key, sizeof(trace_key));
+
+       info->skbid = (u32)siphash_3u32(hash32_ptr(skb),
+                                       skb_get_hash(skb),
+                                       skb->skb_iif,
+                                       &trace_key);
 }