The Linux Kernel Mailing List
 help / color / mirror / Atom feed
* [PATCH v3 0/4] ceph: bound untrusted MDS and monitor reply decoders
@ 2026-07-07 18:05 Michael Bommarito
  2026-07-07 18:05 ` [PATCH v3 1/4] ceph: bound xattr value length in __build_xattrs() Michael Bommarito
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Michael Bommarito @ 2026-07-07 18:05 UTC (permalink / raw)
  To: Ilya Dryomov, Alex Markuze, Viacheslav Dubeyko; +Cc: ceph-devel, linux-kernel

This is v3 of the CephFS decoder-bound series. The four bugs are
independent; each bounds an untrusted length or count that a malicious or
compromised MDS or monitor controls on the wire.

  1/4 rejects a final xattr value length that runs past the xattr blob.
  2/4 bounds MDSCapAuth path and fs_name copies in handle_session().
  3/4 bounds the mdsmap export_targets array for info_v 2/3.
  4/4 caps delegated-inode parsing by per-session population and by one
      reply's aggregate interval length.

Patches 1/4 and 2/4 carry Viacheslav Dubeyko's Reviewed-by from v1. Patch
3/4 carries his Reviewed-by from v2. Patch 4/4 is reworked to address his
v2 review, described below.

Changes in v3:

  - Rebased onto ceph/testing (base-commit fc67edb66b3c9 below); v2 no
    longer applied there. The series applies cleanly with git am on that
    base. Patches 1-3 are unchanged apart from the rebase.

  - Patch 4/4 reworked per review of v2:

    - The per-session count is now enforced in a single place,
      ceph_insert_deleg_ino(), using atomic_add_unless() rather than an
      increment-then-decrement pattern. Since that helper is the only
      caller that grows the count, the per-session population can never
      exceed CEPH_MAX_DELEG_INOS, so the separate per-session pre-check in
      the decode loop is dropped.

    - The per-reply aggregate check is kept. It is the bound that stops one
      reply from spinning the insert loop on duplicate ranges without
      growing the per-session count, which the per-insert cap alone does
      not catch.

    - CEPH_MAX_DELEG_INOS is cast to u64 where it is compared against and
      subtracted from the u64 interval length.

    - The redundant warning in the decode loop is removed;
      ceph_insert_deleg_ino() already logs when the cap is reached, and the
      warnings now report the counts involved.

Michael Bommarito (4):
  ceph: bound xattr value length in __build_xattrs()
  ceph: bound MDSCapAuth path and fs_name decode in handle_session()
  ceph: bound num_export_targets array for mds info v2/v3
  ceph: cap delegated inode count in ceph_parse_deleg_inos()

 fs/ceph/mds_client.c | 67 ++++++++++++++++++++++++++++++++++++++------
 fs/ceph/mds_client.h |  1 +
 fs/ceph/mdsmap.c     |  7 ++++-
 fs/ceph/super.h      |  9 ++++++
 fs/ceph/xattr.c      |  1 +
 5 files changed, 76 insertions(+), 9 deletions(-)


base-commit: fc67edb66b3c9924c4e0bb366a92b32ea13c526a
--
2.53.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v3 1/4] ceph: bound xattr value length in __build_xattrs()
  2026-07-07 18:05 [PATCH v3 0/4] ceph: bound untrusted MDS and monitor reply decoders Michael Bommarito
@ 2026-07-07 18:05 ` Michael Bommarito
  2026-07-07 18:05 ` [PATCH v3 2/4] ceph: bound MDSCapAuth path and fs_name decode in handle_session() Michael Bommarito
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 6+ messages in thread
From: Michael Bommarito @ 2026-07-07 18:05 UTC (permalink / raw)
  To: Ilya Dryomov, Alex Markuze, Viacheslav Dubeyko; +Cc: ceph-devel, linux-kernel

__build_xattrs() decodes the MDS-supplied xattr blob one attribute at a
time. For each attribute it reads a 32-bit name length, advances past the
name bytes, reads a 32-bit value length, records the value pointer, and
advances past the value bytes. The two length fields are read with
ceph_decode_32_safe(), but the value bytes themselves are advanced over
with a bare "p += len" and no ceph_decode_need() check that "len" bytes
remain in the blob.

For every attribute except the last, the next iteration's
ceph_decode_32_safe() on the following name length implicitly verifies
that the previous value did not run past the blob end. The final
attribute has no successor, so its decoded value length is never checked
against the blob bounds. A malicious or compromised metadata server can
set the last attribute's value length larger than the bytes actually
present in the blob.

The blob is a dedicated kvmalloc() allocation sized to the wire length
(ceph_buffer_new() in ceph_fill_inode()). __set_xattr() records the
oversized length in xattr->val_len verbatim, and a later getxattr(2) runs
memcpy(value, xattr->val, xattr->val_len) into a user-supplied buffer,
copying bytes past the end of the allocation back to user space.

Impact: a malicious metadata server discloses adjacent kernel heap bytes
to a local user via getxattr(2) on a CephFS file. Add the missing
ceph_decode_need() so an out-of-bounds value length on the final
attribute fails the decode and returns -EIO instead of being stored.

Fixes: 355da1eb7a1f ("ceph: inode operations")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
---
 fs/ceph/xattr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
index 860fc8e1867df..cc4ffbbcb719a 100644
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -848,6 +848,7 @@ static int __build_xattrs(struct inode *inode)
 			name = p;
 			p += len;
 			ceph_decode_32_safe(&p, end, len, bad);
+			ceph_decode_need(&p, end, len, bad);
 			val = p;
 			p += len;
 
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v3 2/4] ceph: bound MDSCapAuth path and fs_name decode in handle_session()
  2026-07-07 18:05 [PATCH v3 0/4] ceph: bound untrusted MDS and monitor reply decoders Michael Bommarito
  2026-07-07 18:05 ` [PATCH v3 1/4] ceph: bound xattr value length in __build_xattrs() Michael Bommarito
@ 2026-07-07 18:05 ` Michael Bommarito
  2026-07-07 18:05 ` [PATCH v3 3/4] ceph: bound num_export_targets array for mds info v2/v3 Michael Bommarito
  2026-07-07 18:06 ` [PATCH v3 4/4] ceph: cap delegated inode count in ceph_parse_deleg_inos() Michael Bommarito
  3 siblings, 0 replies; 6+ messages in thread
From: Michael Bommarito @ 2026-07-07 18:05 UTC (permalink / raw)
  To: Ilya Dryomov, Alex Markuze, Viacheslav Dubeyko; +Cc: ceph-devel, linux-kernel

handle_session() decodes the MDSCapAuth records carried by a
CEPH_SESSION_OPEN message (msg_version >= 6). For each record the
match.path and match.fs_name byte strings are read by first decoding a
32-bit length and then copying that many bytes with the bare
ceph_decode_copy(). Unlike the surrounding fields, which all use the
_safe decode variants, these two copies are not preceded by a
ceph_decode_need() bounds check, and the enclosing MDSCapAuth and
MDSCapMatch struct_len fields are skipped rather than enforced as an
upper bound. A length larger than the bytes remaining in the message
front makes ceph_decode_copy() read past the end of the front buffer.

The message front is a dedicated allocation (ceph_msg_new2() ->
kvmalloc), so the over-read runs off that object. A malicious or
compromised MDS can trigger this with the first post-connect message on
mount, with no client-side user interaction; under KASAN it is reported
as a slab-out-of-bounds read in handle_session().

Impact: a malicious MDS can force the kernel client to read up to 4 GiB
past the message front allocation during session setup, crashing the
client (out-of-bounds read).

Switch both copies to ceph_decode_copy_safe(), which performs the
ceph_decode_need() bounds check before the copy and branches to the
existing bad label, matching the rest of the decoder and the error path
that frees the partially decoded cap_auths array.

Fixes: 1d17de9534cb ("ceph: save cap_auths in MDS client when session is opened")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
---
 fs/ceph/mds_client.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 18be36c2e9e87..94f76e9fe4ead 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -4434,7 +4434,9 @@ static void handle_session(struct ceph_mds_session *session,
 					pr_err_client(cl, "No memory for path\n");
 					goto fail;
 				}
-				ceph_decode_copy(&p, cap_auths[i].match.path, _len);
+				ceph_decode_copy_safe(&p, end,
+						      cap_auths[i].match.path,
+						      _len, bad);
 
 				/* Remove the tailing '/' */
 				while (_len && cap_auths[i].match.path[_len - 1] == '/') {
@@ -4451,7 +4453,9 @@ static void handle_session(struct ceph_mds_session *session,
 					pr_err_client(cl, "No memory for fs_name\n");
 					goto fail;
 				}
-				ceph_decode_copy(&p, cap_auths[i].match.fs_name, _len);
+				ceph_decode_copy_safe(&p, end,
+						      cap_auths[i].match.fs_name,
+						      _len, bad);
 			}
 
 			ceph_decode_8_safe(&p, end, cap_auths[i].match.root_squash, bad);
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v3 3/4] ceph: bound num_export_targets array for mds info v2/v3
  2026-07-07 18:05 [PATCH v3 0/4] ceph: bound untrusted MDS and monitor reply decoders Michael Bommarito
  2026-07-07 18:05 ` [PATCH v3 1/4] ceph: bound xattr value length in __build_xattrs() Michael Bommarito
  2026-07-07 18:05 ` [PATCH v3 2/4] ceph: bound MDSCapAuth path and fs_name decode in handle_session() Michael Bommarito
@ 2026-07-07 18:05 ` Michael Bommarito
  2026-07-07 18:06 ` [PATCH v3 4/4] ceph: cap delegated inode count in ceph_parse_deleg_inos() Michael Bommarito
  3 siblings, 0 replies; 6+ messages in thread
From: Michael Bommarito @ 2026-07-07 18:05 UTC (permalink / raw)
  To: Ilya Dryomov, Alex Markuze, Viacheslav Dubeyko; +Cc: ceph-devel, linux-kernel

ceph_mdsmap_decode() in fs/ceph/mdsmap.c reads num_export_targets from
each per-mds info record and advances the decode cursor by
num_export_targets * sizeof(u32) without first checking that many bytes
remain. The only upper-bound check that catches a runaway cursor
(*p > info_end) is gated on info_v >= 4, because info_end is left NULL
for info_v 2 and 3. When the monitor sends an MDS map whose per-mds
info version is 2 or 3 with an oversized num_export_targets, the cursor
moves past the message front buffer and the later export-targets loop
calls the unchecked ceph_decode_32() on out-of-bounds memory.

A kernel client processes CEPH_MSG_MDS_MAP from its monitor session
(net/ceph/mon_client.c dispatches it; fs/ceph/super.c routes it to
ceph_mdsc_handle_mdsmap(), which sets end to the front buffer bound and
calls ceph_mdsmap_decode()). A malicious or compromised monitor, or an
on-path attacker on an unsigned/unencrypted messenger session, can
therefore drive an out-of-bounds read in the client kernel; on x86_64
with KASAN it is reported as a slab-out-of-bounds read in
ceph_mdsmap_decode(). The decoded values land in the internal
info->export_targets[] array, so the consequence is a kernel
out-of-bounds read, not an information leak to the attacker.

Impact: a malicious or compromised Ceph monitor sending an MDS map with
a per-mds info version of 2 or 3 and an oversized num_export_targets
field triggers an out-of-bounds read in the CephFS client kernel.

Add a ceph_decode_need() for the export-targets array before advancing
the cursor, so the bound is enforced for every info_v >= 2, not only
info_v >= 4. This mirrors the count-then-need idiom already used for
m_data_pg_pools later in the same function.

Compute the export-targets byte count with size_mul() and reuse that
checked length when advancing the cursor, so the attacker-controlled
num_export_targets multiplication fails closed on overflow rather than
relying on the later kcalloc() guard.

Fixes: d463a43d69f4 ("ceph: CEPH_FEATURE_MDSENC support")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>
---
 fs/ceph/mdsmap.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/ceph/mdsmap.c b/fs/ceph/mdsmap.c
index d8e46eb7e5eb5..c0f63f0460f67 100644
--- a/fs/ceph/mdsmap.c
+++ b/fs/ceph/mdsmap.c
@@ -3,6 +3,7 @@
 
 #include <linux/bug.h>
 #include <linux/err.h>
+#include <linux/overflow.h>
 #include <linux/random.h>
 #include <linux/slab.h>
 #include <linux/types.h>
@@ -126,6 +127,7 @@ struct ceph_mdsmap *ceph_mdsmap_decode(struct ceph_mds_client *mdsc, void **p,
 	u8 mdsmap_v;
 	u16 mdsmap_ev;
 	u32 target;
+	size_t export_targets_len;
 
 	m = kzalloc_obj(*m, GFP_NOFS);
 	if (!m)
@@ -224,8 +226,11 @@ struct ceph_mdsmap *ceph_mdsmap_decode(struct ceph_mds_client *mdsc, void **p,
 		*p += namelen;
 		if (info_v >= 2) {
 			ceph_decode_32_safe(p, end, num_export_targets, bad);
+			export_targets_len = size_mul(num_export_targets,
+						      sizeof(u32));
+			ceph_decode_need(p, end, export_targets_len, bad);
 			pexport_targets = *p;
-			*p += num_export_targets * sizeof(u32);
+			*p += export_targets_len;
 		} else {
 			num_export_targets = 0;
 		}
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v3 4/4] ceph: cap delegated inode count in ceph_parse_deleg_inos()
  2026-07-07 18:05 [PATCH v3 0/4] ceph: bound untrusted MDS and monitor reply decoders Michael Bommarito
                   ` (2 preceding siblings ...)
  2026-07-07 18:05 ` [PATCH v3 3/4] ceph: bound num_export_targets array for mds info v2/v3 Michael Bommarito
@ 2026-07-07 18:06 ` Michael Bommarito
  2026-07-07 18:30   ` Viacheslav Dubeyko
  3 siblings, 1 reply; 6+ messages in thread
From: Michael Bommarito @ 2026-07-07 18:06 UTC (permalink / raw)
  To: Ilya Dryomov, Alex Markuze, Viacheslav Dubeyko; +Cc: ceph-devel, linux-kernel

ceph_parse_deleg_inos() decodes interval sets of delegated inode numbers
from an MDS create-with-delegation reply. For each set it reads a 64-bit
start and a 64-bit len with ceph_decode_64_safe(), which only validates
that the eight bytes are present in the message, not the value, and then
loops over len while inserting entries into s_delegated_inos.

len is fully attacker controlled. A malicious or compromised MDS can send
one huge interval, many intervals in one reply, duplicate intervals, or
repeated replies that accumulate delegated inodes on the same session.
The original code bounded none of these and could spin the insert loop or
grow the xarray without limit.

Bound both dimensions with a single enforcement point. Track the number
of delegated inodes held by each MDS session in an atomic counter and
grow it only in ceph_insert_deleg_ino(), which uses atomic_add_unless()
to refuse to push the count past CEPH_MAX_DELEG_INOS. Because that helper
is the only place the counter grows, the per-session population can never
exceed the cap, so no separate per-session pre-check is needed. The
counter is decremented when async create consumes a delegated inode or
when an insert fails, incremented when a delegated inode is restored,
initialized with the session xarray, and reset when reconnect destroys
the xarray.

A per-session cap alone still lets one reply spin the insert loop on
duplicate ranges without growing the counter, so also cap the aggregate
interval length accepted from a single reply. Together these bound both
the loop trip count per reply and the xarray population across replies.

The cap is a fixed, client-chosen constant rather than a value derived
from the MDS. mds_client_prealloc_inos is a userspace MDS configuration
option; it is never sent to the kernel client on the wire, and a
server-supplied bound could not be trusted for a defensive limit in any
case. The constant is set well above that option's documented default of
1000 (a generous multiple), so legitimate refill behavior is unaffected
while the CPU and xarray memory a malformed delegation stream can consume
stays bounded.

Impact: a malicious or compromised Ceph MDS can no longer make a client
spin through an unbounded delegated-inode interval or grow one session's
delegated-inode xarray without limit.

Fixes: d48464878708 ("ceph: decode interval_sets for delegated inos")
Cc: stable@vger.kernel.org
Suggested-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 fs/ceph/mds_client.c | 59 +++++++++++++++++++++++++++++++++++++++-----
 fs/ceph/mds_client.h |  1 +
 fs/ceph/super.h      |  9 +++++++
 3 files changed, 63 insertions(+), 6 deletions(-)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 94f76e9fe4ead..7b00b83371f7e 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -618,10 +618,36 @@ static int parse_reply_info_filelock(void **p, void *end,
 
 #define DELEGATED_INO_AVAILABLE		xa_mk_value(1)
 
+static int ceph_insert_deleg_ino(struct ceph_mds_session *s, u64 ino)
+{
+	struct ceph_client *cl = s->s_mdsc->fsc->client;
+	int err;
+
+	/*
+	 * Cap how many delegated inodes a single session may hold. This is
+	 * the only place that grows the count, so atomic_add_unless() bounds
+	 * it at exactly CEPH_MAX_DELEG_INOS; s_num_deleg_inos can never exceed
+	 * that.
+	 */
+	if (!atomic_add_unless(&s->s_num_deleg_inos, 1, CEPH_MAX_DELEG_INOS)) {
+		pr_warn_ratelimited_client(cl,
+			"MDS session already holds %d delegated inodes\n",
+			CEPH_MAX_DELEG_INOS);
+		return -EOVERFLOW;
+	}
+
+	err = xa_insert(&s->s_delegated_inos, ino, DELEGATED_INO_AVAILABLE,
+			GFP_KERNEL);
+	if (err)
+		atomic_dec(&s->s_num_deleg_inos);
+	return err;
+}
+
 static int ceph_parse_deleg_inos(void **p, void *end,
 				 struct ceph_mds_session *s)
 {
 	struct ceph_client *cl = s->s_mdsc->fsc->client;
+	u64 msg_deleg_inos = 0;
 	u32 sets;
 
 	ceph_decode_32_safe(p, end, sets, bad);
@@ -639,16 +665,34 @@ static int ceph_parse_deleg_inos(void **p, void *end,
 				start, len);
 			continue;
 		}
+
+		/*
+		 * Bound the number of inodes one reply may delegate.
+		 * ceph_insert_deleg_ino() separately caps the per-session
+		 * population, so this only has to stop one reply from spinning
+		 * the insert loop under an attacker-controlled len.
+		 */
+		if (len > (u64)CEPH_MAX_DELEG_INOS ||
+		    msg_deleg_inos > (u64)CEPH_MAX_DELEG_INOS - len) {
+			pr_warn_ratelimited_client(cl,
+				"MDS reply delegates too many inodes (have %llu, +%llu, max %d)\n",
+				msg_deleg_inos, len, CEPH_MAX_DELEG_INOS);
+			return -EIO;
+		}
+		msg_deleg_inos += len;
+
 		while (len--) {
-			int err = xa_insert(&s->s_delegated_inos, start++,
-					    DELEGATED_INO_AVAILABLE,
-					    GFP_KERNEL);
+			int err = ceph_insert_deleg_ino(s, start++);
+
 			if (!err) {
 				doutc(cl, "added delegated inode 0x%llx\n", start - 1);
 			} else if (err == -EBUSY) {
 				pr_warn_client(cl,
 					"MDS delegated inode 0x%llx more than once.\n",
 					start - 1);
+			} else if (err == -EOVERFLOW) {
+				/* ceph_insert_deleg_ino() already warned. */
+				return -EIO;
 			} else {
 				return err;
 			}
@@ -666,16 +710,17 @@ u64 ceph_get_deleg_ino(struct ceph_mds_session *s)
 
 	xa_for_each(&s->s_delegated_inos, ino, val) {
 		val = xa_erase(&s->s_delegated_inos, ino);
-		if (val == DELEGATED_INO_AVAILABLE)
+		if (val == DELEGATED_INO_AVAILABLE) {
+			atomic_dec(&s->s_num_deleg_inos);
 			return ino;
+		}
 	}
 	return 0;
 }
 
 int ceph_restore_deleg_ino(struct ceph_mds_session *s, u64 ino)
 {
-	return xa_insert(&s->s_delegated_inos, ino, DELEGATED_INO_AVAILABLE,
-			 GFP_KERNEL);
+	return ceph_insert_deleg_ino(s, ino);
 }
 #else /* BITS_PER_LONG == 64 */
 /*
@@ -1062,6 +1107,7 @@ static struct ceph_mds_session *register_session(struct ceph_mds_client *mdsc,
 	INIT_LIST_HEAD(&s->s_waiting);
 	INIT_LIST_HEAD(&s->s_unsafe);
 	xa_init(&s->s_delegated_inos);
+	atomic_set(&s->s_num_deleg_inos, 0);
 	INIT_LIST_HEAD(&s->s_cap_releases);
 	INIT_WORK(&s->s_cap_release_work, ceph_cap_release_work);
 
@@ -5103,6 +5149,7 @@ static int send_mds_reconnect(struct ceph_mds_client *mdsc,
 
 	/* Serialized by s_mutex against concurrent ceph_get_deleg_ino(). */
 	xa_destroy(&session->s_delegated_inos);
+	atomic_set(&session->s_num_deleg_inos, 0);
 	if (session->s_state == CEPH_MDS_SESSION_CLOSED ||
 	    session->s_state == CEPH_MDS_SESSION_REJECTED) {
 		pr_info_client(cl, "mds%d skipping reconnect, session %s\n",
diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
index 731d6ad04956d..d1a10c4bd4d2d 100644
--- a/fs/ceph/mds_client.h
+++ b/fs/ceph/mds_client.h
@@ -299,6 +299,7 @@ struct ceph_mds_session {
 	struct list_head  s_waiting;  /* waiting requests */
 	struct list_head  s_unsafe;   /* unsafe requests */
 	struct xarray	  s_delegated_inos;
+	atomic_t	  s_num_deleg_inos;
 };
 
 /*
diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index 3737ea7ed88b3..fb2b0a4b7bb3e 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -642,6 +642,15 @@ static inline int ceph_ino_compare(struct inode *inode, void *data)
 #define CEPH_MDS_INO_LOG_OFFSET		(2 * CEPH_MAX_MDS)
 #define CEPH_INO_SYSTEM_BASE		((6*CEPH_MAX_MDS) + (CEPH_MAX_MDS * CEPH_NUM_STRAY))
 
+/*
+ * Upper bound on the number of delegated inodes a single MDS session may
+ * hold. The MDS normally hands out a small preallocation window (the
+ * userspace mds_client_prealloc_inos option defaults to 1000) and refills
+ * it as the client consumes entries. This leaves generous headroom while
+ * bounding the CPU and memory a malformed delegation interval can consume.
+ */
+#define CEPH_MAX_DELEG_INOS		8192
+
 static inline bool ceph_vino_is_reserved(const struct ceph_vino vino)
 {
 	if (vino.ino >= CEPH_INO_SYSTEM_BASE ||
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re:  [PATCH v3 4/4] ceph: cap delegated inode count in ceph_parse_deleg_inos()
  2026-07-07 18:06 ` [PATCH v3 4/4] ceph: cap delegated inode count in ceph_parse_deleg_inos() Michael Bommarito
@ 2026-07-07 18:30   ` Viacheslav Dubeyko
  0 siblings, 0 replies; 6+ messages in thread
From: Viacheslav Dubeyko @ 2026-07-07 18:30 UTC (permalink / raw)
  To: idryomov@gmail.com, Alex Markuze, michael.bommarito@gmail.com,
	slava@dubeyko.com
  Cc: ceph-devel@vger.kernel.org, linux-kernel@vger.kernel.org

Hi Michael,

Sorry for delay with the review.

On Tue, 2026-07-07 at 14:06 -0400, Michael Bommarito wrote:
> ceph_parse_deleg_inos() decodes interval sets of delegated inode numbers
> from an MDS create-with-delegation reply. For each set it reads a 64-bit
> start and a 64-bit len with ceph_decode_64_safe(), which only validates
> that the eight bytes are present in the message, not the value, and then
> loops over len while inserting entries into s_delegated_inos.
> 
> len is fully attacker controlled. A malicious or compromised MDS can send
> one huge interval, many intervals in one reply, duplicate intervals, or
> repeated replies that accumulate delegated inodes on the same session.
> The original code bounded none of these and could spin the insert loop or
> grow the xarray without limit.
> 
> Bound both dimensions with a single enforcement point. Track the number
> of delegated inodes held by each MDS session in an atomic counter and
> grow it only in ceph_insert_deleg_ino(), which uses atomic_add_unless()
> to refuse to push the count past CEPH_MAX_DELEG_INOS. Because that helper
> is the only place the counter grows, the per-session population can never
> exceed the cap, so no separate per-session pre-check is needed. The
> counter is decremented when async create consumes a delegated inode or
> when an insert fails, incremented when a delegated inode is restored,
> initialized with the session xarray, and reset when reconnect destroys
> the xarray.
> 
> A per-session cap alone still lets one reply spin the insert loop on
> duplicate ranges without growing the counter, so also cap the aggregate
> interval length accepted from a single reply. Together these bound both
> the loop trip count per reply and the xarray population across replies.
> 
> The cap is a fixed, client-chosen constant rather than a value derived
> from the MDS. mds_client_prealloc_inos is a userspace MDS configuration
> option; it is never sent to the kernel client on the wire, and a
> server-supplied bound could not be trusted for a defensive limit in any
> case. The constant is set well above that option's documented default of
> 1000 (a generous multiple), so legitimate refill behavior is unaffected
> while the CPU and xarray memory a malformed delegation stream can consume
> stays bounded.
> 
> Impact: a malicious or compromised Ceph MDS can no longer make a client
> spin through an unbounded delegated-inode interval or grow one session's
> delegated-inode xarray without limit.
> 
> Fixes: d48464878708 ("ceph: decode interval_sets for delegated inos")
> Cc: stable@vger.kernel.org
> Suggested-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
> ---
>  fs/ceph/mds_client.c | 59 +++++++++++++++++++++++++++++++++++++++-----
>  fs/ceph/mds_client.h |  1 +
>  fs/ceph/super.h      |  9 +++++++
>  3 files changed, 63 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 94f76e9fe4ead..7b00b83371f7e 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -618,10 +618,36 @@ static int parse_reply_info_filelock(void **p, void *end,
>  
>  #define DELEGATED_INO_AVAILABLE		xa_mk_value(1)
>  
> +static int ceph_insert_deleg_ino(struct ceph_mds_session *s, u64 ino)
> +{
> +	struct ceph_client *cl = s->s_mdsc->fsc->client;
> +	int err;
> +
> +	/*
> +	 * Cap how many delegated inodes a single session may hold. This is
> +	 * the only place that grows the count, so atomic_add_unless() bounds
> +	 * it at exactly CEPH_MAX_DELEG_INOS; s_num_deleg_inos can never exceed
> +	 * that.
> +	 */
> +	if (!atomic_add_unless(&s->s_num_deleg_inos, 1, CEPH_MAX_DELEG_INOS)) {
> +		pr_warn_ratelimited_client(cl,
> +			"MDS session already holds %d delegated inodes\n",
> +			CEPH_MAX_DELEG_INOS);
> +		return -EOVERFLOW;
> +	}
> +
> +	err = xa_insert(&s->s_delegated_inos, ino, DELEGATED_INO_AVAILABLE,
> +			GFP_KERNEL);
> +	if (err)
> +		atomic_dec(&s->s_num_deleg_inos);
> +	return err;
> +}
> +
>  static int ceph_parse_deleg_inos(void **p, void *end,
>  				 struct ceph_mds_session *s)
>  {
>  	struct ceph_client *cl = s->s_mdsc->fsc->client;
> +	u64 msg_deleg_inos = 0;
>  	u32 sets;
>  
>  	ceph_decode_32_safe(p, end, sets, bad);
> @@ -639,16 +665,34 @@ static int ceph_parse_deleg_inos(void **p, void *end,
>  				start, len);
>  			continue;
>  		}
> +
> +		/*
> +		 * Bound the number of inodes one reply may delegate.
> +		 * ceph_insert_deleg_ino() separately caps the per-session
> +		 * population, so this only has to stop one reply from spinning
> +		 * the insert loop under an attacker-controlled len.
> +		 */
> +		if (len > (u64)CEPH_MAX_DELEG_INOS ||
> +		    msg_deleg_inos > (u64)CEPH_MAX_DELEG_INOS - len) {
> +			pr_warn_ratelimited_client(cl,
> +				"MDS reply delegates too many inodes (have %llu, +%llu, max %d)\n",
> +				msg_deleg_inos, len, CEPH_MAX_DELEG_INOS);
> +			return -EIO;
> +		}
> +		msg_deleg_inos += len;
> +
>  		while (len--) {
> -			int err = xa_insert(&s->s_delegated_inos, start++,
> -					    DELEGATED_INO_AVAILABLE,
> -					    GFP_KERNEL);
> +			int err = ceph_insert_deleg_ino(s, start++);
> +
>  			if (!err) {
>  				doutc(cl, "added delegated inode 0x%llx\n", start - 1);
>  			} else if (err == -EBUSY) {
>  				pr_warn_client(cl,
>  					"MDS delegated inode 0x%llx more than once.\n",
>  					start - 1);
> +			} else if (err == -EOVERFLOW) {
> +				/* ceph_insert_deleg_ino() already warned. */
> +				return -EIO;
>  			} else {
>  				return err;
>  			}
> @@ -666,16 +710,17 @@ u64 ceph_get_deleg_ino(struct ceph_mds_session *s)
>  
>  	xa_for_each(&s->s_delegated_inos, ino, val) {
>  		val = xa_erase(&s->s_delegated_inos, ino);
> -		if (val == DELEGATED_INO_AVAILABLE)
> +		if (val == DELEGATED_INO_AVAILABLE) {
> +			atomic_dec(&s->s_num_deleg_inos);
>  			return ino;
> +		}
>  	}
>  	return 0;
>  }
>  
>  int ceph_restore_deleg_ino(struct ceph_mds_session *s, u64 ino)
>  {
> -	return xa_insert(&s->s_delegated_inos, ino, DELEGATED_INO_AVAILABLE,
> -			 GFP_KERNEL);
> +	return ceph_insert_deleg_ino(s, ino);
>  }
>  #else /* BITS_PER_LONG == 64 */
>  /*
> @@ -1062,6 +1107,7 @@ static struct ceph_mds_session *register_session(struct ceph_mds_client *mdsc,
>  	INIT_LIST_HEAD(&s->s_waiting);
>  	INIT_LIST_HEAD(&s->s_unsafe);
>  	xa_init(&s->s_delegated_inos);
> +	atomic_set(&s->s_num_deleg_inos, 0);
>  	INIT_LIST_HEAD(&s->s_cap_releases);
>  	INIT_WORK(&s->s_cap_release_work, ceph_cap_release_work);
>  
> @@ -5103,6 +5149,7 @@ static int send_mds_reconnect(struct ceph_mds_client *mdsc,
>  
>  	/* Serialized by s_mutex against concurrent ceph_get_deleg_ino(). */
>  	xa_destroy(&session->s_delegated_inos);
> +	atomic_set(&session->s_num_deleg_inos, 0);
>  	if (session->s_state == CEPH_MDS_SESSION_CLOSED ||
>  	    session->s_state == CEPH_MDS_SESSION_REJECTED) {
>  		pr_info_client(cl, "mds%d skipping reconnect, session %s\n",
> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> index 731d6ad04956d..d1a10c4bd4d2d 100644
> --- a/fs/ceph/mds_client.h
> +++ b/fs/ceph/mds_client.h
> @@ -299,6 +299,7 @@ struct ceph_mds_session {
>  	struct list_head  s_waiting;  /* waiting requests */
>  	struct list_head  s_unsafe;   /* unsafe requests */
>  	struct xarray	  s_delegated_inos;
> +	atomic_t	  s_num_deleg_inos;
>  };
>  
>  /*
> diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> index 3737ea7ed88b3..fb2b0a4b7bb3e 100644
> --- a/fs/ceph/super.h
> +++ b/fs/ceph/super.h
> @@ -642,6 +642,15 @@ static inline int ceph_ino_compare(struct inode *inode, void *data)
>  #define CEPH_MDS_INO_LOG_OFFSET		(2 * CEPH_MAX_MDS)
>  #define CEPH_INO_SYSTEM_BASE		((6*CEPH_MAX_MDS) + (CEPH_MAX_MDS * CEPH_NUM_STRAY))
>  
> +/*
> + * Upper bound on the number of delegated inodes a single MDS session may
> + * hold. The MDS normally hands out a small preallocation window (the
> + * userspace mds_client_prealloc_inos option defaults to 1000) and refills
> + * it as the client consumes entries. This leaves generous headroom while
> + * bounding the CPU and memory a malformed delegation interval can consume.
> + */
> +#define CEPH_MAX_DELEG_INOS		8192
> +
>  static inline bool ceph_vino_is_reserved(const struct ceph_vino vino)
>  {
>  	if (vino.ino >= CEPH_INO_SYSTEM_BASE ||

Looks good.

Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>

Thanks,
Slava.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-07-07 18:30 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-07 18:05 [PATCH v3 0/4] ceph: bound untrusted MDS and monitor reply decoders Michael Bommarito
2026-07-07 18:05 ` [PATCH v3 1/4] ceph: bound xattr value length in __build_xattrs() Michael Bommarito
2026-07-07 18:05 ` [PATCH v3 2/4] ceph: bound MDSCapAuth path and fs_name decode in handle_session() Michael Bommarito
2026-07-07 18:05 ` [PATCH v3 3/4] ceph: bound num_export_targets array for mds info v2/v3 Michael Bommarito
2026-07-07 18:06 ` [PATCH v3 4/4] ceph: cap delegated inode count in ceph_parse_deleg_inos() Michael Bommarito
2026-07-07 18:30   ` Viacheslav Dubeyko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox