Re: [alsa-devel] [PATCH] ALSA: pcm: Fill silence on buffers at hw_params

[Takashi Iwai <tiwai@suse.de>]

On Tue, 10 Dec 2019 14:02:19 +0100,
Takashi Iwai wrote:
> 
> The current PCM code doesn't initialize explicitly the buffers
> allocated for PCM streams, hence it might leak some uninitialized
> kernel data by mmapping or reading the buffer before actually reading
> or writing.
> 
> Since this is a common problem, let's initialize the data on the
> buffers each hw_params properly.  For that, an existing helper
> function is exposed as snd_pcm_fill_silence_frames() and call it from
> snd_pcm_hw_params().
> 
> Reported-and-tested-by: Lionel Koenig <lionel.koenig@gmail.com>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

On the second thought, this use of fill_silence ops might have some
side-effect on the drivers that do actually transfer stream data by
that ops.  So I dropped this patch again.

Will submit the revised patch later.  Sorry for the trouble.


Takashi

> ---
>  sound/core/pcm_lib.c    | 13 ++++---------
>  sound/core/pcm_local.h  |  2 ++
>  sound/core/pcm_native.c |  3 +++
>  3 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index 2236b5e0c1f2..3c63324b8bb7 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -30,9 +30,6 @@
>  #define trace_applptr(substream, prev, curr)
>  #endif
>  
> -static int fill_silence_frames(struct snd_pcm_substream *substream,
> -			       snd_pcm_uframes_t off, snd_pcm_uframes_t frames);
> -
>  /*
>   * fill ring buffer with silence
>   * runtime->silence_start: starting pointer to silence area
> @@ -100,7 +97,7 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_ufram
>  	ofs = runtime->silence_start % runtime->buffer_size;
>  	while (frames > 0) {
>  		transfer = ofs + frames > runtime->buffer_size ? runtime->buffer_size - ofs : frames;
> -		err = fill_silence_frames(substream, ofs, transfer);
> +		err = snd_pcm_fill_silence_frames(substream, ofs, transfer);
>  		snd_BUG_ON(err < 0);
>  		runtime->silence_filled += transfer;
>  		frames -= transfer;
> @@ -1945,8 +1942,6 @@ static int fill_silence(struct snd_pcm_substream *substream, int channel,
>  {
>  	struct snd_pcm_runtime *runtime = substream->runtime;
>  
> -	if (substream->stream != SNDRV_PCM_STREAM_PLAYBACK)
> -		return 0;
>  	if (substream->ops->fill_silence)
>  		return substream->ops->fill_silence(substream, channel,
>  						    hwoff, bytes);
> @@ -2030,10 +2025,10 @@ static int noninterleaved_copy(struct snd_pcm_substream *substream,
>  }
>  
>  /* fill silence on the given buffer position;
> - * called from snd_pcm_playback_silence()
> + * called from snd_pcm_playback_silence() and snd_pcm_hw_params()
>   */
> -static int fill_silence_frames(struct snd_pcm_substream *substream,
> -			       snd_pcm_uframes_t off, snd_pcm_uframes_t frames)
> +int snd_pcm_fill_silence_frames(struct snd_pcm_substream *substream,
> +				snd_pcm_uframes_t off, snd_pcm_uframes_t frames)
>  {
>  	if (substream->runtime->access == SNDRV_PCM_ACCESS_RW_INTERLEAVED ||
>  	    substream->runtime->access == SNDRV_PCM_ACCESS_MMAP_INTERLEAVED)
> diff --git a/sound/core/pcm_local.h b/sound/core/pcm_local.h
> index 384efd002984..ac4f455b1fff 100644
> --- a/sound/core/pcm_local.h
> +++ b/sound/core/pcm_local.h
> @@ -34,6 +34,8 @@ int snd_pcm_update_hw_ptr(struct snd_pcm_substream *substream);
>  
>  void snd_pcm_playback_silence(struct snd_pcm_substream *substream,
>  			      snd_pcm_uframes_t new_hw_ptr);
> +int snd_pcm_fill_silence_frames(struct snd_pcm_substream *substream,
> +				snd_pcm_uframes_t off, snd_pcm_uframes_t frames);
>  
>  static inline snd_pcm_uframes_t
>  snd_pcm_avail(struct snd_pcm_substream *substream)
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 1fe581167b7b..d6f270c639b4 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -739,6 +739,9 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream,
>  	while (runtime->boundary * 2 <= LONG_MAX - runtime->buffer_size)
>  		runtime->boundary *= 2;
>  
> +	/* clear the buffer once for avoiding information leaks */
> +	snd_pcm_fill_silence_frames(substream, 0, runtime->period_size);
> +
>  	snd_pcm_timer_resolution_change(substream);
>  	snd_pcm_set_state(substream, SNDRV_PCM_STATE_SETUP);
>  
> -- 
> 2.16.4
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel