diff mbox series

ASoC: 88pm860x: refactor deprecated strncpy

Message ID 20230727-sound-soc-codecs-v1-1-562fa2836bf4@google.com (mailing list archive)
State Accepted
Commit a9a65b87a5553a4ecabad7093ef6a1088bb71b88
Headers show
Series ASoC: 88pm860x: refactor deprecated strncpy | expand

Commit Message

Justin Stitt July 27, 2023, 10:46 p.m. UTC 
`strncpy` is deprecated for use on NUL-terminated destination strings [1].

A suitable replacement is `strscpy` [2] due to the fact that it
guarantees NUL-termination on its destination buffer argument which is
_not_ always the case for `strncpy`!

In this case, though, there was care taken to ensure that the
destination buffer would be NUL-terminated. The destination buffer is
zero-initialized and each `pm860x->name[i]` has a size of
`MAX_NAME_LENGTH + 1`. This means that there is unlikely to be a bug
here.

However, in an attempt to eliminate the usage of the `strncpy` API as
well as disambiguate implementations, replacements such as: `strscpy`,
`strscpy_pad`, `strtomem` and `strtomem_pad` should be preferred.

We are able to eliminate the need for `len + 1` since `strscpy`
guarantees NUL-termination for its destination buffer as per its
implementation [3]:

|       /* Hit buffer length without finding a NUL; force NUL-termination. */
|       if (res)
| 	        dest[res-1] = '\0';

[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
[3]: https://elixir.bootlin.com/linux/v6.3/source/lib/string.c#L183

Link: https://github.com/KSPP/linux/issues/90
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
 sound/soc/codecs/88pm860x-codec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


---
base-commit: 57012c57536f8814dec92e74197ee96c3498d24e
change-id: 20230727-sound-soc-codecs-947fcb9536a7

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

Comments

Mark Brown July 28, 2023, 4:35 p.m. UTC | #1 
On Thu, 27 Jul 2023 22:46:13 +0000, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
> 
> A suitable replacement is `strscpy` [2] due to the fact that it
> guarantees NUL-termination on its destination buffer argument which is
> _not_ always the case for `strncpy`!
> 
> In this case, though, there was care taken to ensure that the
> destination buffer would be NUL-terminated. The destination buffer is
> zero-initialized and each `pm860x->name[i]` has a size of
> `MAX_NAME_LENGTH + 1`. This means that there is unlikely to be a bug
> here.
> 
> [...]

Applied to

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

Thanks!

[1/1] ASoC: 88pm860x: refactor deprecated strncpy
      commit: a9a65b87a5553a4ecabad7093ef6a1088bb71b88

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
Kees Cook July 28, 2023, 9:05 p.m. UTC | #2 
On Thu, Jul 27, 2023 at 10:46:13PM +0000, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
> 
> A suitable replacement is `strscpy` [2] due to the fact that it
> guarantees NUL-termination on its destination buffer argument which is
> _not_ always the case for `strncpy`!
> 
> In this case, though, there was care taken to ensure that the
> destination buffer would be NUL-terminated. The destination buffer is
> zero-initialized and each `pm860x->name[i]` has a size of
> `MAX_NAME_LENGTH + 1`. This means that there is unlikely to be a bug
> here.
> 
> However, in an attempt to eliminate the usage of the `strncpy` API as
> well as disambiguate implementations, replacements such as: `strscpy`,
> `strscpy_pad`, `strtomem` and `strtomem_pad` should be preferred.
> 
> We are able to eliminate the need for `len + 1` since `strscpy`
> guarantees NUL-termination for its destination buffer as per its
> implementation [3]:
> 
> |       /* Hit buffer length without finding a NUL; force NUL-termination. */
> |       if (res)
> | 	        dest[res-1] = '\0';
> 
> [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
> [3]: https://elixir.bootlin.com/linux/v6.3/source/lib/string.c#L183
> 
> Link: https://github.com/KSPP/linux/issues/90
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
>  sound/soc/codecs/88pm860x-codec.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/codecs/88pm860x-codec.c b/sound/soc/codecs/88pm860x-codec.c
> index 3574c68e0dda..d99b674d574b 100644
> --- a/sound/soc/codecs/88pm860x-codec.c
> +++ b/sound/soc/codecs/88pm860x-codec.c
> @@ -143,7 +143,7 @@ struct pm860x_priv {
>  	struct pm860x_det	det;
>  
>  	int			irq[4];
> -	unsigned char		name[4][MAX_NAME_LEN+1];
> +	unsigned char		name[4][MAX_NAME_LEN];
>  };
>  
>  /* -9450dB to 0dB in 150dB steps ( mute instead of -9450dB) */
> @@ -1373,7 +1373,7 @@ static int pm860x_codec_probe(struct platform_device *pdev)
>  			return -EINVAL;
>  		}
>  		pm860x->irq[i] = res->start + chip->irq_base;
> -		strncpy(pm860x->name[i], res->name, MAX_NAME_LEN);
> +		strscpy(pm860x->name[i], res->name, MAX_NAME_LEN);

res->name is (perhaps) unbounded in length:

struct resource {
	...
        const char *name;
	...
};

So reducing struct pm860x_priv::name's size _might_ have a user-visible
effect, but probably not.

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

-Kees
diff mbox series

Patch

diff --git a/sound/soc/codecs/88pm860x-codec.c b/sound/soc/codecs/88pm860x-codec.c
index 3574c68e0dda..d99b674d574b 100644
--- a/sound/soc/codecs/88pm860x-codec.c
+++ b/sound/soc/codecs/88pm860x-codec.c
@@ -143,7 +143,7 @@  struct pm860x_priv {
 	struct pm860x_det	det;
 
 	int			irq[4];
-	unsigned char		name[4][MAX_NAME_LEN+1];
+	unsigned char		name[4][MAX_NAME_LEN];
 };
 
 /* -9450dB to 0dB in 150dB steps ( mute instead of -9450dB) */
@@ -1373,7 +1373,7 @@  static int pm860x_codec_probe(struct platform_device *pdev)
 			return -EINVAL;
 		}
 		pm860x->irq[i] = res->start + chip->irq_base;
-		strncpy(pm860x->name[i], res->name, MAX_NAME_LEN);
+		strscpy(pm860x->name[i], res->name, MAX_NAME_LEN);
 	}
 
 	ret = devm_snd_soc_register_component(&pdev->dev,