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

[Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>]

On Fri, 2026-04-03 at 16:56 +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, the
> returned length also includes the leading underscore that stays in place
> ahead of the encoded text. On 64-bit kernels, a long inode suffix can
> push the final encoded name past `NAME_MAX` even though the encrypted
> prefix itself 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@iscas.ac.cn>
> ---
>  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..eeba8ffb0554 100644
> --- a/fs/ceph/crypto.c
> +++ b/fs/ceph/crypto.c
> @@ -208,6 +208,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
>  	struct ceph_client *cl = ceph_inode_to_client(parent);
>  	struct inode *dir = parent;
>  	char *p = buf;
> +	char suffix[1 + 20 + 1];

I really dislike these hardcoded constants. And I don't quite follow what 1, 20,
and 1 means. Why these numbers? Please, introduce the named constants.

>  	u32 len;
>  	int name_len = elen;
>  	int ret;
> @@ -271,8 +272,20 @@ 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>
> +		ret = snprintf(suffix, sizeof(suffix), "_%lu", dir->i_ino);
> +		if (ret < 0) {

The snprintf never returns negative. Are you sure that this check makes sense?

/**
 * sprintf - Format a string and place it in a buffer
 * @buf: The buffer to place the result into
 * @fmt: The format string to use
 * @...: Arguments for the format string
 *
 * The return value is the number of characters written into @buf not including
 * the trailing '\0'. Use snprintf() or scnprintf() in order to avoid
 * buffer overflows.
 *
 * See the vsnprintf() documentation for format string extensions over C99.
 */

> +			elen = ret;
> +			goto out;
> +		}
> +		if (ret >= NAME_MAX - elen) {

Technically speaking, we have only WARN_ON(elen > 240) check before. But it
doesn't prevent elen to be bigger than elen. So, potentially, this logic could
be dangerous enough because we could have overflow here.

Thanks,
Slava.

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