linux-kernel - Re: [PATCH] ALSA: wavefront: fix buffer overflow in 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: <87ikfzezyn.wl-tiwai@suse.de>
Date: Tue, 28 Oct 2025 17:46:40 +0100
From: Takashi Iwai <tiwai@...e.de>
To: moonafterrain@...look.com
Cc: Jaroslav Kysela <perex@...ex.cz>,
	Takashi Iwai <tiwai@...e.com>,
	linux-sound@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	stable@...r.kernel.org,
	Yuhao Jiang <danisjiang@...il.com>
Subject: Re: [PATCH] ALSA: wavefront: fix buffer overflow in longname construction

On Tue, 28 Oct 2025 17:26:43 +0100,
moonafterrain@...look.com wrote:
> 
> From: Junrui Luo <moonafterrain@...look.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@...il.com>
> Reported-by: Junrui Luo <moonafterrain@...look.com>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@...r.kernel.org
> Signed-off-by: Junrui Luo <moonafterrain@...look.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