From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755824AbaHVFXZ (ORCPT ); Fri, 22 Aug 2014 01:23:25 -0400 Received: from cantor2.suse.de ([195.135.220.15]:52793 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751927AbaHVFXX (ORCPT ); Fri, 22 Aug 2014 01:23:23 -0400 Date: Fri, 22 Aug 2014 07:23:19 +0200 Message-ID: From: Takashi Iwai To: Clemens Ladisch Cc: Tommi Rantala , Jaroslav Kysela , Dave Jones , alsa-devel@alsa-project.org, LKML , trinity@vger.kernel.org Subject: Re: [alsa-devel] /proc/asound/card0/oss_mixer stack corruption In-Reply-To: <53F64099.8000601@ladisch.de> References: <53F64099.8000601@ladisch.de> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/24.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org At Thu, 21 Aug 2014 20:55:21 +0200, Clemens Ladisch wrote: > > Tommi Rantala wrote: > > Trinity discovered that writing 128 bytes to > > /proc/asound/card0/oss_mixer triggers a stack corruption. > > > > Call Trace: > > [] __stack_chk_fail+0x16/0x20 > > [] snd_mixer_oss_proc_write+0x24a/0x270 > > snd_info_get_line() wants the len parameter to be one less than the > buffer size, but it isn't: > > while (!snd_info_get_line(buffer, line, sizeof(line))) { > > Not that *any* other caller got it correct either: > > sound/core/oss/pcm_oss.c: while (!snd_info_get_line(buffer, line, sizeof(line))) { > sound/core/pcm.c: if (!snd_info_get_line(buffer, line, sizeof(line))) > sound/core/pcm_memory.c: if (!snd_info_get_line(buffer, line, sizeof(line))) { > sound/drivers/dummy.c: while (!snd_info_get_line(buffer, line, sizeof(line))) { > sound/pci/ac97/ac97_proc.c: while (!snd_info_get_line(buffer, line, sizeof(line))) { > sound/pci/ca0106/ca0106_proc.c: while (!snd_info_get_line(buffer, line, sizeof(line))) { > sound/pci/ca0106/ca0106_proc.c: while (!snd_info_get_line(buffer, line, sizeof(line))) { > sound/pci/ca0106/ca0106_proc.c: while (!snd_info_get_line(buffer, line, sizeof(line))) { > sound/pci/emu10k1/emu10k1x.c: while (!snd_info_get_line(buffer, line, sizeof(line))) { > sound/pci/emu10k1/emuproc.c: while (!snd_info_get_line(buffer, line, sizeof(line))) { > sound/pci/emu10k1/emuproc.c: while (!snd_info_get_line(buffer, line, sizeof(line))) { > sound/pci/hda/hda_eld.c: while (!snd_info_get_line(buffer, line, sizeof(line))) { > sound/pci/ice1712/pontis.c: while (!snd_info_get_line(buffer, line, sizeof(line))) { > sound/pci/ice1712/prodigy_hifi.c: while (!snd_info_get_line(buffer, line, sizeof(line))) { > sound/pci/lola/lola_proc.c: while (!snd_info_get_line(buffer, line, sizeof(line))) { > sound/pci/pcxhr/pcxhr.c: while (!snd_info_get_line(buffer, line, sizeof(line))) { > > Oh well. At least these proc files are writable only by root, and the > fix is easy: Indeed. Applied now, thanks. Takashi > > --8<---------------------------------------------------------------->8-- > ALSA: core: fix buffer overflow in snd_info_get_line() > > snd_info_get_line() documents that its last parameter must be one > less than the buffer size, but this API design guarantees that > (literally) every caller gets it wrong. > > Just change this parameter to have its obvious meaning. > > Reported-by: Tommi Rantala > Cc: # v2.2.26+ > Signed-off-by: Clemens Ladisch > --- > sound/core/info.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > --- a/sound/core/info.c > +++ b/sound/core/info.c > @@ -684,7 +684,7 @@ int snd_info_card_free(struct snd_card *card) > * snd_info_get_line - read one line from the procfs buffer > * @buffer: the procfs buffer > * @line: the buffer to store > - * @len: the max. buffer size - 1 > + * @len: the max. buffer size > * > * Reads one line from the buffer and stores the string. > * > @@ -704,7 +704,7 @@ int snd_info_get_line(struct snd_info_buffer *buffer, char *line, int len) > buffer->stop = 1; > if (c == '\n') > break; > - if (len) { > + if (len > 1) { > len--; > *line++ = c; > } >