[PATCH] ASoC: intel: avs: refactor strncpy usage in topology

[PATCH] ASoC: intel: avs: refactor strncpy usage in topology

[justinstitt]

`strncpy` is deprecated for use on NUL-terminated destination strings
[1].

A suitable replacement is `strscpy` [2].

There are some hopes that someday the `strncpy` api could be ripped out
due to the vast number of suitable replacements (strscpy, strscpy_pad,
strtomem, strtomem_pad, strlcpy) [1].

[1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
[2]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html

---


Link: https://github.com/KSPP/linux/issues/90
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
 sound/soc/intel/avs/topology.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sound/soc/intel/avs/topology.c b/sound/soc/intel/avs/topology.c
index cdb4ec500261..45d0eb2a8e71 100644
--- a/sound/soc/intel/avs/topology.c
+++ b/sound/soc/intel/avs/topology.c
@@ -1388,12 +1388,12 @@ static int avs_route_load(struct snd_soc_component *comp, int index,
 		port = __ffs(mach->mach_params.i2s_link_mask);
 
 		snprintf(buf, len, route->source, port);
-		strncpy((char *)route->source, buf, len);
+		strscpy((char *)route->source, buf, len);
 		snprintf(buf, len, route->sink, port);
-		strncpy((char *)route->sink, buf, len);
+		strscpy((char *)route->sink, buf, len);
 		if (route->control) {
 			snprintf(buf, len, route->control, port);
-			strncpy((char *)route->control, buf, len);
+			strscpy((char *)route->control, buf, len);
 		}
 	}
 

---
base-commit: 0b4a9fdc9317440a71d4d4c264a5650bf4a90f3c
change-id: 20230725-sound-soc-intel-avs-remove-deprecated-strncpy-2bc41a5a5f81

Best regards,
-- 
Justin Stitt <justinstitt@google.com>

Re: [PATCH] ASoC: intel: avs: refactor strncpy usage in topology

[Kees Cook]

On Tue, Jul 25, 2023 at 10:08:38PM +0000, justinstitt@google.com wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings
> [1].
> 
> A suitable replacement is `strscpy` [2].

For future patches like this, can you include the rationale for _why_
strscpy() is suitable? (i.e. how did you check that it doesn't need
zero-padding, the dest is expected to always be NUL-terminated, etc.)

I had fun looking through this code -- it's a bit of a maze! I can see
in the initializer for "route" (soc_tplg_dapm_graph_elems_load()), that
all the strings pointed at from "elem" are being checked for NUL-termination.

That this code is doing an in-place rewrite of the string is pretty
unusual (i.e. specifically casting around the "const"), but it looks
quite intentional. :)

> There are some hopes that someday the `strncpy` api could be ripped out

This can be rephrased, perhaps, as:

There is a goal to eliminate the `strncpy` API in the kernel, as its
use is too ambiguous, instead moving to the unambiguous replacements
(strscpy, strscpy_pad, strtomem, strtomem_pad, strlcpy) [1].

> due to the vast number of suitable replacements (strscpy, strscpy_pad,
> strtomem, strtomem_pad, strlcpy) [1].
> 
> [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> [2]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html
> 
> ---
^^^^ this triple-dash is going to cause some tooling to lose your S-o-b,
as it indicates the end of the commit log.

> 
> 
> Link: https://github.com/KSPP/linux/issues/90
> Signed-off-by: Justin Stitt <justinstitt@google.com>

But otherwise, looks good:

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  sound/soc/intel/avs/topology.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/soc/intel/avs/topology.c b/sound/soc/intel/avs/topology.c
> index cdb4ec500261..45d0eb2a8e71 100644
> --- a/sound/soc/intel/avs/topology.c
> +++ b/sound/soc/intel/avs/topology.c
> @@ -1388,12 +1388,12 @@ static int avs_route_load(struct snd_soc_component *comp, int index,
>  		port = __ffs(mach->mach_params.i2s_link_mask);
>  
>  		snprintf(buf, len, route->source, port);
> -		strncpy((char *)route->source, buf, len);
> +		strscpy((char *)route->source, buf, len);
>  		snprintf(buf, len, route->sink, port);
> -		strncpy((char *)route->sink, buf, len);
> +		strscpy((char *)route->sink, buf, len);
>  		if (route->control) {
>  			snprintf(buf, len, route->control, port);
> -			strncpy((char *)route->control, buf, len);
> +			strscpy((char *)route->control, buf, len);
>  		}
>  	}
>  
> 
> ---
> base-commit: 0b4a9fdc9317440a71d4d4c264a5650bf4a90f3c
> change-id: 20230725-sound-soc-intel-avs-remove-deprecated-strncpy-2bc41a5a5f81
> 
> Best regards,
> -- 
> Justin Stitt <justinstitt@google.com>
> 

-- 
Kees Cook

Re: [PATCH] ASoC: intel: avs: refactor strncpy usage in topology

[Amadeusz Sławiński]

On 7/26/2023 12:08 AM, justinstitt@google.com wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings
> [1].
> 
> A suitable replacement is `strscpy` [2].
> 
> There are some hopes that someday the `strncpy` api could be ripped out
> due to the vast number of suitable replacements (strscpy, strscpy_pad,
> strtomem, strtomem_pad, strlcpy) [1].
> 
> [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> [2]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html
> 
> ---
> 
> 
> Link: https://github.com/KSPP/linux/issues/90
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
>   sound/soc/intel/avs/topology.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/soc/intel/avs/topology.c b/sound/soc/intel/avs/topology.c
> index cdb4ec500261..45d0eb2a8e71 100644
> --- a/sound/soc/intel/avs/topology.c
> +++ b/sound/soc/intel/avs/topology.c
> @@ -1388,12 +1388,12 @@ static int avs_route_load(struct snd_soc_component *comp, int index,
>   		port = __ffs(mach->mach_params.i2s_link_mask);
>   
>   		snprintf(buf, len, route->source, port);
> -		strncpy((char *)route->source, buf, len);
> +		strscpy((char *)route->source, buf, len);
>   		snprintf(buf, len, route->sink, port);
> -		strncpy((char *)route->sink, buf, len);
> +		strscpy((char *)route->sink, buf, len);
>   		if (route->control) {
>   			snprintf(buf, len, route->control, port);
> -			strncpy((char *)route->control, buf, len);
> +			strscpy((char *)route->control, buf, len);
>   		}
>   	}
>   
> 

In this case snprintf adds NUL at the end and we strncpy using same 
size, so there should always be NUL at the end, so replacing it with 
strscpy shouldn't really change anything, so

Reviewed-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>

Re: [PATCH] ASoC: intel: avs: refactor strncpy usage in topology

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 849 bytes --]

On Tue, Jul 25, 2023 at 10:08:38PM +0000, justinstitt@google.com wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings
> [1].
> 
> A suitable replacement is `strscpy` [2].
> 
> There are some hopes that someday the `strncpy` api could be ripped out
> due to the vast number of suitable replacements (strscpy, strscpy_pad,
> strtomem, strtomem_pad, strlcpy) [1].
> 
> [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> [2]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html
> 
> ---
> 
> 
> Link: https://github.com/KSPP/linux/issues/90
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---

You've put your signoff after a --- which means it gets deleted when
applied, don't do this.  The Signoff should be start of the main
changelog.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: intel: avs: refactor strncpy usage in topology

[Mark Brown]

On Tue, 25 Jul 2023 22:08:38 +0000, justinstitt@google.com wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings
> [1].
> 
> A suitable replacement is `strscpy` [2].
> 
> There are some hopes that someday the `strncpy` api could be ripped out
> due to the vast number of suitable replacements (strscpy, strscpy_pad,
> strtomem, strtomem_pad, strlcpy) [1].
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: intel: avs: refactor strncpy usage in topology
      commit: f6500ec12c1ec745fbec20fd4734b832bbfd4aac

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark