[20/22] sound: pci: avoid string overflow warnings

Message ID 20170714120720.906842-21-arnd@arndb.de
State New
Headers show
Series
  • gcc-7 -Wformat-* warnings
Related show

Commit Message

Arnd Bergmann July 14, 2017, 12:07 p.m.
With gcc-7, we get various warnings about a possible string overflow:

sound/pci/rme9652/hdspm.c: In function 'snd_hdspm_create_alsa_devices':
sound/pci/rme9652/hdspm.c:2123:17: error: ' MIDIoverMADI' directive writing 13 bytes into a region of size between 1 and 32 [-Werror=format-overflow=]
sound/pci/pcxhr/pcxhr.c: In function 'pcxhr_probe':
sound/pci/pcxhr/pcxhr.c:1647:28: error: ' [PCM #' directive writing 7 bytes into a region of size between 1 and 32 [-Werror=format-overflow=]
sound/pci/mixart/mixart.c: In function 'snd_mixart_probe':
sound/pci/mixart/mixart.c:1353:28: error: ' [PCM #' directive writing 7 bytes into a region of size between 1 and 32 [-Werror=format-overflow=]
   sprintf(card->shortname, "%s [PCM #%d]", mgr->shortname, i);
                            ^~~~~~~~~~~~~~
sound/pci/mixart/mixart.c:1353:28: note: using the range [-2147483648, 2147483647] for directive argument
sound/pci/mixart/mixart.c:1353:3: note: 'sprintf' output between 10 and 51 bytes into a destination of size 32
   sprintf(card->shortname, "%s [PCM #%d]", mgr->shortname, i);
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
sound/pci/mixart/mixart.c:1354:27: error: ' [PCM #' directive writing 7 bytes into a region of size between 1 and 80 [-Werror=format-overflow=]
   sprintf(card->longname, "%s [PCM #%d]", mgr->longname, i);
                           ^~~~~~~~~~~~~~
sound/pci/mixart/mixart.c:1354:27: note: using the range [-2147483648, 2147483647] for directive argument
sound/pci/mixart/mixart.c:1354:3: note: 'sprintf' output between 10 and 99 bytes into a destination of size 80

I have checked these all and found that the driver-private
shortname strings for mixart and pcxhr are longer than necessary,
and making them shorter will be safe while also making it clear
that no overflow can happen when they get passed as a substring
into the card shortname.

For hdspm, we have a local buffer of the same size as its substring.
In this case, making the buffer a little longer is safe as the
functions that take it as an argument all use length checking and
the strings we pass into it are actually short enough.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
 sound/pci/mixart/mixart.h | 4 ++--
 sound/pci/pcxhr/pcxhr.h   | 4 ++--
 sound/pci/rme9652/hdspm.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

-- 
2.9.0

Comments

Takashi Iwai July 14, 2017, 12:28 p.m. | #1
On Fri, 14 Jul 2017 14:07:12 +0200,
Arnd Bergmann wrote:
> 

> With gcc-7, we get various warnings about a possible string overflow:

> 

> sound/pci/rme9652/hdspm.c: In function 'snd_hdspm_create_alsa_devices':

> sound/pci/rme9652/hdspm.c:2123:17: error: ' MIDIoverMADI' directive writing 13 bytes into a region of size between 1 and 32 [-Werror=format-overflow=]

> sound/pci/pcxhr/pcxhr.c: In function 'pcxhr_probe':

> sound/pci/pcxhr/pcxhr.c:1647:28: error: ' [PCM #' directive writing 7 bytes into a region of size between 1 and 32 [-Werror=format-overflow=]

> sound/pci/mixart/mixart.c: In function 'snd_mixart_probe':

> sound/pci/mixart/mixart.c:1353:28: error: ' [PCM #' directive writing 7 bytes into a region of size between 1 and 32 [-Werror=format-overflow=]

>    sprintf(card->shortname, "%s [PCM #%d]", mgr->shortname, i);

>                             ^~~~~~~~~~~~~~

> sound/pci/mixart/mixart.c:1353:28: note: using the range [-2147483648, 2147483647] for directive argument

> sound/pci/mixart/mixart.c:1353:3: note: 'sprintf' output between 10 and 51 bytes into a destination of size 32

>    sprintf(card->shortname, "%s [PCM #%d]", mgr->shortname, i);

>    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

> sound/pci/mixart/mixart.c:1354:27: error: ' [PCM #' directive writing 7 bytes into a region of size between 1 and 80 [-Werror=format-overflow=]

>    sprintf(card->longname, "%s [PCM #%d]", mgr->longname, i);

>                            ^~~~~~~~~~~~~~

> sound/pci/mixart/mixart.c:1354:27: note: using the range [-2147483648, 2147483647] for directive argument

> sound/pci/mixart/mixart.c:1354:3: note: 'sprintf' output between 10 and 99 bytes into a destination of size 80

> 

> I have checked these all and found that the driver-private

> shortname strings for mixart and pcxhr are longer than necessary,

> and making them shorter will be safe while also making it clear

> that no overflow can happen when they get passed as a substring

> into the card shortname.

> 

> For hdspm, we have a local buffer of the same size as its substring.

> In this case, making the buffer a little longer is safe as the

> functions that take it as an argument all use length checking and

> the strings we pass into it are actually short enough.

> 

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>


Thanks for the patch.  I have seen it but ignored, so far, as not sure
which action is the best.  An alternative solution is to use
snprintf() blindly, for example.

For mixart, it's even better to drop mgr->shortname[] and longname[]
assignment.  The shortname is the fixed string, and the longname is
used only at copying to card->longname, so we can create a string
there from the scratch.


Takashi

> ---

>  sound/pci/mixart/mixart.h | 4 ++--

>  sound/pci/pcxhr/pcxhr.h   | 4 ++--

>  sound/pci/rme9652/hdspm.c | 2 +-

>  3 files changed, 5 insertions(+), 5 deletions(-)

> 

> diff --git a/sound/pci/mixart/mixart.h b/sound/pci/mixart/mixart.h

> index 426743871540..c8309e327663 100644

> --- a/sound/pci/mixart/mixart.h

> +++ b/sound/pci/mixart/mixart.h

> @@ -75,8 +75,8 @@ struct mixart_mgr {

>  	struct mem_area mem[2];

>  

>  	/* share the name */

> -	char shortname[32];         /* short name of this soundcard */

> -	char longname[80];          /* name of this soundcard */

> +	char shortname[16];         /* short name of this soundcard */

> +	char longname[40];          /* name of this soundcard */

>  

>  	/* one and only blocking message or notification may be pending  */

>  	u32 pending_event;

> diff --git a/sound/pci/pcxhr/pcxhr.h b/sound/pci/pcxhr/pcxhr.h

> index 9e39e509a3ef..4909a43ce3d9 100644

> --- a/sound/pci/pcxhr/pcxhr.h

> +++ b/sound/pci/pcxhr/pcxhr.h

> @@ -75,8 +75,8 @@ struct pcxhr_mgr {

>  	unsigned long port[3];

>  

>  	/* share the name */

> -	char shortname[32];		/* short name of this soundcard */

> -	char longname[96];		/* name of this soundcard */

> +	char shortname[16];		/* short name of this soundcard */

> +	char longname[40];		/* name of this soundcard */

>  

>  	struct pcxhr_rmh *prmh;

>  

> diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c

> index 254c3d040118..a1cbf5938a0e 100644

> --- a/sound/pci/rme9652/hdspm.c

> +++ b/sound/pci/rme9652/hdspm.c

> @@ -2061,7 +2061,7 @@ static int snd_hdspm_create_midi(struct snd_card *card,

>  				 struct hdspm *hdspm, int id)

>  {

>  	int err;

> -	char buf[32];

> +	char buf[64];

>  

>  	hdspm->midi[id].id = id;

>  	hdspm->midi[id].hdspm = hdspm;

> -- 

> 2.9.0

> 

>
Arnd Bergmann July 18, 2017, 11:52 a.m. | #2
On Fri, Jul 14, 2017 at 2:28 PM, Takashi Iwai <tiwai@suse.de> wrote:
> On Fri, 14 Jul 2017 14:07:12 +0200,

>

> Thanks for the patch.  I have seen it but ignored, so far, as not sure

> which action is the best.  An alternative solution is to use

> snprintf() blindly, for example.

>

> For mixart, it's even better to drop mgr->shortname[] and longname[]

> assignment.  The shortname is the fixed string, and the longname is

> used only at copying to card->longname, so we can create a string

> there from the scratch.


I've done that now, and tried to be a little smarter with the other
conversions. I also found related problems in ISA drivers after
randconfig testing and fixed those as well.

Sent a 7-patch series now as a replacement.

       Arnd

Patch

diff --git a/sound/pci/mixart/mixart.h b/sound/pci/mixart/mixart.h
index 426743871540..c8309e327663 100644
--- a/sound/pci/mixart/mixart.h
+++ b/sound/pci/mixart/mixart.h
@@ -75,8 +75,8 @@  struct mixart_mgr {
 	struct mem_area mem[2];
 
 	/* share the name */
-	char shortname[32];         /* short name of this soundcard */
-	char longname[80];          /* name of this soundcard */
+	char shortname[16];         /* short name of this soundcard */
+	char longname[40];          /* name of this soundcard */
 
 	/* one and only blocking message or notification may be pending  */
 	u32 pending_event;
diff --git a/sound/pci/pcxhr/pcxhr.h b/sound/pci/pcxhr/pcxhr.h
index 9e39e509a3ef..4909a43ce3d9 100644
--- a/sound/pci/pcxhr/pcxhr.h
+++ b/sound/pci/pcxhr/pcxhr.h
@@ -75,8 +75,8 @@  struct pcxhr_mgr {
 	unsigned long port[3];
 
 	/* share the name */
-	char shortname[32];		/* short name of this soundcard */
-	char longname[96];		/* name of this soundcard */
+	char shortname[16];		/* short name of this soundcard */
+	char longname[40];		/* name of this soundcard */
 
 	struct pcxhr_rmh *prmh;
 
diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c
index 254c3d040118..a1cbf5938a0e 100644
--- a/sound/pci/rme9652/hdspm.c
+++ b/sound/pci/rme9652/hdspm.c
@@ -2061,7 +2061,7 @@  static int snd_hdspm_create_midi(struct snd_card *card,
 				 struct hdspm *hdspm, int id)
 {
 	int err;
-	char buf[32];
+	char buf[64];
 
 	hdspm->midi[id].id = id;
 	hdspm->midi[id].hdspm = hdspm;