On Thu, 2025-09-11 at 15:02 +0530, khiremat@xxxxxxxxxx wrote: > From: Kotresh HR <khiremat@xxxxxxxxxx> > > The mds auth caps check should also validate the > fsname along with the associated caps. Not doing > so would result in applying the mds auth caps of > one fs on to the other fs in a multifs ceph cluster. > The bug causes multiple issues w.r.t user > authentication, following is one such example. > > Steps to Reproduce (on vstart cluster): > 1. Create two file systems in a cluster, say 'fsname1' and 'fsname2' > 2. Authorize read only permission to the user 'client.usr' on fs 'fsname1' > $ceph fs authorize fsname1 client.usr / r > 3. Authorize read and write permission to the same user 'client.usr' on fs 'fsname2' > $ceph fs authorize fsname2 client.usr / rw > 4. Update the keyring > $ceph auth get client.usr >> ./keyring > > With above permssions for the user 'client.usr', following is the > expectation. > a. The 'client.usr' should be able to only read the contents > and not allowed to create or delete files on file system 'fsname1'. > b. The 'client.usr' should be able to read/write on file system 'fsname2'. > > But, with this bug, the 'client.usr' is allowed to read/write on file > system 'fsname1'. See below. > > 5. Mount the file system 'fsname1' with the user 'client.usr' > $sudo bin/mount.ceph usr@.fsname1=/ /kmnt_fsname1_usr/ > 6. Try creating a file on file system 'fsname1' with user 'client.usr'. This > should fail but passes with this bug. > $touch /kmnt_fsname1_usr/file1 > 7. Mount the file system 'fsname1' with the user 'client.admin' and create a > file. > $sudo bin/mount.ceph admin@.fsname1=/ /kmnt_fsname1_admin > $echo "data" > /kmnt_fsname1_admin/admin_file1 > 8. Try removing an existing file on file system 'fsname1' with the user > 'client.usr'. This shoudn't succeed but succeeds with the bug. > $rm -f /kmnt_fsname1_usr/admin_file1 > > For more information, please take a look at the corresponding mds/fuse patch > and tests added by looking into the tracker mentioned below. > > v2: Fix a possible null dereference in doutc > v3: Don't store fsname from mdsmap, validate against > ceph_mount_options's fsname and use it > v4: Code refactor, better warning message and > fix possible compiler warning > > URL: https://tracker.ceph.com/issues/72167 > Signed-off-by: Kotresh HR <khiremat@xxxxxxxxxx> > --- > fs/ceph/mds_client.c | 8 ++++++++ > fs/ceph/mdsmap.c | 13 ++++++++++++- > fs/ceph/super.c | 14 -------------- > fs/ceph/super.h | 14 ++++++++++++++ > 4 files changed, 34 insertions(+), 15 deletions(-) > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c > index 179130948041..97f9de888713 100644 > --- a/fs/ceph/mds_client.c > +++ b/fs/ceph/mds_client.c > @@ -5689,11 +5689,19 @@ static int ceph_mds_auth_match(struct ceph_mds_client *mdsc, > u32 caller_uid = from_kuid(&init_user_ns, cred->fsuid); > u32 caller_gid = from_kgid(&init_user_ns, cred->fsgid); > struct ceph_client *cl = mdsc->fsc->client; > + const char *fs_name = mdsc->fsc->mount_options->mds_namespace; > const char *spath = mdsc->fsc->mount_options->server_path; > bool gid_matched = false; > u32 gid, tlen, len; > int i, j; > > + doutc(cl, "fsname check fs_name=%s match.fs_name=%s\n", > + fs_name, auth->match.fs_name ? auth->match.fs_name : ""); > + if (auth->match.fs_name && strcmp(auth->match.fs_name, fs_name)) { > + /* fsname check failed, try next one */ I am still thinking that "check failed" sounds too strong here. Names simply don't match to each others and we need to check next one. But "check failed" sounds like error condition but not the normal flow. Potentially, I used the word "fail" too frequently for commenting the error conditions. :) > + return 0; > + } > + > doutc(cl, "match.uid %lld\n", auth->match.uid); > if (auth->match.uid != MDS_AUTH_UID_ANY) { > if (auth->match.uid != caller_uid) > diff --git a/fs/ceph/mdsmap.c b/fs/ceph/mdsmap.c > index 8109aba66e02..de457470d07c 100644 > --- a/fs/ceph/mdsmap.c > +++ b/fs/ceph/mdsmap.c > @@ -353,10 +353,21 @@ struct ceph_mdsmap *ceph_mdsmap_decode(struct ceph_mds_client *mdsc, void **p, > __decode_and_drop_type(p, end, u8, bad_ext); > } > if (mdsmap_ev >= 8) { > + u32 fsname_len; > /* enabled */ > ceph_decode_8_safe(p, end, m->m_enabled, bad_ext); > /* fs_name */ > - ceph_decode_skip_string(p, end, bad_ext); > + ceph_decode_32_safe(p, end, fsname_len, bad_ext); > + > + /* validate fsname against mds_namespace */ > + if (!namespace_equals(mdsc->fsc->mount_options, (const char *)*p, fsname_len)) { > + pr_warn_client(cl, "fsname %*pE doesn't match mds_namespace %s\n", > + (int)fsname_len, (char *)*p, > + mdsc->fsc->mount_options->mds_namespace); > + goto bad; > + } > + // skip fsname after validation > + ceph_decode_skip_n(p, end, fsname_len, bad); > } > /* damaged */ > if (mdsmap_ev >= 9) { > diff --git a/fs/ceph/super.c b/fs/ceph/super.c > index 0e6787501b2f..2c6c45b2056d 100644 > --- a/fs/ceph/super.c > +++ b/fs/ceph/super.c > @@ -246,20 +246,6 @@ static void canonicalize_path(char *path) > path[j] = '\0'; > } > > -/* > - * Check if the mds namespace in ceph_mount_options matches > - * the passed in namespace string. First time match (when > - * ->mds_namespace is NULL) is treated specially, since > - * ->mds_namespace needs to be initialized by the caller. > - */ > -static int namespace_equals(struct ceph_mount_options *fsopt, > - const char *namespace, size_t len) > -{ > - return !(fsopt->mds_namespace && > - (strlen(fsopt->mds_namespace) != len || > - strncmp(fsopt->mds_namespace, namespace, len))); > -} > - > static int ceph_parse_old_source(const char *dev_name, const char *dev_name_end, > struct fs_context *fc) > { > diff --git a/fs/ceph/super.h b/fs/ceph/super.h > index 8cba1ce3b8b0..bb992f12e3b0 100644 > --- a/fs/ceph/super.h > +++ b/fs/ceph/super.h > @@ -104,6 +104,20 @@ struct ceph_mount_options { > struct fscrypt_dummy_policy dummy_enc_policy; > }; > > +/* > + * Check if the mds namespace in ceph_mount_options matches > + * the passed in namespace string. First time match (when > + * ->mds_namespace is NULL) is treated specially, since > + * ->mds_namespace needs to be initialized by the caller. > + */ > +static inline int namespace_equals(struct ceph_mount_options *fsopt, > + const char *namespace, size_t len) > +{ > + return !(fsopt->mds_namespace && > + (strlen(fsopt->mds_namespace) != len || > + strncmp(fsopt->mds_namespace, namespace, len))); > +} > + > /* mount state */ > enum { > CEPH_MOUNT_MOUNTING, Looks good. Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@xxxxxxx> Let me run xfstests on your patch. I'll be back with the results ASAP. Thanks, Slava.
