ALSA: xen-front: refactor deprecated strncpy

Commit Message

Justin Stitt July 27, 2023, 9:53 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`!

It should be noted that, in this case, the destination buffer has a
length strictly greater than the source string. Moreover, the source
string is NUL-terminated (and so is the destination) which means there
was no real bug happening here. Nonetheless, this patch would get us one
step closer to eliminating the `strncpy` API in the kernel, as its use
is too ambiguous. We need to favor less ambiguous replacements such as:
strscpy, strscpy_pad, strtomem and strtomem_pad (amongst others).

Technically, my patch yields subtly different behavior. The original
implementation with `strncpy` would fill the entire destination buffer
with null bytes [3] while `strscpy` will leave the junk, uninitialized
bytes trailing after the _mandatory_ NUL-termination. So, if somehow
`pcm->name` or `card->driver/shortname/longname` require this
NUL-padding behavior then `strscpy_pad` should be used. My
interpretation, though, is that the aforementioned fields are just fine
as NUL-terminated strings. Please correct my assumptions if needed and
I'll send in a v2.

[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://linux.die.net/man/3/strncpy

Link: https://github.com/KSPP/linux/issues/90
Signed-off-by: Justin Stitt <[e-mail]>
---
 sound/xen/xen_snd_front_alsa.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)


---
base-commit: 57012c57536f8814dec92e74197ee96c3498d24e
change-id: 20230727-sound-xen-398c9927b3d8

Best regards,
--
Justin Stitt <[e-mail]>

Comments

Elliott Mitchell July 28, 2023, 4:46 a.m. UTC | #1

On Thu, Jul 27, 2023 at 09:53:24PM +0000, Justin Stitt wrote:
> Technically, my patch yields subtly different behavior. The original
> implementation with `strncpy` would fill the entire destination buffer
> with null bytes [3] while `strscpy` will leave the junk, uninitialized
> bytes trailing after the _mandatory_ NUL-termination. So, if somehow
> `pcm->name` or `card->driver/shortname/longname` require this
> NUL-padding behavior then `strscpy_pad` should be used. My
> interpretation, though, is that the aforementioned fields are just fine
> as NUL-terminated strings. Please correct my assumptions if needed and
> I'll send in a v2.

"uninitialized bytes" => "leak of sensitive information" => "security hole"

One hopes the unitialized bytes don't contain sensitive information, but
that is the start of the chain.  One can hope the VM on the other end is
friendly, but that isn't something to rely on.

I'm not in charge of any of the appropriate subsystems, I just happened
to randomly look at this as message on a mailing list I'm on.  Could be
the maintainers will find this acceptable.

Kees Cook July 28, 2023, 6:38 p.m. UTC | #2

On Thu, Jul 27, 2023 at 09:46:36PM -0700, Elliott Mitchell wrote:
> On Thu, Jul 27, 2023 at 09:53:24PM +0000, Justin Stitt wrote:
> > Technically, my patch yields subtly different behavior. The original
> > implementation with `strncpy` would fill the entire destination buffer
> > with null bytes [3] while `strscpy` will leave the junk, uninitialized
> > bytes trailing after the _mandatory_ NUL-termination. So, if somehow
> > `pcm->name` or `card->driver/shortname/longname` require this
> > NUL-padding behavior then `strscpy_pad` should be used. My
> > interpretation, though, is that the aforementioned fields are just fine
> > as NUL-terminated strings. Please correct my assumptions if needed and
> > I'll send in a v2.
> 
> "uninitialized bytes" => "leak of sensitive information" => "security hole"

For xen_snd_front_alsa_init(), "card" is already zero-initialized in
snd_card_new().

For new_pcm_instance(), "pcm" is already zero-initialized in
_snd_pcm_new().

So things look good to me!

Reviewed-by: Kees Cook <[e-mail]>

Takashi Iwai July 29, 2023, noon UTC | #3

On Thu, 27 Jul 2023 23:53:24 +0200,
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`!
> 
> It should be noted that, in this case, the destination buffer has a
> length strictly greater than the source string. Moreover, the source
> string is NUL-terminated (and so is the destination) which means there
> was no real bug happening here. Nonetheless, this patch would get us one
> step closer to eliminating the `strncpy` API in the kernel, as its use
> is too ambiguous. We need to favor less ambiguous replacements such as:
> strscpy, strscpy_pad, strtomem and strtomem_pad (amongst others).
> 
> Technically, my patch yields subtly different behavior. The original
> implementation with `strncpy` would fill the entire destination buffer
> with null bytes [3] while `strscpy` will leave the junk, uninitialized
> bytes trailing after the _mandatory_ NUL-termination. So, if somehow
> `pcm->name` or `card->driver/shortname/longname` require this
> NUL-padding behavior then `strscpy_pad` should be used. My
> interpretation, though, is that the aforementioned fields are just fine
> as NUL-terminated strings. Please correct my assumptions if needed and
> I'll send in a v2.
> 
> [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://linux.die.net/man/3/strncpy
> 
> Link: https://github.com/KSPP/linux/issues/90
> Signed-off-by: Justin Stitt <[e-mail]>

Applied now.  Thanks.


Takashi

Patch

diff --git a/sound/xen/xen_snd_front_alsa.c b/sound/xen/xen_snd_front_alsa.c
index db917453a473..7a3dfce97c15 100644
--- a/sound/xen/xen_snd_front_alsa.c
+++ b/sound/xen/xen_snd_front_alsa.c
@@ -783,7 +783,7 @@  static int new_pcm_instance(struct xen_snd_front_card_info *card_info,
 	pcm->info_flags = 0;
 	/* we want to handle all PCM operations in non-atomic context */
 	pcm->nonatomic = true;
-	strncpy(pcm->name, "Virtual card PCM", sizeof(pcm->name));
+	strscpy(pcm->name, "Virtual card PCM", sizeof(pcm->name));
 
 	if (instance_cfg->num_streams_pb)
 		snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK,
@@ -835,9 +835,9 @@  int xen_snd_front_alsa_init(struct xen_snd_front_info *front_info)
 			goto fail;
 	}
 
-	strncpy(card->driver, XENSND_DRIVER_NAME, sizeof(card->driver));
-	strncpy(card->shortname, cfg->name_short, sizeof(card->shortname));
-	strncpy(card->longname, cfg->name_long, sizeof(card->longname));
+	strscpy(card->driver, XENSND_DRIVER_NAME, sizeof(card->driver));
+	strscpy(card->shortname, cfg->name_short, sizeof(card->shortname));
+	strscpy(card->longname, cfg->name_long, sizeof(card->longname));
 
 	ret = snd_card_register(card);
 	if (ret < 0)