Discussion:
[PATCH 0/2] ASoC: two trivial fixes
Daniel Mack
2014-10-19 07:07:34 UTC
Permalink
Just two random fixes spotted by Coverity.

Daniel Mack (2):
ASoC: soc-compress: consolidate two identical branches
ASoC: fsl: use strncpy() to prevent copying of over-long names

sound/soc/fsl/fsl_asrc.c | 2 +-
sound/soc/fsl/fsl_esai.c | 2 +-
sound/soc/soc-compress.c | 11 ++---------
3 files changed, 4 insertions(+), 11 deletions(-)
--
2.1.0
Daniel Mack
2014-10-19 07:07:35 UTC
Permalink
The actions taken in both branches are identical, so we can simplify the
code. Spotted by Coverity.

Signed-off-by: Daniel Mack <***@zonque.org>
---
sound/soc/soc-compress.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
index cecfab3..590a82f 100644
--- a/sound/soc/soc-compress.c
+++ b/sound/soc/soc-compress.c
@@ -258,10 +258,7 @@ static int soc_compr_free_fe(struct snd_compr_stream *cstream)
list_for_each_entry(dpcm, &fe->dpcm[stream].be_clients, list_be)
dpcm->state = SND_SOC_DPCM_LINK_STATE_FREE;

- if (stream == SNDRV_PCM_STREAM_PLAYBACK)
- dpcm_dapm_stream_event(fe, stream, SND_SOC_DAPM_STREAM_STOP);
- else
- dpcm_dapm_stream_event(fe, stream, SND_SOC_DAPM_STREAM_STOP);
+ dpcm_dapm_stream_event(fe, stream, SND_SOC_DAPM_STREAM_STOP);

fe->dpcm[stream].state = SND_SOC_DPCM_STATE_CLOSE;
fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
@@ -456,11 +453,7 @@ static int soc_compr_set_params_fe(struct snd_compr_stream *cstream,
if (ret < 0)
goto out;

- if (stream == SNDRV_PCM_STREAM_PLAYBACK)
- dpcm_dapm_stream_event(fe, stream, SND_SOC_DAPM_STREAM_START);
- else
- dpcm_dapm_stream_event(fe, stream, SND_SOC_DAPM_STREAM_START);
-
+ dpcm_dapm_stream_event(fe, stream, SND_SOC_DAPM_STREAM_START);
fe->dpcm[stream].state = SND_SOC_DPCM_STATE_PREPARE;

out:
--
2.1.0
Daniel Mack
2014-10-19 07:07:36 UTC
Permalink
Use strncpy() instead of strcpy(). That's not a security issue, as the
source buffer is taken from DT nodes, but we should still enforce bound
checks. Spotted by Coverity.

Signed-off-by: Daniel Mack <***@zonque.org>
---
sound/soc/fsl/fsl_asrc.c | 2 +-
sound/soc/fsl/fsl_esai.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 8221104..dd00b9d 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -792,7 +792,7 @@ static int fsl_asrc_probe(struct platform_device *pdev)
return -ENOMEM;

asrc_priv->pdev = pdev;
- strcpy(asrc_priv->name, np->name);
+ strncpy(asrc_priv->name, np->name, sizeof(asrc_priv->name) - 1);

/* Get the addresses and IRQ */
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index a3b29ed..fda1d46 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -729,7 +729,7 @@ static int fsl_esai_probe(struct platform_device *pdev)
return -ENOMEM;

esai_priv->pdev = pdev;
- strcpy(esai_priv->name, np->name);
+ strncpy(esai_priv->name, np->name, sizeof(esai_priv->name) - 1);

if (of_property_read_bool(np, "big-endian"))
fsl_esai_regmap_config.val_format_endian = REGMAP_ENDIAN_BIG;
--
2.1.0
Mike Looijmans
2014-10-20 05:55:22 UTC
Permalink
This hardly improves matters. When the source string is larger than the
buffer, the destination may not be nul-terminated.
Also, strncpy ALWAYS writes the full buffer so it may be wasting cycles when
the destination buffer is large.

I'm sure the kernel offers a better alternative. Even "snprintf" is a better
alternative.

Mike.
Post by Daniel Mack
Use strncpy() instead of strcpy(). That's not a security issue, as the
source buffer is taken from DT nodes, but we should still enforce bound
checks. Spotted by Coverity.
---
sound/soc/fsl/fsl_asrc.c | 2 +-
sound/soc/fsl/fsl_esai.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 8221104..dd00b9d 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -792,7 +792,7 @@ static int fsl_asrc_probe(struct platform_device *pdev)
return -ENOMEM;
asrc_priv->pdev = pdev;
- strcpy(asrc_priv->name, np->name);
+ strncpy(asrc_priv->name, np->name, sizeof(asrc_priv->name) - 1);
/* Get the addresses and IRQ */
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index a3b29ed..fda1d46 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -729,7 +729,7 @@ static int fsl_esai_probe(struct platform_device *pdev)
return -ENOMEM;
esai_priv->pdev = pdev;
- strcpy(esai_priv->name, np->name);
+ strncpy(esai_priv->name, np->name, sizeof(esai_priv->name) - 1);
if (of_property_read_bool(np, "big-endian"))
fsl_esai_regmap_config.val_format_endian = REGMAP_ENDIAN_BIG;
Met vriendelijke groet / kind regards,

Mike Looijmans

TOPIC Embedded Systems
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: (+31) (0) 499 33 69 79
Telefax: (+31) (0) 499 33 69 70
E-mail: ***@topic.nl
Website: www.topic.nl

Please consider the environment before printing this e-mail

Topic zoekt gedreven (embedded) software specialisten!
http://topic.nl/vacatures/topic-zoekt-software-engineers/
Takashi Iwai
2014-10-22 11:50:03 UTC
Permalink
At Mon, 20 Oct 2014 07:55:22 +0200,
Post by Mike Looijmans
This hardly improves matters. When the source string is larger than the
buffer, the destination may not be nul-terminated.
Also, strncpy ALWAYS writes the full buffer so it may be wasting cycles when
the destination buffer is large.
Indeed, strncpy() should be avoided.
Post by Mike Looijmans
I'm sure the kernel offers a better alternative. Even "snprintf" is a better
alternative.
strlcpy() is the best choice.


Takashi
Post by Mike Looijmans
Mike.
Post by Daniel Mack
Use strncpy() instead of strcpy(). That's not a security issue, as the
source buffer is taken from DT nodes, but we should still enforce bound
checks. Spotted by Coverity.
---
sound/soc/fsl/fsl_asrc.c | 2 +-
sound/soc/fsl/fsl_esai.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 8221104..dd00b9d 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -792,7 +792,7 @@ static int fsl_asrc_probe(struct platform_device *pdev)
return -ENOMEM;
asrc_priv->pdev = pdev;
- strcpy(asrc_priv->name, np->name);
+ strncpy(asrc_priv->name, np->name, sizeof(asrc_priv->name) - 1);
/* Get the addresses and IRQ */
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index a3b29ed..fda1d46 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -729,7 +729,7 @@ static int fsl_esai_probe(struct platform_device *pdev)
return -ENOMEM;
esai_priv->pdev = pdev;
- strcpy(esai_priv->name, np->name);
+ strncpy(esai_priv->name, np->name, sizeof(esai_priv->name) - 1);
if (of_property_read_bool(np, "big-endian"))
fsl_esai_regmap_config.val_format_endian = REGMAP_ENDIAN_BIG;
Met vriendelijke groet / kind regards,
Mike Looijmans
TOPIC Embedded Systems
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: (+31) (0) 499 33 69 79
Telefax: (+31) (0) 499 33 69 70
Website: www.topic.nl
Please consider the environment before printing this e-mail
Topic zoekt gedreven (embedded) software specialisten!
http://topic.nl/vacatures/topic-zoekt-software-engineers/
_______________________________________________
Alsa-devel mailing list
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Mark Brown
2014-10-22 10:42:10 UTC
Permalink
Post by Daniel Mack
Just two random fixes spotted by Coverity.
Applied both. I'd been holding off on these waiting for the developers
of the relevant code to review but I now notice that you didn't CC them
which is probably why they didn't respond!
Loading...