sound/pci/ice1712: replace strcpy() with scnprintf()

PATCH Superseded Joey Pabalinas 3 participants 7 messages Mar 1, 2018 Sound

Share & tools

Joey Pabalinas

author

2018-03-01 11:45 UTC #0 lore raw

PATCH 1/1 sound/pci/ice1712: replace strcpy() with scnprintf()

Replace unsafe uses of strcpy() to copy the name argument into the sid.name buffer with scnprintf() to guard against possible buffer overflows.

Even though we don't actually care about the return value in this specific case, scnprintf() is still preferred over snprintf() due to scnprintf() returning the much more logical length of what was *actually* encoded into the destination array instead of returning length the resulting string *would* be, assuming it all fit into the destination array as snprintf() does.

Maybe one day someone will use the return value, and since the behavior is exactly the same apart from the return value it would be better to account for that possibility and program defensively.

Signed-off-by Joey Pabalinas <[email protected]>

 2 files changed, 8 insertions(+), 8 deletions(-)

sound/pci/ice1712/juli.c +4 -4

index 0dbaccf61f33270608..0abacc64168f895d53 100644
@@ -26,6 +26,7 @@
 #include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/init.h>
+#include <linux/kernel.h>
 #include <linux/slab.h>
 #include <sound/core.h>
 #include <sound/tlv.h>

@@ -425,10 +426,9 @@ DECLARE_TLV_DB_SCALE(juli_master_db_scale, -6350, 50, 1);
 static struct snd_kcontrol *ctl_find(struct snd_card *card,
 				     const char *name)
 {
-	struct snd_ctl_elem_id sid;
-	memset(&sid, 0, sizeof(sid));
-	/* FIXME: strcpy is bad. */
-	strcpy(sid.name, name);
+	struct snd_ctl_elem_id sid = {0};
+
+	scnprintf(sid.name, sizeof(sid.name), "%s", name);
 	sid.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
 	return snd_ctl_find_id(card, &sid);
 }

sound/pci/ice1712/quartet.c +4 -4

index d145b5eb7ff86d978d..332f6226548c6a089a 100644
@@ -25,6 +25,7 @@
 #include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/init.h>
+#include <linux/kernel.h>
 #include <linux/slab.h>
 #include <sound/core.h>
 #include <sound/tlv.h>

@@ -785,10 +786,9 @@ DECLARE_TLV_DB_SCALE(qtet_master_db_scale, -6350, 50, 1);
 static struct snd_kcontrol *ctl_find(struct snd_card *card,
 				     const char *name)
 {
-	struct snd_ctl_elem_id sid;
-	memset(&sid, 0, sizeof(sid));
-	/* FIXME: strcpy is bad. */
-	strcpy(sid.name, name);
+	struct snd_ctl_elem_id sid = {0};
+
+	scnprintf(sid.name, sizeof(sid.name), "%s", name);
 	sid.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
 	return snd_ctl_find_id(card, &sid);
 }

Signature · 1 line

2.16.2

Andy Shevchenko

2018-03-01 13:55 UTC #1 lore raw

sound/pci/ice1712/juli.c DECLARE_TLV_DB_SCALE(juli_master_db_scale, -6350, 50, 1); · line 431

Quoted 12 lines · Joey Pabalinas

Replace unsafe uses of strcpy() to copy the name argument into the sid.name buffer with scnprintf() to guard against possible buffer overflows.
sound/pci/ice1712/juli.c +3 -4
-       struct snd_ctl_elem_id sid;
-       memset(&sid, 0, sizeof(sid));
-       /* FIXME: strcpy is bad. */
-       strcpy(sid.name, name);
+       struct snd_ctl_elem_id sid = {0};
+
+       scnprintf(sid.name, sizeof(sid.name), "%s", name);

So, why not just use strlcpy()?
scnprintf() here adds an overhead for no benefit.

sound/pci/ice1712/juli.c DECLARE_TLV_DB_SCALE(juli_master_db_scale, -6350, 50, 1); · line 433

Quoted 12 lines

        sid.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
        return snd_ctl_find_id(card, &sid);

sound/pci/ice1712/juli.c +3 -4

-       struct snd_ctl_elem_id sid;
-       memset(&sid, 0, sizeof(sid));
-       /* FIXME: strcpy is bad. */
-       strcpy(sid.name, name);
+       struct snd_ctl_elem_id sid = {0};
+
+       scnprintf(sid.name, sizeof(sid.name), "%s", name);
        sid.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
        return snd_ctl_find_id(card, &sid);

Ditto.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Joey Pabalinas

author

2018-03-01 14:10 UTC #2 lore raw

Quoted 2 lines · Andy Shevchenko

So, why not just use strlcpy()?
scnprintf() here adds an overhead for no benefit.

That's a good point, actually. Revising it now; patch will follow shortly.

Signature · 1 line

Joey Pabalinas

Joey Pabalinas

author

2018-03-01 14:17 UTC #3 lore raw

PATCH v2 1/1 sound/pci/ice1712: replace strcpy() with strlcpy()

In reply to Andy Shevchenko

Replace unsafe usages of strcpy() to copy the name argument into the sid.name buffer with strlcpy() to guard against possible buffer overflows.

Signed-off-by Joey Pabalinas <[email protected]>

 2 files changed, 8 insertions(+), 8 deletions(-)

sound/pci/ice1712/juli.c +4 -4

index 0dbaccf61f33270608..21806bab4757241a9e 100644
@@ -27,6 +27,7 @@
 #include <linux/interrupt.h>
 #include <linux/init.h>
 #include <linux/slab.h>
+#include <linux/string.h>
 #include <sound/core.h>
 #include <sound/tlv.h>

@@ -425,10 +426,9 @@ DECLARE_TLV_DB_SCALE(juli_master_db_scale, -6350, 50, 1);
 static struct snd_kcontrol *ctl_find(struct snd_card *card,
 				     const char *name)
 {
-	struct snd_ctl_elem_id sid;
-	memset(&sid, 0, sizeof(sid));
-	/* FIXME: strcpy is bad. */
-	strcpy(sid.name, name);
+	struct snd_ctl_elem_id sid = {0};
+
+	strlcpy(sid.name, name, sizeof(sid.name));
 	sid.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
 	return snd_ctl_find_id(card, &sid);
 }

sound/pci/ice1712/quartet.c +4 -4

index d145b5eb7ff86d978d..5bc836241c977feb51 100644
@@ -26,6 +26,7 @@
 #include <linux/interrupt.h>
 #include <linux/init.h>
 #include <linux/slab.h>
+#include <linux/string.h>
 #include <sound/core.h>
 #include <sound/tlv.h>
 #include <sound/info.h>

@@ -785,10 +786,9 @@ DECLARE_TLV_DB_SCALE(qtet_master_db_scale, -6350, 50, 1);
 static struct snd_kcontrol *ctl_find(struct snd_card *card,
 				     const char *name)
 {
-	struct snd_ctl_elem_id sid;
-	memset(&sid, 0, sizeof(sid));
-	/* FIXME: strcpy is bad. */
-	strcpy(sid.name, name);
+	struct snd_ctl_elem_id sid = {0};
+
+	strlcpy(sid.name, name, sizeof(sid.name));
 	sid.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
 	return snd_ctl_find_id(card, &sid);
 }

Signature · 1 line

2.16.2

Andy Shevchenko

Reviewed-by on v2 1/1

2018-03-01 14:21 UTC #4 lore raw

Re: [PATCH v2] sound/pci/ice1712: replace strcpy() with strlcpy()

Quoted 6 lines · Joey Pabalinas

Replace unsafe usages of strcpy() to copy the name argument into the sid.name buffer with strlcpy() to guard against possible buffer overflows.
Signed-off-by Joey Pabalinas <[email protected]>
Suggested-by Andy Shevchenko <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

Quoted 56 lines

 2 files changed, 8 insertions(+), 8 deletions(-)

sound/pci/ice1712/juli.c +4 -4

index 0dbaccf61f33270608..21806bab4757241a9e 100644
@@ -27,6 +27,7 @@
 #include <linux/interrupt.h>
 #include <linux/init.h>
 #include <linux/slab.h>
+#include <linux/string.h>
 #include <sound/core.h>
 #include <sound/tlv.h>

@@ -425,10 +426,9 @@ DECLARE_TLV_DB_SCALE(juli_master_db_scale, -6350, 50, 1);
 static struct snd_kcontrol *ctl_find(struct snd_card *card,
                                     const char *name)
 {
-       struct snd_ctl_elem_id sid;
-       memset(&sid, 0, sizeof(sid));
-       /* FIXME: strcpy is bad. */
-       strcpy(sid.name, name);
+       struct snd_ctl_elem_id sid = {0};
+
+       strlcpy(sid.name, name, sizeof(sid.name));
        sid.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
        return snd_ctl_find_id(card, &sid);
 }

sound/pci/ice1712/quartet.c +4 -5

index d145b5eb7ff86d978d..5bc836241c977feb51 100644
@@ -26,6 +26,7 @@
 #include <linux/interrupt.h>
 #include <linux/init.h>
 #include <linux/slab.h>
+#include <linux/string.h>
 #include <sound/core.h>
 #include <sound/tlv.h>
 #include <sound/info.h>

@@ -785,10 +786,9 @@ DECLARE_TLV_DB_SCALE(qtet_master_db_scale, -6350, 50, 1);
 static struct snd_kcontrol *ctl_find(struct snd_card *card,
                                     const char *name)
 {
-       struct snd_ctl_elem_id sid;
-       memset(&sid, 0, sizeof(sid));
-       /* FIXME: strcpy is bad. */
-       strcpy(sid.name, name);
+       struct snd_ctl_elem_id sid = {0};
+
+       strlcpy(sid.name, name, sizeof(sid.name));
        sid.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
        return snd_ctl_find_id(card, &sid);
 }
--

2.16.2

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Takashi Iwai

Applied

2018-03-01 15:13 UTC #5 lore raw

Re: [PATCH v2] sound/pci/ice1712: replace strcpy() with strlcpy()

In reply to Joey Pabalinas

Quoted 9 lines · Joey Pabalinas

Replace unsafe usages of strcpy() to copy the name argument into the sid.name buffer with strlcpy() to guard against possible buffer overflows.
Signed-off-by Joey Pabalinas <[email protected]>
Suggested-by Andy Shevchenko <[email protected]>
 2 files changed, 8 insertions(+), 8 deletions(-)

Thanks, applied now.

Takashi

Joey Pabalinas

author

2018-03-01 15:22 UTC #6 lore raw

Re: [PATCH v2] sound/pci/ice1712: replace strcpy() with strlcpy()

Quoted 1 line · Takashi Iwai

Thanks, applied now.

Cheers

Signature · 1 line

Joey Pabalinas