Re: [PATCH net] netfilter: xt_dccp: reject short option lengths

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch turns option lengths 0 and 1 into a hotdrop instead
  of a no-match, and the commit message does not say so.
- [Low] The commit body is an email cover letter pasted into the
  permanent changelog.
- [Low] Whether xt_dccp drops a malformed option TLV now depends on which
  malformed form it is and on option order.

--

netfilter: xt_dccp: reject short option lengths

This patch changes dccp_find_option() in net/netfilter/xt_dccp.c so that
a multi-byte DCCP option with a length byte of 0 or 1 is treated as
invalid. The walker previously advanced by one byte and could read the
length byte as the next option type. With this patch it sets hotdrop and
stops.

Link: https://lists.openwall.net/netdev/2026/10/20/477

> Hello,

[Severity: Low]
This isn't a bug, but the changelog reads like an email cover letter that
was pasted in as the commit body. It opens with "Hello,".

It also includes reproduction notes that would usually go below the ---
line: QEMU and gcc versions, gist URLs, tested SHAs, and build and boot
steps.

The technical explanation ends mid-sentence:

> The option walker should reject a TLV length below two before advancing:
>
> Fixes: 1d3de414eb20 ("[NETFILTER]: New iptables DCCP protocol header match")

Nothing follows the colon. The message also doesn't say what the code now
does with a length of 0 or 1, which is to hotdrop the packet.

> diff --git a/net/netfilter/xt_dccp.c b/net/netfilter/xt_dccp.c
> index 23a4e3e8df7de..e0d7f43749588 100644
> --- a/net/netfilter/xt_dccp.c
> +++ b/net/netfilter/xt_dccp.c
> @@ -62,10 +62,15 @@ dccp_find_option(u_int8_t option,
>  			return true;
>  		}
>  
> -		if (op[i] <= DCCPO_MAX_RESERVED || i == optlen - 1)
> +		if (op[i] <= DCCPO_MAX_RESERVED || i == optlen - 1) {
>  			i++;
> -		else
> -			i += op[i + 1] ? : 1;
> +		} else {
> +			if (op[i + 1] < 2) {
> +				spin_unlock_bh(&dccp_buflock);
> +				goto invalid;
> +			}

[Severity: Medium]
Is the change from a no-match to a hotdrop intended here?

The goto invalid path sets *hotdrop = true. dccp_mt() passes &par->hotdrop
down through match_option(), and ipt_do_table() then stops rule traversal
and drops the packet, whatever the target or chain policy:

net/ipv4/netfilter/ip_tables.c:ipt_do_table() {
    ...
	} while (!acpar.hotdrop);
    ...
	if (acpar.hotdrop)
		return NF_DROP;
    ...
}

Before this patch, a packet with options {32, 1} just failed the match and
kept going. With this patch it looks like the packet is also dropped in
setups such as these:

- an ACCEPT policy chain with a non-terminal --dccp-option N rule, for
  example LOG or MARK, or a rule in the mangle or raw table

- an inverted rule such as "! --dccp-option 5 -j ACCEPT", which used to
  match {32, 1} or {32, 0} because option 5 is absent

The commit message only describes the DROP policy allow-list case. In that
case hotdrop and no-match give the same result.

nft_exthdr_dccp_eval() handles the same malformed TLV as "option not
present" and does not drop the packet:

net/netfilter/nft_exthdr.c:nft_exthdr_dccp_eval() {
    ...
		if (len < 2)
			goto err;
    ...
err:
	*dest = 0;
}

xt_dccp already hotdrops when doff is shorter than the header, so hotdrop
may be the intended policy. Could the changelog say why hotdrop was chosen
over returning false? Could it also note that the iptables and nftables
DCCP option matches now give different results for these packets?

> +			i += op[i + 1];
> +		}
>  	}

[Severity: Low]
With the new hotdrop, dccp_find_option() now handles malformed TLVs in
different ways. The hotdrop only fires for a length of 0 or 1, and only if
the walker reaches that option before it finds the requested type.

Other malformed forms still end as a plain no-match:

- a length larger than the remaining option area, where
  i += op[i + 1] runs past optlen and the function returns false

- a multi-byte type (above DCCPO_MAX_RESERVED) in the last byte, which
  the i == optlen - 1 branch skips as if it were a single-byte option

Also, the op[i] == option check returns true before any length is checked.
So {32, 1} still matches --dccp-option 32, and {1, 32, 1} matches
--dccp-option 1 without the bad trailing TLV being examined.

Take an inverted rule like "! --dccp-option 5 -j ACCEPT". It would accept
{32, 0xff} and a trailing {..., 32}, but hotdrop {32, 1}, even though all
three are malformed. nft_exthdr_dccp_eval() treats every one of these as
"not present".

None of these cases brings back the false match this patch fixes. Once
lengths are at least 2, the walk stays on TLV boundaries.

Should these malformed forms all be handled the same way?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261105081735.749379-1-shawnq1%40163.com