* Re: [PATCH] ALSA: wavefront: fix buffer overflow in longname construction
2025-10-28 16:26 [PATCH] ALSA: wavefront: fix buffer overflow in longname construction moonafterrain
@ 2025-10-28 16:46 ` Takashi Iwai
0 siblings, 0 replies; 2+ messages in thread
From: Takashi Iwai @ 2025-10-28 16:46 UTC (permalink / raw)
To: moonafterrain
Cc: Jaroslav Kysela, Takashi Iwai, linux-sound, linux-kernel, stable,
Yuhao Jiang
On Tue, 28 Oct 2025 17:26:43 +0100,
moonafterrain@outlook.com wrote:
>
> From: Junrui Luo <moonafterrain@outlook.com>
>
> The snd_wavefront_probe() function constructs the card->longname string
> using unsafe sprintf() calls that can overflow the 80-byte buffer when
> module parameters contain large values.
>
> The vulnerability exists at wavefront.c where multiple sprintf()
> operations append to card->longname without length checking.
>
> Fix by replacing all sprintf() calls with scnprintf() and proper length
> tracking to ensure writes never exceed sizeof(card->longname).
>
> Reported-by: Yuhao Jiang <danisjiang@gmail.com>
> Reported-by: Junrui Luo <moonafterrain@outlook.com>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
> ---
> sound/isa/wavefront/wavefront.c | 40 ++++++++++++++++++++-------------
> 1 file changed, 25 insertions(+), 15 deletions(-)
>
> diff --git a/sound/isa/wavefront/wavefront.c b/sound/isa/wavefront/wavefront.c
> index 07c68568091d..74ea3a67620c 100644
> --- a/sound/isa/wavefront/wavefront.c
> +++ b/sound/isa/wavefront/wavefront.c
> @@ -343,6 +343,7 @@ snd_wavefront_probe (struct snd_card *card, int dev)
> struct snd_rawmidi *ics2115_external_rmidi = NULL;
> struct snd_hwdep *fx_processor;
> int hw_dev = 0, midi_dev = 0, err;
> + size_t len, rem;
>
> /* --------- PCM --------------- */
>
> @@ -492,26 +493,35 @@ snd_wavefront_probe (struct snd_card *card, int dev)
> length restrictions
> */
>
> - sprintf(card->longname, "%s PCM 0x%lx irq %d dma %d",
> - card->driver,
> - chip->port,
> - cs4232_pcm_irq[dev],
> - dma1[dev]);
> + len = scnprintf(card->longname, sizeof(card->longname),
> + "%s PCM 0x%lx irq %d dma %d",
> + card->driver,
> + chip->port,
> + cs4232_pcm_irq[dev],
> + dma1[dev]);
>
> - if (dma2[dev] >= 0 && dma2[dev] < 8)
> - sprintf(card->longname + strlen(card->longname), "&%d", dma2[dev]);
> + if (dma2[dev] >= 0 && dma2[dev] < 8 && len < sizeof(card->longname)) {
> + rem = sizeof(card->longname) - len;
> + len += scnprintf(card->longname + len, rem, "&%d", dma2[dev]);
> + }
>
> if (cs4232_mpu_port[dev] > 0 && cs4232_mpu_port[dev] != SNDRV_AUTO_PORT) {
> - sprintf (card->longname + strlen (card->longname),
> - " MPU-401 0x%lx irq %d",
> - cs4232_mpu_port[dev],
> - cs4232_mpu_irq[dev]);
> + if (len < sizeof(card->longname)) {
> + rem = sizeof(card->longname) - len;
> + len += scnprintf(card->longname + len, rem,
> + " MPU-401 0x%lx irq %d",
> + cs4232_mpu_port[dev],
> + cs4232_mpu_irq[dev]);
> + }
> }
>
> - sprintf (card->longname + strlen (card->longname),
> - " SYNTH 0x%lx irq %d",
> - ics2115_port[dev],
> - ics2115_irq[dev]);
> + if (len < sizeof(card->longname)) {
> + rem = sizeof(card->longname) - len;
> + scnprintf(card->longname + len, rem,
> + " SYNTH 0x%lx irq %d",
> + ics2115_port[dev],
> + ics2115_irq[dev]);
> + }
>
> return snd_card_register(card);
> }
Thanks for the patch. But the code change is way too complex for the
gain it can have.
There can't be any overflow in the current code as is, because the
buffer size is large enough. snd_card.longname[] is 80 bytes, hence
even if you count all other fields, it can't overflow, practically
seen.
If the code change were trivial, such a fix would still make sense.
But the proposed patch makes the code just harder to understand and
more error-prone.
If you're still interested in "fixing" such cases, I guess we may
introduce a new helper to append a format string, e.g.
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;
}
Then the rest change would be really trivial, just to replace the
snprintf(buf + strlen()) with this new function call. There seem many
other codes doing the similar things, and they can be replaced
gracefully, too.
thanks,
Takashi
^ permalink raw reply [flat|nested] 2+ messages in thread