[PATCH 0/2] ceph: don't let a bad mon_addr= reach the empty-monmap BUG_ON()

[PATCH 0/2] ceph: don't let a bad mon_addr= reach the empty-monmap BUG_ON()

[Yogesh Gaur]

syzbot can panic the kernel from fsconfig(2) by mounting cephfs with a
mon_addr= that does not parse:

  kernel BUG at net/ceph/mon_client.c:211!
  RIP: 0010:pick_new_mon net/ceph/mon_client.c:211 [inline]
  RIP: 0010:__open_session+0x60f/0x740 net/ceph/mon_client.c:245
   ceph_monc_open_session+0x46/0x60 net/ceph/mon_client.c:534
   __ceph_open_session+0x135/0x590 net/ceph/ceph_common.c:798
   ceph_real_mount fs/ceph/super.c:1173 [inline]
   ceph_get_tree+0xd98/0x1fd0 fs/ceph/super.c:1362
   vfs_cmd_create+0xd7/0x2a0 fs/fsopen.c:231
   __do_sys_fsconfig+0x55a/0xcb0 fs/fsopen.c:463

  https://syzkaller.appspot.com/bug?extid=1dbed5969931c19d3eb5

The sequence is fsopen("ceph"), fsconfig(FSCONFIG_SET_STRING, "mon_addr",
<something that does not resolve>) - which returns an error that the
caller simply ignores - and then FSCONFIG_CMD_CREATE. Two things have to
be true for that to end in a BUG():

 1/ ceph_parse_mon_addr() commits the string to fsopt->mon_addr before
    validating it, so the rejected value survives the failed fsconfig()
    and satisfies ceph_get_tree()'s "No monitor address" check.

 2/ ceph_parse_ips() never got as far as writing *count, so num_mon stays
    0, and build_initial_monmap() happily builds a monmap with no
    monitors in it. pick_new_mon() then trips its own invariant.

Patch 1 closes 2/ in libceph, which is what actually stops the BUG().
It is the same rule commit 40480eee361e ("libceph: Reject monmaps
advertising zero monitors") applied to monmaps arriving from the wire,
now applied to the one built locally from mount options. Patch 2 closes
1/ so the bad string is not kept in the first place and the failure is
reported where it happens.

Either patch on its own stops the reported crash; I think both are worth
having, but say the word and I will send just the first.

Compile-tested only: x86_64 defconfig + CONFIG_CEPH_LIB=m +
CONFIG_CEPH_FS=m, net/ceph/mon_client.o and fs/ceph/super.o both W=1
clean, checkpatch --strict clean on both patches. I have no ceph cluster
here and did not run the syzbot reproducer, so this is reasoning from the
code and the report, not an observed fix - please treat it accordingly.

Yogesh Gaur (2):
  libceph: reject an initial monmap with no monitors
  ceph: don't keep a mon_addr= that failed to parse

 fs/ceph/super.c       | 9 +++++++--
 net/ceph/mon_client.c | 3 +++
 2 files changed, 10 insertions(+), 2 deletions(-)

-- 
2.55.0.windows.5

[PATCH 1/2] libceph: reject an initial monmap with no monitors

[Yogesh Gaur]

build_initial_monmap() takes the monitor count straight from
ceph_options::num_mon and builds a monmap with that many entries without
checking that there is at least one. ceph_parse_ips() writes *count only
after it has parsed an address successfully, so a ceph_options that went
through a failed parse still has num_mon == 0 and the resulting monmap
has no monitors in it at all.

The first thing the client does with that monmap is open a session, and
pick_new_mon() asserts the invariant that it is non-empty:

  kernel BUG at net/ceph/mon_client.c:211!
  RIP: 0010:pick_new_mon net/ceph/mon_client.c:211 [inline]
  RIP: 0010:__open_session+0x60f/0x740 net/ceph/mon_client.c:245
  Call Trace:
   ceph_monc_open_session+0x46/0x60 net/ceph/mon_client.c:534
   __ceph_open_session+0x135/0x590 net/ceph/ceph_common.c:798
   ceph_real_mount fs/ceph/super.c:1173 [inline]
   ceph_get_tree+0xd98/0x1fd0 fs/ceph/super.c:1362
   vfs_get_tree+0x92/0x320 fs/super.c:1947
   vfs_cmd_create+0xd7/0x2a0 fs/fsopen.c:231
   __do_sys_fsconfig+0x55a/0xcb0 fs/fsopen.c:463

The reproducer gets there through the new device syntax: an fsconfig()
mon_addr= that does not resolve ("libceph: resolve '127.0.0.' (ret=-3):
failed") leaves fsopt->mon_addr set but num_mon at 0, so the "No monitor
address" check in ceph_get_tree() passes and the mount proceeds with an
empty monmap. The next patch stops that string from being kept, but the
assertion should not be reachable from a mount option in the first place.

Commit 40480eee361e ("libceph: Reject monmaps advertising zero monitors")
made the same BUG_ON() unreachable from the wire by rejecting
num_mon == 0 in ceph_monmap_decode(). Apply the same rule to the initial
monmap, which is the only other way one gets built, and fail the mount
with -EINVAL rather than assert afterwards.

Reported-by: syzbot+1dbed5969931c19d3eb5@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=1dbed5969931c19d3eb5
Fixes: 7b19b4db5add ("ceph: new device mount syntax")
Assisted-by: LLM
Signed-off-by: Yogesh Gaur <yogeshgaur.83@gmail.com>
---
 net/ceph/mon_client.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/ceph/mon_client.c b/net/ceph/mon_client.c
index c56457378d00..bcbcf47506c9 100644
--- a/net/ceph/mon_client.c
+++ b/net/ceph/mon_client.c
@@ -1141,6 +1141,9 @@ static int build_initial_monmap(struct ceph_mon_client *monc)
 	int num_mon = opt->num_mon;
 	int i;
 
+	if (num_mon < 1 || num_mon > CEPH_MAX_MON)
+		return -EINVAL;
+
 	/* build initial monmap */
 	monc->monmap = kzalloc_flex(*monc->monmap, mon_inst, num_mon);
 	if (!monc->monmap)
-- 
2.55.0.windows.5

[PATCH 2/2] ceph: don't keep a mon_addr= that failed to parse

[Yogesh Gaur]

ceph_parse_mon_addr() stores the string in fsopt->mon_addr before handing
it to ceph_parse_mon_ips(), and then returns that function's error. So
after a mon_addr= that does not parse, fsconfig() reports the failure but
the mount context keeps the rejected string.

That matters because ceph_get_tree() uses fsopt->mon_addr as the "monitor
addresses were supplied" test:

	if (fsopt->new_dev_syntax && !fsopt->mon_addr)
		return invalfc(fc, "No monitor address");

Userspace is free to ignore the error from fsconfig(FSCONFIG_SET_STRING)
and go straight to FSCONFIG_CMD_CREATE. The check then passes on a value
that was rejected, and the mount continues with ceph_options::num_mon
still 0 - which is how syzbot reached the BUG_ON() in pick_new_mon()
fixed by the previous patch.

Parse first and commit only on success. A failed mon_addr= now leaves any
previously accepted value in place instead of replacing it with one that
does not parse.

Reported-by: syzbot+1dbed5969931c19d3eb5@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=1dbed5969931c19d3eb5
Fixes: 7b19b4db5add ("ceph: new device mount syntax")
Assisted-by: LLM
Signed-off-by: Yogesh Gaur <yogeshgaur.83@gmail.com>
---
 fs/ceph/super.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index 72935f665f11..9e0aee8f33ff 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -394,13 +394,18 @@ static int ceph_parse_mon_addr(struct fs_parameter *param,
 {
 	struct ceph_parse_opts_ctx *pctx = fc->fs_private;
 	struct ceph_mount_options *fsopt = pctx->opts;
+	int ret;
+
+	ret = ceph_parse_mon_ips(param->string, strlen(param->string),
+				 pctx->copts, fc->log.log, '/');
+	if (ret)
+		return ret;
 
 	kfree(fsopt->mon_addr);
 	fsopt->mon_addr = param->string;
 	param->string = NULL;
 
-	return ceph_parse_mon_ips(fsopt->mon_addr, strlen(fsopt->mon_addr),
-				  pctx->copts, fc->log.log, '/');
+	return 0;
 }
 
 static int ceph_parse_mount_param(struct fs_context *fc,
-- 
2.55.0.windows.5