Re: [PATCH v2] ceph: bound encrypted snapshot suffix formatting

[Viacheslav Dubeyko]

On Tue, 2026-04-07 at 09:57 +0800, Pengpeng Hou wrote:
> ceph_encode_encrypted_dname() base64-encodes the encrypted snapshot
> name into the caller buffer and then, for long snapshot names, appends
> _<ino> with sprintf(p + elen, ...).
> 
> Some callers only provide NAME_MAX bytes. For long snapshot names, a
> large inode suffix can push the final encoded name past NAME_MAX even
> though the encrypted prefix stayed within the documented 240-byte
> budget.
> 
> Format the suffix into a small local buffer first and reject names
> whose suffix would exceed the caller's NAME_MAX output buffer.
> 
> Signed-off-by: Pengpeng Hou <pengpeng@xxxxxxxxxxx>
> ---
> Changes since v1:
> - replace the raw suffix-size constants with a named maximum
> - drop the impossible negative snprintf() check
> - keep the NAME_MAX bound check local to the formatted suffix length
> 
> fs/ceph/crypto.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> index f3de43ccb470..7712557660c3 100644
> --- a/fs/ceph/crypto.c
> +++ b/fs/ceph/crypto.c
> @@ -15,6 +15,8 @@
>  #include "mds_client.h"
>  #include "crypto.h"
>  
> +#define CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX	sizeof("_18446744073709551615")

These define is much better. But I am still thinking could we have a more
elegant solution here? :) Do you have any ideas? Maybe, do we need to introduce
some macro DECIMAL_DIGITS_MAX()? At minimum, we need to have a comment here.

> +
>  static int ceph_crypt_get_context(struct inode *inode, void *ctx, size_t len)
>  {
>  	struct ceph_inode_info *ci = ceph_inode(inode);
> @@ -271,8 +273,19 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
>  
>  	/* To understand the 240 limit, see CEPH_NOHASH_NAME_MAX comments */
>  	WARN_ON(elen > 240);
> -	if (dir != parent) // leading _ is already there; append _<inum>
> -		elen += 1 + sprintf(p + elen, "_%ld", dir->i_ino);
> +	if (dir != parent) {
> +		/* leading '_' is already there; append _<inum> */
> +		char suffix[CEPH_ENCRYPTED_SNAP_INO_SUFFIX_MAX];
> +
> +		ret = snprintf(suffix, sizeof(suffix), "_%lu", dir->i_ino);
> +		if (ret >= sizeof(suffix) || ret >= NAME_MAX - elen) {

It looks like that ret >= sizeof(suffix) is dead code. The sizeof(suffix) = 22.
The maximum possible suffix is _18446744073709551615 = 21 chars → snprintf
returns 21, never ≥ 22.

> +			elen = -ENAMETOOLONG;
> +			goto out;
> +		}
> +
> +		memcpy(p + elen, suffix, ret);
> +		elen += ret + 1;

I believe we need to have a comment here. The +1 is not for the NUL — it
accounts for the leading _ at buf[0]. Without a comment it could be confusing.

Thanks,
Slava.

> +	}
>  
>  out:
>  	kfree(cryptbuf);