All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kees Cook <keescook@chromium.org>
To: Cezary Rojewski <cezary.rojewski@intel.com>
Cc: "Kees Cook" <keescook@chromium.org>,
	"Sasa Ostrouska" <casaxa@gmail.com>,
	"Pierre-Louis Bossart" <pierre-louis.bossart@linux.intel.com>,
	"Liam Girdwood" <liam.r.girdwood@linux.intel.com>,
	"Peter Ujfalusi" <peter.ujfalusi@linux.intel.com>,
	"Bard Liao" <yung-chuan.liao@linux.intel.com>,
	"Ranjani Sridharan" <ranjani.sridharan@linux.intel.com>,
	"Kai Vehmanen" <kai.vehmanen@linux.intel.com>,
	"Mark Brown" <broonie@kernel.org>,
	"Takashi Iwai" <tiwai@suse.com>,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	"Amadeusz Sławiński" <amadeuszx.slawinski@linux.intel.com>,
	alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org,
	linux-hardening@vger.kernel.org
Subject: [PATCH] ASoC: Intel: Skylake: Replace 1-element array with flex-array
Date: Thu,  9 Feb 2023 21:14:51 -0800	[thread overview]
Message-ID: <20230210051447.never.204-kees@kernel.org> (raw)

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.

Normally when switching from a 1-element array to a flex-array, any
related size calculations must be adjusted too. However, it seems the
original code was over-allocating space, since 1 extra u32 would be
included by the sizeof():

                param_size = sizeof(struct skl_cpr_cfg);
                param_size += mconfig->formats_config[SKL_PARAM_INIT].caps_size;

But the copy uses caps_size bytes, and cap_size / 4 (i.e. sizeof(u32))
for the length tracking:

        memcpy(cpr_mconfig->gtw_cfg.config_data,
                        mconfig->formats_config[SKL_PARAM_INIT].caps,
                        mconfig->formats_config[SKL_PARAM_INIT].caps_size);

        cpr_mconfig->gtw_cfg.config_length =
                        (mconfig->formats_config[SKL_PARAM_INIT].caps_size) / 4;

Therefore, no size calculations need adjusting. Change the struct
skl_cpr_gtw_cfg config_data member to be a true flexible array, which
also fixes the over-allocation, and silences this memcpy run-time false
positive:

  memcpy: detected field-spanning write (size 100) of single field "cpr_mconfig->gtw_cfg.config_data" at sound/soc/intel/skylake/skl-messages.c:554 (size 4)

[1] For lots of details, see both:
    https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
    https://people.kernel.org/kees/bounded-flexible-arrays-in-c

Reported-by: Sasa Ostrouska <casaxa@gmail.com>
Link: https://lore.kernel.org/all/CALFERdwvq5day_sbDfiUsMSZCQu9HG8-SBpOZDNPeMdZGog6XA@mail.gmail.com/
Cc: Cezary Rojewski <cezary.rojewski@intel.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: "Amadeusz Sławiński" <amadeuszx.slawinski@linux.intel.com>
Cc: alsa-devel@alsa-project.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 sound/soc/intel/skylake/skl-topology.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/intel/skylake/skl-topology.h b/sound/soc/intel/skylake/skl-topology.h
index 6db0fd7bad49..ad94f8020c27 100644
--- a/sound/soc/intel/skylake/skl-topology.h
+++ b/sound/soc/intel/skylake/skl-topology.h
@@ -115,7 +115,7 @@ struct skl_cpr_gtw_cfg {
 	u32 dma_buffer_size;
 	u32 config_length;
 	/* not mandatory; required only for DMIC/I2S */
-	u32 config_data[1];
+	u32 config_data[];
 } __packed;
 
 struct skl_dma_control {
-- 
2.34.1


WARNING: multiple messages have this Message-ID (diff)
From: Kees Cook <keescook@chromium.org>
To: Cezary Rojewski <cezary.rojewski@intel.com>
Cc: "Kees Cook" <keescook@chromium.org>,
	"Sasa Ostrouska" <casaxa@gmail.com>,
	"Pierre-Louis Bossart" <pierre-louis.bossart@linux.intel.com>,
	"Liam Girdwood" <liam.r.girdwood@linux.intel.com>,
	"Peter Ujfalusi" <peter.ujfalusi@linux.intel.com>,
	"Bard Liao" <yung-chuan.liao@linux.intel.com>,
	"Ranjani Sridharan" <ranjani.sridharan@linux.intel.com>,
	"Kai Vehmanen" <kai.vehmanen@linux.intel.com>,
	"Mark Brown" <broonie@kernel.org>,
	"Jaroslav Kysela" <perex@perex.cz>,
	"Takashi Iwai" <tiwai@suse.com>,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	"Amadeusz Sławiński" <amadeuszx.slawinski@linux.intel.com>,
	alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org,
	linux-hardening@vger.kernel.org
Subject: [PATCH] ASoC: Intel: Skylake: Replace 1-element array with flex-array
Date: Thu,  9 Feb 2023 21:14:51 -0800	[thread overview]
Message-ID: <20230210051447.never.204-kees@kernel.org> (raw)

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.

Normally when switching from a 1-element array to a flex-array, any
related size calculations must be adjusted too. However, it seems the
original code was over-allocating space, since 1 extra u32 would be
included by the sizeof():

                param_size = sizeof(struct skl_cpr_cfg);
                param_size += mconfig->formats_config[SKL_PARAM_INIT].caps_size;

But the copy uses caps_size bytes, and cap_size / 4 (i.e. sizeof(u32))
for the length tracking:

        memcpy(cpr_mconfig->gtw_cfg.config_data,
                        mconfig->formats_config[SKL_PARAM_INIT].caps,
                        mconfig->formats_config[SKL_PARAM_INIT].caps_size);

        cpr_mconfig->gtw_cfg.config_length =
                        (mconfig->formats_config[SKL_PARAM_INIT].caps_size) / 4;

Therefore, no size calculations need adjusting. Change the struct
skl_cpr_gtw_cfg config_data member to be a true flexible array, which
also fixes the over-allocation, and silences this memcpy run-time false
positive:

  memcpy: detected field-spanning write (size 100) of single field "cpr_mconfig->gtw_cfg.config_data" at sound/soc/intel/skylake/skl-messages.c:554 (size 4)

[1] For lots of details, see both:
    https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
    https://people.kernel.org/kees/bounded-flexible-arrays-in-c

Reported-by: Sasa Ostrouska <casaxa@gmail.com>
Link: https://lore.kernel.org/all/CALFERdwvq5day_sbDfiUsMSZCQu9HG8-SBpOZDNPeMdZGog6XA@mail.gmail.com/
Cc: Cezary Rojewski <cezary.rojewski@intel.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: "Amadeusz Sławiński" <amadeuszx.slawinski@linux.intel.com>
Cc: alsa-devel@alsa-project.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 sound/soc/intel/skylake/skl-topology.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/intel/skylake/skl-topology.h b/sound/soc/intel/skylake/skl-topology.h
index 6db0fd7bad49..ad94f8020c27 100644
--- a/sound/soc/intel/skylake/skl-topology.h
+++ b/sound/soc/intel/skylake/skl-topology.h
@@ -115,7 +115,7 @@ struct skl_cpr_gtw_cfg {
 	u32 dma_buffer_size;
 	u32 config_length;
 	/* not mandatory; required only for DMIC/I2S */
-	u32 config_data[1];
+	u32 config_data[];
 } __packed;
 
 struct skl_dma_control {
-- 
2.34.1


             reply	other threads:[~2023-02-10  5:16 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-02-10  5:14 Kees Cook [this message]
2023-02-10  5:14 ` [PATCH] ASoC: Intel: Skylake: Replace 1-element array with flex-array Kees Cook
2023-02-10  6:39 ` Péter Ujfalusi
2023-02-10 13:10 ` Amadeusz Sławiński
2023-02-10 19:06   ` Kees Cook
2023-02-13 12:52     ` Amadeusz Sławiński

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230210051447.never.204-kees@kernel.org \
    --to=keescook@chromium.org \
    --cc=alsa-devel@alsa-project.org \
    --cc=amadeuszx.slawinski@linux.intel.com \
    --cc=broonie@kernel.org \
    --cc=casaxa@gmail.com \
    --cc=cezary.rojewski@intel.com \
    --cc=gustavoars@kernel.org \
    --cc=kai.vehmanen@linux.intel.com \
    --cc=liam.r.girdwood@linux.intel.com \
    --cc=linux-hardening@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peter.ujfalusi@linux.intel.com \
    --cc=pierre-louis.bossart@linux.intel.com \
    --cc=ranjani.sridharan@linux.intel.com \
    --cc=tiwai@suse.com \
    --cc=yung-chuan.liao@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.