netfilter: nf_tables: fix dst corruption in same register operation

For lshift and rshift, the shift operations are performed in a loop over
32-bit words. The loop calculates the shifted value and write it to dst,
and then immediately reads from src to calculate the carry for the next
iteration. Because src and dst could point to the same memory location,
the carry is incorrectly calculated using the newly modified dst value
instead of the original src value.

Adding a temporary local variable to cache the original value before
writing to dst and using it for the carry calculation solves the
problem. In addition, partial overlap is rejected from control plane for
all kind of operations including byteorder. This was tested with the
following bytecode:

table test_table ip flags 0 use 1 handle 1
ip test_table test_chain use 3 type filter hook input prio 0 policy accept packets 0 bytes 0 flags 1
ip test_table test_chain 2
  [ immediate reg 1 0x44332211 0x88776655 ]
  [ bitwise reg 1 = ( reg 1 << 0x08000000 ) ]
  [ cmp eq reg 1 0x66443322 0x00887766 ]
  [ counter pkts 0 bytes 0 ]
ip test_table test_chain 4 3
  [ immediate reg 1 0x44332211 0x88776655 ]
  [ bitwise reg 1 = ( reg 1 << 0x08000000 ) ]
  [ cmp eq reg 1 0x55443322 0x00887766 ]
  [ counter pkts 21794 bytes 1917798 ]

Fixes: 567d746b55bc ("netfilter: bitwise: add support for shifts.")
Acked-by: Jeremy Sowden <jeremy@azazel.net>
Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
Signed-off-by: Florian Westphal <fw@strlen.de>

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index cff7b773e9721d5941c821939fd3efee941447b1..9d844354c4d9568e0041f10ea39f8754b0d79441 100644 (file)
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -180,6 +180,13 @@ static inline u64 nft_reg_load64(const u32 *sreg)
        return get_unaligned((u64 *)sreg);
 }
 
+static inline bool nft_reg_overlap(u8 src, u8 dst, u32 len)
+{
+       unsigned int n = DIV_ROUND_UP(len, sizeof(u32));
+
+       return src != dst && src < dst + n && dst < src + n;
+}
+
 static inline void nft_data_copy(u32 *dst, const struct nft_data *src,
                                 unsigned int len)
 {

diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
index 94dccdcfa06bba6e439dd5e3240d5300380d247e..785b8e9731d197d1e21834d873993b6c7578bc05 100644 (file)
--- a/net/netfilter/nft_bitwise.c
+++ b/net/netfilter/nft_bitwise.c
@@ -43,8 +43,10 @@ static void nft_bitwise_eval_lshift(u32 *dst, const u32 *src,
        u32 carry = 0;
 
        for (i = DIV_ROUND_UP(priv->len, sizeof(u32)); i > 0; i--) {
-               dst[i - 1] = (src[i - 1] << shift) | carry;
-               carry = src[i - 1] >> (BITS_PER_TYPE(u32) - shift);
+               u32 tmp_src = src[i - 1];
+
+               dst[i - 1] = (tmp_src << shift) | carry;
+               carry = tmp_src >> (BITS_PER_TYPE(u32) - shift);
        }
 }
 
@@ -56,8 +58,10 @@ static void nft_bitwise_eval_rshift(u32 *dst, const u32 *src,
        u32 carry = 0;
 
        for (i = 0; i < DIV_ROUND_UP(priv->len, sizeof(u32)); i++) {
-               dst[i] = carry | (src[i] >> shift);
-               carry = src[i] << (BITS_PER_TYPE(u32) - shift);
+               u32 tmp_src = src[i];
+
+               dst[i] = carry | (tmp_src >> shift);
+               carry = tmp_src << (BITS_PER_TYPE(u32) - shift);
        }
 }
 
@@ -235,6 +239,9 @@ static int nft_bitwise_init_bool(const struct nft_ctx *ctx,
                                              &priv->sreg2, priv->len);
                if (err < 0)
                        return err;
+
+               if (nft_reg_overlap(priv->sreg2, priv->dreg, priv->len))
+                       return -EINVAL;
        }
 
        return 0;
@@ -265,6 +272,9 @@ static int nft_bitwise_init(const struct nft_ctx *ctx,
        if (err < 0)
                return err;
 
+       if (nft_reg_overlap(priv->sreg, priv->dreg, priv->len))
+               return -EINVAL;
+
        if (tb[NFTA_BITWISE_OP]) {
                priv->op = ntohl(nla_get_be32(tb[NFTA_BITWISE_OP]));
                switch (priv->op) {

diff --git a/net/netfilter/nft_byteorder.c b/net/netfilter/nft_byteorder.c
index e00dddfa2fc0abd9ac782d945592ebe9b517e8f1..2316c77f422803063fd0dfb0bb1b83c0b8798805 100644 (file)
--- a/net/netfilter/nft_byteorder.c
+++ b/net/netfilter/nft_byteorder.c
@@ -144,9 +144,16 @@ static int nft_byteorder_init(const struct nft_ctx *ctx,
        if (err < 0)
                return err;
 
-       return nft_parse_register_store(ctx, tb[NFTA_BYTEORDER_DREG],
-                                       &priv->dreg, NULL, NFT_DATA_VALUE,
-                                       priv->len);
+       err = nft_parse_register_store(ctx, tb[NFTA_BYTEORDER_DREG],
+                                      &priv->dreg, NULL, NFT_DATA_VALUE,
+                                      priv->len);
+       if (err < 0)
+               return err;
+
+       if (nft_reg_overlap(priv->sreg, priv->dreg, priv->len))
+               return -EINVAL;
+
+       return 0;
 }
 
 static int nft_byteorder_dump(struct sk_buff *skb,