Commit bb061d3d authored by Florian Westphal's avatar Florian Westphal Committed by Pablo Neira Ayuso
Browse files

netfilter: nft_byteorder: remove multi-register support 



64bit byteorder conversion is broken when several registers need to be
converted because the source register array advances in steps for 4 bytes
instead of 8:

  for (i = ...
      src64 = nft_reg_load64(&src[i]);
                             ~~~~~ u32 *src
      nft_reg_store64(&dst64[i],

Remove the multi-register support, it has other issues as well:

Pablo points out that commit
caf3ef74 ("netfilter: nf_tables: prevent OOB access in nft_byteorder_eval")
alters semantics: before the loop operated on registers, i.e.
 for ( ... )
   dst32[i] = htons((u16)src32[i])

 .. but after the patch it will operate on bytes, which makes this
 useless to convert e.g. concatenations, which store each compound
 in its own register.

Multi-convert of u32 has one theoretical application:

ct mark . meta mark . tcp dport @intervalset

Because ct mark and meta mark are host byte order, use with
intervals has to convert the byteorder for ct/meta mark value
to network byte order (bigendian).

nftables emits this:
 [ meta load mark => reg 1 ]
 [ byteorder reg 1 = hton(reg 1, 4, 4) ]
 [ ct load mark => reg 9 ]
 [ byteorder reg 9 = hton(reg 9, 4, 4) ]
 ...

I.e. two separate calls.  Theoretically it could be changed to do:
 [ meta load mark => reg 1 ]
 [ ct load mark => reg 9 ]
 [ byteorder reg 1 = htonl(reg 1, 4, 8) ]
 ...

But then all it would take to change the set to
meta mark . tcp dport . ct mark

... and we'd be back to two "byteorder" calls. IOW, support to
convert a range of registers is both dysfunctional and dubious.

Simplify this: remove the feature.

Pablo Neira Ayuso points out that nftables before 1.1.0 can generate
incorrect byteorder conversions, see 9fe58952c45a,
"evaluate: skip byteorder conversion for selector smaller than 2 bytes"
in nftables.git).  Affected rulesets fail to load with this change and
old userspace due to 'len != size' check.

Fixes: c301f098 ("netfilter: nf_tables: fix pointer math issue in nft_byteorder_eval()")
Cc: <stable+noautosel@kernel.org> # may break rule load with old nftables versions
Reported-by: default avatarMichal Kubecek <mkubecek@suse.cz>
Link: https://lore.kernel.org/netfilter-devel/20240206104336.ctigqpkunom2ufmn@lion.mk-sys.cz/


Signed-off-by: default avatarFlorian Westphal <fw@strlen.de>
Signed-off-by: default avatarPablo Neira Ayuso <pablo@netfilter.org>
parent 67ba971a

net/netfilter/nft_byteorder.c

+20 −31
Original line number Diff line number Diff line
@@ -19,7 +19,6 @@ struct nft_byteorder { 
	u8			sreg;

	u8			dreg;

	enum nft_byteorder_ops	op:8;

	u8			len;

	u8			size;

};
@@ -28,13 +27,8 @@ void nft_byteorder_eval(const struct nft_expr *expr, 
			const struct nft_pktinfo *pkt)

{

	const struct nft_byteorder *priv = nft_expr_priv(expr);

	u32 *src = &regs->data[priv->sreg];

	const u32 *src = &regs->data[priv->sreg];

	u32 *dst = &regs->data[priv->dreg];

	u16 *s16, *d16;

	unsigned int i;



	s16 = (void *)src;

	d16 = (void *)dst;



	switch (priv->size) {

	case 8: {
@@ -43,18 +37,14 @@ void nft_byteorder_eval(const struct nft_expr *expr, 


		switch (priv->op) {

		case NFT_BYTEORDER_NTOH:

			for (i = 0; i < priv->len / 8; i++) {

				src64 = nft_reg_load64(&src[i]);

				nft_reg_store64(&dst64[i],

						be64_to_cpu((__force __be64)src64));

			}

			src64 = nft_reg_load64(src);



			nft_reg_store64(dst64, be64_to_cpu((__force __be64)src64));

			break;

		case NFT_BYTEORDER_HTON:

			for (i = 0; i < priv->len / 8; i++) {

				src64 = (__force __u64)

					cpu_to_be64(nft_reg_load64(&src[i]));

				nft_reg_store64(&dst64[i], src64);

			}

			src64 = (__force __u64)cpu_to_be64(nft_reg_load64(src));



			nft_reg_store64(dst64, src64);

			break;

		}

		break;
@@ -62,24 +52,20 @@ void nft_byteorder_eval(const struct nft_expr *expr, 
	case 4:

		switch (priv->op) {

		case NFT_BYTEORDER_NTOH:

			for (i = 0; i < priv->len / 4; i++)

				dst[i] = ntohl((__force __be32)src[i]);

			*dst = ntohl((__force __be32)*src);

			break;

		case NFT_BYTEORDER_HTON:

			for (i = 0; i < priv->len / 4; i++)

				dst[i] = (__force __u32)htonl(src[i]);

			*dst = (__force __u32)htonl(*src);

			break;

		}

		break;

	case 2:

		switch (priv->op) {

		case NFT_BYTEORDER_NTOH:

			for (i = 0; i < priv->len / 2; i++)

				d16[i] = ntohs((__force __be16)s16[i]);

			nft_reg_store16(dst, ntohs(nft_reg_load_be16(src)));

			break;

		case NFT_BYTEORDER_HTON:

			for (i = 0; i < priv->len / 2; i++)

				d16[i] = (__force __u16)htons(s16[i]);

			nft_reg_store_be16(dst, htons(nft_reg_load16(src)));

			break;

		}

		break;
@@ -137,20 +123,22 @@ static int nft_byteorder_init(const struct nft_ctx *ctx, 
	if (err < 0)

		return err;



	priv->len = len;

	/* no longer support multi-reg conversions */

	if (len != size)

		return -EOPNOTSUPP;



	err = nft_parse_register_load(ctx, tb[NFTA_BYTEORDER_SREG], &priv->sreg,

				      priv->len);

				      len);

	if (err < 0)

		return err;



	err = nft_parse_register_store(ctx, tb[NFTA_BYTEORDER_DREG],

				       &priv->dreg, NULL, NFT_DATA_VALUE,

				       priv->len);

				       len);

	if (err < 0)

		return err;



	if (nft_reg_overlap(priv->sreg, priv->dreg, priv->len))

	if (nft_reg_overlap(priv->sreg, priv->dreg, len))

		return -EINVAL;



	return 0;
@@ -167,10 +155,11 @@ static int nft_byteorder_dump(struct sk_buff *skb, 
		goto nla_put_failure;

	if (nla_put_be32(skb, NFTA_BYTEORDER_OP, htonl(priv->op)))

		goto nla_put_failure;

	if (nla_put_be32(skb, NFTA_BYTEORDER_LEN, htonl(priv->len)))

		goto nla_put_failure;

	if (nla_put_be32(skb, NFTA_BYTEORDER_SIZE, htonl(priv->size)))

		goto nla_put_failure;

	/* compatibility for old userspace which permitted size != len */

	if (nla_put_be32(skb, NFTA_BYTEORDER_LEN, htonl(priv->size)))

		goto nla_put_failure;

	return 0;



nla_put_failure: