[RFC PATCH v3 0/4] ceph: add support for snapshot names encryption

[RFC PATCH v3 0/4] ceph: add support for snapshot names encryption

[Luís Henriques]

Hi!

Here's another iteration on my encrypted snapshot names series.  And
here's the changes since v1:

- Use ceph_find_inode() instead of ceph_get_inode() for finding a snapshot
  parent in function parse_longname().  I've also added a fallback to
  ceph_get_inode() in case we fail to find the inode.  This may happen if,
  for example, the mount root doesn't include that inode.  The iput() was
  also complemented by a discard_new_inode() if the inode is in the I_NEW
  state. (patch 0002)

- Move the check for '_' snapshots further up in the ceph_fname_to_usr()
  and ceph_encode_encrypted_dname().  This fixes the case pointed out by
  Xiubo in v2. (patch 0002)

- Use NAME_MAX for tmp arrays (patch 0002)

- Added an extra patch for replacing the base64url encoding by a different
  encoding standard, the one used for IMAP mailboxes (which uses '+' and
  ',' instead of '-' and '_').  This should fix the issue with snapshot
  names starting with '_'. (patch 0003)

Regarding this last patch, there are other alternatives:

1. Simply replace any initial '_' in snapshot names by another character
   (I was using the '=').  This was a bit more hacky because this name
   could never be sent as-is to the base64 functions, so the '=' would
   need to be replace back by an '_'.

2. Append an extra (known) char to every snapshot name.  This would also
   be hacky because it would need to be removed again for base64
   operations.  And the snapshot name size limitations would need to be
   adjusted too.

3. Modify the fscrypt base64 encoding/decoding functions to receive an
   alternative table to use in these operations.  This would need to be
   accepted by the fscrypt maintainers, of course.

As before, in order to test this code the following PRs are required:

  mds: add protection from clients without fscrypt support #45073
  mds: use the whole string as the snapshot long name #45192
  mds: support alternate names for snapshots #45224
  mds: limit the snapshot names to 240 characters #45312

Changes since v1:

- Dropped the dentry->d_flags change in ceph_mkdir().  Thanks to Xiubo
  suggestion, patch 0001 now skips calling ceph_fscrypt_prepare_context()
  if we're handling a snapshot.

- Added error handling to ceph_get_snapdir() in patch 0001 (Jeff had
  already pointed that out but I forgot to include that change in previous
  revision).

- Rebased patch 0002 to the latest wip-fscrypt branch.

- Added some documentation regarding snapshots naming restrictions.

Luís Henriques (4):
  ceph: add support for encrypted snapshot names
  ceph: handle encrypted snapshot names in subdirectories
  ceph: update documentation regarding snapshot naming limitations
  ceph: replace base64url by the encoding used for mailbox names

 Documentation/filesystems/ceph.rst |  10 ++
 fs/ceph/crypto.c                   | 238 +++++++++++++++++++++++++----
 fs/ceph/crypto.h                   |  14 +-
 fs/ceph/dir.c                      |   2 +-
 fs/ceph/inode.c                    |  33 +++-
 5 files changed, 259 insertions(+), 38 deletions(-)

[RFC PATCH v3 1/4] ceph: add support for encrypted snapshot names

[Luís Henriques]

Since filenames in encrypted directories are already encrypted and shown
as a base64-encoded string when the directory is locked, snapshot names
should show a similar behaviour.

Signed-off-by: Luís Henriques <lhenriques@suse.de>
---
 fs/ceph/inode.c | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 7b670e2405c1..359e29896f16 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -91,9 +91,15 @@ struct inode *ceph_new_inode(struct inode *dir, struct dentry *dentry,
 	if (err < 0)
 		goto out_err;
 
-	err = ceph_fscrypt_prepare_context(dir, inode, as_ctx);
-	if (err)
-		goto out_err;
+	/*
+	 * We'll skip setting fscrypt context for snapshots, leaving that for
+	 * the handle_reply().
+	 */
+	if (ceph_snap(dir) != CEPH_SNAPDIR) {
+		err = ceph_fscrypt_prepare_context(dir, inode, as_ctx);
+		if (err)
+			goto out_err;
+	}
 
 	return inode;
 out_err:
@@ -157,6 +163,7 @@ struct inode *ceph_get_snapdir(struct inode *parent)
 	};
 	struct inode *inode = ceph_get_inode(parent->i_sb, vino, NULL);
 	struct ceph_inode_info *ci = ceph_inode(inode);
+	int ret = -ENOTDIR;
 
 	if (IS_ERR(inode))
 		return inode;
@@ -182,6 +189,22 @@ struct inode *ceph_get_snapdir(struct inode *parent)
 	ci->i_rbytes = 0;
 	ci->i_btime = ceph_inode(parent)->i_btime;
 
+	/* if encrypted, just borrow fscrypt_auth from parent */
+	if (IS_ENCRYPTED(parent)) {
+		struct ceph_inode_info *pci = ceph_inode(parent);
+
+		ci->fscrypt_auth = kmemdup(pci->fscrypt_auth,
+					   pci->fscrypt_auth_len,
+					   GFP_KERNEL);
+		if (ci->fscrypt_auth) {
+			inode->i_flags |= S_ENCRYPTED;
+			ci->fscrypt_auth_len = pci->fscrypt_auth_len;
+		} else {
+			dout("Failed to alloc snapdir fscrypt_auth\n");
+			ret = -ENOMEM;
+			goto err;
+		}
+	}
 	if (inode->i_state & I_NEW) {
 		inode->i_op = &ceph_snapdir_iops;
 		inode->i_fop = &ceph_snapdir_fops;
@@ -195,7 +218,7 @@ struct inode *ceph_get_snapdir(struct inode *parent)
 		discard_new_inode(inode);
 	else
 		iput(inode);
-	return ERR_PTR(-ENOTDIR);
+	return ERR_PTR(ret);
 }
 
 const struct inode_operations ceph_file_iops = {

[RFC PATCH v3 2/4] ceph: handle encrypted snapshot names in subdirectories

[Luís Henriques]

When creating a snapshot, the .snap directories for every subdirectory will
show the snapshot name in the "long format":

  # mkdir .snap/my-snap
  # ls my-dir/.snap/
  _my-snap_1099511627782

Encrypted snapshots will need to be able to handle these snapshot names by
encrypting/decrypting only the snapshot part of the string ('my-snap').

Also, since the MDS prevents snapshot names to be bigger than 240 characters
it is necessary to adapt CEPH_NOHASH_NAME_MAX to accommodate this extra
limitation.

Signed-off-by: Luís Henriques <lhenriques@suse.de>
---
 fs/ceph/crypto.c | 189 ++++++++++++++++++++++++++++++++++++++++-------
 fs/ceph/crypto.h |  11 ++-
 2 files changed, 169 insertions(+), 31 deletions(-)

diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
index beb73bbdd868..caa9863dee93 100644
--- a/fs/ceph/crypto.c
+++ b/fs/ceph/crypto.c
@@ -128,16 +128,100 @@ void ceph_fscrypt_as_ctx_to_req(struct ceph_mds_request *req, struct ceph_acl_se
 	swap(req->r_fscrypt_auth, as->fscrypt_auth);
 }
 
-int ceph_encode_encrypted_dname(const struct inode *parent, struct qstr *d_name, char *buf)
+/*
+ * User-created snapshots can't start with '_'.  Snapshots that start with this
+ * character are special (hint: there aren't real snapshots) and use the
+ * following format:
+ *
+ *   _<SNAPSHOT-NAME>_<INODE-NUMBER>
+ *
+ * where:
+ *  - <SNAPSHOT-NAME> - the real snapshot name that may need to be decrypted,
+ *  - <INODE-NUMBER> - the inode number for the actual snapshot
+ *
+ * This function parses these snapshot names and returns the inode
+ * <INODE-NUMBER>.  'name_len' will also bet set with the <SNAPSHOT-NAME>
+ * length.
+ */
+static struct inode *parse_longname(const struct inode *parent, const char *name,
+				    int *name_len)
 {
+	struct inode *dir = NULL;
+	struct ceph_vino vino = { .snap = CEPH_NOSNAP };
+	char *inode_number;
+	char *name_end;
+	int orig_len = *name_len;
+	int ret = -EIO;
+
+	/* Skip initial '_' */
+	name++;
+	name_end = strrchr(name, '_');
+	if (!name_end) {
+		dout("Failed to parse long snapshot name: %s\n", name);
+		return ERR_PTR(-EIO);
+	}
+	*name_len = (name_end - name);
+	if (*name_len <= 0) {
+		pr_err("Failed to parse long snapshot name\n");
+		return ERR_PTR(-EIO);
+	}
+
+	/* Get the inode number */
+	inode_number = kmemdup_nul(name_end + 1,
+				   orig_len - *name_len - 2,
+				   GFP_KERNEL);
+	if (!inode_number)
+		return ERR_PTR(-ENOMEM);
+	ret = kstrtou64(inode_number, 0, &vino.ino);
+	if (ret) {
+		dout("Failed to parse inode number: %s\n", name);
+		dir = ERR_PTR(ret);
+		goto out;
+	}
+
+	/* And finally the inode */
+	dir = ceph_find_inode(parent->i_sb, vino);
+	if (!dir) {
+		/* This can happen if we're not mounting cephfs on the root */
+		dir = ceph_get_inode(parent->i_sb, vino, NULL);
+		if (!dir)
+			dir = ERR_PTR(-ENOENT);
+	}
+	if (IS_ERR(dir))
+		dout("Can't find inode %s (%s)\n", inode_number, name);
+
+out:
+	kfree(inode_number);
+	return dir;
+}
+
+int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name, char *buf)
+{
+	struct inode *dir = parent;
+	struct qstr iname;
 	u32 len;
+	int name_len;
 	int elen;
 	int ret;
-	u8 *cryptbuf;
+	u8 *cryptbuf = NULL;
+
+	iname.name = d_name->name;
+	name_len = d_name->len;
+
+	/* Handle the special case of snapshot names that start with '_' */
+	if ((ceph_snap(dir) == CEPH_SNAPDIR) && (name_len > 0) &&
+	    (iname.name[0] == '_')) {
+		dir = parse_longname(parent, iname.name, &name_len);
+		if (IS_ERR(dir))
+			return PTR_ERR(dir);
+		iname.name++; /* skip initial '_' */
+	}
+	iname.len = name_len;
 
-	if (!fscrypt_has_encryption_key(parent)) {
+	if (!fscrypt_has_encryption_key(dir)) {
 		memcpy(buf, d_name->name, d_name->len);
-		return d_name->len;
+		elen = d_name->len;
+		goto out;
 	}
 
 	/*
@@ -146,18 +230,22 @@ int ceph_encode_encrypted_dname(const struct inode *parent, struct qstr *d_name,
 	 *
 	 * See: fscrypt_setup_filename
 	 */
-	if (!fscrypt_fname_encrypted_size(parent, d_name->len, NAME_MAX, &len))
-		return -ENAMETOOLONG;
+	if (!fscrypt_fname_encrypted_size(dir, iname.len, NAME_MAX, &len)) {
+		elen = -ENAMETOOLONG;
+		goto out;
+	}
 
 	/* Allocate a buffer appropriate to hold the result */
 	cryptbuf = kmalloc(len > CEPH_NOHASH_NAME_MAX ? NAME_MAX : len, GFP_KERNEL);
-	if (!cryptbuf)
-		return -ENOMEM;
+	if (!cryptbuf) {
+		elen = -ENOMEM;
+		goto out;
+	}
 
-	ret = fscrypt_fname_encrypt(parent, d_name, cryptbuf, len);
+	ret = fscrypt_fname_encrypt(dir, &iname, cryptbuf, len);
 	if (ret) {
-		kfree(cryptbuf);
-		return ret;
+		elen = ret;
+		goto out;
 	}
 
 	/* hash the end if the name is long enough */
@@ -173,12 +261,29 @@ int ceph_encode_encrypted_dname(const struct inode *parent, struct qstr *d_name,
 
 	/* base64 encode the encrypted name */
 	elen = fscrypt_base64url_encode(cryptbuf, len, buf);
-	kfree(cryptbuf);
 	dout("base64-encoded ciphertext name = %.*s\n", elen, buf);
+
+	WARN_ON(elen > (CEPH_NOHASH_NAME_MAX + SHA256_DIGEST_SIZE));
+	if ((elen > 0) && (dir != parent)) {
+		char tmp_buf[NAME_MAX];
+
+		elen = snprintf(tmp_buf, sizeof(tmp_buf), "_%.*s_%ld",
+				elen, buf, dir->i_ino);
+		memcpy(buf, tmp_buf, elen);
+	}
+
+out:
+	kfree(cryptbuf);
+	if (dir != parent) {
+		if ((dir->i_state & I_NEW))
+			discard_new_inode(dir);
+		else
+			iput(dir);
+	}
 	return elen;
 }
 
-int ceph_encode_encrypted_fname(const struct inode *parent, struct dentry *dentry, char *buf)
+int ceph_encode_encrypted_fname(struct inode *parent, struct dentry *dentry, char *buf)
 {
 	WARN_ON_ONCE(!fscrypt_has_encryption_key(parent));
 
@@ -203,29 +308,42 @@ int ceph_encode_encrypted_fname(const struct inode *parent, struct dentry *dentr
 int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
 		      struct fscrypt_str *oname, bool *is_nokey)
 {
-	int ret;
+	struct inode *dir = fname->dir;
 	struct fscrypt_str _tname = FSTR_INIT(NULL, 0);
 	struct fscrypt_str iname;
-
-	if (!IS_ENCRYPTED(fname->dir)) {
-		oname->name = fname->name;
-		oname->len = fname->name_len;
-		return 0;
-	}
+	char *name = fname->name;
+	int name_len = fname->name_len;
+	int ret;
 
 	/* Sanity check that the resulting name will fit in the buffer */
 	if (fname->name_len > NAME_MAX || fname->ctext_len > NAME_MAX)
 		return -EIO;
 
-	ret = __fscrypt_prepare_readdir(fname->dir);
+	/* Handle the special case of snapshot names that start with '_' */
+	if ((ceph_snap(dir) == CEPH_SNAPDIR) && (name_len > 0) &&
+	    (name[0] == '_')) {
+		dir = parse_longname(dir, name, &name_len);
+		if (IS_ERR(dir))
+			return PTR_ERR(dir);
+		name++; /* skip initial '_' */
+	}
+
+	if (!IS_ENCRYPTED(dir)) {
+		oname->name = fname->name;
+		oname->len = fname->name_len;
+		ret = 0;
+		goto out_inode;
+	}
+
+	ret = __fscrypt_prepare_readdir(dir);
 	if (ret)
-		return ret;
+		goto out_inode;
 
 	/*
 	 * Use the raw dentry name as sent by the MDS instead of
 	 * generating a nokey name via fscrypt.
 	 */
-	if (!fscrypt_has_encryption_key(fname->dir)) {
+	if (!fscrypt_has_encryption_key(dir)) {
 		if (fname->no_copy)
 			oname->name = fname->name;
 		else
@@ -233,7 +351,8 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
 		oname->len = fname->name_len;
 		if (is_nokey)
 			*is_nokey = true;
-		return 0;
+		ret = 0;
+		goto out_inode;
 	}
 
 	if (fname->ctext_len == 0) {
@@ -242,11 +361,11 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
 		if (!tname) {
 			ret = fscrypt_fname_alloc_buffer(NAME_MAX, &_tname);
 			if (ret)
-				return ret;
+				goto out_inode;
 			tname = &_tname;
 		}
 
-		declen = fscrypt_base64url_decode(fname->name, fname->name_len, tname->name);
+		declen = fscrypt_base64url_decode(name, name_len, tname->name);
 		if (declen <= 0) {
 			ret = -EIO;
 			goto out;
@@ -258,9 +377,25 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
 		iname.len = fname->ctext_len;
 	}
 
-	ret = fscrypt_fname_disk_to_usr(fname->dir, 0, 0, &iname, oname);
+	ret = fscrypt_fname_disk_to_usr(dir, 0, 0, &iname, oname);
+	if (!ret && (dir != fname->dir)) {
+		char tmp_buf[FSCRYPT_BASE64URL_CHARS(NAME_MAX)];
+
+		name_len = snprintf(tmp_buf, sizeof(tmp_buf), "_%.*s_%ld",
+				    oname->len, oname->name, dir->i_ino);
+		memcpy(oname->name, tmp_buf, name_len);
+		oname->len = name_len;
+	}
+
 out:
 	fscrypt_fname_free_buffer(&_tname);
+out_inode:
+	if ((dir != fname->dir) && !IS_ERR(dir)) {
+		if ((dir->i_state & I_NEW))
+			discard_new_inode(dir);
+		else
+			iput(dir);
+	}
 	return ret;
 }
 
diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h
index 62f0ddd30dee..3273d076a9e5 100644
--- a/fs/ceph/crypto.h
+++ b/fs/ceph/crypto.h
@@ -82,13 +82,16 @@ static inline u32 ceph_fscrypt_auth_len(struct ceph_fscrypt_auth *fa)
  * struct fscrypt_ceph_nokey_name {
  *	u8 bytes[157];
  *	u8 sha256[SHA256_DIGEST_SIZE];
- * }; // 189 bytes => 252 bytes base64-encoded, which is <= NAME_MAX (255)
+ * }; // 180 bytes => 240 bytes base64-encoded, which is <= NAME_MAX (255)
+ *
+ * (240 bytes is the maximum size allowed for snapshot names to take into
+ *  account the format: '_<SNAPSHOT-NAME>_<INODE-NUMBER>'.)
  *
  * Note that for long names that end up having their tail portion hashed, we
  * must also store the full encrypted name (in the dentry's alternate_name
  * field).
  */
-#define CEPH_NOHASH_NAME_MAX (189 - SHA256_DIGEST_SIZE)
+#define CEPH_NOHASH_NAME_MAX (180 - SHA256_DIGEST_SIZE)
 
 void ceph_fscrypt_set_ops(struct super_block *sb);
 
@@ -97,8 +100,8 @@ void ceph_fscrypt_free_dummy_policy(struct ceph_fs_client *fsc);
 int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *inode,
 				 struct ceph_acl_sec_ctx *as);
 void ceph_fscrypt_as_ctx_to_req(struct ceph_mds_request *req, struct ceph_acl_sec_ctx *as);
-int ceph_encode_encrypted_dname(const struct inode *parent, struct qstr *d_name, char *buf);
-int ceph_encode_encrypted_fname(const struct inode *parent, struct dentry *dentry, char *buf);
+int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name, char *buf);
+int ceph_encode_encrypted_fname(struct inode *parent, struct dentry *dentry, char *buf);
 
 static inline int ceph_fname_alloc_buffer(struct inode *parent, struct fscrypt_str *fname)
 {

[RFC PATCH v3 3/4] ceph: update documentation regarding snapshot naming limitations

[Luís Henriques]

Signed-off-by: Luís Henriques <lhenriques@suse.de>
---
 Documentation/filesystems/ceph.rst | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/filesystems/ceph.rst b/Documentation/filesystems/ceph.rst
index 4942e018db85..d487cabe792d 100644
--- a/Documentation/filesystems/ceph.rst
+++ b/Documentation/filesystems/ceph.rst
@@ -57,6 +57,16 @@ a snapshot on any subdirectory (and its nested contents) in the
 system.  Snapshot creation and deletion are as simple as 'mkdir
 .snap/foo' and 'rmdir .snap/foo'.
 
+Snapshot names have two limitations:
+
+* They can not start with an underscore ('_'), as these names are reserved
+  for internal usage by the MDS.
+* They can not exceed 240 characters in size.  This is because the MDS makes
+  use of long snapshot names internally, which follow the format:
+  `_<SNAPSHOT-NAME>_<INODE-NUMBER>`.  Since filenames in general can't have
+  more than 255 characters, and `<node-id>` takes 13 characters, the long
+  snapshot names can take as much as 255 - 1 - 1 - 13 = 240.
+
 Ceph also provides some recursive accounting on directories for nested
 files and bytes.  That is, a 'getfattr -d foo' on any directory in the
 system will reveal the total number of nested regular files and

[RFC PATCH v3 4/4] ceph: replace base64url by the encoding used for mailbox names

[Luís Henriques]

The base64url encoding includes the '_' character, which may cause problems
in snapshot names (if the name starts with '_').  Thus, use the base64
encoding defined for IMAP mailbox names (RFC 3501), which uses '+' and ','
instead of '-' and '_'.

Signed-off-by: Luís Henriques <lhenriques@suse.de>
---
 fs/ceph/crypto.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++--
 fs/ceph/crypto.h |  3 +++
 fs/ceph/dir.c    |  2 +-
 fs/ceph/inode.c  |  2 +-
 4 files changed, 54 insertions(+), 4 deletions(-)

diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
index caa9863dee93..d6f1c444ce91 100644
--- a/fs/ceph/crypto.c
+++ b/fs/ceph/crypto.c
@@ -7,6 +7,53 @@
 #include "mds_client.h"
 #include "crypto.h"
 
+static const char base64_table[65] =
+        "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+,";
+
+int ceph_base64_encode(const u8 *src, int srclen, char *dst)
+{
+	u32 ac = 0;
+	int bits = 0;
+	int i;
+	char *cp = dst;
+
+	for (i = 0; i < srclen; i++) {
+		ac = (ac << 8) | src[i];
+		bits += 8;
+		do {
+			bits -= 6;
+			*cp++ = base64_table[(ac >> bits) & 0x3f];
+		} while (bits >= 6);
+	}
+	if (bits)
+		*cp++ = base64_table[(ac << (6 - bits)) & 0x3f];
+	return cp - dst;
+}
+
+int ceph_base64_decode(const char *src, int srclen, u8 *dst)
+{
+	u32 ac = 0;
+	int bits = 0;
+	int i;
+	u8 *bp = dst;
+
+	for (i = 0; i < srclen; i++) {
+		const char *p = strchr(base64_table, src[i]);
+
+		if (p == NULL || src[i] == 0)
+			return -1;
+		ac = (ac << 6) | (p - base64_table);
+		bits += 6;
+		if (bits >= 8) {
+			bits -= 8;
+			*bp++ = (u8)(ac >> bits);
+		}
+	}
+	if (ac & ((1 << bits) - 1))
+		return -1;
+	return bp - dst;
+}
+
 static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
 {
 	struct ceph_inode_info *ci = ceph_inode(inode);
@@ -260,7 +307,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name, char
 	}
 
 	/* base64 encode the encrypted name */
-	elen = fscrypt_base64url_encode(cryptbuf, len, buf);
+	elen = ceph_base64_encode(cryptbuf, len, buf);
 	dout("base64-encoded ciphertext name = %.*s\n", elen, buf);
 
 	WARN_ON(elen > (CEPH_NOHASH_NAME_MAX + SHA256_DIGEST_SIZE));
@@ -365,7 +412,7 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
 			tname = &_tname;
 		}
 
-		declen = fscrypt_base64url_decode(name, name_len, tname->name);
+		declen = ceph_base64_decode(name, name_len, tname->name);
 		if (declen <= 0) {
 			ret = -EIO;
 			goto out;
diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h
index 3273d076a9e5..d22316011810 100644
--- a/fs/ceph/crypto.h
+++ b/fs/ceph/crypto.h
@@ -93,6 +93,9 @@ static inline u32 ceph_fscrypt_auth_len(struct ceph_fscrypt_auth *fa)
  */
 #define CEPH_NOHASH_NAME_MAX (180 - SHA256_DIGEST_SIZE)
 
+int ceph_base64_encode(const u8 *src, int srclen, char *dst);
+int ceph_base64_decode(const char *src, int srclen, u8 *dst);
+
 void ceph_fscrypt_set_ops(struct super_block *sb);
 
 void ceph_fscrypt_free_dummy_policy(struct ceph_fs_client *fsc);
diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 5ae5cb778389..417d8c3a7edd 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -960,7 +960,7 @@ static int prep_encrypted_symlink_target(struct ceph_mds_request *req, const cha
 		goto out;
 	}
 
-	len = fscrypt_base64url_encode(osd_link.name, osd_link.len, req->r_path2);
+	len = ceph_base64_encode(osd_link.name, osd_link.len, req->r_path2);
 	req->r_path2[len] = '\0';
 out:
 	fscrypt_fname_free_buffer(&osd_link);
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 359e29896f16..8fd493257e0b 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -875,7 +875,7 @@ static int decode_encrypted_symlink(const char *encsym, int enclen, u8 **decsym)
 	if (!sym)
 		return -ENOMEM;
 
-	declen = fscrypt_base64url_decode(encsym, enclen, sym);
+	declen = ceph_base64_decode(encsym, enclen, sym);
 	if (declen < 0) {
 		pr_err("%s: can't decode symlink (%d). Content: %.*s\n", __func__, declen, enclen, encsym);
 		kfree(sym);

Re: [RFC PATCH v3 2/4] ceph: handle encrypted snapshot names in subdirectories

[Xiubo Li]

On 3/17/22 11:45 PM, Luís Henriques wrote:
> When creating a snapshot, the .snap directories for every subdirectory will
> show the snapshot name in the "long format":
>
>    # mkdir .snap/my-snap
>    # ls my-dir/.snap/
>    _my-snap_1099511627782
>
> Encrypted snapshots will need to be able to handle these snapshot names by
> encrypting/decrypting only the snapshot part of the string ('my-snap').
>
> Also, since the MDS prevents snapshot names to be bigger than 240 characters
> it is necessary to adapt CEPH_NOHASH_NAME_MAX to accommodate this extra
> limitation.
>
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
>   fs/ceph/crypto.c | 189 ++++++++++++++++++++++++++++++++++++++++-------
>   fs/ceph/crypto.h |  11 ++-
>   2 files changed, 169 insertions(+), 31 deletions(-)
>
> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> index beb73bbdd868..caa9863dee93 100644
> --- a/fs/ceph/crypto.c
> +++ b/fs/ceph/crypto.c
> @@ -128,16 +128,100 @@ void ceph_fscrypt_as_ctx_to_req(struct ceph_mds_request *req, struct ceph_acl_se
>   	swap(req->r_fscrypt_auth, as->fscrypt_auth);
>   }
>   
> -int ceph_encode_encrypted_dname(const struct inode *parent, struct qstr *d_name, char *buf)
> +/*
> + * User-created snapshots can't start with '_'.  Snapshots that start with this
> + * character are special (hint: there aren't real snapshots) and use the
> + * following format:
> + *
> + *   _<SNAPSHOT-NAME>_<INODE-NUMBER>
> + *
> + * where:
> + *  - <SNAPSHOT-NAME> - the real snapshot name that may need to be decrypted,
> + *  - <INODE-NUMBER> - the inode number for the actual snapshot
> + *
> + * This function parses these snapshot names and returns the inode
> + * <INODE-NUMBER>.  'name_len' will also bet set with the <SNAPSHOT-NAME>
> + * length.
> + */
> +static struct inode *parse_longname(const struct inode *parent, const char *name,
> +				    int *name_len)
>   {
> +	struct inode *dir = NULL;
> +	struct ceph_vino vino = { .snap = CEPH_NOSNAP };
> +	char *inode_number;
> +	char *name_end;
> +	int orig_len = *name_len;
> +	int ret = -EIO;
> +
> +	/* Skip initial '_' */
> +	name++;
> +	name_end = strrchr(name, '_');
> +	if (!name_end) {
> +		dout("Failed to parse long snapshot name: %s\n", name);
> +		return ERR_PTR(-EIO);
> +	}
> +	*name_len = (name_end - name);
> +	if (*name_len <= 0) {
> +		pr_err("Failed to parse long snapshot name\n");
> +		return ERR_PTR(-EIO);
> +	}
> +
> +	/* Get the inode number */
> +	inode_number = kmemdup_nul(name_end + 1,
> +				   orig_len - *name_len - 2,
> +				   GFP_KERNEL);
> +	if (!inode_number)
> +		return ERR_PTR(-ENOMEM);
> +	ret = kstrtou64(inode_number, 0, &vino.ino);
> +	if (ret) {
> +		dout("Failed to parse inode number: %s\n", name);
> +		dir = ERR_PTR(ret);
> +		goto out;
> +	}
> +
> +	/* And finally the inode */
> +	dir = ceph_find_inode(parent->i_sb, vino);
> +	if (!dir) {
> +		/* This can happen if we're not mounting cephfs on the root */
> +		dir = ceph_get_inode(parent->i_sb, vino, NULL);

In this case IMO you should lookup the inode from MDS instead create it 
in the cache, which won't setup the encryption info needed.

So later when you try to use this to dencrypt the snapshot names, you 
will hit errors ? And also the case Jeff mentioned in previous thread 
could happen.

I figured out another approach could resolve this more gracefully:

For all the subdirs just let them inherit the encryption info from the 
same ancestor, which is initially encrypted, then in ceph_new_inode() 
you can just skip setting up the encryption info for all the subdirs and 
in MDS side will send back the parent's encryption info and fill it in 
handle_reply(), this is just what the .snap does.

Then here you can use current inode to do the dencryption for all the 
snapshots including the long snapshot names.

I have raise one PR and send a kclient patch for the above basic 
framework [1][2]. But there still need a little more work you need to do 
based them:

When lssnap you need to add one flag in LeaseStat to tell the kclient 
whether the long snap names are encrypted, this is very easy in MDS 
side. Then in kclient side you can just skip dencrypting the long snap 
names which are from none-encyrpted parents and for all the other just 
use current inode to do the dencryption. No need to search the parent 
inodes for long snaps.

And when lookuping a long snap name, which could be encyrpted and could 
be not, then you need to parse the inode out and lookup the inode from 
MDS if it does not exist in cache.


[1] https://github.com/ceph/ceph/pull/45516

[2] https://patchwork.kernel.org/project/ceph-devel/list/?series=624492


> +		if (!dir)
> +			dir = ERR_PTR(-ENOENT);
> +	}
> +	if (IS_ERR(dir))
> +		dout("Can't find inode %s (%s)\n", inode_number, name);
> +
> +out:
> +	kfree(inode_number);
> +	return dir;
> +}
> +
> +int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name, char *buf)
> +{
> +	struct inode *dir = parent;
> +	struct qstr iname;
>   	u32 len;
> +	int name_len;
>   	int elen;
>   	int ret;
> -	u8 *cryptbuf;
> +	u8 *cryptbuf = NULL;
> +
> +	iname.name = d_name->name;
> +	name_len = d_name->len;
> +
> +	/* Handle the special case of snapshot names that start with '_' */
> +	if ((ceph_snap(dir) == CEPH_SNAPDIR) && (name_len > 0) &&
> +	    (iname.name[0] == '_')) {
> +		dir = parse_longname(parent, iname.name, &name_len);
> +		if (IS_ERR(dir))
> +			return PTR_ERR(dir);
> +		iname.name++; /* skip initial '_' */
> +	}
> +	iname.len = name_len;
>   
> -	if (!fscrypt_has_encryption_key(parent)) {
> +	if (!fscrypt_has_encryption_key(dir)) {
>   		memcpy(buf, d_name->name, d_name->len);
> -		return d_name->len;
> +		elen = d_name->len;
> +		goto out;
>   	}
>   
>   	/*
> @@ -146,18 +230,22 @@ int ceph_encode_encrypted_dname(const struct inode *parent, struct qstr *d_name,
>   	 *
>   	 * See: fscrypt_setup_filename
>   	 */
> -	if (!fscrypt_fname_encrypted_size(parent, d_name->len, NAME_MAX, &len))
> -		return -ENAMETOOLONG;
> +	if (!fscrypt_fname_encrypted_size(dir, iname.len, NAME_MAX, &len)) {
> +		elen = -ENAMETOOLONG;
> +		goto out;
> +	}
>   
>   	/* Allocate a buffer appropriate to hold the result */
>   	cryptbuf = kmalloc(len > CEPH_NOHASH_NAME_MAX ? NAME_MAX : len, GFP_KERNEL);
> -	if (!cryptbuf)
> -		return -ENOMEM;
> +	if (!cryptbuf) {
> +		elen = -ENOMEM;
> +		goto out;
> +	}
>   
> -	ret = fscrypt_fname_encrypt(parent, d_name, cryptbuf, len);
> +	ret = fscrypt_fname_encrypt(dir, &iname, cryptbuf, len);
>   	if (ret) {
> -		kfree(cryptbuf);
> -		return ret;
> +		elen = ret;
> +		goto out;
>   	}
>   
>   	/* hash the end if the name is long enough */
> @@ -173,12 +261,29 @@ int ceph_encode_encrypted_dname(const struct inode *parent, struct qstr *d_name,
>   
>   	/* base64 encode the encrypted name */
>   	elen = fscrypt_base64url_encode(cryptbuf, len, buf);
> -	kfree(cryptbuf);
>   	dout("base64-encoded ciphertext name = %.*s\n", elen, buf);
> +
> +	WARN_ON(elen > (CEPH_NOHASH_NAME_MAX + SHA256_DIGEST_SIZE));
> +	if ((elen > 0) && (dir != parent)) {
> +		char tmp_buf[NAME_MAX];
> +
> +		elen = snprintf(tmp_buf, sizeof(tmp_buf), "_%.*s_%ld",
> +				elen, buf, dir->i_ino);
> +		memcpy(buf, tmp_buf, elen);
> +	}
> +
> +out:
> +	kfree(cryptbuf);
> +	if (dir != parent) {
> +		if ((dir->i_state & I_NEW))
> +			discard_new_inode(dir);
> +		else
> +			iput(dir);
> +	}
>   	return elen;
>   }
>   
> -int ceph_encode_encrypted_fname(const struct inode *parent, struct dentry *dentry, char *buf)
> +int ceph_encode_encrypted_fname(struct inode *parent, struct dentry *dentry, char *buf)
>   {
>   	WARN_ON_ONCE(!fscrypt_has_encryption_key(parent));
>   
> @@ -203,29 +308,42 @@ int ceph_encode_encrypted_fname(const struct inode *parent, struct dentry *dentr
>   int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
>   		      struct fscrypt_str *oname, bool *is_nokey)
>   {
> -	int ret;
> +	struct inode *dir = fname->dir;
>   	struct fscrypt_str _tname = FSTR_INIT(NULL, 0);
>   	struct fscrypt_str iname;
> -
> -	if (!IS_ENCRYPTED(fname->dir)) {
> -		oname->name = fname->name;
> -		oname->len = fname->name_len;
> -		return 0;
> -	}
> +	char *name = fname->name;
> +	int name_len = fname->name_len;
> +	int ret;
>   
>   	/* Sanity check that the resulting name will fit in the buffer */
>   	if (fname->name_len > NAME_MAX || fname->ctext_len > NAME_MAX)
>   		return -EIO;
>   
> -	ret = __fscrypt_prepare_readdir(fname->dir);
> +	/* Handle the special case of snapshot names that start with '_' */
> +	if ((ceph_snap(dir) == CEPH_SNAPDIR) && (name_len > 0) &&
> +	    (name[0] == '_')) {
> +		dir = parse_longname(dir, name, &name_len);
> +		if (IS_ERR(dir))
> +			return PTR_ERR(dir);
> +		name++; /* skip initial '_' */
> +	}
> +
> +	if (!IS_ENCRYPTED(dir)) {
> +		oname->name = fname->name;
> +		oname->len = fname->name_len;
> +		ret = 0;
> +		goto out_inode;
> +	}
> +
> +	ret = __fscrypt_prepare_readdir(dir);
>   	if (ret)
> -		return ret;
> +		goto out_inode;
>   
>   	/*
>   	 * Use the raw dentry name as sent by the MDS instead of
>   	 * generating a nokey name via fscrypt.
>   	 */
> -	if (!fscrypt_has_encryption_key(fname->dir)) {
> +	if (!fscrypt_has_encryption_key(dir)) {
>   		if (fname->no_copy)
>   			oname->name = fname->name;
>   		else
> @@ -233,7 +351,8 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
>   		oname->len = fname->name_len;
>   		if (is_nokey)
>   			*is_nokey = true;
> -		return 0;
> +		ret = 0;
> +		goto out_inode;
>   	}
>   
>   	if (fname->ctext_len == 0) {
> @@ -242,11 +361,11 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
>   		if (!tname) {
>   			ret = fscrypt_fname_alloc_buffer(NAME_MAX, &_tname);
>   			if (ret)
> -				return ret;
> +				goto out_inode;
>   			tname = &_tname;
>   		}
>   
> -		declen = fscrypt_base64url_decode(fname->name, fname->name_len, tname->name);
> +		declen = fscrypt_base64url_decode(name, name_len, tname->name);
>   		if (declen <= 0) {
>   			ret = -EIO;
>   			goto out;
> @@ -258,9 +377,25 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
>   		iname.len = fname->ctext_len;
>   	}
>   
> -	ret = fscrypt_fname_disk_to_usr(fname->dir, 0, 0, &iname, oname);
> +	ret = fscrypt_fname_disk_to_usr(dir, 0, 0, &iname, oname);
> +	if (!ret && (dir != fname->dir)) {
> +		char tmp_buf[FSCRYPT_BASE64URL_CHARS(NAME_MAX)];
> +
> +		name_len = snprintf(tmp_buf, sizeof(tmp_buf), "_%.*s_%ld",
> +				    oname->len, oname->name, dir->i_ino);
> +		memcpy(oname->name, tmp_buf, name_len);
> +		oname->len = name_len;
> +	}
> +
>   out:
>   	fscrypt_fname_free_buffer(&_tname);
> +out_inode:
> +	if ((dir != fname->dir) && !IS_ERR(dir)) {
> +		if ((dir->i_state & I_NEW))
> +			discard_new_inode(dir);
> +		else
> +			iput(dir);
> +	}
>   	return ret;
>   }
>   
> diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h
> index 62f0ddd30dee..3273d076a9e5 100644
> --- a/fs/ceph/crypto.h
> +++ b/fs/ceph/crypto.h
> @@ -82,13 +82,16 @@ static inline u32 ceph_fscrypt_auth_len(struct ceph_fscrypt_auth *fa)
>    * struct fscrypt_ceph_nokey_name {
>    *	u8 bytes[157];
>    *	u8 sha256[SHA256_DIGEST_SIZE];
> - * }; // 189 bytes => 252 bytes base64-encoded, which is <= NAME_MAX (255)
> + * }; // 180 bytes => 240 bytes base64-encoded, which is <= NAME_MAX (255)
> + *
> + * (240 bytes is the maximum size allowed for snapshot names to take into
> + *  account the format: '_<SNAPSHOT-NAME>_<INODE-NUMBER>'.)
>    *
>    * Note that for long names that end up having their tail portion hashed, we
>    * must also store the full encrypted name (in the dentry's alternate_name
>    * field).
>    */
> -#define CEPH_NOHASH_NAME_MAX (189 - SHA256_DIGEST_SIZE)
> +#define CEPH_NOHASH_NAME_MAX (180 - SHA256_DIGEST_SIZE)
>   
>   void ceph_fscrypt_set_ops(struct super_block *sb);
>   
> @@ -97,8 +100,8 @@ void ceph_fscrypt_free_dummy_policy(struct ceph_fs_client *fsc);
>   int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *inode,
>   				 struct ceph_acl_sec_ctx *as);
>   void ceph_fscrypt_as_ctx_to_req(struct ceph_mds_request *req, struct ceph_acl_sec_ctx *as);
> -int ceph_encode_encrypted_dname(const struct inode *parent, struct qstr *d_name, char *buf);
> -int ceph_encode_encrypted_fname(const struct inode *parent, struct dentry *dentry, char *buf);
> +int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name, char *buf);
> +int ceph_encode_encrypted_fname(struct inode *parent, struct dentry *dentry, char *buf);
>   
>   static inline int ceph_fname_alloc_buffer(struct inode *parent, struct fscrypt_str *fname)
>   {
>

Re: [RFC PATCH v3 4/4] ceph: replace base64url by the encoding used for mailbox names

[Xiubo Li]

On 3/17/22 11:45 PM, Luís Henriques wrote:
> The base64url encoding includes the '_' character, which may cause problems
> in snapshot names (if the name starts with '_').  Thus, use the base64
> encoding defined for IMAP mailbox names (RFC 3501), which uses '+' and ','
> instead of '-' and '_'.
>
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
>   fs/ceph/crypto.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++--
>   fs/ceph/crypto.h |  3 +++
>   fs/ceph/dir.c    |  2 +-
>   fs/ceph/inode.c  |  2 +-
>   4 files changed, 54 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> index caa9863dee93..d6f1c444ce91 100644
> --- a/fs/ceph/crypto.c
> +++ b/fs/ceph/crypto.c
> @@ -7,6 +7,53 @@
>   #include "mds_client.h"
>   #include "crypto.h"
>   
> +static const char base64_table[65] =
> +        "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+,";
> +
> +int ceph_base64_encode(const u8 *src, int srclen, char *dst)
> +{
> +	u32 ac = 0;
> +	int bits = 0;
> +	int i;
> +	char *cp = dst;
> +
> +	for (i = 0; i < srclen; i++) {
> +		ac = (ac << 8) | src[i];
> +		bits += 8;
> +		do {
> +			bits -= 6;
> +			*cp++ = base64_table[(ac >> bits) & 0x3f];
> +		} while (bits >= 6);
> +	}
> +	if (bits)
> +		*cp++ = base64_table[(ac << (6 - bits)) & 0x3f];
> +	return cp - dst;
> +}
> +
> +int ceph_base64_decode(const char *src, int srclen, u8 *dst)
> +{
> +	u32 ac = 0;
> +	int bits = 0;
> +	int i;
> +	u8 *bp = dst;
> +
> +	for (i = 0; i < srclen; i++) {
> +		const char *p = strchr(base64_table, src[i]);
> +
> +		if (p == NULL || src[i] == 0)
> +			return -1;
> +		ac = (ac << 6) | (p - base64_table);
> +		bits += 6;
> +		if (bits >= 8) {
> +			bits -= 8;
> +			*bp++ = (u8)(ac >> bits);
> +		}
> +	}
> +	if (ac & ((1 << bits) - 1))
> +		return -1;
> +	return bp - dst;
> +}

Maybe this should be in fs/crypto.c ?

-- Xiubo

> +
>   static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
>   {
>   	struct ceph_inode_info *ci = ceph_inode(inode);
> @@ -260,7 +307,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name, char
>   	}
>   
>   	/* base64 encode the encrypted name */
> -	elen = fscrypt_base64url_encode(cryptbuf, len, buf);
> +	elen = ceph_base64_encode(cryptbuf, len, buf);
>   	dout("base64-encoded ciphertext name = %.*s\n", elen, buf);
>   
>   	WARN_ON(elen > (CEPH_NOHASH_NAME_MAX + SHA256_DIGEST_SIZE));
> @@ -365,7 +412,7 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
>   			tname = &_tname;
>   		}
>   
> -		declen = fscrypt_base64url_decode(name, name_len, tname->name);
> +		declen = ceph_base64_decode(name, name_len, tname->name);
>   		if (declen <= 0) {
>   			ret = -EIO;
>   			goto out;
> diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h
> index 3273d076a9e5..d22316011810 100644
> --- a/fs/ceph/crypto.h
> +++ b/fs/ceph/crypto.h
> @@ -93,6 +93,9 @@ static inline u32 ceph_fscrypt_auth_len(struct ceph_fscrypt_auth *fa)
>    */
>   #define CEPH_NOHASH_NAME_MAX (180 - SHA256_DIGEST_SIZE)
>   
> +int ceph_base64_encode(const u8 *src, int srclen, char *dst);
> +int ceph_base64_decode(const char *src, int srclen, u8 *dst);
> +
>   void ceph_fscrypt_set_ops(struct super_block *sb);
>   
>   void ceph_fscrypt_free_dummy_policy(struct ceph_fs_client *fsc);
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index 5ae5cb778389..417d8c3a7edd 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -960,7 +960,7 @@ static int prep_encrypted_symlink_target(struct ceph_mds_request *req, const cha
>   		goto out;
>   	}
>   
> -	len = fscrypt_base64url_encode(osd_link.name, osd_link.len, req->r_path2);
> +	len = ceph_base64_encode(osd_link.name, osd_link.len, req->r_path2);
>   	req->r_path2[len] = '\0';
>   out:
>   	fscrypt_fname_free_buffer(&osd_link);
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index 359e29896f16..8fd493257e0b 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -875,7 +875,7 @@ static int decode_encrypted_symlink(const char *encsym, int enclen, u8 **decsym)
>   	if (!sym)
>   		return -ENOMEM;
>   
> -	declen = fscrypt_base64url_decode(encsym, enclen, sym);
> +	declen = ceph_base64_decode(encsym, enclen, sym);
>   	if (declen < 0) {
>   		pr_err("%s: can't decode symlink (%d). Content: %.*s\n", __func__, declen, enclen, encsym);
>   		kfree(sym);
>

Re: [RFC PATCH v3 2/4] ceph: handle encrypted snapshot names in subdirectories

[Jeff Layton]

On Fri, 2022-03-18 at 12:57 +0800, Xiubo Li wrote:
> On 3/17/22 11:45 PM, Luís Henriques wrote:
> > When creating a snapshot, the .snap directories for every subdirectory will
> > show the snapshot name in the "long format":
> > 
> >    # mkdir .snap/my-snap
> >    # ls my-dir/.snap/
> >    _my-snap_1099511627782
> > 
> > Encrypted snapshots will need to be able to handle these snapshot names by
> > encrypting/decrypting only the snapshot part of the string ('my-snap').
> > 
> > Also, since the MDS prevents snapshot names to be bigger than 240 characters
> > it is necessary to adapt CEPH_NOHASH_NAME_MAX to accommodate this extra
> > limitation.
> > 
> > Signed-off-by: Luís Henriques <lhenriques@suse.de>
> > ---
> >   fs/ceph/crypto.c | 189 ++++++++++++++++++++++++++++++++++++++++-------
> >   fs/ceph/crypto.h |  11 ++-
> >   2 files changed, 169 insertions(+), 31 deletions(-)
> > 
> > diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> > index beb73bbdd868..caa9863dee93 100644
> > --- a/fs/ceph/crypto.c
> > +++ b/fs/ceph/crypto.c
> > @@ -128,16 +128,100 @@ void ceph_fscrypt_as_ctx_to_req(struct ceph_mds_request *req, struct ceph_acl_se
> >   	swap(req->r_fscrypt_auth, as->fscrypt_auth);
> >   }
> >   
> > -int ceph_encode_encrypted_dname(const struct inode *parent, struct qstr *d_name, char *buf)
> > +/*
> > + * User-created snapshots can't start with '_'.  Snapshots that start with this
> > + * character are special (hint: there aren't real snapshots) and use the
> > + * following format:
> > + *
> > + *   _<SNAPSHOT-NAME>_<INODE-NUMBER>
> > + *
> > + * where:
> > + *  - <SNAPSHOT-NAME> - the real snapshot name that may need to be decrypted,
> > + *  - <INODE-NUMBER> - the inode number for the actual snapshot
> > + *
> > + * This function parses these snapshot names and returns the inode
> > + * <INODE-NUMBER>.  'name_len' will also bet set with the <SNAPSHOT-NAME>
> > + * length.
> > + */
> > +static struct inode *parse_longname(const struct inode *parent, const char *name,
> > +				    int *name_len)
> >   {
> > +	struct inode *dir = NULL;
> > +	struct ceph_vino vino = { .snap = CEPH_NOSNAP };
> > +	char *inode_number;
> > +	char *name_end;
> > +	int orig_len = *name_len;
> > +	int ret = -EIO;
> > +
> > +	/* Skip initial '_' */
> > +	name++;
> > +	name_end = strrchr(name, '_');
> > +	if (!name_end) {
> > +		dout("Failed to parse long snapshot name: %s\n", name);
> > +		return ERR_PTR(-EIO);
> > +	}
> > +	*name_len = (name_end - name);
> > +	if (*name_len <= 0) {
> > +		pr_err("Failed to parse long snapshot name\n");
> > +		return ERR_PTR(-EIO);
> > +	}
> > +
> > +	/* Get the inode number */
> > +	inode_number = kmemdup_nul(name_end + 1,
> > +				   orig_len - *name_len - 2,
> > +				   GFP_KERNEL);
> > +	if (!inode_number)
> > +		return ERR_PTR(-ENOMEM);
> > +	ret = kstrtou64(inode_number, 0, &vino.ino);
> > +	if (ret) {
> > +		dout("Failed to parse inode number: %s\n", name);
> > +		dir = ERR_PTR(ret);
> > +		goto out;
> > +	}
> > +
> > +	/* And finally the inode */
> > +	dir = ceph_find_inode(parent->i_sb, vino);
> > +	if (!dir) {
> > +		/* This can happen if we're not mounting cephfs on the root */
> > +		dir = ceph_get_inode(parent->i_sb, vino, NULL);
> 
> In this case IMO you should lookup the inode from MDS instead create it 
> in the cache, which won't setup the encryption info needed.
> 
> So later when you try to use this to dencrypt the snapshot names, you 
> will hit errors ? And also the case Jeff mentioned in previous thread 
> could happen.
> 
> I figured out another approach could resolve this more gracefully:
> 
> For all the subdirs just let them inherit the encryption info from the 
> same ancestor, which is initially encrypted, then in ceph_new_inode() 
> you can just skip setting up the encryption info for all the subdirs and 
> in MDS side will send back the parent's encryption info and fill it in 
> handle_reply(), this is just what the .snap does.
> 
> Then here you can use current inode to do the dencryption for all the 
> snapshots including the long snapshot names.
> 
> I have raise one PR and send a kclient patch for the above basic 
> framework [1][2]. But there still need a little more work you need to do 
> based them:
> 
> When lssnap you need to add one flag in LeaseStat to tell the kclient 
> whether the long snap names are encrypted, this is very easy in MDS 
> side. Then in kclient side you can just skip dencrypting the long snap 
> names which are from none-encyrpted parents and for all the other just 
> use current inode to do the dencryption. No need to search the parent 
> inodes for long snaps.
> 
> And when lookuping a long snap name, which could be encyrpted and could 
> be not, then you need to parse the inode out and lookup the inode from 
> MDS if it does not exist in cache.
> 
> 
> [1] https://github.com/ceph/ceph/pull/45516
> 
> [2] https://patchwork.kernel.org/project/ceph-devel/list/?series=624492
> 


So basically all directories and parents would share the same nonce?

That doesn't sound very secure. Doing that for snapshots is one thing,
but I think having a different nonce for each directories is generally a
better outcome.

Can we not just do this sort of inheritance for snapshot directories?


> 
> > +		if (!dir)
> > +			dir = ERR_PTR(-ENOENT);
> > +	}
> > +	if (IS_ERR(dir))
> > +		dout("Can't find inode %s (%s)\n", inode_number, name);
> > +
> > +out:
> > +	kfree(inode_number);
> > +	return dir;
> > +}
> > +
> > +int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name, char *buf)
> > +{
> > +	struct inode *dir = parent;
> > +	struct qstr iname;
> >   	u32 len;
> > +	int name_len;
> >   	int elen;
> >   	int ret;
> > -	u8 *cryptbuf;
> > +	u8 *cryptbuf = NULL;
> > +
> > +	iname.name = d_name->name;
> > +	name_len = d_name->len;
> > +
> > +	/* Handle the special case of snapshot names that start with '_' */
> > +	if ((ceph_snap(dir) == CEPH_SNAPDIR) && (name_len > 0) &&
> > +	    (iname.name[0] == '_')) {
> > +		dir = parse_longname(parent, iname.name, &name_len);
> > +		if (IS_ERR(dir))
> > +			return PTR_ERR(dir);
> > +		iname.name++; /* skip initial '_' */
> > +	}
> > +	iname.len = name_len;
> >   
> > -	if (!fscrypt_has_encryption_key(parent)) {
> > +	if (!fscrypt_has_encryption_key(dir)) {
> >   		memcpy(buf, d_name->name, d_name->len);
> > -		return d_name->len;
> > +		elen = d_name->len;
> > +		goto out;
> >   	}
> >   
> >   	/*
> > @@ -146,18 +230,22 @@ int ceph_encode_encrypted_dname(const struct inode *parent, struct qstr *d_name,
> >   	 *
> >   	 * See: fscrypt_setup_filename
> >   	 */
> > -	if (!fscrypt_fname_encrypted_size(parent, d_name->len, NAME_MAX, &len))
> > -		return -ENAMETOOLONG;
> > +	if (!fscrypt_fname_encrypted_size(dir, iname.len, NAME_MAX, &len)) {
> > +		elen = -ENAMETOOLONG;
> > +		goto out;
> > +	}
> >   
> >   	/* Allocate a buffer appropriate to hold the result */
> >   	cryptbuf = kmalloc(len > CEPH_NOHASH_NAME_MAX ? NAME_MAX : len, GFP_KERNEL);
> > -	if (!cryptbuf)
> > -		return -ENOMEM;
> > +	if (!cryptbuf) {
> > +		elen = -ENOMEM;
> > +		goto out;
> > +	}
> >   
> > -	ret = fscrypt_fname_encrypt(parent, d_name, cryptbuf, len);
> > +	ret = fscrypt_fname_encrypt(dir, &iname, cryptbuf, len);
> >   	if (ret) {
> > -		kfree(cryptbuf);
> > -		return ret;
> > +		elen = ret;
> > +		goto out;
> >   	}
> >   
> >   	/* hash the end if the name is long enough */
> > @@ -173,12 +261,29 @@ int ceph_encode_encrypted_dname(const struct inode *parent, struct qstr *d_name,
> >   
> >   	/* base64 encode the encrypted name */
> >   	elen = fscrypt_base64url_encode(cryptbuf, len, buf);
> > -	kfree(cryptbuf);
> >   	dout("base64-encoded ciphertext name = %.*s\n", elen, buf);
> > +
> > +	WARN_ON(elen > (CEPH_NOHASH_NAME_MAX + SHA256_DIGEST_SIZE));
> > +	if ((elen > 0) && (dir != parent)) {
> > +		char tmp_buf[NAME_MAX];
> > +
> > +		elen = snprintf(tmp_buf, sizeof(tmp_buf), "_%.*s_%ld",
> > +				elen, buf, dir->i_ino);
> > +		memcpy(buf, tmp_buf, elen);
> > +	}
> > +
> > +out:
> > +	kfree(cryptbuf);
> > +	if (dir != parent) {
> > +		if ((dir->i_state & I_NEW))
> > +			discard_new_inode(dir);
> > +		else
> > +			iput(dir);
> > +	}
> >   	return elen;
> >   }
> >   
> > -int ceph_encode_encrypted_fname(const struct inode *parent, struct dentry *dentry, char *buf)
> > +int ceph_encode_encrypted_fname(struct inode *parent, struct dentry *dentry, char *buf)
> >   {
> >   	WARN_ON_ONCE(!fscrypt_has_encryption_key(parent));
> >   
> > @@ -203,29 +308,42 @@ int ceph_encode_encrypted_fname(const struct inode *parent, struct dentry *dentr
> >   int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
> >   		      struct fscrypt_str *oname, bool *is_nokey)
> >   {
> > -	int ret;
> > +	struct inode *dir = fname->dir;
> >   	struct fscrypt_str _tname = FSTR_INIT(NULL, 0);
> >   	struct fscrypt_str iname;
> > -
> > -	if (!IS_ENCRYPTED(fname->dir)) {
> > -		oname->name = fname->name;
> > -		oname->len = fname->name_len;
> > -		return 0;
> > -	}
> > +	char *name = fname->name;
> > +	int name_len = fname->name_len;
> > +	int ret;
> >   
> >   	/* Sanity check that the resulting name will fit in the buffer */
> >   	if (fname->name_len > NAME_MAX || fname->ctext_len > NAME_MAX)
> >   		return -EIO;
> >   
> > -	ret = __fscrypt_prepare_readdir(fname->dir);
> > +	/* Handle the special case of snapshot names that start with '_' */
> > +	if ((ceph_snap(dir) == CEPH_SNAPDIR) && (name_len > 0) &&
> > +	    (name[0] == '_')) {
> > +		dir = parse_longname(dir, name, &name_len);
> > +		if (IS_ERR(dir))
> > +			return PTR_ERR(dir);
> > +		name++; /* skip initial '_' */
> > +	}
> > +
> > +	if (!IS_ENCRYPTED(dir)) {
> > +		oname->name = fname->name;
> > +		oname->len = fname->name_len;
> > +		ret = 0;
> > +		goto out_inode;
> > +	}
> > +
> > +	ret = __fscrypt_prepare_readdir(dir);
> >   	if (ret)
> > -		return ret;
> > +		goto out_inode;
> >   
> >   	/*
> >   	 * Use the raw dentry name as sent by the MDS instead of
> >   	 * generating a nokey name via fscrypt.
> >   	 */
> > -	if (!fscrypt_has_encryption_key(fname->dir)) {
> > +	if (!fscrypt_has_encryption_key(dir)) {
> >   		if (fname->no_copy)
> >   			oname->name = fname->name;
> >   		else
> > @@ -233,7 +351,8 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
> >   		oname->len = fname->name_len;
> >   		if (is_nokey)
> >   			*is_nokey = true;
> > -		return 0;
> > +		ret = 0;
> > +		goto out_inode;
> >   	}
> >   
> >   	if (fname->ctext_len == 0) {
> > @@ -242,11 +361,11 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
> >   		if (!tname) {
> >   			ret = fscrypt_fname_alloc_buffer(NAME_MAX, &_tname);
> >   			if (ret)
> > -				return ret;
> > +				goto out_inode;
> >   			tname = &_tname;
> >   		}
> >   
> > -		declen = fscrypt_base64url_decode(fname->name, fname->name_len, tname->name);
> > +		declen = fscrypt_base64url_decode(name, name_len, tname->name);
> >   		if (declen <= 0) {
> >   			ret = -EIO;
> >   			goto out;
> > @@ -258,9 +377,25 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
> >   		iname.len = fname->ctext_len;
> >   	}
> >   
> > -	ret = fscrypt_fname_disk_to_usr(fname->dir, 0, 0, &iname, oname);
> > +	ret = fscrypt_fname_disk_to_usr(dir, 0, 0, &iname, oname);
> > +	if (!ret && (dir != fname->dir)) {
> > +		char tmp_buf[FSCRYPT_BASE64URL_CHARS(NAME_MAX)];
> > +
> > +		name_len = snprintf(tmp_buf, sizeof(tmp_buf), "_%.*s_%ld",
> > +				    oname->len, oname->name, dir->i_ino);
> > +		memcpy(oname->name, tmp_buf, name_len);
> > +		oname->len = name_len;
> > +	}
> > +
> >   out:
> >   	fscrypt_fname_free_buffer(&_tname);
> > +out_inode:
> > +	if ((dir != fname->dir) && !IS_ERR(dir)) {
> > +		if ((dir->i_state & I_NEW))
> > +			discard_new_inode(dir);
> > +		else
> > +			iput(dir);
> > +	}
> >   	return ret;
> >   }
> >   
> > diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h
> > index 62f0ddd30dee..3273d076a9e5 100644
> > --- a/fs/ceph/crypto.h
> > +++ b/fs/ceph/crypto.h
> > @@ -82,13 +82,16 @@ static inline u32 ceph_fscrypt_auth_len(struct ceph_fscrypt_auth *fa)
> >    * struct fscrypt_ceph_nokey_name {
> >    *	u8 bytes[157];
> >    *	u8 sha256[SHA256_DIGEST_SIZE];
> > - * }; // 189 bytes => 252 bytes base64-encoded, which is <= NAME_MAX (255)
> > + * }; // 180 bytes => 240 bytes base64-encoded, which is <= NAME_MAX (255)
> > + *
> > + * (240 bytes is the maximum size allowed for snapshot names to take into
> > + *  account the format: '_<SNAPSHOT-NAME>_<INODE-NUMBER>'.)
> >    *
> >    * Note that for long names that end up having their tail portion hashed, we
> >    * must also store the full encrypted name (in the dentry's alternate_name
> >    * field).
> >    */
> > -#define CEPH_NOHASH_NAME_MAX (189 - SHA256_DIGEST_SIZE)
> > +#define CEPH_NOHASH_NAME_MAX (180 - SHA256_DIGEST_SIZE)
> >   
> >   void ceph_fscrypt_set_ops(struct super_block *sb);
> >   
> > @@ -97,8 +100,8 @@ void ceph_fscrypt_free_dummy_policy(struct ceph_fs_client *fsc);
> >   int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *inode,
> >   				 struct ceph_acl_sec_ctx *as);
> >   void ceph_fscrypt_as_ctx_to_req(struct ceph_mds_request *req, struct ceph_acl_sec_ctx *as);
> > -int ceph_encode_encrypted_dname(const struct inode *parent, struct qstr *d_name, char *buf);
> > -int ceph_encode_encrypted_fname(const struct inode *parent, struct dentry *dentry, char *buf);
> > +int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name, char *buf);
> > +int ceph_encode_encrypted_fname(struct inode *parent, struct dentry *dentry, char *buf);
> >   
> >   static inline int ceph_fname_alloc_buffer(struct inode *parent, struct fscrypt_str *fname)
> >   {
> > 
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC PATCH v3 2/4] ceph: handle encrypted snapshot names in subdirectories

[Luís Henriques]

Xiubo Li <xiubli@redhat.com> writes:

> On 3/17/22 11:45 PM, Luís Henriques wrote:
>> When creating a snapshot, the .snap directories for every subdirectory will
>> show the snapshot name in the "long format":
>>
>>    # mkdir .snap/my-snap
>>    # ls my-dir/.snap/
>>    _my-snap_1099511627782
>>
>> Encrypted snapshots will need to be able to handle these snapshot names by
>> encrypting/decrypting only the snapshot part of the string ('my-snap').
>>
>> Also, since the MDS prevents snapshot names to be bigger than 240 characters
>> it is necessary to adapt CEPH_NOHASH_NAME_MAX to accommodate this extra
>> limitation.
>>
>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>> ---
>>   fs/ceph/crypto.c | 189 ++++++++++++++++++++++++++++++++++++++++-------
>>   fs/ceph/crypto.h |  11 ++-
>>   2 files changed, 169 insertions(+), 31 deletions(-)
>>
>> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
>> index beb73bbdd868..caa9863dee93 100644
>> --- a/fs/ceph/crypto.c
>> +++ b/fs/ceph/crypto.c
>> @@ -128,16 +128,100 @@ void ceph_fscrypt_as_ctx_to_req(struct ceph_mds_request *req, struct ceph_acl_se
>>   	swap(req->r_fscrypt_auth, as->fscrypt_auth);
>>   }
>>   -int ceph_encode_encrypted_dname(const struct inode *parent, struct qstr
>> *d_name, char *buf)
>> +/*
>> + * User-created snapshots can't start with '_'.  Snapshots that start with this
>> + * character are special (hint: there aren't real snapshots) and use the
>> + * following format:
>> + *
>> + *   _<SNAPSHOT-NAME>_<INODE-NUMBER>
>> + *
>> + * where:
>> + *  - <SNAPSHOT-NAME> - the real snapshot name that may need to be decrypted,
>> + *  - <INODE-NUMBER> - the inode number for the actual snapshot
>> + *
>> + * This function parses these snapshot names and returns the inode
>> + * <INODE-NUMBER>.  'name_len' will also bet set with the <SNAPSHOT-NAME>
>> + * length.
>> + */
>> +static struct inode *parse_longname(const struct inode *parent, const char *name,
>> +				    int *name_len)
>>   {
>> +	struct inode *dir = NULL;
>> +	struct ceph_vino vino = { .snap = CEPH_NOSNAP };
>> +	char *inode_number;
>> +	char *name_end;
>> +	int orig_len = *name_len;
>> +	int ret = -EIO;
>> +
>> +	/* Skip initial '_' */
>> +	name++;
>> +	name_end = strrchr(name, '_');
>> +	if (!name_end) {
>> +		dout("Failed to parse long snapshot name: %s\n", name);
>> +		return ERR_PTR(-EIO);
>> +	}
>> +	*name_len = (name_end - name);
>> +	if (*name_len <= 0) {
>> +		pr_err("Failed to parse long snapshot name\n");
>> +		return ERR_PTR(-EIO);
>> +	}
>> +
>> +	/* Get the inode number */
>> +	inode_number = kmemdup_nul(name_end + 1,
>> +				   orig_len - *name_len - 2,
>> +				   GFP_KERNEL);
>> +	if (!inode_number)
>> +		return ERR_PTR(-ENOMEM);
>> +	ret = kstrtou64(inode_number, 0, &vino.ino);
>> +	if (ret) {
>> +		dout("Failed to parse inode number: %s\n", name);
>> +		dir = ERR_PTR(ret);
>> +		goto out;
>> +	}
>> +
>> +	/* And finally the inode */
>> +	dir = ceph_find_inode(parent->i_sb, vino);
>> +	if (!dir) {
>> +		/* This can happen if we're not mounting cephfs on the root */
>> +		dir = ceph_get_inode(parent->i_sb, vino, NULL);
>
> In this case IMO you should lookup the inode from MDS instead create it in the
> cache, which won't setup the encryption info needed.
>
> So later when you try to use this to dencrypt the snapshot names, you will hit
> errors ? And also the case Jeff mentioned in previous thread could happen.

No, I don't see any errors.  The reason is that if we get a I_NEW inode,
we do not have the keys to even decrypt the names.  If you mount a
filesystem using as root a directory that is inside an encrypted
directory, you'll see the encrypted snapshot name:

 # mkdir mydir
 # fscrypt encrypt mydir
 # mkdir -p mydir/a/b/c/d
 # mkdir mydir/a/.snap/myspan
 # umount ...
 # mount <mon>:<port>:/a
 # ls .snap

And we simply can't decrypt it because for that we'd need to have access
to the .fscrypt in the original filesystem mount root.

I haven't tested NFS over ceph (I don't currently have a test environment
for doing that), but I *think* the same thing will happen.  (I can try to
setup this test environment in the next couple of days.)

> I figured out another approach could resolve this more gracefully:

I took a quick look at the PR and the client patch but I suspect that Jeff
is right: this approach may greatly reduce security, which is definitely
not desirable.

Cheers,
-- 
Luís


> For all the subdirs just let them inherit the encryption info from the same
> ancestor, which is initially encrypted, then in ceph_new_inode() you can just
> skip setting up the encryption info for all the subdirs and in MDS side will
> send back the parent's encryption info and fill it in handle_reply(), this is
> just what the .snap does.
>
> Then here you can use current inode to do the dencryption for all the snapshots
> including the long snapshot names.
>
> I have raise one PR and send a kclient patch for the above basic framework
> [1][2]. But there still need a little more work you need to do based them:
>
> When lssnap you need to add one flag in LeaseStat to tell the kclient whether
> the long snap names are encrypted, this is very easy in MDS side. Then in
> kclient side you can just skip dencrypting the long snap names which are from
> none-encyrpted parents and for all the other just use current inode to do the
> dencryption. No need to search the parent inodes for long snaps.
>
> And when lookuping a long snap name, which could be encyrpted and could be not,
> then you need to parse the inode out and lookup the inode from MDS if it does
> not exist in cache.
>
>
> [1] https://github.com/ceph/ceph/pull/45516
>
> [2] https://patchwork.kernel.org/project/ceph-devel/list/?series=624492
>
>
>> +		if (!dir)
>> +			dir = ERR_PTR(-ENOENT);
>> +	}
>> +	if (IS_ERR(dir))
>> +		dout("Can't find inode %s (%s)\n", inode_number, name);
>> +
>> +out:
>> +	kfree(inode_number);
>> +	return dir;
>> +}
>> +
>> +int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name, char *buf)
>> +{
>> +	struct inode *dir = parent;
>> +	struct qstr iname;
>>   	u32 len;
>> +	int name_len;
>>   	int elen;
>>   	int ret;
>> -	u8 *cryptbuf;
>> +	u8 *cryptbuf = NULL;
>> +
>> +	iname.name = d_name->name;
>> +	name_len = d_name->len;
>> +
>> +	/* Handle the special case of snapshot names that start with '_' */
>> +	if ((ceph_snap(dir) == CEPH_SNAPDIR) && (name_len > 0) &&
>> +	    (iname.name[0] == '_')) {
>> +		dir = parse_longname(parent, iname.name, &name_len);
>> +		if (IS_ERR(dir))
>> +			return PTR_ERR(dir);
>> +		iname.name++; /* skip initial '_' */
>> +	}
>> +	iname.len = name_len;
>>   -	if (!fscrypt_has_encryption_key(parent)) {
>> +	if (!fscrypt_has_encryption_key(dir)) {
>>   		memcpy(buf, d_name->name, d_name->len);
>> -		return d_name->len;
>> +		elen = d_name->len;
>> +		goto out;
>>   	}
>>     	/*
>> @@ -146,18 +230,22 @@ int ceph_encode_encrypted_dname(const struct inode *parent, struct qstr *d_name,
>>   	 *
>>   	 * See: fscrypt_setup_filename
>>   	 */
>> -	if (!fscrypt_fname_encrypted_size(parent, d_name->len, NAME_MAX, &len))
>> -		return -ENAMETOOLONG;
>> +	if (!fscrypt_fname_encrypted_size(dir, iname.len, NAME_MAX, &len)) {
>> +		elen = -ENAMETOOLONG;
>> +		goto out;
>> +	}
>>     	/* Allocate a buffer appropriate to hold the result */
>>   	cryptbuf = kmalloc(len > CEPH_NOHASH_NAME_MAX ? NAME_MAX : len, GFP_KERNEL);
>> -	if (!cryptbuf)
>> -		return -ENOMEM;
>> +	if (!cryptbuf) {
>> +		elen = -ENOMEM;
>> +		goto out;
>> +	}
>>   -	ret = fscrypt_fname_encrypt(parent, d_name, cryptbuf, len);
>> +	ret = fscrypt_fname_encrypt(dir, &iname, cryptbuf, len);
>>   	if (ret) {
>> -		kfree(cryptbuf);
>> -		return ret;
>> +		elen = ret;
>> +		goto out;
>>   	}
>>     	/* hash the end if the name is long enough */
>> @@ -173,12 +261,29 @@ int ceph_encode_encrypted_dname(const struct inode *parent, struct qstr *d_name,
>>     	/* base64 encode the encrypted name */
>>   	elen = fscrypt_base64url_encode(cryptbuf, len, buf);
>> -	kfree(cryptbuf);
>>   	dout("base64-encoded ciphertext name = %.*s\n", elen, buf);
>> +
>> +	WARN_ON(elen > (CEPH_NOHASH_NAME_MAX + SHA256_DIGEST_SIZE));
>> +	if ((elen > 0) && (dir != parent)) {
>> +		char tmp_buf[NAME_MAX];
>> +
>> +		elen = snprintf(tmp_buf, sizeof(tmp_buf), "_%.*s_%ld",
>> +				elen, buf, dir->i_ino);
>> +		memcpy(buf, tmp_buf, elen);
>> +	}
>> +
>> +out:
>> +	kfree(cryptbuf);
>> +	if (dir != parent) {
>> +		if ((dir->i_state & I_NEW))
>> +			discard_new_inode(dir);
>> +		else
>> +			iput(dir);
>> +	}
>>   	return elen;
>>   }
>>   -int ceph_encode_encrypted_fname(const struct inode *parent, struct dentry
>> *dentry, char *buf)
>> +int ceph_encode_encrypted_fname(struct inode *parent, struct dentry *dentry, char *buf)
>>   {
>>   	WARN_ON_ONCE(!fscrypt_has_encryption_key(parent));
>>   @@ -203,29 +308,42 @@ int ceph_encode_encrypted_fname(const struct inode
>> *parent, struct dentry *dentr
>>   int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
>>   		      struct fscrypt_str *oname, bool *is_nokey)
>>   {
>> -	int ret;
>> +	struct inode *dir = fname->dir;
>>   	struct fscrypt_str _tname = FSTR_INIT(NULL, 0);
>>   	struct fscrypt_str iname;
>> -
>> -	if (!IS_ENCRYPTED(fname->dir)) {
>> -		oname->name = fname->name;
>> -		oname->len = fname->name_len;
>> -		return 0;
>> -	}
>> +	char *name = fname->name;
>> +	int name_len = fname->name_len;
>> +	int ret;
>>     	/* Sanity check that the resulting name will fit in the buffer */
>>   	if (fname->name_len > NAME_MAX || fname->ctext_len > NAME_MAX)
>>   		return -EIO;
>>   -	ret = __fscrypt_prepare_readdir(fname->dir);
>> +	/* Handle the special case of snapshot names that start with '_' */
>> +	if ((ceph_snap(dir) == CEPH_SNAPDIR) && (name_len > 0) &&
>> +	    (name[0] == '_')) {
>> +		dir = parse_longname(dir, name, &name_len);
>> +		if (IS_ERR(dir))
>> +			return PTR_ERR(dir);
>> +		name++; /* skip initial '_' */
>> +	}
>> +
>> +	if (!IS_ENCRYPTED(dir)) {
>> +		oname->name = fname->name;
>> +		oname->len = fname->name_len;
>> +		ret = 0;
>> +		goto out_inode;
>> +	}
>> +
>> +	ret = __fscrypt_prepare_readdir(dir);
>>   	if (ret)
>> -		return ret;
>> +		goto out_inode;
>>     	/*
>>   	 * Use the raw dentry name as sent by the MDS instead of
>>   	 * generating a nokey name via fscrypt.
>>   	 */
>> -	if (!fscrypt_has_encryption_key(fname->dir)) {
>> +	if (!fscrypt_has_encryption_key(dir)) {
>>   		if (fname->no_copy)
>>   			oname->name = fname->name;
>>   		else
>> @@ -233,7 +351,8 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
>>   		oname->len = fname->name_len;
>>   		if (is_nokey)
>>   			*is_nokey = true;
>> -		return 0;
>> +		ret = 0;
>> +		goto out_inode;
>>   	}
>>     	if (fname->ctext_len == 0) {
>> @@ -242,11 +361,11 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
>>   		if (!tname) {
>>   			ret = fscrypt_fname_alloc_buffer(NAME_MAX, &_tname);
>>   			if (ret)
>> -				return ret;
>> +				goto out_inode;
>>   			tname = &_tname;
>>   		}
>>   -		declen = fscrypt_base64url_decode(fname->name, fname->name_len,
>> tname->name);
>> +		declen = fscrypt_base64url_decode(name, name_len, tname->name);
>>   		if (declen <= 0) {
>>   			ret = -EIO;
>>   			goto out;
>> @@ -258,9 +377,25 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
>>   		iname.len = fname->ctext_len;
>>   	}
>>   -	ret = fscrypt_fname_disk_to_usr(fname->dir, 0, 0, &iname, oname);
>> +	ret = fscrypt_fname_disk_to_usr(dir, 0, 0, &iname, oname);
>> +	if (!ret && (dir != fname->dir)) {
>> +		char tmp_buf[FSCRYPT_BASE64URL_CHARS(NAME_MAX)];
>> +
>> +		name_len = snprintf(tmp_buf, sizeof(tmp_buf), "_%.*s_%ld",
>> +				    oname->len, oname->name, dir->i_ino);
>> +		memcpy(oname->name, tmp_buf, name_len);
>> +		oname->len = name_len;
>> +	}
>> +
>>   out:
>>   	fscrypt_fname_free_buffer(&_tname);
>> +out_inode:
>> +	if ((dir != fname->dir) && !IS_ERR(dir)) {
>> +		if ((dir->i_state & I_NEW))
>> +			discard_new_inode(dir);
>> +		else
>> +			iput(dir);
>> +	}
>>   	return ret;
>>   }
>>   diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h
>> index 62f0ddd30dee..3273d076a9e5 100644
>> --- a/fs/ceph/crypto.h
>> +++ b/fs/ceph/crypto.h
>> @@ -82,13 +82,16 @@ static inline u32 ceph_fscrypt_auth_len(struct ceph_fscrypt_auth *fa)
>>    * struct fscrypt_ceph_nokey_name {
>>    *	u8 bytes[157];
>>    *	u8 sha256[SHA256_DIGEST_SIZE];
>> - * }; // 189 bytes => 252 bytes base64-encoded, which is <= NAME_MAX (255)
>> + * }; // 180 bytes => 240 bytes base64-encoded, which is <= NAME_MAX (255)
>> + *
>> + * (240 bytes is the maximum size allowed for snapshot names to take into
>> + *  account the format: '_<SNAPSHOT-NAME>_<INODE-NUMBER>'.)
>>    *
>>    * Note that for long names that end up having their tail portion hashed, we
>>    * must also store the full encrypted name (in the dentry's alternate_name
>>    * field).
>>    */
>> -#define CEPH_NOHASH_NAME_MAX (189 - SHA256_DIGEST_SIZE)
>> +#define CEPH_NOHASH_NAME_MAX (180 - SHA256_DIGEST_SIZE)
>>     void ceph_fscrypt_set_ops(struct super_block *sb);
>>   @@ -97,8 +100,8 @@ void ceph_fscrypt_free_dummy_policy(struct ceph_fs_client
>> *fsc);
>>   int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *inode,
>>   				 struct ceph_acl_sec_ctx *as);
>>   void ceph_fscrypt_as_ctx_to_req(struct ceph_mds_request *req, struct ceph_acl_sec_ctx *as);
>> -int ceph_encode_encrypted_dname(const struct inode *parent, struct qstr *d_name, char *buf);
>> -int ceph_encode_encrypted_fname(const struct inode *parent, struct dentry *dentry, char *buf);
>> +int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name, char *buf);
>> +int ceph_encode_encrypted_fname(struct inode *parent, struct dentry *dentry, char *buf);
>>     static inline int ceph_fname_alloc_buffer(struct inode *parent, struct
>> fscrypt_str *fname)
>>   {
>>
>

Re: [RFC PATCH v3 4/4] ceph: replace base64url by the encoding used for mailbox names

[Luís Henriques]

Xiubo Li <xiubli@redhat.com> writes:

> On 3/17/22 11:45 PM, Luís Henriques wrote:
>> The base64url encoding includes the '_' character, which may cause problems
>> in snapshot names (if the name starts with '_').  Thus, use the base64
>> encoding defined for IMAP mailbox names (RFC 3501), which uses '+' and ','
>> instead of '-' and '_'.
>>
>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>> ---
>>   fs/ceph/crypto.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++--
>>   fs/ceph/crypto.h |  3 +++
>>   fs/ceph/dir.c    |  2 +-
>>   fs/ceph/inode.c  |  2 +-
>>   4 files changed, 54 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
>> index caa9863dee93..d6f1c444ce91 100644
>> --- a/fs/ceph/crypto.c
>> +++ b/fs/ceph/crypto.c
>> @@ -7,6 +7,53 @@
>>   #include "mds_client.h"
>>   #include "crypto.h"
>>   +static const char base64_table[65] =
>> +        "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+,";
>> +
>> +int ceph_base64_encode(const u8 *src, int srclen, char *dst)
>> +{
>> +	u32 ac = 0;
>> +	int bits = 0;
>> +	int i;
>> +	char *cp = dst;
>> +
>> +	for (i = 0; i < srclen; i++) {
>> +		ac = (ac << 8) | src[i];
>> +		bits += 8;
>> +		do {
>> +			bits -= 6;
>> +			*cp++ = base64_table[(ac >> bits) & 0x3f];
>> +		} while (bits >= 6);
>> +	}
>> +	if (bits)
>> +		*cp++ = base64_table[(ac << (6 - bits)) & 0x3f];
>> +	return cp - dst;
>> +}
>> +
>> +int ceph_base64_decode(const char *src, int srclen, u8 *dst)
>> +{
>> +	u32 ac = 0;
>> +	int bits = 0;
>> +	int i;
>> +	u8 *bp = dst;
>> +
>> +	for (i = 0; i < srclen; i++) {
>> +		const char *p = strchr(base64_table, src[i]);
>> +
>> +		if (p == NULL || src[i] == 0)
>> +			return -1;
>> +		ac = (ac << 6) | (p - base64_table);
>> +		bits += 6;
>> +		if (bits >= 8) {
>> +			bits -= 8;
>> +			*bp++ = (u8)(ac >> bits);
>> +		}
>> +	}
>> +	if (ac & ((1 << bits) - 1))
>> +		return -1;
>> +	return bp - dst;
>> +}
>
> Maybe this should be in fs/crypto.c ?

Yeah, if the solution is to modify the base64 encoding, that's my
preference too (in the series cover-letter this would correspond to
alternative #3).

[ In fact, I've probably done something very wrong here, as I didn't even
  mentioned that this patch is basically a copy of someone else's code; I
  simply modified the table used.  So, please do *not* consider merging
  this patch. ]

Cheers,
-- 
Luís

>
> -- Xiubo
>
>> +
>>   static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
>>   {
>>   	struct ceph_inode_info *ci = ceph_inode(inode);
>> @@ -260,7 +307,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name, char
>>   	}
>>     	/* base64 encode the encrypted name */
>> -	elen = fscrypt_base64url_encode(cryptbuf, len, buf);
>> +	elen = ceph_base64_encode(cryptbuf, len, buf);
>>   	dout("base64-encoded ciphertext name = %.*s\n", elen, buf);
>>     	WARN_ON(elen > (CEPH_NOHASH_NAME_MAX + SHA256_DIGEST_SIZE));
>> @@ -365,7 +412,7 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
>>   			tname = &_tname;
>>   		}
>>   -		declen = fscrypt_base64url_decode(name, name_len, tname->name);
>> +		declen = ceph_base64_decode(name, name_len, tname->name);
>>   		if (declen <= 0) {
>>   			ret = -EIO;
>>   			goto out;
>> diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h
>> index 3273d076a9e5..d22316011810 100644
>> --- a/fs/ceph/crypto.h
>> +++ b/fs/ceph/crypto.h
>> @@ -93,6 +93,9 @@ static inline u32 ceph_fscrypt_auth_len(struct ceph_fscrypt_auth *fa)
>>    */
>>   #define CEPH_NOHASH_NAME_MAX (180 - SHA256_DIGEST_SIZE)
>>   +int ceph_base64_encode(const u8 *src, int srclen, char *dst);
>> +int ceph_base64_decode(const char *src, int srclen, u8 *dst);
>> +
>>   void ceph_fscrypt_set_ops(struct super_block *sb);
>>     void ceph_fscrypt_free_dummy_policy(struct ceph_fs_client *fsc);
>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>> index 5ae5cb778389..417d8c3a7edd 100644
>> --- a/fs/ceph/dir.c
>> +++ b/fs/ceph/dir.c
>> @@ -960,7 +960,7 @@ static int prep_encrypted_symlink_target(struct ceph_mds_request *req, const cha
>>   		goto out;
>>   	}
>>   -	len = fscrypt_base64url_encode(osd_link.name, osd_link.len,
>> req->r_path2);
>> +	len = ceph_base64_encode(osd_link.name, osd_link.len, req->r_path2);
>>   	req->r_path2[len] = '\0';
>>   out:
>>   	fscrypt_fname_free_buffer(&osd_link);
>> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
>> index 359e29896f16..8fd493257e0b 100644
>> --- a/fs/ceph/inode.c
>> +++ b/fs/ceph/inode.c
>> @@ -875,7 +875,7 @@ static int decode_encrypted_symlink(const char *encsym, int enclen, u8 **decsym)
>>   	if (!sym)
>>   		return -ENOMEM;
>>   -	declen = fscrypt_base64url_decode(encsym, enclen, sym);
>> +	declen = ceph_base64_decode(encsym, enclen, sym);
>>   	if (declen < 0) {
>>   		pr_err("%s: can't decode symlink (%d). Content: %.*s\n", __func__, declen, enclen, encsym);
>>   		kfree(sym);
>>
>

Re: [RFC PATCH v3 2/4] ceph: handle encrypted snapshot names in subdirectories

[Xiubo Li]

On 3/18/22 5:57 PM, Jeff Layton wrote:
> On Fri, 2022-03-18 at 12:57 +0800, Xiubo Li wrote:
>> On 3/17/22 11:45 PM, Luís Henriques wrote:
>>> When creating a snapshot, the .snap directories for every subdirectory will
>>> show the snapshot name in the "long format":
>>>
>>>     # mkdir .snap/my-snap
>>>     # ls my-dir/.snap/
>>>     _my-snap_1099511627782
>>>
>>> Encrypted snapshots will need to be able to handle these snapshot names by
>>> encrypting/decrypting only the snapshot part of the string ('my-snap').
>>>
>>> Also, since the MDS prevents snapshot names to be bigger than 240 characters
>>> it is necessary to adapt CEPH_NOHASH_NAME_MAX to accommodate this extra
>>> limitation.
>>>
>>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>>> ---
>>>    fs/ceph/crypto.c | 189 ++++++++++++++++++++++++++++++++++++++++-------
>>>    fs/ceph/crypto.h |  11 ++-
>>>    2 files changed, 169 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
>>> index beb73bbdd868..caa9863dee93 100644
>>> --- a/fs/ceph/crypto.c
>>> +++ b/fs/ceph/crypto.c
>>> @@ -128,16 +128,100 @@ void ceph_fscrypt_as_ctx_to_req(struct ceph_mds_request *req, struct ceph_acl_se
>>>    	swap(req->r_fscrypt_auth, as->fscrypt_auth);
>>>    }
>>>    
>>> -int ceph_encode_encrypted_dname(const struct inode *parent, struct qstr *d_name, char *buf)
>>> +/*
>>> + * User-created snapshots can't start with '_'.  Snapshots that start with this
>>> + * character are special (hint: there aren't real snapshots) and use the
>>> + * following format:
>>> + *
>>> + *   _<SNAPSHOT-NAME>_<INODE-NUMBER>
>>> + *
>>> + * where:
>>> + *  - <SNAPSHOT-NAME> - the real snapshot name that may need to be decrypted,
>>> + *  - <INODE-NUMBER> - the inode number for the actual snapshot
>>> + *
>>> + * This function parses these snapshot names and returns the inode
>>> + * <INODE-NUMBER>.  'name_len' will also bet set with the <SNAPSHOT-NAME>
>>> + * length.
>>> + */
>>> +static struct inode *parse_longname(const struct inode *parent, const char *name,
>>> +				    int *name_len)
>>>    {
>>> +	struct inode *dir = NULL;
>>> +	struct ceph_vino vino = { .snap = CEPH_NOSNAP };
>>> +	char *inode_number;
>>> +	char *name_end;
>>> +	int orig_len = *name_len;
>>> +	int ret = -EIO;
>>> +
>>> +	/* Skip initial '_' */
>>> +	name++;
>>> +	name_end = strrchr(name, '_');
>>> +	if (!name_end) {
>>> +		dout("Failed to parse long snapshot name: %s\n", name);
>>> +		return ERR_PTR(-EIO);
>>> +	}
>>> +	*name_len = (name_end - name);
>>> +	if (*name_len <= 0) {
>>> +		pr_err("Failed to parse long snapshot name\n");
>>> +		return ERR_PTR(-EIO);
>>> +	}
>>> +
>>> +	/* Get the inode number */
>>> +	inode_number = kmemdup_nul(name_end + 1,
>>> +				   orig_len - *name_len - 2,
>>> +				   GFP_KERNEL);
>>> +	if (!inode_number)
>>> +		return ERR_PTR(-ENOMEM);
>>> +	ret = kstrtou64(inode_number, 0, &vino.ino);
>>> +	if (ret) {
>>> +		dout("Failed to parse inode number: %s\n", name);
>>> +		dir = ERR_PTR(ret);
>>> +		goto out;
>>> +	}
>>> +
>>> +	/* And finally the inode */
>>> +	dir = ceph_find_inode(parent->i_sb, vino);
>>> +	if (!dir) {
>>> +		/* This can happen if we're not mounting cephfs on the root */
>>> +		dir = ceph_get_inode(parent->i_sb, vino, NULL);
>> In this case IMO you should lookup the inode from MDS instead create it
>> in the cache, which won't setup the encryption info needed.
>>
>> So later when you try to use this to dencrypt the snapshot names, you
>> will hit errors ? And also the case Jeff mentioned in previous thread
>> could happen.
>>
>> I figured out another approach could resolve this more gracefully:
>>
>> For all the subdirs just let them inherit the encryption info from the
>> same ancestor, which is initially encrypted, then in ceph_new_inode()
>> you can just skip setting up the encryption info for all the subdirs and
>> in MDS side will send back the parent's encryption info and fill it in
>> handle_reply(), this is just what the .snap does.
>>
>> Then here you can use current inode to do the dencryption for all the
>> snapshots including the long snapshot names.
>>
>> I have raise one PR and send a kclient patch for the above basic
>> framework [1][2]. But there still need a little more work you need to do
>> based them:
>>
>> When lssnap you need to add one flag in LeaseStat to tell the kclient
>> whether the long snap names are encrypted, this is very easy in MDS
>> side. Then in kclient side you can just skip dencrypting the long snap
>> names which are from none-encyrpted parents and for all the other just
>> use current inode to do the dencryption. No need to search the parent
>> inodes for long snaps.
>>
>> And when lookuping a long snap name, which could be encyrpted and could
>> be not, then you need to parse the inode out and lookup the inode from
>> MDS if it does not exist in cache.
>>
>>
>> [1] https://github.com/ceph/ceph/pull/45516
>>
>> [2] https://patchwork.kernel.org/project/ceph-devel/list/?series=624492
>>
>
> So basically all directories and parents would share the same nonce?
>
> That doesn't sound very secure. Doing that for snapshots is one thing,
> but I think having a different nonce for each directories is generally a
> better outcome.
>
> Can we not just do this sort of inheritance for snapshot directories?

Yeah, this is just a proposal. Let's drop this.

- Xiubo


>
>>> +		if (!dir)
>>> +			dir = ERR_PTR(-ENOENT);
>>> +	}
>>> +	if (IS_ERR(dir))
>>> +		dout("Can't find inode %s (%s)\n", inode_number, name);
>>> +
>>> +out:
>>> +	kfree(inode_number);
>>> +	return dir;
>>> +}
>>> +
>>> +int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name, char *buf)
>>> +{
>>> +	struct inode *dir = parent;
>>> +	struct qstr iname;
>>>    	u32 len;
>>> +	int name_len;
>>>    	int elen;
>>>    	int ret;
>>> -	u8 *cryptbuf;
>>> +	u8 *cryptbuf = NULL;
>>> +
>>> +	iname.name = d_name->name;
>>> +	name_len = d_name->len;
>>> +
>>> +	/* Handle the special case of snapshot names that start with '_' */
>>> +	if ((ceph_snap(dir) == CEPH_SNAPDIR) && (name_len > 0) &&
>>> +	    (iname.name[0] == '_')) {
>>> +		dir = parse_longname(parent, iname.name, &name_len);
>>> +		if (IS_ERR(dir))
>>> +			return PTR_ERR(dir);
>>> +		iname.name++; /* skip initial '_' */
>>> +	}
>>> +	iname.len = name_len;
>>>    
>>> -	if (!fscrypt_has_encryption_key(parent)) {
>>> +	if (!fscrypt_has_encryption_key(dir)) {
>>>    		memcpy(buf, d_name->name, d_name->len);
>>> -		return d_name->len;
>>> +		elen = d_name->len;
>>> +		goto out;
>>>    	}
>>>    
>>>    	/*
>>> @@ -146,18 +230,22 @@ int ceph_encode_encrypted_dname(const struct inode *parent, struct qstr *d_name,
>>>    	 *
>>>    	 * See: fscrypt_setup_filename
>>>    	 */
>>> -	if (!fscrypt_fname_encrypted_size(parent, d_name->len, NAME_MAX, &len))
>>> -		return -ENAMETOOLONG;
>>> +	if (!fscrypt_fname_encrypted_size(dir, iname.len, NAME_MAX, &len)) {
>>> +		elen = -ENAMETOOLONG;
>>> +		goto out;
>>> +	}
>>>    
>>>    	/* Allocate a buffer appropriate to hold the result */
>>>    	cryptbuf = kmalloc(len > CEPH_NOHASH_NAME_MAX ? NAME_MAX : len, GFP_KERNEL);
>>> -	if (!cryptbuf)
>>> -		return -ENOMEM;
>>> +	if (!cryptbuf) {
>>> +		elen = -ENOMEM;
>>> +		goto out;
>>> +	}
>>>    
>>> -	ret = fscrypt_fname_encrypt(parent, d_name, cryptbuf, len);
>>> +	ret = fscrypt_fname_encrypt(dir, &iname, cryptbuf, len);
>>>    	if (ret) {
>>> -		kfree(cryptbuf);
>>> -		return ret;
>>> +		elen = ret;
>>> +		goto out;
>>>    	}
>>>    
>>>    	/* hash the end if the name is long enough */
>>> @@ -173,12 +261,29 @@ int ceph_encode_encrypted_dname(const struct inode *parent, struct qstr *d_name,
>>>    
>>>    	/* base64 encode the encrypted name */
>>>    	elen = fscrypt_base64url_encode(cryptbuf, len, buf);
>>> -	kfree(cryptbuf);
>>>    	dout("base64-encoded ciphertext name = %.*s\n", elen, buf);
>>> +
>>> +	WARN_ON(elen > (CEPH_NOHASH_NAME_MAX + SHA256_DIGEST_SIZE));
>>> +	if ((elen > 0) && (dir != parent)) {
>>> +		char tmp_buf[NAME_MAX];
>>> +
>>> +		elen = snprintf(tmp_buf, sizeof(tmp_buf), "_%.*s_%ld",
>>> +				elen, buf, dir->i_ino);
>>> +		memcpy(buf, tmp_buf, elen);
>>> +	}
>>> +
>>> +out:
>>> +	kfree(cryptbuf);
>>> +	if (dir != parent) {
>>> +		if ((dir->i_state & I_NEW))
>>> +			discard_new_inode(dir);
>>> +		else
>>> +			iput(dir);
>>> +	}
>>>    	return elen;
>>>    }
>>>    
>>> -int ceph_encode_encrypted_fname(const struct inode *parent, struct dentry *dentry, char *buf)
>>> +int ceph_encode_encrypted_fname(struct inode *parent, struct dentry *dentry, char *buf)
>>>    {
>>>    	WARN_ON_ONCE(!fscrypt_has_encryption_key(parent));
>>>    
>>> @@ -203,29 +308,42 @@ int ceph_encode_encrypted_fname(const struct inode *parent, struct dentry *dentr
>>>    int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
>>>    		      struct fscrypt_str *oname, bool *is_nokey)
>>>    {
>>> -	int ret;
>>> +	struct inode *dir = fname->dir;
>>>    	struct fscrypt_str _tname = FSTR_INIT(NULL, 0);
>>>    	struct fscrypt_str iname;
>>> -
>>> -	if (!IS_ENCRYPTED(fname->dir)) {
>>> -		oname->name = fname->name;
>>> -		oname->len = fname->name_len;
>>> -		return 0;
>>> -	}
>>> +	char *name = fname->name;
>>> +	int name_len = fname->name_len;
>>> +	int ret;
>>>    
>>>    	/* Sanity check that the resulting name will fit in the buffer */
>>>    	if (fname->name_len > NAME_MAX || fname->ctext_len > NAME_MAX)
>>>    		return -EIO;
>>>    
>>> -	ret = __fscrypt_prepare_readdir(fname->dir);
>>> +	/* Handle the special case of snapshot names that start with '_' */
>>> +	if ((ceph_snap(dir) == CEPH_SNAPDIR) && (name_len > 0) &&
>>> +	    (name[0] == '_')) {
>>> +		dir = parse_longname(dir, name, &name_len);
>>> +		if (IS_ERR(dir))
>>> +			return PTR_ERR(dir);
>>> +		name++; /* skip initial '_' */
>>> +	}
>>> +
>>> +	if (!IS_ENCRYPTED(dir)) {
>>> +		oname->name = fname->name;
>>> +		oname->len = fname->name_len;
>>> +		ret = 0;
>>> +		goto out_inode;
>>> +	}
>>> +
>>> +	ret = __fscrypt_prepare_readdir(dir);
>>>    	if (ret)
>>> -		return ret;
>>> +		goto out_inode;
>>>    
>>>    	/*
>>>    	 * Use the raw dentry name as sent by the MDS instead of
>>>    	 * generating a nokey name via fscrypt.
>>>    	 */
>>> -	if (!fscrypt_has_encryption_key(fname->dir)) {
>>> +	if (!fscrypt_has_encryption_key(dir)) {
>>>    		if (fname->no_copy)
>>>    			oname->name = fname->name;
>>>    		else
>>> @@ -233,7 +351,8 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
>>>    		oname->len = fname->name_len;
>>>    		if (is_nokey)
>>>    			*is_nokey = true;
>>> -		return 0;
>>> +		ret = 0;
>>> +		goto out_inode;
>>>    	}
>>>    
>>>    	if (fname->ctext_len == 0) {
>>> @@ -242,11 +361,11 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
>>>    		if (!tname) {
>>>    			ret = fscrypt_fname_alloc_buffer(NAME_MAX, &_tname);
>>>    			if (ret)
>>> -				return ret;
>>> +				goto out_inode;
>>>    			tname = &_tname;
>>>    		}
>>>    
>>> -		declen = fscrypt_base64url_decode(fname->name, fname->name_len, tname->name);
>>> +		declen = fscrypt_base64url_decode(name, name_len, tname->name);
>>>    		if (declen <= 0) {
>>>    			ret = -EIO;
>>>    			goto out;
>>> @@ -258,9 +377,25 @@ int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
>>>    		iname.len = fname->ctext_len;
>>>    	}
>>>    
>>> -	ret = fscrypt_fname_disk_to_usr(fname->dir, 0, 0, &iname, oname);
>>> +	ret = fscrypt_fname_disk_to_usr(dir, 0, 0, &iname, oname);
>>> +	if (!ret && (dir != fname->dir)) {
>>> +		char tmp_buf[FSCRYPT_BASE64URL_CHARS(NAME_MAX)];
>>> +
>>> +		name_len = snprintf(tmp_buf, sizeof(tmp_buf), "_%.*s_%ld",
>>> +				    oname->len, oname->name, dir->i_ino);
>>> +		memcpy(oname->name, tmp_buf, name_len);
>>> +		oname->len = name_len;
>>> +	}
>>> +
>>>    out:
>>>    	fscrypt_fname_free_buffer(&_tname);
>>> +out_inode:
>>> +	if ((dir != fname->dir) && !IS_ERR(dir)) {
>>> +		if ((dir->i_state & I_NEW))
>>> +			discard_new_inode(dir);
>>> +		else
>>> +			iput(dir);
>>> +	}
>>>    	return ret;
>>>    }
>>>    
>>> diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h
>>> index 62f0ddd30dee..3273d076a9e5 100644
>>> --- a/fs/ceph/crypto.h
>>> +++ b/fs/ceph/crypto.h
>>> @@ -82,13 +82,16 @@ static inline u32 ceph_fscrypt_auth_len(struct ceph_fscrypt_auth *fa)
>>>     * struct fscrypt_ceph_nokey_name {
>>>     *	u8 bytes[157];
>>>     *	u8 sha256[SHA256_DIGEST_SIZE];
>>> - * }; // 189 bytes => 252 bytes base64-encoded, which is <= NAME_MAX (255)
>>> + * }; // 180 bytes => 240 bytes base64-encoded, which is <= NAME_MAX (255)
>>> + *
>>> + * (240 bytes is the maximum size allowed for snapshot names to take into
>>> + *  account the format: '_<SNAPSHOT-NAME>_<INODE-NUMBER>'.)
>>>     *
>>>     * Note that for long names that end up having their tail portion hashed, we
>>>     * must also store the full encrypted name (in the dentry's alternate_name
>>>     * field).
>>>     */
>>> -#define CEPH_NOHASH_NAME_MAX (189 - SHA256_DIGEST_SIZE)
>>> +#define CEPH_NOHASH_NAME_MAX (180 - SHA256_DIGEST_SIZE)
>>>    
>>>    void ceph_fscrypt_set_ops(struct super_block *sb);
>>>    
>>> @@ -97,8 +100,8 @@ void ceph_fscrypt_free_dummy_policy(struct ceph_fs_client *fsc);
>>>    int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *inode,
>>>    				 struct ceph_acl_sec_ctx *as);
>>>    void ceph_fscrypt_as_ctx_to_req(struct ceph_mds_request *req, struct ceph_acl_sec_ctx *as);
>>> -int ceph_encode_encrypted_dname(const struct inode *parent, struct qstr *d_name, char *buf);
>>> -int ceph_encode_encrypted_fname(const struct inode *parent, struct dentry *dentry, char *buf);
>>> +int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name, char *buf);
>>> +int ceph_encode_encrypted_fname(struct inode *parent, struct dentry *dentry, char *buf);
>>>    
>>>    static inline int ceph_fname_alloc_buffer(struct inode *parent, struct fscrypt_str *fname)
>>>    {
>>>

Re: [RFC PATCH v3 2/4] ceph: handle encrypted snapshot names in subdirectories

[Xiubo Li]

On 3/18/22 6:53 PM, Luís Henriques wrote:
> Xiubo Li <xiubli@redhat.com> writes:
>
>> On 3/17/22 11:45 PM, Luís Henriques wrote:
>>> When creating a snapshot, the .snap directories for every subdirectory will
>>> show the snapshot name in the "long format":
>>>
>>>     # mkdir .snap/my-snap
>>>     # ls my-dir/.snap/
>>>     _my-snap_1099511627782
>>>
>>> Encrypted snapshots will need to be able to handle these snapshot names by
>>> encrypting/decrypting only the snapshot part of the string ('my-snap').
>>>
>>> Also, since the MDS prevents snapshot names to be bigger than 240 characters
>>> it is necessary to adapt CEPH_NOHASH_NAME_MAX to accommodate this extra
>>> limitation.
>>>
>>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>>> ---
>>>    fs/ceph/crypto.c | 189 ++++++++++++++++++++++++++++++++++++++++-------
>>>    fs/ceph/crypto.h |  11 ++-
>>>    2 files changed, 169 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
>>> index beb73bbdd868..caa9863dee93 100644
>>> --- a/fs/ceph/crypto.c
>>> +++ b/fs/ceph/crypto.c
>>> @@ -128,16 +128,100 @@ void ceph_fscrypt_as_ctx_to_req(struct ceph_mds_request *req, struct ceph_acl_se
>>>    	swap(req->r_fscrypt_auth, as->fscrypt_auth);
>>>    }
>>>    -int ceph_encode_encrypted_dname(const struct inode *parent, struct qstr
>>> *d_name, char *buf)
>>> +/*
>>> + * User-created snapshots can't start with '_'.  Snapshots that start with this
>>> + * character are special (hint: there aren't real snapshots) and use the
>>> + * following format:
>>> + *
>>> + *   _<SNAPSHOT-NAME>_<INODE-NUMBER>
>>> + *
>>> + * where:
>>> + *  - <SNAPSHOT-NAME> - the real snapshot name that may need to be decrypted,
>>> + *  - <INODE-NUMBER> - the inode number for the actual snapshot
>>> + *
>>> + * This function parses these snapshot names and returns the inode
>>> + * <INODE-NUMBER>.  'name_len' will also bet set with the <SNAPSHOT-NAME>
>>> + * length.
>>> + */
>>> +static struct inode *parse_longname(const struct inode *parent, const char *name,
>>> +				    int *name_len)
>>>    {
>>> +	struct inode *dir = NULL;
>>> +	struct ceph_vino vino = { .snap = CEPH_NOSNAP };
>>> +	char *inode_number;
>>> +	char *name_end;
>>> +	int orig_len = *name_len;
>>> +	int ret = -EIO;
>>> +
>>> +	/* Skip initial '_' */
>>> +	name++;
>>> +	name_end = strrchr(name, '_');
>>> +	if (!name_end) {
>>> +		dout("Failed to parse long snapshot name: %s\n", name);
>>> +		return ERR_PTR(-EIO);
>>> +	}
>>> +	*name_len = (name_end - name);
>>> +	if (*name_len <= 0) {
>>> +		pr_err("Failed to parse long snapshot name\n");
>>> +		return ERR_PTR(-EIO);
>>> +	}
>>> +
>>> +	/* Get the inode number */
>>> +	inode_number = kmemdup_nul(name_end + 1,
>>> +				   orig_len - *name_len - 2,
>>> +				   GFP_KERNEL);
>>> +	if (!inode_number)
>>> +		return ERR_PTR(-ENOMEM);
>>> +	ret = kstrtou64(inode_number, 0, &vino.ino);
>>> +	if (ret) {
>>> +		dout("Failed to parse inode number: %s\n", name);
>>> +		dir = ERR_PTR(ret);
>>> +		goto out;
>>> +	}
>>> +
>>> +	/* And finally the inode */
>>> +	dir = ceph_find_inode(parent->i_sb, vino);
>>> +	if (!dir) {
>>> +		/* This can happen if we're not mounting cephfs on the root */
>>> +		dir = ceph_get_inode(parent->i_sb, vino, NULL);
>> In this case IMO you should lookup the inode from MDS instead create it in the
>> cache, which won't setup the encryption info needed.
>>
>> So later when you try to use this to dencrypt the snapshot names, you will hit
>> errors ? And also the case Jeff mentioned in previous thread could happen.
> No, I don't see any errors.  The reason is that if we get a I_NEW inode,
> we do not have the keys to even decrypt the names.  If you mount a
> filesystem using as root a directory that is inside an encrypted
> directory, you'll see the encrypted snapshot name:
>
>   # mkdir mydir
>   # fscrypt encrypt mydir
>   # mkdir -p mydir/a/b/c/d
>   # mkdir mydir/a/.snap/myspan
>   # umount ...
>   # mount <mon>:<port>:/a
>   # ls .snap
>
> And we simply can't decrypt it because for that we'd need to have access
> to the .fscrypt in the original filesystem mount root.

Should we resolve this issue ? Something like try to copy the .fscrypt 
when mounting '/a' ?

-- Xiubo

Re: [RFC PATCH v3 2/4] ceph: handle encrypted snapshot names in subdirectories

[Luís Henriques]

Xiubo Li <xiubli@redhat.com> writes:

> On 3/18/22 6:53 PM, Luís Henriques wrote:
>> Xiubo Li <xiubli@redhat.com> writes:
>>
>>> On 3/17/22 11:45 PM, Luís Henriques wrote:
>>>> When creating a snapshot, the .snap directories for every subdirectory will
>>>> show the snapshot name in the "long format":
>>>>
>>>>     # mkdir .snap/my-snap
>>>>     # ls my-dir/.snap/
>>>>     _my-snap_1099511627782
>>>>
>>>> Encrypted snapshots will need to be able to handle these snapshot names by
>>>> encrypting/decrypting only the snapshot part of the string ('my-snap').
>>>>
>>>> Also, since the MDS prevents snapshot names to be bigger than 240 characters
>>>> it is necessary to adapt CEPH_NOHASH_NAME_MAX to accommodate this extra
>>>> limitation.
>>>>
>>>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>>>> ---
>>>>    fs/ceph/crypto.c | 189 ++++++++++++++++++++++++++++++++++++++++-------
>>>>    fs/ceph/crypto.h |  11 ++-
>>>>    2 files changed, 169 insertions(+), 31 deletions(-)
>>>>
>>>> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
>>>> index beb73bbdd868..caa9863dee93 100644
>>>> --- a/fs/ceph/crypto.c
>>>> +++ b/fs/ceph/crypto.c
>>>> @@ -128,16 +128,100 @@ void ceph_fscrypt_as_ctx_to_req(struct ceph_mds_request *req, struct ceph_acl_se
>>>>    	swap(req->r_fscrypt_auth, as->fscrypt_auth);
>>>>    }
>>>>    -int ceph_encode_encrypted_dname(const struct inode *parent, struct qstr
>>>> *d_name, char *buf)
>>>> +/*
>>>> + * User-created snapshots can't start with '_'.  Snapshots that start with this
>>>> + * character are special (hint: there aren't real snapshots) and use the
>>>> + * following format:
>>>> + *
>>>> + *   _<SNAPSHOT-NAME>_<INODE-NUMBER>
>>>> + *
>>>> + * where:
>>>> + *  - <SNAPSHOT-NAME> - the real snapshot name that may need to be decrypted,
>>>> + *  - <INODE-NUMBER> - the inode number for the actual snapshot
>>>> + *
>>>> + * This function parses these snapshot names and returns the inode
>>>> + * <INODE-NUMBER>.  'name_len' will also bet set with the <SNAPSHOT-NAME>
>>>> + * length.
>>>> + */
>>>> +static struct inode *parse_longname(const struct inode *parent, const char *name,
>>>> +				    int *name_len)
>>>>    {
>>>> +	struct inode *dir = NULL;
>>>> +	struct ceph_vino vino = { .snap = CEPH_NOSNAP };
>>>> +	char *inode_number;
>>>> +	char *name_end;
>>>> +	int orig_len = *name_len;
>>>> +	int ret = -EIO;
>>>> +
>>>> +	/* Skip initial '_' */
>>>> +	name++;
>>>> +	name_end = strrchr(name, '_');
>>>> +	if (!name_end) {
>>>> +		dout("Failed to parse long snapshot name: %s\n", name);
>>>> +		return ERR_PTR(-EIO);
>>>> +	}
>>>> +	*name_len = (name_end - name);
>>>> +	if (*name_len <= 0) {
>>>> +		pr_err("Failed to parse long snapshot name\n");
>>>> +		return ERR_PTR(-EIO);
>>>> +	}
>>>> +
>>>> +	/* Get the inode number */
>>>> +	inode_number = kmemdup_nul(name_end + 1,
>>>> +				   orig_len - *name_len - 2,
>>>> +				   GFP_KERNEL);
>>>> +	if (!inode_number)
>>>> +		return ERR_PTR(-ENOMEM);
>>>> +	ret = kstrtou64(inode_number, 0, &vino.ino);
>>>> +	if (ret) {
>>>> +		dout("Failed to parse inode number: %s\n", name);
>>>> +		dir = ERR_PTR(ret);
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +	/* And finally the inode */
>>>> +	dir = ceph_find_inode(parent->i_sb, vino);
>>>> +	if (!dir) {
>>>> +		/* This can happen if we're not mounting cephfs on the root */
>>>> +		dir = ceph_get_inode(parent->i_sb, vino, NULL);
>>> In this case IMO you should lookup the inode from MDS instead create it in the
>>> cache, which won't setup the encryption info needed.
>>>
>>> So later when you try to use this to dencrypt the snapshot names, you will hit
>>> errors ? And also the case Jeff mentioned in previous thread could happen.
>> No, I don't see any errors.  The reason is that if we get a I_NEW inode,
>> we do not have the keys to even decrypt the names.  If you mount a
>> filesystem using as root a directory that is inside an encrypted
>> directory, you'll see the encrypted snapshot name:
>>
>>   # mkdir mydir
>>   # fscrypt encrypt mydir
>>   # mkdir -p mydir/a/b/c/d
>>   # mkdir mydir/a/.snap/myspan
>>   # umount ...
>>   # mount <mon>:<port>:/a
>>   # ls .snap
>>
>> And we simply can't decrypt it because for that we'd need to have access
>> to the .fscrypt in the original filesystem mount root.
>
> Should we resolve this issue ? Something like try to copy the .fscrypt when
> mounting '/a' ?

I don't think this is an issue.  If an admin mounts a filesystem this way,
he must know what he's doing.  Being unable to decrypt a directory because
he picked the wrong root is a user error.  (Having documentation will
help, of course.)

Also, where would we copy the .fscrypt from?  You can run 'fscrypt setup'
as many times as you want in a single cephfs, you simply need to use
different root directories.  So, yeah, my opinion is that we simply need
to hand this gracefully in the client.

Cheers,
-- 
Luís