netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH nf-next] netfilter: nft_exthdr: add boolean DCCP option matching
@ 2023-03-12 14:37 Jeremy Sowden
  2023-03-12 15:17 ` Florian Westphal
  2023-03-16  9:23 ` Florian Westphal
  0 siblings, 2 replies; 7+ messages in thread
From: Jeremy Sowden @ 2023-03-12 14:37 UTC (permalink / raw)
  To: Netfilter Devel

The xt_dccp iptables module supports the matching of DCCP packets based
on the presence or absence of DCCP options.  Extend nft_exthdr to add
this functionality to nftables.

Link: https://bugzilla.netfilter.org/show_bug.cgi?id=930
Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 include/uapi/linux/netfilter/nf_tables.h |   2 +
 net/netfilter/nft_exthdr.c               | 105 +++++++++++++++++++++++
 2 files changed, 107 insertions(+)

diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index 9c6f02c26054..1406952e7139 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -859,12 +859,14 @@ enum nft_exthdr_flags {
  * @NFT_EXTHDR_OP_TCP: match against tcp options
  * @NFT_EXTHDR_OP_IPV4: match against ipv4 options
  * @NFT_EXTHDR_OP_SCTP: match against sctp chunks
+ * @NFT_EXTHDR_OP_DCCP: match against dccp otions
  */
 enum nft_exthdr_op {
 	NFT_EXTHDR_OP_IPV6,
 	NFT_EXTHDR_OP_TCPOPT,
 	NFT_EXTHDR_OP_IPV4,
 	NFT_EXTHDR_OP_SCTP,
+	NFT_EXTHDR_OP_DCCP,
 	__NFT_EXTHDR_OP_MAX
 };
 #define NFT_EXTHDR_OP_MAX	(__NFT_EXTHDR_OP_MAX - 1)
diff --git a/net/netfilter/nft_exthdr.c b/net/netfilter/nft_exthdr.c
index a54a7f772cec..204feefbb7ea 100644
--- a/net/netfilter/nft_exthdr.c
+++ b/net/netfilter/nft_exthdr.c
@@ -10,6 +10,7 @@
 #include <linux/netlink.h>
 #include <linux/netfilter.h>
 #include <linux/netfilter/nf_tables.h>
+#include <linux/dccp.h>
 #include <linux/sctp.h>
 #include <net/netfilter/nf_tables_core.h>
 #include <net/netfilter/nf_tables.h>
@@ -25,6 +26,17 @@ struct nft_exthdr {
 	u8			flags;
 };
 
+struct nft_exthdr_dccp {
+	struct nft_exthdr exthdr;
+	/* A buffer into which to copy the DCCP packet options for parsing.  The
+	 * options are located between the packet header and its data.  The
+	 * offset of the data from the start of the header is stored in an 8-bit
+	 * field as the number of 32-bit words, so the options will definitely
+	 * be shorter than `4 * U8_MAX` bytes.
+	 */
+	u8 optbuf[4 * U8_MAX];
+};
+
 static unsigned int optlen(const u8 *opt, unsigned int offset)
 {
 	/* Beware zero-length options: make finite progress */
@@ -406,6 +418,70 @@ static void nft_exthdr_sctp_eval(const struct nft_expr *expr,
 		regs->verdict.code = NFT_BREAK;
 }
 
+static void nft_exthdr_dccp_eval(const struct nft_expr *expr,
+				 struct nft_regs *regs,
+				 const struct nft_pktinfo *pkt)
+{
+	struct nft_exthdr_dccp *priv_dccp = nft_expr_priv(expr);
+	struct nft_exthdr *priv = &priv_dccp->exthdr;
+	u32 *dest = &regs->data[priv->dreg];
+	unsigned int optoff, optlen, i;
+	const struct dccp_hdr *dh;
+	struct dccp_hdr _dh;
+	const u8 *options;
+
+	if (pkt->tprot != IPPROTO_DCCP || pkt->fragoff)
+		goto err;
+
+	dh = skb_header_pointer(pkt->skb, nft_thoff(pkt), sizeof(_dh), &_dh);
+	if (!dh)
+		goto err;
+
+	if (dh->dccph_doff * 4 < __dccp_hdr_len(dh))
+		goto err;
+
+	optoff = __dccp_hdr_len(dh);
+	optlen = dh->dccph_doff * 4 - optoff;
+
+	if (!optlen)
+		goto err;
+
+	options = skb_header_pointer(pkt->skb, nft_thoff(pkt) + optoff, optlen,
+				     priv_dccp->optbuf);
+	if (!options)
+		goto err;
+
+	for (i = 0; i < optlen; ) {
+		/* Options 0 - 31 are 1B in the length.  Options 32 et seq. are
+		 * at least 2B long.  In all cases, the first byte contains the
+		 * option type.  In multi-byte options, the second byte contains
+		 * the option length, which must be at least two; if it is
+		 * greater than two, there are `len - 2` following bytes of
+		 * option data.
+		 */
+		unsigned int len;
+
+		if (options[i] > 31 && (optlen - i < 2 || options[i + 1] < 2))
+			goto err;
+
+		len = options[i] > 31 ? options[i + 1] : 1;
+
+		if (optlen - i < len)
+			goto err;
+
+		if (options[i] != priv->type) {
+			i += len;
+			continue;
+		}
+
+		*dest = 1;
+		return;
+	}
+
+err:
+	*dest = 0;
+}
+
 static const struct nla_policy nft_exthdr_policy[NFTA_EXTHDR_MAX + 1] = {
 	[NFTA_EXTHDR_DREG]		= { .type = NLA_U32 },
 	[NFTA_EXTHDR_TYPE]		= { .type = NLA_U8 },
@@ -557,6 +633,22 @@ static int nft_exthdr_ipv4_init(const struct nft_ctx *ctx,
 	return 0;
 }
 
+static int nft_exthdr_dccp_init(const struct nft_ctx *ctx,
+				const struct nft_expr *expr,
+				const struct nlattr * const tb[])
+{
+	struct nft_exthdr *priv = nft_expr_priv(expr);
+	int err = nft_exthdr_init(ctx, expr, tb);
+
+	if (err < 0)
+		return err;
+
+	if (!(priv->flags & NFT_EXTHDR_F_PRESENT))
+		return -EOPNOTSUPP;
+
+	return 0;
+}
+
 static int nft_exthdr_dump_common(struct sk_buff *skb, const struct nft_exthdr *priv)
 {
 	if (nla_put_u8(skb, NFTA_EXTHDR_TYPE, priv->type))
@@ -686,6 +778,15 @@ static const struct nft_expr_ops nft_exthdr_sctp_ops = {
 	.reduce		= nft_exthdr_reduce,
 };
 
+static const struct nft_expr_ops nft_exthdr_dccp_ops = {
+	.type		= &nft_exthdr_type,
+	.size		= NFT_EXPR_SIZE(sizeof(struct nft_exthdr_dccp)),
+	.eval		= nft_exthdr_dccp_eval,
+	.init		= nft_exthdr_dccp_init,
+	.dump		= nft_exthdr_dump,
+	.reduce		= nft_exthdr_reduce,
+};
+
 static const struct nft_expr_ops *
 nft_exthdr_select_ops(const struct nft_ctx *ctx,
 		      const struct nlattr * const tb[])
@@ -720,6 +821,10 @@ nft_exthdr_select_ops(const struct nft_ctx *ctx,
 		if (tb[NFTA_EXTHDR_DREG])
 			return &nft_exthdr_sctp_ops;
 		break;
+	case NFT_EXTHDR_OP_DCCP:
+		if (tb[NFTA_EXTHDR_DREG])
+			return &nft_exthdr_dccp_ops;
+		break;
 	}
 
 	return ERR_PTR(-EOPNOTSUPP);
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH nf-next] netfilter: nft_exthdr: add boolean DCCP option matching
  2023-03-12 14:37 [PATCH nf-next] netfilter: nft_exthdr: add boolean DCCP option matching Jeremy Sowden
@ 2023-03-12 15:17 ` Florian Westphal
  2023-03-12 17:51   ` Jeremy Sowden
  2023-03-16  9:23 ` Florian Westphal
  1 sibling, 1 reply; 7+ messages in thread
From: Florian Westphal @ 2023-03-12 15:17 UTC (permalink / raw)
  To: Jeremy Sowden; +Cc: Netfilter Devel

Jeremy Sowden <jeremy@azazel.net> wrote:
> The xt_dccp iptables module supports the matching of DCCP packets based
> on the presence or absence of DCCP options.  Extend nft_exthdr to add
> this functionality to nftables.

Is DCCP even used in reality?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH nf-next] netfilter: nft_exthdr: add boolean DCCP option matching
  2023-03-12 15:17 ` Florian Westphal
@ 2023-03-12 17:51   ` Jeremy Sowden
  0 siblings, 0 replies; 7+ messages in thread
From: Jeremy Sowden @ 2023-03-12 17:51 UTC (permalink / raw)
  To: Florian Westphal; +Cc: Netfilter Devel

[-- Attachment #1: Type: text/plain, Size: 411 bytes --]

On 2023-03-12, at 16:17:31 +0100, Florian Westphal wrote:
> Jeremy Sowden <jeremy@azazel.net> wrote:
> > The xt_dccp iptables module supports the matching of DCCP packets based
> > on the presence or absence of DCCP options.  Extend nft_exthdr to add
> > this functionality to nftables.
> 
> Is DCCP even used in reality?

Now that you mentioned it, it doesn't seem to be very widely adopted. :)

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH nf-next] netfilter: nft_exthdr: add boolean DCCP option matching
  2023-03-12 14:37 [PATCH nf-next] netfilter: nft_exthdr: add boolean DCCP option matching Jeremy Sowden
  2023-03-12 15:17 ` Florian Westphal
@ 2023-03-16  9:23 ` Florian Westphal
  2023-03-16 18:56   ` Jeremy Sowden
  2023-03-17 22:31   ` Jeremy Sowden
  1 sibling, 2 replies; 7+ messages in thread
From: Florian Westphal @ 2023-03-16  9:23 UTC (permalink / raw)
  To: Jeremy Sowden; +Cc: Netfilter Devel

Jeremy Sowden <jeremy@azazel.net> wrote:
> The xt_dccp iptables module supports the matching of DCCP packets based
> on the presence or absence of DCCP options.  Extend nft_exthdr to add
> this functionality to nftables.
> 
> Link: https://bugzilla.netfilter.org/show_bug.cgi?id=930
> Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
> ---
>  include/uapi/linux/netfilter/nf_tables.h |   2 +
>  net/netfilter/nft_exthdr.c               | 105 +++++++++++++++++++++++
> +struct nft_exthdr_dccp {
> +	struct nft_exthdr exthdr;
> +	/* A buffer into which to copy the DCCP packet options for parsing.  The
> +	 * options are located between the packet header and its data.  The
> +	 * offset of the data from the start of the header is stored in an 8-bit
> +	 * field as the number of 32-bit words, so the options will definitely
> +	 * be shorter than `4 * U8_MAX` bytes.
> +	 */
> +	u8 optbuf[4 * U8_MAX];
> +};
> +
>  static unsigned int optlen(const u8 *opt, unsigned int offset)
>  {
>  	/* Beware zero-length options: make finite progress */
> @@ -406,6 +418,70 @@ static void nft_exthdr_sctp_eval(const struct nft_expr *expr,
>  		regs->verdict.code = NFT_BREAK;
>  }
>  
> +static void nft_exthdr_dccp_eval(const struct nft_expr *expr,
> +				 struct nft_regs *regs,
> +				 const struct nft_pktinfo *pkt)
> +{
> +	struct nft_exthdr_dccp *priv_dccp = nft_expr_priv(expr);
> +	struct nft_exthdr *priv = &priv_dccp->exthdr;
> +	u32 *dest = &regs->data[priv->dreg];
> +	unsigned int optoff, optlen, i;
> +	const struct dccp_hdr *dh;
> +	struct dccp_hdr _dh;
> +	const u8 *options;
> +
> +	if (pkt->tprot != IPPROTO_DCCP || pkt->fragoff)
> +		goto err;
> +
> +	dh = skb_header_pointer(pkt->skb, nft_thoff(pkt), sizeof(_dh), &_dh);
> +	if (!dh)
> +		goto err;
> +
> +	if (dh->dccph_doff * 4 < __dccp_hdr_len(dh))
> +		goto err;
> +
> +	optoff = __dccp_hdr_len(dh);
> +	optlen = dh->dccph_doff * 4 - optoff;

Perhaps reorder this slightly:

     optoff = __dccp_hdr_len(dh);
     if (dh->dccph_doff * 4 <= optoff)
	     goto err;

     optlen = dh->dccph_doff * 4 - optoff;

     options = skb_header_pointer(pkt->skb, nft_thoff(pkt) + optoff, optlen,
				     priv_dccp->optbuf);

This isn't safe.  priv_dccp->optbuf is neither percpu nor is there
something that prevents a softinterrupt from firing.

I suggest you have a look at 'pipapo' set type which uses percpu scratch
maps.

Yet another alternative is to provide a small onstack scratch buffer,
say 256 byte, and fall back to kmalloc for larger spaces.

Or, always use a on-stack buffer that gets re-used for each of the
parsed options.

> +	for (i = 0; i < optlen; ) {
> +		/* Options 0 - 31 are 1B in the length.  Options 32 et seq. are
> +		 * at least 2B long.  In all cases, the first byte contains the
> +		 * option type.  In multi-byte options, the second byte contains
> +		 * the option length, which must be at least two; if it is
> +		 * greater than two, there are `len - 2` following bytes of
> +		 * option data.
> +		 */
> +		unsigned int len;
> +
> +		if (options[i] > 31 && (optlen - i < 2 || options[i + 1] < 2))
> +			goto err;
> +
> +		len = options[i] > 31 ? options[i + 1] : 1;
> +
> +		if (optlen - i < len)
> +			goto err;
> +
> +		if (options[i] != priv->type) {
> +			i += len;

I think this needs to guard against len == 0?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH nf-next] netfilter: nft_exthdr: add boolean DCCP option matching
  2023-03-16  9:23 ` Florian Westphal
@ 2023-03-16 18:56   ` Jeremy Sowden
  2023-03-17 22:31   ` Jeremy Sowden
  1 sibling, 0 replies; 7+ messages in thread
From: Jeremy Sowden @ 2023-03-16 18:56 UTC (permalink / raw)
  To: Florian Westphal; +Cc: Netfilter Devel

[-- Attachment #1: Type: text/plain, Size: 4305 bytes --]

On 2023-03-16, at 10:23:34 +0100, Florian Westphal wrote:
> Jeremy Sowden <jeremy@azazel.net> wrote:
> > The xt_dccp iptables module supports the matching of DCCP packets based
> > on the presence or absence of DCCP options.  Extend nft_exthdr to add
> > this functionality to nftables.
> > 
> > Link: https://bugzilla.netfilter.org/show_bug.cgi?id=930
> > Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
> > ---
> >  include/uapi/linux/netfilter/nf_tables.h |   2 +
> >  net/netfilter/nft_exthdr.c               | 105 +++++++++++++++++++++++
> > +struct nft_exthdr_dccp {
> > +	struct nft_exthdr exthdr;
> > +	/* A buffer into which to copy the DCCP packet options for parsing.  The
> > +	 * options are located between the packet header and its data.  The
> > +	 * offset of the data from the start of the header is stored in an 8-bit
> > +	 * field as the number of 32-bit words, so the options will definitely
> > +	 * be shorter than `4 * U8_MAX` bytes.
> > +	 */
> > +	u8 optbuf[4 * U8_MAX];
> > +};
> > +
> >  static unsigned int optlen(const u8 *opt, unsigned int offset)
> >  {
> >  	/* Beware zero-length options: make finite progress */
> > @@ -406,6 +418,70 @@ static void nft_exthdr_sctp_eval(const struct nft_expr *expr,
> >  		regs->verdict.code = NFT_BREAK;
> >  }
> >  
> > +static void nft_exthdr_dccp_eval(const struct nft_expr *expr,
> > +				 struct nft_regs *regs,
> > +				 const struct nft_pktinfo *pkt)
> > +{
> > +	struct nft_exthdr_dccp *priv_dccp = nft_expr_priv(expr);
> > +	struct nft_exthdr *priv = &priv_dccp->exthdr;
> > +	u32 *dest = &regs->data[priv->dreg];
> > +	unsigned int optoff, optlen, i;
> > +	const struct dccp_hdr *dh;
> > +	struct dccp_hdr _dh;
> > +	const u8 *options;
> > +
> > +	if (pkt->tprot != IPPROTO_DCCP || pkt->fragoff)
> > +		goto err;
> > +
> > +	dh = skb_header_pointer(pkt->skb, nft_thoff(pkt), sizeof(_dh), &_dh);
> > +	if (!dh)
> > +		goto err;
> > +
> > +	if (dh->dccph_doff * 4 < __dccp_hdr_len(dh))
> > +		goto err;
> > +
> > +	optoff = __dccp_hdr_len(dh);
> > +	optlen = dh->dccph_doff * 4 - optoff;
> 
> Perhaps reorder this slightly:
> 
>      optoff = __dccp_hdr_len(dh);
>      if (dh->dccph_doff * 4 <= optoff)
> 	     goto err;
> 
>      optlen = dh->dccph_doff * 4 - optoff;
> 
>      options = skb_header_pointer(pkt->skb, nft_thoff(pkt) + optoff, optlen,
> 				     priv_dccp->optbuf);
> 
> This isn't safe.  priv_dccp->optbuf is neither percpu nor is there
> something that prevents a softinterrupt from firing.
> 
> I suggest you have a look at 'pipapo' set type which uses percpu scratch
> maps.
> 
> Yet another alternative is to provide a small onstack scratch buffer,
> say 256 byte, and fall back to kmalloc for larger spaces.
> 
> Or, always use a on-stack buffer that gets re-used for each of the
> parsed options.

I was trying to avoid an on-stack buffer, 'cause it would be quite big,
while improving concurrency over xt_dccp.c, which has a single file-
scope buffer protected by a spin-lock.  I'll take a look at pipapo.
Thanks for the tips.

> > +	for (i = 0; i < optlen; ) {
> > +		/* Options 0 - 31 are 1B in the length.  Options 32 et seq. are
> > +		 * at least 2B long.  In all cases, the first byte contains the
> > +		 * option type.  In multi-byte options, the second byte contains
> > +		 * the option length, which must be at least two; if it is
> > +		 * greater than two, there are `len - 2` following bytes of
> > +		 * option data.
> > +		 */
> > +		unsigned int len;
> > +
> > +		if (options[i] > 31 && (optlen - i < 2 || options[i + 1] < 2))
> > +			goto err;
> > +
> > +		len = options[i] > 31 ? options[i + 1] : 1;
> > +
> > +		if (optlen - i < len)
> > +			goto err;
> > +
> > +		if (options[i] != priv->type) {
> > +			i += len;
> 
> I think this needs to guard against len == 0?

`len` should be at least 1:

> > +		if (options[i] > 31 && (optlen - i < 2 || options[i + 1] < 2))
> > +			goto err;
> > +

If options[i] > 31, we verify that we can get the length from
options[i + 1] and that it is at least 2.

> > +		len = options[i] > 31 ? options[i + 1] : 1;

If options[i] > 31, we assign options[i + 1], which we know is at
least two, to len; otheriwse, we assign 1.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH nf-next] netfilter: nft_exthdr: add boolean DCCP option matching
  2023-03-16  9:23 ` Florian Westphal
  2023-03-16 18:56   ` Jeremy Sowden
@ 2023-03-17 22:31   ` Jeremy Sowden
  2023-03-26 21:01     ` Pablo Neira Ayuso
  1 sibling, 1 reply; 7+ messages in thread
From: Jeremy Sowden @ 2023-03-17 22:31 UTC (permalink / raw)
  To: Florian Westphal; +Cc: Netfilter Devel

[-- Attachment #1: Type: text/plain, Size: 3609 bytes --]

On 2023-03-16, at 10:23:34 +0100, Florian Westphal wrote:
> Jeremy Sowden <jeremy@azazel.net> wrote:
> > The xt_dccp iptables module supports the matching of DCCP packets based
> > on the presence or absence of DCCP options.  Extend nft_exthdr to add
> > this functionality to nftables.
> > 
> > Link: https://bugzilla.netfilter.org/show_bug.cgi?id=930
> > Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
> > ---
> >  include/uapi/linux/netfilter/nf_tables.h |   2 +
> >  net/netfilter/nft_exthdr.c               | 105 +++++++++++++++++++++++
> > +struct nft_exthdr_dccp {
> > +	struct nft_exthdr exthdr;
> > +	/* A buffer into which to copy the DCCP packet options for parsing.  The
> > +	 * options are located between the packet header and its data.  The
> > +	 * offset of the data from the start of the header is stored in an 8-bit
> > +	 * field as the number of 32-bit words, so the options will definitely
> > +	 * be shorter than `4 * U8_MAX` bytes.
> > +	 */
> > +	u8 optbuf[4 * U8_MAX];
> > +};
> > +
> >  static unsigned int optlen(const u8 *opt, unsigned int offset)
> >  {
> >  	/* Beware zero-length options: make finite progress */
> > @@ -406,6 +418,70 @@ static void nft_exthdr_sctp_eval(const struct nft_expr *expr,
> >  		regs->verdict.code = NFT_BREAK;
> >  }
> >  
> > +static void nft_exthdr_dccp_eval(const struct nft_expr *expr,
> > +				 struct nft_regs *regs,
> > +				 const struct nft_pktinfo *pkt)
> > +{
> > +	struct nft_exthdr_dccp *priv_dccp = nft_expr_priv(expr);
> > +	struct nft_exthdr *priv = &priv_dccp->exthdr;
> > +	u32 *dest = &regs->data[priv->dreg];
> > +	unsigned int optoff, optlen, i;
> > +	const struct dccp_hdr *dh;
> > +	struct dccp_hdr _dh;
> > +	const u8 *options;
> > +
> > +	if (pkt->tprot != IPPROTO_DCCP || pkt->fragoff)
> > +		goto err;
> > +
> > +	dh = skb_header_pointer(pkt->skb, nft_thoff(pkt), sizeof(_dh), &_dh);
> > +	if (!dh)
> > +		goto err;
> > +
> > +	if (dh->dccph_doff * 4 < __dccp_hdr_len(dh))
> > +		goto err;
> > +
> > +	optoff = __dccp_hdr_len(dh);
> > +	optlen = dh->dccph_doff * 4 - optoff;
> 
> Perhaps reorder this slightly:
> 
>      optoff = __dccp_hdr_len(dh);
>      if (dh->dccph_doff * 4 <= optoff)
> 	     goto err;
> 
>      optlen = dh->dccph_doff * 4 - optoff;
> 
>      options = skb_header_pointer(pkt->skb, nft_thoff(pkt) + optoff, optlen,
> 				     priv_dccp->optbuf);
> 
> This isn't safe.  priv_dccp->optbuf is neither percpu nor is there
> something that prevents a softinterrupt from firing.
> 
> I suggest you have a look at 'pipapo' set type which uses percpu scratch
> maps.
> 
> Yet another alternative is to provide a small onstack scratch buffer,
> say 256 byte, and fall back to kmalloc for larger spaces.
> 
> Or, always use a on-stack buffer that gets re-used for each of the
> parsed options.

On giving it some more thought, it occurred to me that there would be no
need to read the option data, only the type and length, so one could do
something like this:

	optoff = __dccp_hdr_len(dh);
	if (dh->dccph_doff * 4 <= optoff)
		goto err;

	optlen = dh->dccph_doff * 4 - optoff;

	for (i = 0; i < optlen; ) {
		u8 buf[2], *ptr, type, len;

		ptr = skb_header_pointer(pkt->skb, thoff + optoff + i,
					 optlen - i > 1 ? 2 : 1, &buf);
		if (!ptr)
			goto err;

		type = ptr[0];

		if (type <= 31)
			len = 1;
		else {
			if (optlen - i < 2)
				goto err;

			len = ptr[1];

			if (len < 2)
				goto err;
		}

		if (type == priv->type) {
			*dest = 1;
			return;
		}

		i += len;
	}

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH nf-next] netfilter: nft_exthdr: add boolean DCCP option matching
  2023-03-17 22:31   ` Jeremy Sowden
@ 2023-03-26 21:01     ` Pablo Neira Ayuso
  0 siblings, 0 replies; 7+ messages in thread
From: Pablo Neira Ayuso @ 2023-03-26 21:01 UTC (permalink / raw)
  To: Jeremy Sowden; +Cc: Florian Westphal, Netfilter Devel

On Fri, Mar 17, 2023 at 10:31:43PM +0000, Jeremy Sowden wrote:
> On 2023-03-16, at 10:23:34 +0100, Florian Westphal wrote:
> > Jeremy Sowden <jeremy@azazel.net> wrote:
> > > The xt_dccp iptables module supports the matching of DCCP packets based
> > > on the presence or absence of DCCP options.  Extend nft_exthdr to add
> > > this functionality to nftables.
> > > 
> > > Link: https://bugzilla.netfilter.org/show_bug.cgi?id=930
> > > Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
> > > ---
> > >  include/uapi/linux/netfilter/nf_tables.h |   2 +
> > >  net/netfilter/nft_exthdr.c               | 105 +++++++++++++++++++++++
> > > +struct nft_exthdr_dccp {
> > > +	struct nft_exthdr exthdr;
> > > +	/* A buffer into which to copy the DCCP packet options for parsing.  The
> > > +	 * options are located between the packet header and its data.  The
> > > +	 * offset of the data from the start of the header is stored in an 8-bit
> > > +	 * field as the number of 32-bit words, so the options will definitely
> > > +	 * be shorter than `4 * U8_MAX` bytes.
> > > +	 */
> > > +	u8 optbuf[4 * U8_MAX];
> > > +};
> > > +
> > >  static unsigned int optlen(const u8 *opt, unsigned int offset)
> > >  {
> > >  	/* Beware zero-length options: make finite progress */
> > > @@ -406,6 +418,70 @@ static void nft_exthdr_sctp_eval(const struct nft_expr *expr,
> > >  		regs->verdict.code = NFT_BREAK;
> > >  }
> > >  
> > > +static void nft_exthdr_dccp_eval(const struct nft_expr *expr,
> > > +				 struct nft_regs *regs,
> > > +				 const struct nft_pktinfo *pkt)
> > > +{
> > > +	struct nft_exthdr_dccp *priv_dccp = nft_expr_priv(expr);
> > > +	struct nft_exthdr *priv = &priv_dccp->exthdr;
> > > +	u32 *dest = &regs->data[priv->dreg];
> > > +	unsigned int optoff, optlen, i;
> > > +	const struct dccp_hdr *dh;
> > > +	struct dccp_hdr _dh;
> > > +	const u8 *options;
> > > +
> > > +	if (pkt->tprot != IPPROTO_DCCP || pkt->fragoff)
> > > +		goto err;
> > > +
> > > +	dh = skb_header_pointer(pkt->skb, nft_thoff(pkt), sizeof(_dh), &_dh);
> > > +	if (!dh)
> > > +		goto err;
> > > +
> > > +	if (dh->dccph_doff * 4 < __dccp_hdr_len(dh))
> > > +		goto err;
> > > +
> > > +	optoff = __dccp_hdr_len(dh);
> > > +	optlen = dh->dccph_doff * 4 - optoff;
> > 
> > Perhaps reorder this slightly:
> > 
> >      optoff = __dccp_hdr_len(dh);
> >      if (dh->dccph_doff * 4 <= optoff)
> > 	     goto err;
> > 
> >      optlen = dh->dccph_doff * 4 - optoff;
> > 
> >      options = skb_header_pointer(pkt->skb, nft_thoff(pkt) + optoff, optlen,
> > 				     priv_dccp->optbuf);
> > 
> > This isn't safe.  priv_dccp->optbuf is neither percpu nor is there
> > something that prevents a softinterrupt from firing.
> > 
> > I suggest you have a look at 'pipapo' set type which uses percpu scratch
> > maps.
> > 
> > Yet another alternative is to provide a small onstack scratch buffer,
> > say 256 byte, and fall back to kmalloc for larger spaces.
> > 
> > Or, always use a on-stack buffer that gets re-used for each of the
> > parsed options.
> 
> On giving it some more thought, it occurred to me that there would be no
> need to read the option data, only the type and length, so one could do
> something like this:
> 
> 	optoff = __dccp_hdr_len(dh);
> 	if (dh->dccph_doff * 4 <= optoff)
> 		goto err;
> 
> 	optlen = dh->dccph_doff * 4 - optoff;
> 
> 	for (i = 0; i < optlen; ) {
> 		u8 buf[2], *ptr, type, len;
> 
> 		ptr = skb_header_pointer(pkt->skb, thoff + optoff + i,
> 					 optlen - i > 1 ? 2 : 1, &buf);
> 		if (!ptr)
> 			goto err;
> 
> 		type = ptr[0];
> 
> 		if (type <= 31)
> 			len = 1;
> 		else {
> 			if (optlen - i < 2)
> 				goto err;
> 
> 			len = ptr[1];
> 
> 			if (len < 2)
> 				goto err;
> 		}
> 
> 		if (type == priv->type) {
> 			*dest = 1;
> 			return;
> 		}
> 
> 		i += len;
> 	}

I believe this should be fine.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-03-26 21:01 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-12 14:37 [PATCH nf-next] netfilter: nft_exthdr: add boolean DCCP option matching Jeremy Sowden
2023-03-12 15:17 ` Florian Westphal
2023-03-12 17:51   ` Jeremy Sowden
2023-03-16  9:23 ` Florian Westphal
2023-03-16 18:56   ` Jeremy Sowden
2023-03-17 22:31   ` Jeremy Sowden
2023-03-26 21:01     ` Pablo Neira Ayuso

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).