| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <eafac8bf-009c-402a-a9a7-d8881100f3ca@huawei.com>
Date: Wed, 14 May 2025 15:04:12 +0800
From: Li Lingfeng <lilingfeng3@...wei.com>
To: Jeff Layton <jlayton@...nel.org>, <chuck.lever@...cle.com>,
<neilb@...e.de>, <neil@...wn.name>, <okorniev@...hat.com>,
<Dai.Ngo@...cle.com>, <tom@...pey.com>
CC: <linux-nfs@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<yukuai1@...weicloud.com>, <houtao1@...wei.com>, <yi.zhang@...wei.com>,
<yangerkun@...wei.com>, <lilingfeng@...weicloud.com>
Subject: Re: [PATCH] nfsd: Invoke tracking callbacks only after initialization
is complete
Hi Jeff,
Thank you for the review.
Delaying the assignment of client_tracking_ops until after initialization
completes successfully would indeed avoid the race condition without
needing an additional flag.
My initial choice to add client_tracking_init_done was motivated by
minimizing changes to existing code paths. Since client_tracking_ops is
checked in multiple callback functions, altering its assignment logic
would require auditing all references. The flag allowed a localized fix
without restructuring the initialization sequence.
I'll try to rework the patch to use a temporary variable for the ops during
initialization and only assign it to nn->client_tracking_ops once all setup
steps are confirmed successful. This should ensure that no callbacks are
invoked prematurely.
Lingfeng.
在 2025/5/13 20:34, Jeff Layton 写道:
> On Tue, 2025-05-13 at 15:43 +0800, Li Lingfeng wrote:
>> Checking whether tracking callbacks can be called based on whether
>> nn->client_tracking_ops is NULL may lead to callbacks being invoked
>> before tracking initialization completes, causing resource access
>> violations (UAF, NULL pointer dereference). Examples:
>>
>> 1) nfsd4_client_tracking_init
>> // set nn->client_tracking_ops
>> nfsd4_cld_tracking_init
>> nfs4_cld_state_init
>> nn->reclaim_str_hashtbl = kmalloc_array
>> ... // error path, goto err
>> nfs4_cld_state_shutdown
>> kfree(nn->reclaim_str_hashtbl)
>> write_v4_end_grace
>> nfsd4_end_grace
>> nfsd4_record_grace_done
>> nfsd4_cld_grace_done
>> nfs4_release_reclaim
>> nn->reclaim_str_hashtbl[i]
>> // UAF
>> // clear nn->client_tracking_ops
>>
>> 2) nfsd4_client_tracking_init
>> // set nn->client_tracking_ops
>> nfsd4_cld_tracking_init
>> write_v4_end_grace
>> nfsd4_end_grace
>> nfsd4_record_grace_done
>> nfsd4_cld_grace_done
>> alloc_cld_upcall
>> cn = nn->cld_net
>> spin_lock // cn->cn_lock
>> // NULL deref
>> // error path, skip init pipe
>> __nfsd4_init_cld_pipe
>> cn = kzalloc
>> nn->cld_net = cn
>> // clear nn->client_tracking_ops
>>
>> After nfsd mounts, users can trigger grace_done callbacks via
>> /proc/fs/nfsd/v4_end_grace. If resources are uninitialized or freed
>> in error paths, this causes access violations.
>>
>> Instead of adding locks for specific resources(e.g., reclaim_str_hashtbl),
>> introducing a flag to indicate whether tracking initialization has
>> completed and checking this flag before invoking callbacks may be better.
>>
>> Fixes: 52e19c09a183 ("nfsd: make reclaim_str_hashtbl allocated per net")
>> Signed-off-by: Li Lingfeng <lilingfeng3@...wei.com>
>> ---
>> fs/nfsd/netns.h | 1 +
>> fs/nfsd/nfs4recover.c | 13 +++++++++----
>> 2 files changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
>> index 3e2d0fde80a7..dbd782d6b063 100644
>> --- a/fs/nfsd/netns.h
>> +++ b/fs/nfsd/netns.h
>> @@ -113,6 +113,7 @@ struct nfsd_net {
>>
>> struct file *rec_file;
>> bool in_grace;
>> + bool client_tracking_init_done;
>> const struct nfsd4_client_tracking_ops *client_tracking_ops;
>>
>> time64_t nfsd4_lease;
>> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
>> index c1d9bd07285f..6c27c1252c0e 100644
>> --- a/fs/nfsd/nfs4recover.c
>> +++ b/fs/nfsd/nfs4recover.c
>> @@ -2096,7 +2096,11 @@ nfsd4_client_tracking_init(struct net *net)
>> pr_warn("NFSD: Unable to initialize client recovery tracking! (%d)\n", status);
>> pr_warn("NFSD: Is nfsdcld running? If not, enable CONFIG_NFSD_LEGACY_CLIENT_TRACKING.\n");
>> nn->client_tracking_ops = NULL;
>> + nn->client_tracking_init_done = false;
>> + } else {
>> + nn->client_tracking_init_done = true;
>> }
>> +
> The problem seems real (theoretically at least), but I'm not a fan of
> this fix.
>
> If the problem is as you say, then why not just delay the setting of
> the client_tracking_ops until there is a method that works. IOW, set a
> temporary variable with an ops pointer and only assign
> client_tracking_ops at the end of the function/
>
> Would that also fix this issue?
>
>> return status;
>> }
>>
>> @@ -2105,6 +2109,7 @@ nfsd4_client_tracking_exit(struct net *net)
>> {
>> struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>>
>> + nn->client_tracking_init_done = false;
>> if (nn->client_tracking_ops) {
>> if (nn->client_tracking_ops->exit)
>> nn->client_tracking_ops->exit(net);
>> @@ -2117,7 +2122,7 @@ nfsd4_client_record_create(struct nfs4_client *clp)
>> {
>> struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
>>
>> - if (nn->client_tracking_ops)
>> + if (nn->client_tracking_ops && nn->client_tracking_init_done)
>> nn->client_tracking_ops->create(clp);
>> }
>>
>> @@ -2126,7 +2131,7 @@ nfsd4_client_record_remove(struct nfs4_client *clp)
>> {
>> struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
>>
>> - if (nn->client_tracking_ops)
>> + if (nn->client_tracking_ops && nn->client_tracking_init_done)
>> nn->client_tracking_ops->remove(clp);
>> }
>>
>> @@ -2135,7 +2140,7 @@ nfsd4_client_record_check(struct nfs4_client *clp)
>> {
>> struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
>>
>> - if (nn->client_tracking_ops)
>> + if (nn->client_tracking_ops && nn->client_tracking_init_done)
>> return nn->client_tracking_ops->check(clp);
>>
>> return -EOPNOTSUPP;
>> @@ -2144,7 +2149,7 @@ nfsd4_client_record_check(struct nfs4_client *clp)
>> void
>> nfsd4_record_grace_done(struct nfsd_net *nn)
>> {
>> - if (nn->client_tracking_ops)
>> + if (nn->client_tracking_ops && nn->client_tracking_init_done)
>> nn->client_tracking_ops->grace_done(nn);
>> }
>>
Powered by blists - more mailing lists