Re: [PATCH nf,v4] netfilter: ctnetlink: ignore explicit helper on new expectations

[Fernando Fernandez Mancera <fmancera@suse.de>]

On 3/30/26 4:36 PM, Pablo Neira Ayuso wrote:
> Use the existing master conntrack helper, anything else is not really
> supported and it just makes validation more complicated, so just ignore
> what helper userspace suggests for this expectation.
> 
> This was uncovered when validating CTA_EXPECT_CLASS via different helper
> provided by userspace than the existing master conntrack helper:
> 
>    BUG: KASAN: slab-out-of-bounds in nf_ct_expect_related_report+0x2479/0x27c0
>    Read of size 4 at addr ffff8880043fe408 by task poc/102
>    Call Trace:
>     nf_ct_expect_related_report+0x2479/0x27c0
>     ctnetlink_create_expect+0x22b/0x3b0
>     ctnetlink_new_expect+0x4bd/0x5c0
>     nfnetlink_rcv_msg+0x67a/0x950
>     netlink_rcv_skb+0x120/0x350
> 
> Allowing to read kernel memory bytes off the expectation boundary.
> 
> CTA_EXPECT_HELP_NAME is still used to offer the helper name to userspace
> via netlink dump.
> 
> Fixes: bd0779370588 ("netfilter: nfnetlink_queue: allow to attach expectations to conntracks")
> Reported-by: Qi Tang <tpluszz77@gmail.com>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> v4: actually... remove this entire refetch
> 
> @@ -3576,8 +3569,6 @@ ctnetlink_alloc_expect(const struct nlattr * const cda[], struct nf_conn *ct,
>   #ifdef CONFIG_NF_CONNTRACK_ZONES
>          exp->zone = ct->zone;
>   #endif
> -       if (!helper)
> -               helper = rcu_dereference(help->helper);
>          rcu_assign_pointer(exp->helper, helper);
>          exp->tuple = *tuple;
>          exp->mask.src.u3 = mask->src.u3;
> 

Just a note, I spend some time trying to apply the patch due to this. 
Drop it before running git am if you are experiencing the same problem.

> 
>   net/netfilter/nf_conntrack_netlink.c | 54 +++++-----------------------
>   1 file changed, 9 insertions(+), 45 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index 35f859b24103..ec6771a0926c 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -2636,7 +2636,6 @@ static const struct nla_policy exp_nla_policy[CTA_EXPECT_MAX+1] = {
>   
>   static struct nf_conntrack_expect *
>   ctnetlink_alloc_expect(const struct nlattr *const cda[], struct nf_conn *ct,
> -		       struct nf_conntrack_helper *helper,
>   		       struct nf_conntrack_tuple *tuple,
>   		       struct nf_conntrack_tuple *mask);
>   
> @@ -2865,7 +2864,6 @@ ctnetlink_glue_attach_expect(const struct nlattr *attr, struct nf_conn *ct,
>   {
>   	struct nlattr *cda[CTA_EXPECT_MAX+1];
>   	struct nf_conntrack_tuple tuple, mask;
> -	struct nf_conntrack_helper *helper = NULL;
>   	struct nf_conntrack_expect *exp;
>   	int err;
>   
> @@ -2879,17 +2877,8 @@ ctnetlink_glue_attach_expect(const struct nlattr *attr, struct nf_conn *ct,
>   	if (err < 0)
>   		return err;
>   
> -	if (cda[CTA_EXPECT_HELP_NAME]) {
> -		const char *helpname = nla_data(cda[CTA_EXPECT_HELP_NAME]);
> -
> -		helper = __nf_conntrack_helper_find(helpname, nf_ct_l3num(ct),
> -						    nf_ct_protonum(ct));
> -		if (helper == NULL)
> -			return -EOPNOTSUPP;
> -	}
> -

I wonder if we should return -EOPNOTSUPP here and be explicit about it.

I know the rule is "do not break userspace" but as you mentioned on the 
commit message, this was not really supported. Better just explicitly 
fail so if by any chance someone expects this to work, they will notice.

Other than that, LGTM.