[PATCH v2] keys: Replace deprecated strncpy in ecryptfs_fill_auth_tok

[PATCH v2] keys: Replace deprecated strncpy in ecryptfs_fill_auth_tok

[Thorsten Blum]

strncpy() is deprecated for NUL-terminated destination buffers; use
strscpy_pad() instead to retain the zero-padding behavior of strncpy().

strscpy_pad() automatically determines the size of the fixed-length
destination buffer via sizeof() when the optional size argument is
omitted, making an explicit size unnecessary.

In encrypted_init(), the source string 'key_desc' is validated by
valid_ecryptfs_desc() before calling ecryptfs_fill_auth_tok(), and is
therefore NUL-terminated and satisfies the __must_be_cstr() requirement
of strscpy_pad().

No functional changes.

Link: https://github.com/KSPP/linux/issues/90
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
Changes in v2:
- Improve commit message as suggested by Jarkko and Kees
- Link to v1: https://lore.kernel.org/lkml/20251009180316.394708-3-thorsten.blum@linux.dev/
---
 security/keys/encrypted-keys/ecryptfs_format.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/security/keys/encrypted-keys/ecryptfs_format.c b/security/keys/encrypted-keys/ecryptfs_format.c
index 8fdd76105ce3..2fc6f3a66135 100644
--- a/security/keys/encrypted-keys/ecryptfs_format.c
+++ b/security/keys/encrypted-keys/ecryptfs_format.c
@@ -54,8 +54,7 @@ int ecryptfs_fill_auth_tok(struct ecryptfs_auth_tok *auth_tok,
 	auth_tok->version = (((uint16_t)(major << 8) & 0xFF00)
 			     | ((uint16_t)minor & 0x00FF));
 	auth_tok->token_type = ECRYPTFS_PASSWORD;
-	strncpy((char *)auth_tok->token.password.signature, key_desc,
-		ECRYPTFS_PASSWORD_SIG_SIZE);
+	strscpy_pad(auth_tok->token.password.signature, key_desc);
 	auth_tok->token.password.session_key_encryption_key_bytes =
 		ECRYPTFS_MAX_KEY_BYTES;
 	/*
-- 
2.51.0

Re: [PATCH v2] keys: Replace deprecated strncpy in ecryptfs_fill_auth_tok

[Jarkko Sakkinen]

On Fri, Oct 10, 2025 at 06:13:41PM +0200, Thorsten Blum wrote:
> strncpy() is deprecated for NUL-terminated destination buffers; use
> strscpy_pad() instead to retain the zero-padding behavior of strncpy().
> 
> strscpy_pad() automatically determines the size of the fixed-length
> destination buffer via sizeof() when the optional size argument is
> omitted, making an explicit size unnecessary.
> 
> In encrypted_init(), the source string 'key_desc' is validated by
> valid_ecryptfs_desc() before calling ecryptfs_fill_auth_tok(), and is
> therefore NUL-terminated and satisfies the __must_be_cstr() requirement
> of strscpy_pad().
> 
> No functional changes.

It's a functional change (for better!) because it transforms to safer
semantics ;-) And yeah as years pass by commit messages like these
have more value than code changes themselves (as far backtracking
and bisecting is concerned).

So if you don't mind, I'll delete the very last one sentence paragraph,
and with that

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

Thank you.

> 
> Link: https://github.com/KSPP/linux/issues/90
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
> Changes in v2:
> - Improve commit message as suggested by Jarkko and Kees
> - Link to v1: https://lore.kernel.org/lkml/20251009180316.394708-3-thorsten.blum@linux.dev/
> ---
>  security/keys/encrypted-keys/ecryptfs_format.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/security/keys/encrypted-keys/ecryptfs_format.c b/security/keys/encrypted-keys/ecryptfs_format.c
> index 8fdd76105ce3..2fc6f3a66135 100644
> --- a/security/keys/encrypted-keys/ecryptfs_format.c
> +++ b/security/keys/encrypted-keys/ecryptfs_format.c
> @@ -54,8 +54,7 @@ int ecryptfs_fill_auth_tok(struct ecryptfs_auth_tok *auth_tok,
>  	auth_tok->version = (((uint16_t)(major << 8) & 0xFF00)
>  			     | ((uint16_t)minor & 0x00FF));
>  	auth_tok->token_type = ECRYPTFS_PASSWORD;
> -	strncpy((char *)auth_tok->token.password.signature, key_desc,
> -		ECRYPTFS_PASSWORD_SIG_SIZE);
> +	strscpy_pad(auth_tok->token.password.signature, key_desc);
>  	auth_tok->token.password.session_key_encryption_key_bytes =
>  		ECRYPTFS_MAX_KEY_BYTES;
>  	/*
> -- 
> 2.51.0
> 

BR, Jarkko

Re: [PATCH v2] keys: Replace deprecated strncpy in ecryptfs_fill_auth_tok

[Kees Cook]

On Fri, Oct 10, 2025 at 06:13:41PM +0200, Thorsten Blum wrote:
> strncpy() is deprecated for NUL-terminated destination buffers; use
> strscpy_pad() instead to retain the zero-padding behavior of strncpy().
> 
> strscpy_pad() automatically determines the size of the fixed-length
> destination buffer via sizeof() when the optional size argument is
> omitted, making an explicit size unnecessary.

I would explicitly say that the old code was NUL terminating the buffer
due to it being "ECRYPTFS_PASSWORD_SIG_SIZE + 1" sized with strncpy
left to fill ECRYPTFS_PASSWORD_SIG_SIZE. And then you have to answer the
question, "how was this initialized?" and trace it back to:

        epayload = kzalloc(sizeof(*epayload) + payload_datalen +
                           datablob_len + HASH_SIZE + 1, GFP_KERNEL);

so the final byte was always being zeroed there, but now we're
explicitly zeroing it (good). So there _is_ a functional change (we're
writing 1 more byte here now), but it's more robust that way. There is
no expected _logical_ change, though, yes.

> 
> In encrypted_init(), the source string 'key_desc' is validated by
> valid_ecryptfs_desc() before calling ecryptfs_fill_auth_tok(), and is
> therefore NUL-terminated and satisfies the __must_be_cstr() requirement
> of strscpy_pad().
> 
> No functional changes.
> 
> Link: https://github.com/KSPP/linux/issues/90
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>

With "ECRYPTFS_PASSWORD_SIG_SIZE + 1" and tracing of the destination
buffer initialization added to the commit log:

Reviewed-by: Kees Cook <kees@kernel.org>

-Kees

-- 
Kees Cook

Re: [PATCH v2] keys: Replace deprecated strncpy in ecryptfs_fill_auth_tok

[Jarkko Sakkinen]

On Fri, Oct 10, 2025 at 03:48:48PM -0700, Kees Cook wrote:
> On Fri, Oct 10, 2025 at 06:13:41PM +0200, Thorsten Blum wrote:
> > strncpy() is deprecated for NUL-terminated destination buffers; use
> > strscpy_pad() instead to retain the zero-padding behavior of strncpy().
> > 
> > strscpy_pad() automatically determines the size of the fixed-length
> > destination buffer via sizeof() when the optional size argument is
> > omitted, making an explicit size unnecessary.
> 
> I would explicitly say that the old code was NUL terminating the buffer
> due to it being "ECRYPTFS_PASSWORD_SIG_SIZE + 1" sized with strncpy
> left to fill ECRYPTFS_PASSWORD_SIG_SIZE. And then you have to answer the
> question, "how was this initialized?" and trace it back to:
> 
>         epayload = kzalloc(sizeof(*epayload) + payload_datalen +
>                            datablob_len + HASH_SIZE + 1, GFP_KERNEL);
> 
> so the final byte was always being zeroed there, but now we're
> explicitly zeroing it (good). So there _is_ a functional change (we're
> writing 1 more byte here now), but it's more robust that way. There is
> no expected _logical_ change, though, yes.

Thanks for the remarks.

Thorsten, would you mind posting +1 with the commit message changes,
and reviewed-by tags (from me and Kees).

> 
> > 
> > In encrypted_init(), the source string 'key_desc' is validated by
> > valid_ecryptfs_desc() before calling ecryptfs_fill_auth_tok(), and is
> > therefore NUL-terminated and satisfies the __must_be_cstr() requirement
> > of strscpy_pad().
> > 
> > No functional changes.

[just as reminder: removing this sentence was my earlier remark]

> > 
> > Link: https://github.com/KSPP/linux/issues/90
> > Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> 
> With "ECRYPTFS_PASSWORD_SIG_SIZE + 1" and tracing of the destination
> buffer initialization added to the commit log:
> 
> Reviewed-by: Kees Cook <kees@kernel.org>
> 
> -Kees
> 
> -- 
> Kees Cook

BR, Jarkko