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

[Weiming Shi <bestswngs@gmail.com>]

On 26-03-22 09:39, Florian Westphal wrote:
> 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!

Hi,

Thanks for the review! I'll prepare a v2 patch that adds the family
check in xt_check_match/xt_check_target.

Weiming Shi