Re: [PATCH nf] netfilter: xt_devgroup: reject unsupported families in checkentry

[Florian Westphal <fw@strlen.de>]

bestswngs@gmail.com <bestswngs@gmail.com> wrote:
> From: Weiming Shi <bestswngs@gmail.com>
> 
> devgroup_mt_checkentry() validates hook_mask using NF_INET_* constants,
> but the match is registered with NFPROTO_UNSPEC, which allows it to be
> used from any protocol family through nft_compat.
>
> On an ARP nftables output chain, nft_compat passes
> hook_mask = 1 << NF_ARP_OUT. Because NF_ARP_OUT == 1 == NF_INET_LOCAL_IN,
> the source-group hook validation incorrectly accepts the rule. At runtime
> arp_xmit() invokes the chain with state->in == NULL, and devgroup_mt()
> dereferences xt_in(par)->group, crashing the kernel:
> 
>  Oops: general protection fault, probably for non-canonical address 0xdffffc0000000044: 0000 [#1] SMP KASAN NOPTI
>  KASAN: null-ptr-deref in range [0x0000000000000220-0x0000000000000227]
>  RIP: 0010:devgroup_mt+0xff/0x350
>  Call Trace:
>   <TASK>
>   nft_match_eval (net/netfilter/nft_compat.c:407)
>   nft_do_chain (net/netfilter/nf_tables_core.c:285)
>   nft_do_chain_arp (net/netfilter/nft_chain_filter.c:61)
>   nf_hook_slow (net/netfilter/core.c:623)
>   arp_xmit (net/ipv4/arp.c:666)
>   arp_solicit (net/ipv4/arp.c:393)
>   neigh_probe (net/core/neighbour.c:1098)
>   __neigh_event_send (net/core/neighbour.c:1277)
>   neigh_resolve_output (net/core/neighbour.c:1604)
>   ip_finish_output2 (net/ipv4/ip_output.c:237)
>   </TASK>
>  Kernel panic - not syncing: Fatal exception in interrupt
> 
> Reject families whose hook numbering differs from the NF_INET_* scheme
> early in checkentry. NFPROTO_INET and NFPROTO_BRIDGE share the same
> five-hook layout (PRE_ROUTING ... POST_ROUTING) and the same
> state->in/state->out semantics as IPv4/IPv6, so they are safe.
> ARP only has three hooks (IN=0, OUT=1, FORWARD=2) with different
> semantics, causing the numbering collision that triggers this bug.

I think we should solve this in x_tables.c so we don't have to ensure
all the .checkentry functions provide for this.
While this patch solves the specific module at hand, it begs
the question if the same bug pattern exist exlsewhere.

xt_check_match and xt_check_target should call a common
helper, this helper checks that if the match/target is UNSPEC
and has .hooks != 0, then the calling family is IPV4, IPV6 or BRIDGE.

All others can be rejected.

Would you make such a patch?

Thanks!