[PATCH 0/8] ASoC/SOF/Intel/AMD: cleanups for GCC11 -fanalyzer checks
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(-)
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)
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);
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)
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; }
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; }
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); }
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);
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);
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
participants (2)
-
Mark Brown -
Pierre-Louis Bossart