| Message ID | 20230727-sound-xen-v1-1-89dd161351f1@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 44900c3ee4a1047bf896ca4cd9a9c69000f04701 |
| Headers | show |
| Series | ALSA: xen-front: refactor deprecated strncpy | expand |
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.
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 <keescook@chromium.org>
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 <justinstitt@google.com> Applied now. Thanks. Takashi
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)
`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 <justinstitt@google.com> --- 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 <justinstitt@google.com>