[PATCH 0/8] ASoC/SOF/Intel/AMD: cleanups for GCC11 -fanalyzer checks

[PATCH 0/8] ASoC/SOF/Intel/AMD: cleanups for GCC11 -fanalyzer checks

[Pierre-Louis Bossart]

GCC11 provides an '-fanalyzer' static analysis option which does not
provide too many false-positives. This patch cleans-up known
problematic code paths to help enable this capability in CI. We've
used this for about a month already.

Pierre-Louis Bossart (8):
  ASoC: SOF: sof-client-probes-ipc4: add checks to prevent static
    analysis warnings
  ASoC: SOF: ipc3: add checks to prevent static analysis warnings
  ASoC: SOF: topology: simplify code to prevent static analysis warnings
  ASoC: SOF: imx: remove error checks on NULL ipc
  ASoC: SOF: mediatek: remove error checks on NULL ipc
  ASoC: Intel: bdw_rt286: add checks to avoid static analysis warnings
  ASoC: Intel: atom: remove static analysis false positive
  ASoC: amd: acp5x-mach:add checks to avoid static analysis warnings

 include/linux/firmware/imx/dsp.h               |  6 ------
 include/linux/firmware/mediatek/mtk-adsp-ipc.h |  6 ------
 sound/soc/amd/vangogh/acp5x-mach.c             |  3 +++
 sound/soc/intel/atom/sst/sst_stream.c          | 13 +++++++------
 sound/soc/intel/boards/bdw_rt286.c             |  6 ++++++
 sound/soc/sof/ipc3.c                           |  2 +-
 sound/soc/sof/sof-client-probes-ipc4.c         |  9 +++++++++
 sound/soc/sof/topology.c                       |  5 +++--
 8 files changed, 29 insertions(+), 21 deletions(-)

-- 
2.39.2

[PATCH 1/8] ASoC: SOF: sof-client-probes-ipc4: add checks to prevent static analysis warnings

[Pierre-Louis Bossart]

make KCFLAGS='-fanalyzer' sound/soc/sof/ reports several NULL pointer
dereference paths.

sof_ipc4_probe_get_module_info() can return a NULL value, but it's
only tested in the init state. Static analyzers cannot know the probe
state machine and hence flags a potential issue for all calls of that
function.

Squelch these errors by adding the same check consistently.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Reviewed-by: Daniel Baluta <daniel.baluta@nxp.com>
Reviewed-by: Yaochun Hung <yc.hung@mediatek.com>
---
 sound/soc/sof/sof-client-probes-ipc4.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/sound/soc/sof/sof-client-probes-ipc4.c b/sound/soc/sof/sof-client-probes-ipc4.c
index ea21ef176c42..c56a85854d92 100644
--- a/sound/soc/sof/sof-client-probes-ipc4.c
+++ b/sound/soc/sof/sof-client-probes-ipc4.c
@@ -146,6 +146,9 @@ static int ipc4_probes_deinit(struct sof_client_dev *cdev)
 	struct sof_man4_module *mentry = sof_ipc4_probe_get_module_info(cdev);
 	struct sof_ipc4_msg msg;
 
+	if (!mentry)
+		return -ENODEV;
+
 	msg.primary = mentry->id;
 	msg.primary |= SOF_IPC4_MSG_TYPE_SET(SOF_IPC4_MOD_DELETE_INSTANCE);
 	msg.primary |= SOF_IPC4_MSG_DIR(SOF_IPC4_MSG_REQUEST);
@@ -197,6 +200,9 @@ static int ipc4_probes_points_add(struct sof_client_dev *cdev,
 	struct sof_ipc4_msg msg;
 	int i, ret;
 
+	if (!mentry)
+		return -ENODEV;
+
 	/* The sof_probe_point_desc and sof_ipc4_probe_point structs
 	 * are of same size and even the integers are the same in the
 	 * same order, and similar meaning, but since there is no
@@ -247,6 +253,9 @@ static int ipc4_probes_points_remove(struct sof_client_dev *cdev,
 	u32 *probe_point_ids;
 	int i, ret;
 
+	if (!mentry)
+		return -ENODEV;
+
 	probe_point_ids = kcalloc(num_buffer_id, sizeof(*probe_point_ids),
 				  GFP_KERNEL);
 	if (!probe_point_ids)
-- 
2.39.2

[PATCH 2/8] ASoC: SOF: ipc3: add checks to prevent static analysis warnings

[Pierre-Louis Bossart]

make KCFLAGS='-fanalyzer' sound/soc/sof/ reports an issue with memcpy:

sound/soc/sof/ipc3.c: In function ‘ipc3_wait_tx_done’:
sound/soc/sof/ipc3.c:309:33: error: use of NULL ‘reply_data’ where
non-null expected [CWE-476] [-Werror=analyzer-null-argument]

  309 |                        memcpy(reply_data, msg->reply_data,
      |                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  310 |                               msg->reply_size);

The finding is legit with this call:
    return sof_ipc3_tx_msg(sdev, &pm_ctx, sizeof(pm_ctx), NULL, 0, false);

Static analysis has no way of knowing that the reply will be zero-sized.

Add a check to only do the memcpy if the reply size is not zero and
the destination pointer is not NULL.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Reviewed-by: Daniel Baluta <daniel.baluta@nxp.com>
Reviewed-by: Yaochun Hung <yc.hung@mediatek.com>
---
 sound/soc/sof/ipc3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/sof/ipc3.c b/sound/soc/sof/ipc3.c
index 2c5aac31e8b0..09834205b119 100644
--- a/sound/soc/sof/ipc3.c
+++ b/sound/soc/sof/ipc3.c
@@ -312,7 +312,7 @@ static int ipc3_wait_tx_done(struct snd_sof_ipc *ipc, void *reply_data)
 		} else {
 			if (sof_debug_check_flag(SOF_DBG_PRINT_IPC_SUCCESS_LOGS))
 				ipc3_log_header(sdev->dev, "ipc tx succeeded", hdr->cmd);
-			if (msg->reply_size)
+			if (reply_data && msg->reply_size)
 				/* copy the data returned from DSP */
 				memcpy(reply_data, msg->reply_data,
 				       msg->reply_size);
-- 
2.39.2

[PATCH 3/8] ASoC: SOF: topology: simplify code to prevent static analysis warnings

[Pierre-Louis Bossart]

make KCFLAGS='-fanalyzer' sound/soc/sof/intel/ reports a possible NULL
pointer dereference.

sound/soc/sof/topology.c:1136:21: error: dereference of NULL ‘w’
[CWE-476] [-Werror=analyzer-null-dereference]

 1136 |     strcmp(w->sname, rtd->dai_link->stream_name))

The code is rather confusing and can be simplified to make static
analysis happy. No functionality change.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Reviewed-by: Daniel Baluta <daniel.baluta@nxp.com>
Reviewed-by: Yaochun Hung <yc.hung@mediatek.com>
---
 sound/soc/sof/topology.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c
index 698129dccc7d..3866dd3cba69 100644
--- a/sound/soc/sof/topology.c
+++ b/sound/soc/sof/topology.c
@@ -1117,10 +1117,11 @@ static void sof_disconnect_dai_widget(struct snd_soc_component *scomp,
 {
 	struct snd_soc_card *card = scomp->card;
 	struct snd_soc_pcm_runtime *rtd;
+	const char *sname = w->sname;
 	struct snd_soc_dai *cpu_dai;
 	int i, stream;
 
-	if (!w->sname)
+	if (!sname)
 		return;
 
 	if (w->id == snd_soc_dapm_dai_out)
@@ -1133,7 +1134,7 @@ static void sof_disconnect_dai_widget(struct snd_soc_component *scomp,
 	list_for_each_entry(rtd, &card->rtd_list, list) {
 		/* does stream match DAI link ? */
 		if (!rtd->dai_link->stream_name ||
-		    strcmp(w->sname, rtd->dai_link->stream_name))
+		    strcmp(sname, rtd->dai_link->stream_name))
 			continue;
 
 		for_each_rtd_cpu_dais(rtd, i, cpu_dai)
-- 
2.39.2

[PATCH 4/8] ASoC: SOF: imx: remove error checks on NULL ipc

[Pierre-Louis Bossart]

imx_dsp_get_data() can return NULL, but the value is not checked
before being usd, leading to static analysis warnings.

sound/soc/sof/imx/imx8.c:85:32: error: dereference of NULL ‘0’
[CWE-476] [-Werror=analyzer-null-dereference]

   85 |         spin_lock_irqsave(&priv->sdev->ipc_lock, flags);
      |                            ~~~~^~~~~~

It appears this is not really a possible problem, so remove those checks.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Reviewed-by: Daniel Baluta <daniel.baluta@nxp.com>
Reviewed-by: Yaochun Hung <yc.hung@mediatek.com>
---
 include/linux/firmware/imx/dsp.h | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/include/linux/firmware/imx/dsp.h b/include/linux/firmware/imx/dsp.h
index 4f7895a3b73c..1f176a2683fe 100644
--- a/include/linux/firmware/imx/dsp.h
+++ b/include/linux/firmware/imx/dsp.h
@@ -37,17 +37,11 @@ struct imx_dsp_ipc {
 
 static inline void imx_dsp_set_data(struct imx_dsp_ipc *ipc, void *data)
 {
-	if (!ipc)
-		return;
-
 	ipc->private_data = data;
 }
 
 static inline void *imx_dsp_get_data(struct imx_dsp_ipc *ipc)
 {
-	if (!ipc)
-		return NULL;
-
 	return ipc->private_data;
 }
 
-- 
2.39.2

[PATCH 5/8] ASoC: SOF: mediatek: remove error checks on NULL ipc

[Pierre-Louis Bossart]

mtk_adsp_ipc_get_data() can return NULL, but the value is not checked
before being used, leading to static analysis warnings.

sound/soc/sof/mediatek/mt8195/mt8195.c:90:32: error: dereference of
NULL ‘0’ [CWE-476] [-Werror=analyzer-null-dereference]

   90 |         spin_lock_irqsave(&priv->sdev->ipc_lock, flags);
      |                            ~~~~^~~~~~

It appears this is not really a possible problem, so remove those checks.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Reviewed-by: Daniel Baluta <daniel.baluta@nxp.com>
Reviewed-by: Yaochun Hung <yc.hung@mediatek.com>
---
 include/linux/firmware/mediatek/mtk-adsp-ipc.h | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/include/linux/firmware/mediatek/mtk-adsp-ipc.h b/include/linux/firmware/mediatek/mtk-adsp-ipc.h
index 28fd313340b8..5b1d16fa3f56 100644
--- a/include/linux/firmware/mediatek/mtk-adsp-ipc.h
+++ b/include/linux/firmware/mediatek/mtk-adsp-ipc.h
@@ -46,17 +46,11 @@ struct mtk_adsp_ipc {
 
 static inline void mtk_adsp_ipc_set_data(struct mtk_adsp_ipc *ipc, void *data)
 {
-	if (!ipc)
-		return;
-
 	ipc->private_data = data;
 }
 
 static inline void *mtk_adsp_ipc_get_data(struct mtk_adsp_ipc *ipc)
 {
-	if (!ipc)
-		return NULL;
-
 	return ipc->private_data;
 }
 
-- 
2.39.2

[PATCH 6/8] ASoC: Intel: bdw_rt286: add checks to avoid static analysis warnings

[Pierre-Louis Bossart]

snd_soc_card_get_codec_dai() can return NULL, but that value is not
checked for, leading to false-positive static analysis warnings

sound/soc/intel/boards/bdw_rt286.c:190:16: error: dereference of NULL
‘codec_dai’ [CWE-476] [-Werror=analyzer-null-dereference]

  190 |         return snd_soc_component_set_jack(codec_dai->component, NULL, NULL);
      |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ‘card_suspend_pre’: event 1
    |
    |  190 |         return snd_soc_component_set_jack(codec_dai->component, NULL, NULL);
    |      |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |                |
    |      |                (1) dereference of NULL ‘codec_dai’

This NULL dereference cannot happen, the codec-dai "rt286-aif1" must exists
otherwise the card would not be created. Static analysis cannot know
that however so we might as well squelch this report.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Reviewed-by: Daniel Baluta <daniel.baluta@nxp.com>
Reviewed-by: Yaochun Hung <yc.hung@mediatek.com>
---
 sound/soc/intel/boards/bdw_rt286.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/sound/soc/intel/boards/bdw_rt286.c b/sound/soc/intel/boards/bdw_rt286.c
index b7687a93a923..036579331d8f 100644
--- a/sound/soc/intel/boards/bdw_rt286.c
+++ b/sound/soc/intel/boards/bdw_rt286.c
@@ -187,6 +187,9 @@ static int card_suspend_pre(struct snd_soc_card *card)
 {
 	struct snd_soc_dai *codec_dai = snd_soc_card_get_codec_dai(card, "rt286-aif1");
 
+	if (!codec_dai)
+		return 0;
+
 	return snd_soc_component_set_jack(codec_dai->component, NULL, NULL);
 }
 
@@ -194,6 +197,9 @@ static int card_resume_post(struct snd_soc_card *card)
 {
 	struct snd_soc_dai *codec_dai = snd_soc_card_get_codec_dai(card, "rt286-aif1");
 
+	if (!codec_dai)
+		return 0;
+
 	return snd_soc_component_set_jack(codec_dai->component, &card_headset, NULL);
 }
 
-- 
2.39.2

[PATCH 7/8] ASoC: Intel: atom: remove static analysis false positive

[Pierre-Louis Bossart]

make KCFLAGS='-fanalyzer' sound/soc/intel/atom/ reports a possible
NULL pointer dereference.

sound/soc/intel/atom/sst/sst_stream.c:221:40: error: dereference of
NULL ‘block’ [CWE-476] [-Werror=analyzer-null-dereference]
  221 |                         unsigned char *r = block->data;

This is a false-positive, the GCC analyzer generated that report by
considering if (bytes->block) as true in some cases and false in
others.

We can simplify the code and use a local variable so that static
analysis does not try to look for cases where bytes->block can be
modified concurrently.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Reviewed-by: Daniel Baluta <daniel.baluta@nxp.com>
Reviewed-by: Yaochun Hung <yc.hung@mediatek.com>
---
 sound/soc/intel/atom/sst/sst_stream.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/sound/soc/intel/atom/sst/sst_stream.c b/sound/soc/intel/atom/sst/sst_stream.c
index 862a19ae5429..288221db7323 100644
--- a/sound/soc/intel/atom/sst/sst_stream.c
+++ b/sound/soc/intel/atom/sst/sst_stream.c
@@ -173,10 +173,11 @@ int sst_send_byte_stream_mrfld(struct intel_sst_drv *sst_drv_ctx,
 	u32 length;
 	int pvt_id, ret = 0;
 	struct sst_block *block = NULL;
+	u8 bytes_block = bytes->block;
 
 	dev_dbg(sst_drv_ctx->dev,
 		"type:%u ipc_msg:%u block:%u task_id:%u pipe: %#x length:%#x\n",
-		bytes->type, bytes->ipc_msg, bytes->block, bytes->task_id,
+		bytes->type, bytes->ipc_msg, bytes_block, bytes->task_id,
 		bytes->pipe_id, bytes->len);
 
 	if (sst_create_ipc_msg(&msg, true))
@@ -185,12 +186,12 @@ int sst_send_byte_stream_mrfld(struct intel_sst_drv *sst_drv_ctx,
 	pvt_id = sst_assign_pvt_id(sst_drv_ctx);
 	sst_fill_header_mrfld(&msg->mrfld_header, bytes->ipc_msg,
 			bytes->task_id, 1, pvt_id);
-	msg->mrfld_header.p.header_high.part.res_rqd = bytes->block;
+	msg->mrfld_header.p.header_high.part.res_rqd = bytes_block;
 	length = bytes->len;
 	msg->mrfld_header.p.header_low_payload = length;
 	dev_dbg(sst_drv_ctx->dev, "length is %d\n", length);
 	memcpy(msg->mailbox_data, &bytes->bytes, bytes->len);
-	if (bytes->block) {
+	if (bytes_block) {
 		block = sst_create_block(sst_drv_ctx, bytes->ipc_msg, pvt_id);
 		if (block == NULL) {
 			kfree(msg);
@@ -203,7 +204,7 @@ int sst_send_byte_stream_mrfld(struct intel_sst_drv *sst_drv_ctx,
 	dev_dbg(sst_drv_ctx->dev, "msg->mrfld_header.p.header_low_payload:%d",
 			msg->mrfld_header.p.header_low_payload);
 
-	if (bytes->block) {
+	if (bytes_block) {
 		ret = sst_wait_timeout(sst_drv_ctx, block);
 		if (ret) {
 			dev_err(sst_drv_ctx->dev, "fw returned err %d\n", ret);
@@ -216,7 +217,7 @@ int sst_send_byte_stream_mrfld(struct intel_sst_drv *sst_drv_ctx,
 		 * copy the reply and send back
 		 * we need to update only sz and payload
 		 */
-		if (bytes->block) {
+		if (bytes_block) {
 			unsigned char *r = block->data;
 
 			dev_dbg(sst_drv_ctx->dev, "read back %d bytes",
@@ -224,7 +225,7 @@ int sst_send_byte_stream_mrfld(struct intel_sst_drv *sst_drv_ctx,
 			memcpy(bytes->bytes, r, bytes->len);
 		}
 	}
-	if (bytes->block)
+	if (bytes_block)
 		sst_free_block(sst_drv_ctx, block);
 out:
 	test_and_clear_bit(pvt_id, &sst_drv_ctx->pvt_id);
-- 
2.39.2

[PATCH 8/8] ASoC: amd: acp5x-mach:add checks to avoid static analysis warnings

[Pierre-Louis Bossart]

snd_soc_card_get_codec_dai() can return NULL, but that value is not
checked for, leading to static analysis warnings.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Reviewed-by: Daniel Baluta <daniel.baluta@nxp.com>
Reviewed-by: Yaochun Hung <yc.hung@mediatek.com>
---
 sound/soc/amd/vangogh/acp5x-mach.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sound/soc/amd/vangogh/acp5x-mach.c b/sound/soc/amd/vangogh/acp5x-mach.c
index 125a8e93478d..eda464545866 100644
--- a/sound/soc/amd/vangogh/acp5x-mach.c
+++ b/sound/soc/amd/vangogh/acp5x-mach.c
@@ -170,6 +170,9 @@ static int acp5x_nau8821_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_dai *dai = snd_soc_card_get_codec_dai(card, ACP5X_NAU8821_DAI_NAME);
 	int ret, bclk;
 
+	if (!dai)
+		return -EINVAL;
+
 	ret = snd_soc_dai_set_sysclk(dai, NAU8821_CLK_FLL_BLK, 0, SND_SOC_CLOCK_IN);
 	if (ret < 0)
 		dev_err(card->dev, "can't set FS clock %d\n", ret);
-- 
2.39.2

Re: [PATCH 0/8] ASoC/SOF/Intel/AMD: cleanups for GCC11 -fanalyzer checks

[Mark Brown]

On Mon, 31 Jul 2023 16:37:40 -0500, Pierre-Louis Bossart wrote:
> GCC11 provides an '-fanalyzer' static analysis option which does not
> provide too many false-positives. This patch cleans-up known
> problematic code paths to help enable this capability in CI. We've
> used this for about a month already.
> 
> Pierre-Louis Bossart (8):
>   ASoC: SOF: sof-client-probes-ipc4: add checks to prevent static
>     analysis warnings
>   ASoC: SOF: ipc3: add checks to prevent static analysis warnings
>   ASoC: SOF: topology: simplify code to prevent static analysis warnings
>   ASoC: SOF: imx: remove error checks on NULL ipc
>   ASoC: SOF: mediatek: remove error checks on NULL ipc
>   ASoC: Intel: bdw_rt286: add checks to avoid static analysis warnings
>   ASoC: Intel: atom: remove static analysis false positive
>   ASoC: amd: acp5x-mach:add checks to avoid static analysis warnings
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/8] ASoC: SOF: sof-client-probes-ipc4: add checks to prevent static analysis warnings
      commit: 390e7066db29b985c5142513955797c1166b623a
[2/8] ASoC: SOF: ipc3: add checks to prevent static analysis warnings
      commit: e44222c213678d6ef646d72cbb9a2eda52f6dc22
[3/8] ASoC: SOF: topology: simplify code to prevent static analysis warnings
      commit: 55cb3dc271d81f1982c949a2ac483a6daf613b92
[4/8] ASoC: SOF: imx: remove error checks on NULL ipc
      commit: e302f8d9f799af57a61a7456451c28f2647e9751
[5/8] ASoC: SOF: mediatek: remove error checks on NULL ipc
      commit: 8cf5286216dcfb942f0e4d7c23ebe06c2ebc1bed
[6/8] ASoC: Intel: bdw_rt286: add checks to avoid static analysis warnings
      commit: 64778b022e629b8ffa97d23a9adbf670aa3bb1d8
[7/8] ASoC: Intel: atom: remove static analysis false positive
      commit: 71d76768fbe72aa70dd61d5714a5579dc4ca61cb
[8/8] ASoC: amd: acp5x-mach:add checks to avoid static analysis warnings
      commit: 871861f6ad6d43b49caade3f42b9d40ca1413e79

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