Re: [PATCH] netfilter: nf_tables: fix use-after-free on ops->dev

[Florian Westphal <fw@strlen.de>]

Helen Koike <koike@igalia.com> wrote:

Phil, can you please take a look at this?

The reg/unregister logic is ... strange.

> But if I understood correctly from your comment below, the proper 
> solution would be to fix the order that the hooks are released, is my 
> understanding correct?

I don't think its about ordering.  I think the code allows to register
devices multiple times in the same flowtable, but UNREG doesn't handle
that.

static int nft_flowtable_event(unsigned long event, struct net_device *dev,
			       struct nft_flowtable *flowtable, bool changename)
{
	struct nf_hook_ops *ops;
	struct nft_hook *hook;
	bool match;

	list_for_each_entry(hook, &flowtable->hook_list, list) {
		ops = nft_hook_find_ops(hook, dev);
		match = !strncmp(hook->ifname, dev->name, hook->ifnamelen);

		switch (event) {
		case NETDEV_UNREGISTER:
			/* NOP if not found or new name still matching */
			if (!ops || (changename && match))
				continue;

			/* flow_offload_netdev_event() cleans up entries for us. */
			nft_unregister_flowtable_ops(dev_net(dev),
						     flowtable, ops);
			list_del_rcu(&ops->list);
			kfree_rcu(ops, rcu);
			break;
		case NETDEV_REGISTER:
			/* NOP if not matching or already registered */
			if (!match || (changename && ops))
				continue;

And *THIS* looks buggy.
Shouldn't that simply be:
			if (!match || ops)
				continue;

Or can you explain why changename has any relevance here?
changename means dev->name has already been updated.

So, we want to skip a new registration if either:
1. the name doesn't match
2. it matches but its already registered.

In case changename is true, only UNREGISTER: case is
relevant: If its not matching anymore -> unregister.

Still matching?  Keep it.  In that case, we havn't
registered the device again because 'ops' was non-null in
REGISTER case.

		}
		break;

If its allowed to register the same device twice (or more), then the
above 'break' needs to be removed, AND one has to alter UNREGISTER
above to loop until no more ops are found, i.e.
		case NETDEV_UNREGISTER:
			/* NOP if not found or new name still matching */
			if (!ops || (changename && match))
				continue;

			do {
				/* flow_offload_netdev_event() cleans up entries for us. */
				nft_unregister_flowtable_ops(dev_net(dev),
							     flowtable, ops);
				list_del_rcu(&ops->list);
				kfree_rcu(ops, rcu);
				ops = nft_hook_find_ops(hook, dev);
			} while (ops);
			break;

Phil, can you please have a look?  Thanks!