Re: [PATCH] ALSA: hda/generic: Replace strlcat() with strscpy()

[Ian Bridges <icb@fastmail.org>]

On Wed, Jul 15, 2026 at 09:54:56AM +0200, Takashi Iwai wrote:
> On Tue, 14 Jul 2026 21:36:37 +0200,
> Ian Bridges wrote:
> > 
> > In preparation for removing the strlcat() API[1], replace its two
> > uses in the generic parser.
> > 
> > Both call sites append one suffix to a string that the function has
> > already bounded to the buffer size. A strscpy() anchored at the
> > current end of the string writes the same bytes, including when the
> > suffix is truncated.
> > 
> > Link: https://github.com/KSPP/linux/issues/370 [1]
> > Signed-off-by: Ian Bridges <icb@fastmail.org>
> > ---
> > The patch was tested as follows.
> > 
> > - W=1 build of sound/hda/, zero warnings.
> > - A userspace differential harness compiled the old and the new
> >   functions side by side. The buffers were byte identical over their
> >   full length, including the bytes after the terminator.
> > - A QEMU runtime compare. A guest with an emulated HDA codec was
> >   booted on the base and on the patched kernel, and the captured
> >   /proc/asound state was byte identical. That covers the PCM stream
> >   names from fill_pcm_stream_name() live.
> > 
> >  sound/hda/codecs/generic.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/sound/hda/codecs/generic.c b/sound/hda/codecs/generic.c
> > index 660a9f2c0ded..5f373cbf4a53 100644
> > --- a/sound/hda/codecs/generic.c
> > +++ b/sound/hda/codecs/generic.c
> > @@ -2712,10 +2712,12 @@ static void get_jack_mode_name(struct hda_codec *codec, hda_nid_t pin,
> >  			       char *name, size_t name_len)
> >  {
> >  	struct hda_gen_spec *spec = codec->spec;
> > +	size_t used;
> >  	int idx = 0;
> >  
> >  	snd_hda_get_pin_label(codec, pin, &spec->autocfg, name, name_len, &idx);
> > -	strlcat(name, " Jack Mode", name_len);
> > +	used = strnlen(name, name_len);
> > +	strscpy(name + used, " Jack Mode", name_len - used);
> >  
> >  	for (; find_kctl_name(codec, name, idx); idx++)
> >  		;
> > @@ -5682,6 +5684,7 @@ static const struct hda_pcm_stream dyn_adc_pcm_analog_capture = {
> >  static void fill_pcm_stream_name(char *str, size_t len, const char *sfx,
> >  				 const char *chip_name)
> >  {
> > +	size_t used;
> >  	char *p;
> >  
> >  	if (*str)
> > @@ -5695,7 +5698,8 @@ static void fill_pcm_stream_name(char *str, size_t len, const char *sfx,
> >  			break;
> >  		}
> >  	}
> > -	strlcat(str, sfx, len);
> > +	used = strnlen(str, len);
> > +	strscpy(str + used, sfx, len - used);
> >  }
> >  
> >  /* copy PCM stream info from @default_str, and override non-NULL entries
> 
> Hmm...  IMHO, those changes are *too ugly*.
> 
> Is the reason to drop strlcat() is only about the performance, no?
> If it were some security hardening, I'd find OK, but if not, this kind
> of change is doubtful: the code path is no hot path, hence the effect
> is nothing, and yet readability becomes significantly worse.
> 
> 
> thanks,
> 
> Takashi

I'm not a core contributor to the Linux hardening project, so I can't
speak on their behalf. However, my understanding is that strlcat()
removal is a hardening target and not purely motivated by
performance. strlcat() is prone to out of bounds reads and to out
of bounds writes when the supplied size does not match the buffer.
Even the fortified version can only catch those when the buffer
sizes are visible to the compiler. Its kernel-doc in
include/linux/fortify-string.h says "Do not use this function".

The readability point is fair. A v2 could keep every call site to
one line with a small local helper:

	hda_append_suffix(name, " Jack Mode", name_len);

A seq_buf conversion would also be possible.

Would either shape be acceptable, or would you rather this file be
left as is?

Thanks,
Ian