| 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: <ae16dcf3d5c569bbff817ca442a7615e816a66e7.camel@kernel.org>
Date: Tue, 13 May 2025 07:34:25 -0500
From: Jeff Layton <jlayton@...nel.org>
To: Li Lingfeng <lilingfeng3@...wei.com>, 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
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);
> }
>
--
Jeff Layton <jlayton@...nel.org>
Powered by blists - more mailing lists