linux-kernel - [PATCH v2] NFSv4/pNFS: fix incorrect uses of list iterator

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-Id: <20220329035633.14380-1-xiam0nd.tong@gmail.com>
Date:   Tue, 29 Mar 2022 11:56:33 +0800
From:   Xiaomeng Tong <xiam0nd.tong@...il.com>
To:     trond.myklebust@...merspace.com, anna@...nel.org
Cc:     bhalevy@...asas.com, bharrosh@...asas.com,
        linux-nfs@...r.kernel.org, linux-kernel@...r.kernel.org,
        Xiaomeng Tong <xiam0nd.tong@...il.com>, stable@...r.kernel.org
Subject: [PATCH v2] NFSv4/pNFS: fix incorrect uses of list iterator

The bug is here:
	if (!server ||
	server->pnfs_curr_ld->id != dev->cbd_layout_type) {

The list iterator value 'server' will *always* be set and non-NULL
by list_for_each_entry_rcu, so it is incorrect to assume that the
iterator value will be NULL if the list is empty or no element is
found (In fact, it will be a bogus pointer to an invalid struct
object containing the HEAD, which is used for above check at next
outer loop). Otherwise it may bypass the check in theory (if
server->pnfs_curr_ld->id == dev->cbd_layout_type, 'server' now is
a bogus pointer) and lead to invalid memory access passing the check.

Furthermore, even if we have a valid pointer, nothing pins the super
block, and so the struct nfs_server could end up getting freed while
we're using it.

Since all we want is a pointer to the struct pnfs_layoutdriver_type,
let's skip all the iteration over super blocks, and just use API to
find the layout driver directly. And to avoid use last found 'ld'
which may not exists any more, just call the API for every 'dev'.

At the same time, move the code to make the logic clearer.

Cc: stable@...r.kernel.org
Fixes: 1be5683b03a76 ("pnfs: CB_NOTIFY_DEVICEID")
Signed-off-by: Xiaomeng Tong <xiam0nd.tong@...il.com>
---

changes since v1:
 - use API to find the layout driver directly (Trond Myklebust)
 - avoid use last found 'ld' (Xiaomeng Tong)
 - code movement (Xiaomeng Tong)

v1:https://lore.kernel.org/lkml/20220327080230.12134-1-xiam0nd.tong@gmail.com/

---
 fs/nfs/callback_proc.c | 32 ++++++++++----------------------
 fs/nfs/pnfs.c          |  5 +++++
 fs/nfs/pnfs.h          |  1 +
 3 files changed, 16 insertions(+), 22 deletions(-)

diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
index c343666d9a42..579887749870 100644
--- a/fs/nfs/callback_proc.c
+++ b/fs/nfs/callback_proc.c
@@ -358,39 +358,27 @@ __be32 nfs4_callback_devicenotify(void *argp, void *resp,
 				  struct cb_process_state *cps)
 {
 	struct cb_devicenotifyargs *args = argp;
+	const struct pnfs_layoutdriver_type *ld;
 	uint32_t i;
-	__be32 res = 0;
-	struct nfs_client *clp = cps->clp;
-	struct nfs_server *server = NULL;
 
-	if (!clp) {
-		res = cpu_to_be32(NFS4ERR_OP_NOT_IN_SESSION);
-		goto out;
+	if (!cps->clp) {
+		kfree(args->devs);
+		return cpu_to_be32(NFS4ERR_OP_NOT_IN_SESSION);
 	}
 
 	for (i = 0; i < args->ndevs; i++) {
 		struct cb_devicenotifyitem *dev = &args->devs[i];
 
-		if (!server ||
-		    server->pnfs_curr_ld->id != dev->cbd_layout_type) {
-			rcu_read_lock();
-			list_for_each_entry_rcu(server, &clp->cl_superblocks, client_link)
-				if (server->pnfs_curr_ld &&
-				    server->pnfs_curr_ld->id == dev->cbd_layout_type) {
-					rcu_read_unlock();
-					goto found;
-				}
-			rcu_read_unlock();
-			continue;
+		ld = pnfs_find_layoutdriver(dev->cbd_layout_type);
+		if (ld) {
+			nfs4_delete_deviceid(ld, cps->clp,
+						&dev->cbd_dev_id);
+			module_put(ld->owner);
 		}
-
-	found:
-		nfs4_delete_deviceid(server->pnfs_curr_ld, clp, &dev->cbd_dev_id);
 	}
 
-out:
 	kfree(args->devs);
-	return res;
+	return 0;
 }
 
 /*
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 7c9090a28e5c..112c36977feb 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -92,6 +92,11 @@ find_pnfs_driver(u32 id)
 	return local;
 }
 
+const struct pnfs_layoutdriver_type *pnfs_find_layoutdriver(u32 id)
+{
+	return find_pnfs_driver(id);
+}
+
 void
 unset_pnfs_layoutdriver(struct nfs_server *nfss)
 {
diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
index f4d7548d67b2..873ea8fe945b 100644
--- a/fs/nfs/pnfs.h
+++ b/fs/nfs/pnfs.h
@@ -234,6 +234,7 @@ struct pnfs_devicelist {
 
 extern int pnfs_register_layoutdriver(struct pnfs_layoutdriver_type *);
 extern void pnfs_unregister_layoutdriver(struct pnfs_layoutdriver_type *);
+extern const struct pnfs_layoutdriver_type *pnfs_find_layoutdriver(u32 id);
 
 /* nfs4proc.c */
 extern size_t max_response_pages(struct nfs_server *server);
-- 
2.17.1