Re: [PATCH] ASoC: Intel: Skylake: Fix struct definition

[Kees Cook <keescook@chromium.org>]

On Mon, Feb 13, 2023 at 09:52:23PM +0100, Amadeusz Sławiński wrote:
> The kernel is globally removing the ambiguous 0-length and 1-element
> arrays in favor of flexible arrays, so that we can gain both compile-time
> and run-time array bounds checking[1]. In this instance, struct
> skl_cpr_cfg contains struct skl_cpr_gtw_cfg, which defined "config_data"
> as a 1-element array.
> 
> However, case present in sound/soc/intel/skylake/skl-topology.h is not a
> simple one as the structure takes part in IPC communication. Apparently
> original definition missed one field, which while not used by AudioDSP
> firmware when there is no additional data, is still expected to be part
> of an IPC message. Currently this works because of how 'config_data' is
> declared: 'config_data[1]'. Now when one replaces it with a flexible
> array there would be one field missing. Update struct declaration to fix
> this.
> 
> Reported-by: Sasa Ostrouska <casaxa@gmail.com>
> Link: https://lore.kernel.org/all/CALFERdwvq5day_sbDfiUsMSZCQu9HG8-SBpOZDNPeMdZGog6XA@mail.gmail.com/
> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Cc: Liam Girdwood <liam.r.girdwood@linux.intel.com>
> Cc: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
> Cc: Bard Liao <yung-chuan.liao@linux.intel.com>
> Cc: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> Cc: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.com>
> Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>
> Cc: alsa-devel@alsa-project.org
> CC: Kees Cook <keescook@chromium.org>
> Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>
> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
> ---
>  sound/soc/intel/skylake/skl-messages.c | 2 +-
>  sound/soc/intel/skylake/skl-topology.h | 5 ++++-
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/intel/skylake/skl-messages.c b/sound/soc/intel/skylake/skl-messages.c
> index 5ab0917a2b3d..d31509298a0a 100644
> --- a/sound/soc/intel/skylake/skl-messages.c
> +++ b/sound/soc/intel/skylake/skl-messages.c
> @@ -549,7 +549,7 @@ static void skl_copy_copier_caps(struct skl_module_cfg *mconfig,
>  	if (mconfig->formats_config[SKL_PARAM_INIT].caps_size == 0)
>  		return;
>  
> -	memcpy(cpr_mconfig->gtw_cfg.config_data,
> +	memcpy(&cpr_mconfig->gtw_cfg.config_data,

Unfortunately, this is going to run afoul of a compiler bug. :( GCC is
still working on getting it fixed (and Clang will follow). But for now,
this will just result in a run-time warning instead, since memcpy won't
be able to "see through" the fact that "config_data" ends with a
flexible array, meaning it will think it has a 4 byte size:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101832

>  			mconfig->formats_config[SKL_PARAM_INIT].caps,
>  			mconfig->formats_config[SKL_PARAM_INIT].caps_size);
>  
> diff --git a/sound/soc/intel/skylake/skl-topology.h b/sound/soc/intel/skylake/skl-topology.h
> index 6db0fd7bad49..30a0977af943 100644
> --- a/sound/soc/intel/skylake/skl-topology.h
> +++ b/sound/soc/intel/skylake/skl-topology.h
> @@ -115,7 +115,10 @@ struct skl_cpr_gtw_cfg {
>  	u32 dma_buffer_size;
>  	u32 config_length;
>  	/* not mandatory; required only for DMIC/I2S */
> -	u32 config_data[1];
> +	struct {
> +		u32 gtw_attrs;
> +		u32 data[];
> +	} config_data;
>  } __packed;

I recommend leaving the original memcpy() as it was, and instead
creating an anonymous union in place of "config_data":

	union {
		u32 gtw_attrs;
		DECLARE_FLEX_ARRAY(u32, data);
	};

>  
>  struct skl_dma_control {
> -- 
> 2.34.1
> 

-- 
Kees Cook