linux-kernel - Re: [PATCH v2] ALSA: wavefront: use scnprintf for longname construction

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <87tsz8hoit.wl-tiwai@suse.de>
Date: Wed, 05 Nov 2025 09:28:10 +0100
From: Takashi Iwai <tiwai@...e.de>
To: 222 Summ <moonafterrain@...look.com>
Cc: Takashi Iwai <tiwai@...e.de>,
	Jaroslav Kysela <perex@...ex.cz>,
	Takashi Iwai <tiwai@...e.com>,
	"linux-sound@...r.kernel.org" <linux-sound@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"stable@...r.kernel.org" <stable@...r.kernel.org>,
	Yuhao Jiang
	<danisjiang@...il.com>
Subject: Re: [PATCH v2] ALSA: wavefront: use scnprintf for longname construction

On Wed, 05 Nov 2025 05:28:27 +0100,
222 Summ wrote:
> 
> Hi Takashi,
> 
> Thank you for your detailed feedback on the v2 patch. I've prepared a v3
> patch series that incorporates your suggestions.
> 
> Based on your comments, I've made the following changes:
> 
> 1. Split the patch into two parts:
>    - Patch 1/2: Adds `scnprintf_append()` to `lib/vsprintf.c`
>    - Patch 2/2: Converts `wavefront.c` to use it
> 2. Fixed the return value you pointed out
> 3. Used strnlen() instead of strlen() for safety
> 
> I plan to submit this as a two-patch series. However, before I send it, I'd like to confirm a few things:
> 
> 1. Is this approach (adding the helper to lib/vsprintf.c) what you had in
>    mind? Or would you prefer a different location?

IMO lib/vsprintf.c should be fine.

> 2. Would you recommend sending both patches together, or waiting until
> patch 1/2 is reviewed and accepted before submitting patch 2/2?

You can try patching not only wavefront.c but covering a few others in
the series at first for showing that it really makes sense to be a
generic helper function.  And, submitting the whole might be better to
understand what's the use and effect, too.

Note that there can be suggestions for other forms, e.g. there have
been some progresses for the continuous string processing, IIRC.
So this is merely one example to achieve the goal.

The merit of this way would be its simplicity, though: you can replace
the code with a single function call without keeping anything else,
and the handling logic is quite straightforward.

> The implementation of scnprintf_append() is shown below:
> +int scnprintf_append(char *buf, size_t size, const char *fmt, ...)
> +{
> +	va_list args;
> +	size_t len;
> +
> +	len = strnlen(buf, size);
> +	if (len >= size)
> +		return len;
> +	va_start(args, fmt);
> +	len += vscnprintf(buf + len, size - len, fmt, args);
> +	va_end(args);
> +	return len;
> +}

This should be with EXPORT_SYMBOL_GPL() (or EXPORT_SYMBOL() is any
user is non-GPL).


thanks,

Takashi

> 
> Thanks again for your guidance.
> 
> Best regards,
> Junrui
> 
> ________________________________________
> From: Takashi Iwai <tiwai@...e.de>
> Sent: Tuesday, November 4, 2025 18:01
> To: moonafterrain@...look.com <moonafterrain@...look.com>
> Cc: Jaroslav Kysela <perex@...ex.cz>; Takashi Iwai <tiwai@...e.com>; linux-sound@...r.kernel.org <linux-sound@...r.kernel.org>; linux-kernel@...r.kernel.org <linux-kernel@...r.kernel.org>; stable@...r.kernel.org <stable@...r.kernel.org>; Yuhao Jiang <danisjiang@...il.com>
> Subject: Re: [PATCH v2] ALSA: wavefront: use scnprintf for longname construction
>  
> On Sun, 02 Nov 2025 16:32:39 +0100,
> moonafterrain@...look.com wrote:
> >
> > From: Junrui Luo <moonafterrain@...look.com>
> >
> > Replace sprintf() calls with scnprintf() and a new scnprintf_append()
> > helper function when constructing card->longname. This improves code
> > readability and provides bounds checking for the 80-byte buffer.
> >
> > While the current parameter ranges don't cause overflow in practice,
> > using safer string functions follows kernel best practices and makes
> > the code more maintainable.
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Cc: stable@...r.kernel.org
> > Signed-off-by: Junrui Luo <moonafterrain@...look.com>
> > ---
> > Changes in v2:
> > - Replace sprintf() calls with scnprintf() and a new scnprintf_append()
> > - Link to v1: https://lore.kernel.org/all/ME2PR01MB3156CEC4F31F253C9B540FB7AFFDA@ME2PR01MB3156.ausprd01.prod.outlook.com/
> 
> Well, my suggestion was that we can apply such conversions once if a
> *generic* helper becomes available; that is, propose
> scnprintf_append() to be put in include/linux/string.h or whatever (I
> guess better in *.c instead of inline), and once if it's accepted, we
> can convert the relevant places (there are many, not only
> wavefront.c).
> 
> BTW:
> 
> > +__printf(3, 4) static int scnprintf_append(char *buf, size_t size, const char *fmt, ...)
> > +{
> > +     va_list args;
> > +     size_t len = strlen(buf);
> > +
> > +     if (len >= size)
> > +             return len;
> > +     va_start(args, fmt);
> > +     len = vscnprintf(buf + len, size - len, fmt, args);
> > +     va_end(args);
> > +     return len;
> 
> The above should be
>         len += vscnprintf(buf + len, size - len, fmt, args);
> so that it returns the full size of the string.
> If it were in user-space, I'd check a negative error code, but the
> Linux kernel implementation doesn't return a negative error code, so
> far.
> I see it's a copy from a code snipped I suggested which already
> contained the error :)
> 
> Also, it might be safer to use strnlen() instead of strlen() for
> avoiding a potential out-of-bound access.
> 
> 
> thanks,
> 
> Takashi