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

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

[Amadeusz Sławiński]

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,
 			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;
 
 struct skl_dma_control {
-- 
2.34.1

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

[Mark Brown]

On Mon, 13 Feb 2023 21:52:23 +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.
> 
> [...]

Applied to

   broonie/sound.git for-next

Thanks!

[1/1] ASoC: Intel: Skylake: Fix struct definition
      commit: 1fd61d018aefc9bf366fd73eddc868163f2ed7da

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

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

[Kees Cook]

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

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

[Cezary Rojewski]

On 2023-02-15 12:17 AM, Kees Cook wrote:
> On Mon, Feb 13, 2023 at 09:52:23PM +0100, Amadeusz Sławiński wrote:

...

>> 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);
> 	};


I appreciate the input. The reason we're sticking to struct is purely 
for readability/maintenance reasons. In the past AudioDSP drivers were 
declaring structs that take part in the IPC communication differently 
than the base AudioDSP firmware did (i.e.: the other participant of the 
IPC). Over the years this resulted in communication issues between the 
software and the firmware teams. Thus, for quite some time now we're 
sticking to same naming/layout convention wherever possible so there is 
no confusion. Works out pretty well if you ask me.

Now, in regard to this very case. 'config_data' consists of field: 
'gateway attributes' and an optional BLOB (hardware configuration) that 
follows it. Often this config is taken directly from one of the ACPI 
tables - NHLT. The NHLT stores hardware configuration bits for I2S/DMIC 
audio endpoints. Unfortunately, each entry within NLHT _is_ the entire 
config, not just the BLOB part.

To make situation clear within the code, config is described with a 
struct, not an union. Given handler may access the entire config through 
&config_data when memcpying, attributes alone through 
config_data.gtw_attrs or the BLOB with config_data.blob. Again, readability.


Kind regards,
Czarek