| Message ID | 20240828044933.676898-1-lilingfeng3@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | nfs: protect nfs41_impl_id by rcu | expand |
Hi Li,
kernel test robot noticed the following build warnings:
[auto build test WARNING on trondmy-nfs/linux-next]
[also build test WARNING on linus/master v6.11-rc5 next-20240828]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Li-Lingfeng/nfs-protect-nfs41_impl_id-by-rcu/20240828-124056
base: git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
patch link: https://lore.kernel.org/r/20240828044933.676898-1-lilingfeng3%40huawei.com
patch subject: [PATCH] nfs: protect nfs41_impl_id by rcu
config: x86_64-randconfig-121-20240829 (https://download.01.org/0day-ci/archive/20240829/202408290616.QG17h6tl-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240829/202408290616.QG17h6tl-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408290616.QG17h6tl-lkp@intel.com/
sparse warnings: (new ones prefixed by >>)
>> fs/nfs/nfs4client.c:296:18: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected void const *objp @@ got struct nfs41_impl_id [noderef] __rcu *cl_implid @@
fs/nfs/nfs4client.c:296:18: sparse: expected void const *objp
fs/nfs/nfs4client.c:296:18: sparse: got struct nfs41_impl_id [noderef] __rcu *cl_implid
vim +296 fs/nfs/nfs4client.c
ec409897e7c715 Bryan Schumaker 2012-07-16 283
ec409897e7c715 Bryan Schumaker 2012-07-16 284 static void nfs4_shutdown_client(struct nfs_client *clp)
ec409897e7c715 Bryan Schumaker 2012-07-16 285 {
ec409897e7c715 Bryan Schumaker 2012-07-16 286 if (__test_and_clear_bit(NFS_CS_RENEWD, &clp->cl_res_state))
ec409897e7c715 Bryan Schumaker 2012-07-16 287 nfs4_kill_renewd(clp);
abf79bb341bf52 Chuck Lever 2013-08-09 288 clp->cl_mvops->shutdown_client(clp);
ec409897e7c715 Bryan Schumaker 2012-07-16 289 nfs4_destroy_callback(clp);
ec409897e7c715 Bryan Schumaker 2012-07-16 290 if (__test_and_clear_bit(NFS_CS_IDMAP, &clp->cl_res_state))
ec409897e7c715 Bryan Schumaker 2012-07-16 291 nfs_idmap_delete(clp);
ec409897e7c715 Bryan Schumaker 2012-07-16 292
ec409897e7c715 Bryan Schumaker 2012-07-16 293 rpc_destroy_wait_queue(&clp->cl_rpcwaitq);
ec409897e7c715 Bryan Schumaker 2012-07-16 294 kfree(clp->cl_serverowner);
ec409897e7c715 Bryan Schumaker 2012-07-16 295 kfree(clp->cl_serverscope);
ec409897e7c715 Bryan Schumaker 2012-07-16 @296 kfree(clp->cl_implid);
ceb3a16c070c40 Trond Myklebust 2015-01-03 297 kfree(clp->cl_owner_id);
ec409897e7c715 Bryan Schumaker 2012-07-16 298 }
ec409897e7c715 Bryan Schumaker 2012-07-16 299
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index b8ffbe52ba15..6bb820bd205e 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -8866,13 +8866,21 @@ struct nfs41_exchange_id_data { struct nfs41_exchange_id_args args; }; +static void nfs4_free_impl_id_rcu(struct rcu_head *head) +{ + struct nfs41_impl_id *impl_id = container_of(head, struct nfs41_impl_id, __rcu_head); + + kfree(impl_id); +} + static void nfs4_exchange_id_release(void *data) { struct nfs41_exchange_id_data *cdata = (struct nfs41_exchange_id_data *)data; nfs_put_client(cdata->args.client); - kfree(cdata->res.impl_id); + if (cdata->res.impl_id) + call_rcu(&cdata->res.impl_id->__rcu_head, nfs4_free_impl_id_rcu); kfree(cdata->res.server_scope); kfree(cdata->res.server_owner); kfree(cdata); @@ -9034,7 +9042,7 @@ static int _nfs4_proc_exchange_id(struct nfs_client *clp, const struct cred *cre swap(clp->cl_serverowner, resp->server_owner); swap(clp->cl_serverscope, resp->server_scope); - swap(clp->cl_implid, resp->impl_id); + resp->impl_id = rcu_replace_pointer(clp->cl_implid, resp->impl_id, 1); /* Save the EXCHANGE_ID verifier session trunk tests */ memcpy(clp->cl_confirm.data, argp->verifier.data, diff --git a/fs/nfs/super.c b/fs/nfs/super.c index 97b386032b71..6097dbe8e334 100644 --- a/fs/nfs/super.c +++ b/fs/nfs/super.c @@ -612,13 +612,19 @@ static void show_pnfs(struct seq_file *m, struct nfs_server *server) static void show_implementation_id(struct seq_file *m, struct nfs_server *nfss) { - if (nfss->nfs_client && nfss->nfs_client->cl_implid) { - struct nfs41_impl_id *impl_id = nfss->nfs_client->cl_implid; + struct nfs_client *clp = nfss->nfs_client; + struct nfs41_impl_id *impl_id; + + if (!clp) + return; + rcu_read_lock(); + impl_id = rcu_dereference(clp->cl_implid); + if (impl_id) seq_printf(m, "\n\timpl_id:\tname='%s',domain='%s'," "date='%llu,%u'", impl_id->name, impl_id->domain, impl_id->date.seconds, impl_id->date.nseconds); - } + rcu_read_unlock(); } #else #if IS_ENABLED(CONFIG_NFS_V4) diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h index 1df86ab98c77..29c98c9df42f 100644 --- a/include/linux/nfs_fs_sb.h +++ b/include/linux/nfs_fs_sb.h @@ -102,7 +102,7 @@ struct nfs_client { bool cl_preserve_clid; struct nfs41_server_owner *cl_serverowner; struct nfs41_server_scope *cl_serverscope; - struct nfs41_impl_id *cl_implid; + struct nfs41_impl_id __rcu *cl_implid; /* nfs 4.1+ state protection modes: */ unsigned long cl_sp4_flags; #define NFS_SP4_MACH_CRED_MINIMAL 1 /* Minimal sp4_mach_cred - state ops diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h index 45623af3e7b8..b3c96ea2a64b 100644 --- a/include/linux/nfs_xdr.h +++ b/include/linux/nfs_xdr.h @@ -1374,6 +1374,7 @@ struct nfs41_impl_id { char domain[NFS4_OPAQUE_LIMIT + 1]; char name[NFS4_OPAQUE_LIMIT + 1]; struct nfstime4 date; + struct rcu_head __rcu_head; }; #define MAX_BIND_CONN_TO_SESSION_RETRIES 3
When performing exchange id call, a new nfs41_impl_id will be allocated to store some information from server. The pointers to the old and new nfs41_impl_ids are swapped, and the old one will be freed. However, UAF may be triggered as follows: After T2 has got a pointer to the nfs41_impl_id, the nfs41_impl_id is freed by T1 before it is used. T1 T2 nfs4_proc_exchange_id _nfs4_proc_exchange_id nfs4_run_exchange_id kzalloc // alloc nfs41_impl_id-B rpc_run_task nfs_show_stats show_implementation_id impl_id = nfss->nfs_client->cl_implid // get alloc nfs41_impl_id-A swap(clp->cl_implid, resp->impl_id) rpc_put_task ... nfs4_exchange_id_release kfree // free nfs41_impl_id-A impl_id->name // UAF Fix this issue by using rcu to protect the nfs41_impl_id. Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com> --- fs/nfs/nfs4proc.c | 12 ++++++++++-- fs/nfs/super.c | 12 +++++++++--- include/linux/nfs_fs_sb.h | 2 +- include/linux/nfs_xdr.h | 1 + 4 files changed, 21 insertions(+), 6 deletions(-)