Date:   Tue, 21 Jan 2020 14:32:36 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     syzbot <syzbot+29125d208b3dae9a7019@...kaller.appspotmail.com>,
        pablo@...filter.org
Cc:     coreteam@...filter.org, davem@...emloft.net, fw@...len.de,
        kadlec@...filter.org, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org, netfilter-devel@...r.kernel.org,
        syzkaller-bugs@...glegroups.com
Subject: Re: KASAN: use-after-free Read in __nf_tables_abort

I think I see the problem, but I'm not sure how you want to fix it...

net/netfilter/nf_tables_api.c
   942  static int nf_tables_newtable(struct net *net, struct sock *nlsk,
   943                                struct sk_buff *skb, const struct nlmsghdr *nlh,
   944                                const struct nlattr * const nla[],
   945                                struct netlink_ext_ack *extack)
   946  {
   947          const struct nfgenmsg *nfmsg = nlmsg_data(nlh);
   948          u8 genmask = nft_genmask_next(net);
   949          int family = nfmsg->nfgen_family;
   950          const struct nlattr *attr;
   951          struct nft_table *table;
   952          u32 flags = 0;
   953          struct nft_ctx ctx;
   954          int err;
   955  
   956          lockdep_assert_held(&net->nft.commit_mutex);
   957          attr = nla[NFTA_TABLE_NAME];
   958          table = nft_table_lookup(net, attr, family, genmask);
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This is looking up table in net->nft.tables

   959          if (IS_ERR(table)) {
   960                  if (PTR_ERR(table) != -ENOENT)
   961                          return PTR_ERR(table);
   962          } else {
   963                  if (nlh->nlmsg_flags & NLM_F_EXCL) {
   964                          NL_SET_BAD_ATTR(extack, attr);
   965                          return -EEXIST;
   966                  }
   967                  if (nlh->nlmsg_flags & NLM_F_REPLACE)
   968                          return -EOPNOTSUPP;
   969  
   970                  nft_ctx_init(&ctx, net, skb, nlh, family, table, NULL, nla);
   971                  return nf_tables_updtable(&ctx);
                               ^^^^^^^^^^^^^^^^^^^^^^^
Then it adds it to &ctx->net->nft.commit_list

   972          }
   973  
   974          if (nla[NFTA_TABLE_FLAGS]) {
   975                  flags = ntohl(nla_get_be32(nla[NFTA_TABLE_FLAGS]));
   976                  if (flags & ~NFT_TABLE_F_DORMANT)
   977                          return -EINVAL;
   978          }
   979  
   980          err = -ENOMEM;
   981          table = kzalloc(sizeof(*table), GFP_KERNEL);
   982          if (table == NULL)
   983                  goto err_kzalloc;
   984  
   985          table->name = nla_strdup(attr, GFP_KERNEL);
   986          if (table->name == NULL)
   987                  goto err_strdup;
   988  
   989          err = rhltable_init(&table->chains_ht, &nft_chain_ht_params);
   990          if (err)
   991                  goto err_chain_ht;
   992  
   993          INIT_LIST_HEAD(&table->chains);
   994          INIT_LIST_HEAD(&table->sets);
   995          INIT_LIST_HEAD(&table->objects);
   996          INIT_LIST_HEAD(&table->flowtables);
   997          table->family = family;
   998          table->flags = flags;
   999          table->handle = ++table_handle;
  1000  
  1001          nft_ctx_init(&ctx, net, skb, nlh, family, table, NULL, nla);
  1002          err = nft_trans_table_add(&ctx, NFT_MSG_NEWTABLE);
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Added to ctx->net->nft.commit_list

  1003          if (err < 0)
  1004                  goto err_trans;
  1005  
  1006          list_add_tail_rcu(&table->list, &net->nft.tables);
                                                ^^^^^^^^^^^^^^^^
Added to net->nft.tables

  1007          return 0;
  1008  err_trans:
  1009          rhltable_destroy(&table->chains_ht);
  1010  err_chain_ht:
  1011          kfree(table->name);
  1012  err_strdup:
  1013          kfree(table);

net/netfilter/nf_tables_api.c
  6995  static void nf_tables_commit_release(struct net *net)
  6996  {
  6997          struct nft_trans *trans;
  6998  
  6999          /* all side effects have to be made visible.
  7000           * For example, if a chain named 'foo' has been deleted, a
  7001           * new transaction must not find it anymore.
  7002           *
  7003           * Memory reclaim happens asynchronously from work queue
  7004           * to prevent expensive synchronize_rcu() in commit phase.
  7005           */
  7006          if (list_empty(&net->nft.commit_list)) {
  7007                  mutex_unlock(&net->nft.commit_mutex);
  7008                  return;
  7009          }
  7010  
  7011          trans = list_last_entry(&net->nft.commit_list,
  7012                                  struct nft_trans, list);
  7013          get_net(trans->ctx.net);
  7014          WARN_ON_ONCE(trans->put_net);
  7015  
  7016          trans->put_net = true;
  7017          spin_lock(&nf_tables_destroy_list_lock);
  7018          list_splice_tail_init(&net->nft.commit_list, &nf_tables_destroy_list);
                                       ^^^^^^^^^^^^^^^^^^^^^  ^^^^^^^^^^^^^^^^^^^^^^
This starts the process of freeing everything from net->nft.commit_list,
but we need to delete it from the net->nft.tables list as well.

  7019          spin_unlock(&nf_tables_destroy_list_lock);
  7020  
  7021          mutex_unlock(&net->nft.commit_mutex);
  7022  
  7023          schedule_work(&trans_destroy_work);
  7024  }

regards,
dan carpenter

Powered by blists - more mailing lists