[PATCH 0/7] ALSA: remove remaining strlcat() users under sound/

Mahad Ibrahim posted 7 patches 1 month, 3 weeks ago
sound/core/ump.c            |  7 +++++--
sound/pci/ac97/ac97_codec.c |  8 ++++----
sound/pci/cmipci.c          |  8 ++++++--
sound/usb/caiaq/input.c     |  5 +++--
sound/usb/card.c            | 27 +++++++++++++++++----------
sound/usb/hiface/chip.c     |  4 ++--
sound/usb/mixer.c           |  8 ++++++--
7 files changed, 43 insertions(+), 24 deletions(-)
[PATCH 0/7] ALSA: remove remaining strlcat() users under sound/
Posted by Mahad Ibrahim 1 month, 3 weeks ago
strlcat() is deprecated and slated for removal once its remaining
users are converted.  This series converts the sound/ users outside
of ASoC.

Each site is converted to an explicit offset plus a bounded copy:
strscpy() where the appended text has no format specifiers, and
scnprintf() where it does or where the resulting length is needed.

After this series the only remaining strlcat() user under sound/ is
sound/soc/codecs/wm_adsp_fw_find_test.c, which goes via ASoC.

Build-tested with allmodconfig and boot-tested on x86_64.

The generated strings were checked against the pre-patch code in a
userspace harness for empty, whitespace-padded, exact-fit and
oversized inputs, and came out identical.  No audio hardware was
available here, so the drivers themselves have not been exercised at
runtime.

Mahad Ibrahim (7):
  ALSA: ump: replace strlcat() with strscpy()
  ALSA: ac97: replace strlcat() with scnprintf()
  ALSA: cmipci: replace strlcat() with strscpy()
  ALSA: caiaq: replace strlcat() with strscpy()
  ALSA: usb-audio: replace strlcat() with append_ctl_name()
  ALSA: hiface: replace strlcat() with scnprintf()
  ALSA: usb-audio: replace strlcat() in longname construction

 sound/core/ump.c            |  7 +++++--
 sound/pci/ac97/ac97_codec.c |  8 ++++----
 sound/pci/cmipci.c          |  8 ++++++--
 sound/usb/caiaq/input.c     |  5 +++--
 sound/usb/card.c            | 27 +++++++++++++++++----------
 sound/usb/hiface/chip.c     |  4 ++--
 sound/usb/mixer.c           |  8 ++++++--
 7 files changed, 43 insertions(+), 24 deletions(-)

-- 
2.54.0
Re: [PATCH 0/7] ALSA: remove remaining strlcat() users under sound/
Posted by Takashi Iwai 1 month, 3 weeks ago
On Fri, 07 Aug 2026 13:41:32 +0200,
Mahad Ibrahim wrote:
> 
> strlcat() is deprecated and slated for removal once its remaining
> users are converted.  This series converts the sound/ users outside
> of ASoC.
> 
> Each site is converted to an explicit offset plus a bounded copy:
> strscpy() where the appended text has no format specifiers, and
> scnprintf() where it does or where the resulting length is needed.
> 
> After this series the only remaining strlcat() user under sound/ is
> sound/soc/codecs/wm_adsp_fw_find_test.c, which goes via ASoC.
> 
> Build-tested with allmodconfig and boot-tested on x86_64.
> 
> The generated strings were checked against the pre-patch code in a
> userspace harness for empty, whitespace-padded, exact-fit and
> oversized inputs, and came out identical.  No audio hardware was
> available here, so the drivers themselves have not been exercised at
> runtime.
> 
> Mahad Ibrahim (7):
>   ALSA: ump: replace strlcat() with strscpy()
>   ALSA: ac97: replace strlcat() with scnprintf()
>   ALSA: cmipci: replace strlcat() with strscpy()
>   ALSA: caiaq: replace strlcat() with strscpy()
>   ALSA: usb-audio: replace strlcat() with append_ctl_name()
>   ALSA: hiface: replace strlcat() with scnprintf()
>   ALSA: usb-audio: replace strlcat() in longname construction

Honestly speaking, I'm against those conversions.
Why do we have to open-code at each place with strlen()+strscpy()?
It's just harder to read than strlcat(), even more error-prone.

If an alternative is something like this, we really should reconsider.


thanks,

Takashi
Re: [PATCH 0/7] ALSA: remove remaining strlcat() users under sound/
Posted by Mahad Ibrahim 1 month, 3 weeks ago
On Fri Aug 7, 2026 at 5:15 PM PKT, Takashi Iwai wrote:
> Honestly speaking, I'm against those conversions.
> Why do we have to open-code at each place with strlen()+strscpy()?
> It's just harder to read than strlcat(), even more error-prone.
>
> If an alternative is something like this, we really should reconsider.

Thank you for the quick response and feedback.

You are right that strlen() + strscpy() is tedious and annoying to read.

sound/ already has helpers that do this, but each is local to one file
with its own signature:

  safe_append_string()  sound/core/ump.c
  append_ctl_name()     sound/usb/mixer.c
  hda_append_suffix()   sound/hda/common/hda_local.h

Each of these functions practice the same string append technique
however to slightly different effect. safe_append_string uses
safe_copy_string() whose function body is above it in the same file.
safe_copy_string() performs analogous to strscpy() however adds a
filter which drops non-printable ASCII characters during the copy phase.

append_ctl_name() is simply an strlcat wrapper which when transitioned
would result in the same strlen() + strscpy(). Only separating feature
is that it returns the length of characters that would have been
written (not necessarily the actual amount). However none of the callers
use its return functionality.

hda_append_suffix() is a verbatim copy of strlen() + strscpy().

A solution I would propose is that all these functions which do the same
thing, aside from safe_append_string, could be moved where they are
accessible globally across sound/. This would remove the redundant need
to use strlen() + strscpy() in replacement for strlcat() and would unify
the sub-system under a single string append API.

safe_append_string is only called once in the entire sub-system, and could
be replaced either within the function with the unified string
append function, and a separate filterer replacing the safe_copy_string
function or removed all together and managed inline within the single
caller. However this function would require a more involved removal as the
internal safe_copy_string is called twice; it is called once in
safe_append_string(), and in sound/core/ump.c for a wrapper function
ump_set_rawmidi_name().

An argument against this suggestion is that it would confine a string
append helper to a single sub-system, while the rest of the kernel uses
something else.

Additionally patch 1/7 shouldn't have open-coded anything at all.
safe_append_string() was already a few hundred lines above the site I
touched, and I should have used it.

This was based on https://github.com/KSPP/linux/issues/370, which I
should have linked in the cover letter.

Thank you for your time.

Best regards,
Mahad Ibrahim
Re: [PATCH 0/7] ALSA: remove remaining strlcat() users under sound/
Posted by Takashi Iwai 1 month, 3 weeks ago
On Fri, 07 Aug 2026 17:41:36 +0200,
Mahad Ibrahim wrote:
> 
> On Fri Aug 7, 2026 at 5:15 PM PKT, Takashi Iwai wrote:
> > Honestly speaking, I'm against those conversions.
> > Why do we have to open-code at each place with strlen()+strscpy()?
> > It's just harder to read than strlcat(), even more error-prone.
> >
> > If an alternative is something like this, we really should reconsider.
> 
> Thank you for the quick response and feedback.
> 
> You are right that strlen() + strscpy() is tedious and annoying to read.
> 
> sound/ already has helpers that do this, but each is local to one file
> with its own signature:
> 
>   safe_append_string()  sound/core/ump.c
>   append_ctl_name()     sound/usb/mixer.c
>   hda_append_suffix()   sound/hda/common/hda_local.h
> 
> Each of these functions practice the same string append technique
> however to slightly different effect. safe_append_string uses
> safe_copy_string() whose function body is above it in the same file.
> safe_copy_string() performs analogous to strscpy() however adds a
> filter which drops non-printable ASCII characters during the copy phase.
> 
> append_ctl_name() is simply an strlcat wrapper which when transitioned
> would result in the same strlen() + strscpy(). Only separating feature
> is that it returns the length of characters that would have been
> written (not necessarily the actual amount). However none of the callers
> use its return functionality.
> 
> hda_append_suffix() is a verbatim copy of strlen() + strscpy().
> 
> A solution I would propose is that all these functions which do the same
> thing, aside from safe_append_string, could be moved where they are
> accessible globally across sound/. This would remove the redundant need
> to use strlen() + strscpy() in replacement for strlcat() and would unify
> the sub-system under a single string append API.
> 
> safe_append_string is only called once in the entire sub-system, and could
> be replaced either within the function with the unified string
> append function, and a separate filterer replacing the safe_copy_string
> function or removed all together and managed inline within the single
> caller. However this function would require a more involved removal as the
> internal safe_copy_string is called twice; it is called once in
> safe_append_string(), and in sound/core/ump.c for a wrapper function
> ump_set_rawmidi_name().
> 
> An argument against this suggestion is that it would confine a string
> append helper to a single sub-system, while the rest of the kernel uses
> something else.
> 
> Additionally patch 1/7 shouldn't have open-coded anything at all.
> safe_append_string() was already a few hundred lines above the site I
> touched, and I should have used it.
> 
> This was based on https://github.com/KSPP/linux/issues/370, which I
> should have linked in the cover letter.
> 
> Thank you for your time.
> 
> Best regards,
> Mahad Ibrahim

Well, that leads to a basic question: why do we have to drop strlcat()
if most of callers would just need the equivalent function.

If strlcat() were super-dangerous, it's understandable to drop.  But,
it's not, and issues discussed in the github are minor and something
that can be addressed in strlcat() implementation; that is, can't we
rather re-implement strlcat() in a safer way, instead of killing it?

Sure, there are code calling strlcat() that could be optimized better.
They can be cleaned up.  But it alone can't be a reason that strlcat()
must die without mercy.


thanks,

Takashi
Re: [PATCH 0/7] ALSA: remove remaining strlcat() users under sound/
Posted by Kees Cook 1 month, 3 weeks ago
On Fri, Aug 07, 2026 at 06:03:58PM +0200, Takashi Iwai wrote:
> If strlcat() were super-dangerous, it's understandable to drop.  But,
> it's not, and issues discussed in the github are minor and something
> that can be addressed in strlcat() implementation; that is, can't we
> rather re-implement strlcat() in a safer way, instead of killing it?
> 
> Sure, there are code calling strlcat() that could be optimized better.
> They can be cleaned up.  But it alone can't be a reason that strlcat()
> must die without mercy.

The risk comes from the compiler having no way to know what the size of
the destination buffer is, as the "char *" argument has no length
associated with it. One thing we can do is change the argument
requirements for strlcat (like we did when designing memtostr, etc),
that requires that the argument explicitly be an array (not a string
pointer), at which point bounds checking can be done.

Usually this requires changing the plumbing of arguments, as a lot of C
code is used to just passing around a bare "char *", etc. And if that
re-plumbing is going to happen, it might as well be seq_buf.

But yes, just replacing it with strlen/strscpy isn't very ergonomic.
Adding the length explicitly with strscpy certainly gets us the bounds
again, but it's _separate_ from the string still, and that will lead to
mistakes too. Better to have it be part of the type (i.e. either an
array or seq_buf).

-Kees

-- 
Kees Cook
Re: [PATCH 0/7] ALSA: remove remaining strlcat() users under sound/
Posted by David Laight 1 month, 3 weeks ago
On Fri, 7 Aug 2026 14:46:44 -0700
Kees Cook <kees@kernel.org> wrote:

> On Fri, Aug 07, 2026 at 06:03:58PM +0200, Takashi Iwai wrote:
> > If strlcat() were super-dangerous, it's understandable to drop.  But,
> > it's not, and issues discussed in the github are minor and something
> > that can be addressed in strlcat() implementation; that is, can't we
> > rather re-implement strlcat() in a safer way, instead of killing it?
> > 
> > Sure, there are code calling strlcat() that could be optimized better.
> > They can be cleaned up.  But it alone can't be a reason that strlcat()
> > must die without mercy.  
> 
> The risk comes from the compiler having no way to know what the size of
> the destination buffer is, as the "char *" argument has no length
> associated with it. One thing we can do is change the argument
> requirements for strlcat (like we did when designing memtostr, etc),
> that requires that the argument explicitly be an array (not a string
> pointer), at which point bounds checking can be done.
> 
> Usually this requires changing the plumbing of arguments, as a lot of C
> code is used to just passing around a bare "char *", etc. And if that
> re-plumbing is going to happen, it might as well be seq_buf.
> 
> But yes, just replacing it with strlen/strscpy isn't very ergonomic.
> Adding the length explicitly with strscpy certainly gets us the bounds
> again, but it's _separate_ from the string still, and that will lead to
> mistakes too. Better to have it be part of the type (i.e. either an
> array or seq_buf).

And, if the destination is an array (where the compiler knows the size)
there is nothing wrong with a 2 argument function.

Like strscpy() you want any result to be the new length of the destination
string.

Embedding a fixed length char[] in a struct can be a simple better option
and lets the compiler do a lot of the checks for you.

	David

> 
> -Kees
>
Re: [PATCH 0/7] ALSA: remove remaining strlcat() users under sound/
Posted by David Laight 1 month, 3 weeks ago
On Fri, 07 Aug 2026 14:15:27 +0200
Takashi Iwai <tiwai@suse.de> wrote:

> On Fri, 07 Aug 2026 13:41:32 +0200,
> Mahad Ibrahim wrote:
> > 
> > strlcat() is deprecated and slated for removal once its remaining
> > users are converted.  This series converts the sound/ users outside
> > of ASoC.
> > 
> > Each site is converted to an explicit offset plus a bounded copy:
> > strscpy() where the appended text has no format specifiers, and
> > scnprintf() where it does or where the resulting length is needed.
> > 
> > After this series the only remaining strlcat() user under sound/ is
> > sound/soc/codecs/wm_adsp_fw_find_test.c, which goes via ASoC.
> > 
> > Build-tested with allmodconfig and boot-tested on x86_64.
> > 
> > The generated strings were checked against the pre-patch code in a
> > userspace harness for empty, whitespace-padded, exact-fit and
> > oversized inputs, and came out identical.  No audio hardware was
> > available here, so the drivers themselves have not been exercised at
> > runtime.
> > 
> > Mahad Ibrahim (7):
> >   ALSA: ump: replace strlcat() with strscpy()
> >   ALSA: ac97: replace strlcat() with scnprintf()
> >   ALSA: cmipci: replace strlcat() with strscpy()
> >   ALSA: caiaq: replace strlcat() with strscpy()
> >   ALSA: usb-audio: replace strlcat() with append_ctl_name()
> >   ALSA: hiface: replace strlcat() with scnprintf()
> >   ALSA: usb-audio: replace strlcat() in longname construction  
> 
> Honestly speaking, I'm against those conversions.
> Why do we have to open-code at each place with strlen()+strscpy()?
> It's just harder to read than strlcat(), even more error-prone.
> 
> If an alternative is something like this, we really should reconsider.

Agreed.
Changing the code to not need strcat() is one thing, repeatedly
implementing a different version is silly.
Even using seq_buf isn't always ideal.

	David

> 
> 
> thanks,
> 
> Takashi
>