On 2023-03-16, at 10:23:34 +0100, Florian Westphal wrote: > Jeremy Sowden <jeremy@xxxxxxxxxx> 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@xxxxxxxxxx> > > --- > > 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 = ®s->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:
signature.asc
Description: PGP signature
