[patch] ALSA: hda - potential (but unlikely) uninitialized variable

[patch] ALSA: hda - potential (but unlikely) uninitialized variable

[Dan Carpenter]

This function is a bit unusual because it accepts negative values as
"conn_len".  It's theoretically possible for both "cache_len" and
"conn_len" to be -ENOSPC and in that case we would oops trying to run
memcmp() on the uninitialized "list" pointer.

My static checker complains about this.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/sound/pci/hda/hda_proc.c b/sound/pci/hda/hda_proc.c
index ce5a6da..387fdfc 100644
--- a/sound/pci/hda/hda_proc.c
+++ b/sound/pci/hda/hda_proc.c
@@ -556,8 +556,8 @@ static void print_conn_list(struct snd_info_buffer *buffer,
 
 	/* Get Cache connections info */
 	cache_len = snd_hda_get_conn_list(codec, nid, &list);
-	if (cache_len != conn_len
-			|| memcmp(list, conn, conn_len)) {
+	if (cache_len < 0 || cache_len != conn_len ||
+	    memcmp(list, conn, conn_len) != 0) {
 		snd_iprintf(buffer, "  In-driver Connection: %d\n", cache_len);
 		if (cache_len > 0) {
 			snd_iprintf(buffer, "    ");

Re: [patch] ALSA: hda - potential (but unlikely) uninitialized variable

[Takashi Iwai]

At Fri, 17 Apr 2015 15:35:15 +0300,
Dan Carpenter wrote:
> 
> This function is a bit unusual because it accepts negative values as
> "conn_len".  It's theoretically possible for both "cache_len" and
> "conn_len" to be -ENOSPC and in that case we would oops trying to run
> memcmp() on the uninitialized "list" pointer.

Yes, that's a bug.  But the check should be rather:

	if (cache_len >= 0 && (cache_len != conn_len ||
		    memcmp(list, conn, conn_len) != 0))

Could you resend with this fix?


thanks,

Takashi


> 
> My static checker complains about this.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/sound/pci/hda/hda_proc.c b/sound/pci/hda/hda_proc.c
> index ce5a6da..387fdfc 100644
> --- a/sound/pci/hda/hda_proc.c
> +++ b/sound/pci/hda/hda_proc.c
> @@ -556,8 +556,8 @@ static void print_conn_list(struct snd_info_buffer *buffer,
>  
>  	/* Get Cache connections info */
>  	cache_len = snd_hda_get_conn_list(codec, nid, &list);
> -	if (cache_len != conn_len
> -			|| memcmp(list, conn, conn_len)) {
> +	if (cache_len < 0 || cache_len != conn_len ||
> +	    memcmp(list, conn, conn_len) != 0) {
>  		snd_iprintf(buffer, "  In-driver Connection: %d\n", cache_len);
>  		if (cache_len > 0) {
>  			snd_iprintf(buffer, "    ");
> 

Re: [patch] ALSA: hda - potential (but unlikely) uninitialized variable

[Dan Carpenter]

On Fri, Apr 17, 2015 at 02:42:05PM +0200, Takashi Iwai wrote:
> At Fri, 17 Apr 2015 15:35:15 +0300,
> Dan Carpenter wrote:
> > 
> > This function is a bit unusual because it accepts negative values as
> > "conn_len".  It's theoretically possible for both "cache_len" and
> > "conn_len" to be -ENOSPC and in that case we would oops trying to run
> > memcmp() on the uninitialized "list" pointer.
> 
> Yes, that's a bug.  But the check should be rather:
> 
> 	if (cache_len >= 0 && (cache_len != conn_len ||
> 		    memcmp(list, conn, conn_len) != 0))
> 
> Could you resend with this fix?

Sure.  Will do.

regards,
dan carpenter

[patch v2] ALSA: hda - potential (but unlikely) uninitialized variable

[Dan Carpenter]

This function is a bit unusual because it accepts negative values as
"conn_len".  It's theoretically possible for both "cache_len" and
"conn_len" to be -ENOSPC and in that case we would oops trying to run
memcmp() on the uninitialized "list" pointer.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: Don't print a negative error code as "In-driver Connection."

diff --git a/sound/pci/hda/hda_proc.c b/sound/pci/hda/hda_proc.c
index ee62307..2f00886 100644
--- a/sound/pci/hda/hda_proc.c
+++ b/sound/pci/hda/hda_proc.c
@@ -582,8 +582,8 @@ static void print_conn_list(struct snd_info_buffer *buffer,
 
 	/* Get Cache connections info */
 	cache_len = snd_hda_get_conn_list(codec, nid, &list);
-	if (cache_len != conn_len
-			|| memcmp(list, conn, conn_len)) {
+	if (cache_len >= 0 && (cache_len != conn_len ||
+			      memcmp(list, conn, conn_len) != 0)) {
 		snd_iprintf(buffer, "  In-driver Connection: %d\n", cache_len);
 		if (cache_len > 0) {
 			snd_iprintf(buffer, "    ");

Re: [patch v2] ALSA: hda - potential (but unlikely) uninitialized variable

[Takashi Iwai]

At Fri, 17 Apr 2015 16:19:46 +0300,
Dan Carpenter wrote:
> 
> This function is a bit unusual because it accepts negative values as
> "conn_len".  It's theoretically possible for both "cache_len" and
> "conn_len" to be -ENOSPC and in that case we would oops trying to run
> memcmp() on the uninitialized "list" pointer.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: Don't print a negative error code as "In-driver Connection."

Applied, thanks.


Takashi

> 
> diff --git a/sound/pci/hda/hda_proc.c b/sound/pci/hda/hda_proc.c
> index ee62307..2f00886 100644
> --- a/sound/pci/hda/hda_proc.c
> +++ b/sound/pci/hda/hda_proc.c
> @@ -582,8 +582,8 @@ static void print_conn_list(struct snd_info_buffer *buffer,
>  
>  	/* Get Cache connections info */
>  	cache_len = snd_hda_get_conn_list(codec, nid, &list);
> -	if (cache_len != conn_len
> -			|| memcmp(list, conn, conn_len)) {
> +	if (cache_len >= 0 && (cache_len != conn_len ||
> +			      memcmp(list, conn, conn_len) != 0)) {
>  		snd_iprintf(buffer, "  In-driver Connection: %d\n", cache_len);
>  		if (cache_len > 0) {
>  			snd_iprintf(buffer, "    ");
>