[PATCH] ALSA: usb-audio: fix possible hang and overflow in parse_uac2_sample_rate_range()

[PATCH] ALSA: usb-audio: fix possible hang and overflow in parse_uac2_sample_rate_range()

[Xi Wang]

A malicious USB device may feed in carefully crafted min/max/res values,
so that the inner loop in parse_uac2_sample_rate_range() could run for
a long time or even never terminate, e.g., given max = INT_MAX.

Also nr_rates could be a large integer, which causes an integer overflow
in the subsequent call to kmalloc() in parse_audio_format_rates_v2().
Thus, kmalloc() would allocate a smaller buffer than expected, leading
to a memory corruption.

To exploit the two vulnerabilities, an attacker needs physical access
to the machine to plug in a malicious USB device.

This patch makes two changes.

1) The type of "rate" is changed to unsigned int, so that the loop could
   stop once "rate" is larger than INT_MAX.

2) Limit nr_rates to avoid overflow.

Signed-off-by: Xi Wang <xi.wang@gmail.com>
---
 sound/usb/format.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/sound/usb/format.c b/sound/usb/format.c
index 89421d1..a99de67 100644
--- a/sound/usb/format.c
+++ b/sound/usb/format.c
@@ -226,7 +226,7 @@ static int parse_uac2_sample_rate_range(struct audioformat *fp, int nr_triplets,
 		int min = combine_quad(&data[2 + 12 * i]);
 		int max = combine_quad(&data[6 + 12 * i]);
 		int res = combine_quad(&data[10 + 12 * i]);
-		int rate;
+		unsigned int rate;
 
 		if ((max < 0) || (min < 0) || (res < 0) || (max < min))
 			continue;
@@ -253,6 +253,9 @@ static int parse_uac2_sample_rate_range(struct audioformat *fp, int nr_triplets,
 			fp->rates |= snd_pcm_rate_to_rate_bit(rate);
 
 			nr_rates++;
+			/* avoid overflow */
+			if (nr_rates == KMALLOC_MAX_SIZE / sizeof(int))
+				break;
 
 			/* avoid endless loop */
 			if (res == 0)
-- 
1.7.5.4

Re: [PATCH] ALSA: usb-audio: fix possible hang and overflow in parse_uac2_sample_rate_range()

[Takashi Iwai]

At Wed,  4 Jan 2012 12:39:09 -0500,
Xi Wang wrote:
> 
> A malicious USB device may feed in carefully crafted min/max/res values,
> so that the inner loop in parse_uac2_sample_rate_range() could run for
> a long time or even never terminate, e.g., given max = INT_MAX.
> 
> Also nr_rates could be a large integer, which causes an integer overflow
> in the subsequent call to kmalloc() in parse_audio_format_rates_v2().
> Thus, kmalloc() would allocate a smaller buffer than expected, leading
> to a memory corruption.
> 
> To exploit the two vulnerabilities, an attacker needs physical access
> to the machine to plug in a malicious USB device.
> 
> This patch makes two changes.
> 
> 1) The type of "rate" is changed to unsigned int, so that the loop could
>    stop once "rate" is larger than INT_MAX.
> 
> 2) Limit nr_rates to avoid overflow.
> 
> Signed-off-by: Xi Wang <xi.wang@gmail.com>

Thanks for the patch.
As of now, I have little time to evaluate, so I might have missed
something, but I wonder whether 

		/* avoid overflow */
		if (nr_rates == KMALLOC_MAX_SIZE / sizeof(int))
			break;

is the best way to check.  This looks ugly to me.
If we need to limit the number of rates, better to define some proper
numbers as the upper limit.  And then, it should warn, not only
breaking loop.

Anyway I'll check this tomorrow in more details.


thanks,

Takashi

> ---
>  sound/usb/format.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/sound/usb/format.c b/sound/usb/format.c
> index 89421d1..a99de67 100644
> --- a/sound/usb/format.c
> +++ b/sound/usb/format.c
> @@ -226,7 +226,7 @@ static int parse_uac2_sample_rate_range(struct audioformat *fp, int nr_triplets,
>  		int min = combine_quad(&data[2 + 12 * i]);
>  		int max = combine_quad(&data[6 + 12 * i]);
>  		int res = combine_quad(&data[10 + 12 * i]);
> -		int rate;
> +		unsigned int rate;
>  
>  		if ((max < 0) || (min < 0) || (res < 0) || (max < min))
>  			continue;
> @@ -253,6 +253,9 @@ static int parse_uac2_sample_rate_range(struct audioformat *fp, int nr_triplets,
>  			fp->rates |= snd_pcm_rate_to_rate_bit(rate);
>  
>  			nr_rates++;
> +			/* avoid overflow */
> +			if (nr_rates == KMALLOC_MAX_SIZE / sizeof(int))
> +				break;
>  
>  			/* avoid endless loop */
>  			if (res == 0)
> -- 
> 1.7.5.4
> 

Re: [PATCH] ALSA: usb-audio: fix possible hang and overflow in parse_uac2_sample_rate_range()

[Takashi Iwai]

At Wed,  4 Jan 2012 12:39:09 -0500,
Xi Wang wrote:
> 
> A malicious USB device may feed in carefully crafted min/max/res values,
> so that the inner loop in parse_uac2_sample_rate_range() could run for
> a long time or even never terminate, e.g., given max = INT_MAX.
> 
> Also nr_rates could be a large integer, which causes an integer overflow
> in the subsequent call to kmalloc() in parse_audio_format_rates_v2().
> Thus, kmalloc() would allocate a smaller buffer than expected, leading
> to a memory corruption.
> 
> To exploit the two vulnerabilities, an attacker needs physical access
> to the machine to plug in a malicious USB device.
> 
> This patch makes two changes.
> 
> 1) The type of "rate" is changed to unsigned int, so that the loop could
>    stop once "rate" is larger than INT_MAX.
> 
> 2) Limit nr_rates to avoid overflow.
> 
> Signed-off-by: Xi Wang <xi.wang@gmail.com>

Thanks for the patch.
As of now, I have little time to evaluate, so I might have missed
something, but I wonder whether 

		/* avoid overflow */
		if (nr_rates == KMALLOC_MAX_SIZE / sizeof(int))
			break;

is the best way to check.  This looks ugly to me.
If we need to limit the number of rates, better to define some proper
numbers as the upper limit.  And then, it should warn, not only
breaking loop.

Anyway I'll check this tomorrow in more details.


thanks,

Takashi

> ---
>  sound/usb/format.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/sound/usb/format.c b/sound/usb/format.c
> index 89421d1..a99de67 100644
> --- a/sound/usb/format.c
> +++ b/sound/usb/format.c
> @@ -226,7 +226,7 @@ static int parse_uac2_sample_rate_range(struct audioformat *fp, int nr_triplets,
>  		int min = combine_quad(&data[2 + 12 * i]);
>  		int max = combine_quad(&data[6 + 12 * i]);
>  		int res = combine_quad(&data[10 + 12 * i]);
> -		int rate;
> +		unsigned int rate;
>  
>  		if ((max < 0) || (min < 0) || (res < 0) || (max < min))
>  			continue;
> @@ -253,6 +253,9 @@ static int parse_uac2_sample_rate_range(struct audioformat *fp, int nr_triplets,
>  			fp->rates |= snd_pcm_rate_to_rate_bit(rate);
>  
>  			nr_rates++;
> +			/* avoid overflow */
> +			if (nr_rates == KMALLOC_MAX_SIZE / sizeof(int))
> +				break;
>  
>  			/* avoid endless loop */
>  			if (res == 0)
> -- 
> 1.7.5.4
> 

Re: [PATCH] ALSA: usb-audio: fix possible hang and overflow in parse_uac2_sample_rate_range()

[Xi Wang]

On Jan 8, 2012, at 4:09 AM, Takashi Iwai wrote:
> As of now, I have little time to evaluate, so I might have missed
> something, but I wonder whether 
> 
> 		/* avoid overflow */
> 		if (nr_rates == KMALLOC_MAX_SIZE / sizeof(int))
> 			break;
> 
> is the best way to check.  This looks ugly to me.
> If we need to limit the number of rates, better to define some proper
> numbers as the upper limit.  And then, it should warn, not only
> breaking loop.

Thanks for looking into this.  Yeah, I agree using something like
MAX_NR_RATES is better.  Is 65535 okay or do we need a larger limit?

- xi

Re: [PATCH] ALSA: usb-audio: fix possible hang and overflow in parse_uac2_sample_rate_range()

[Xi Wang]

On Jan 8, 2012, at 4:09 AM, Takashi Iwai wrote:
> As of now, I have little time to evaluate, so I might have missed
> something, but I wonder whether 
> 
> 		/* avoid overflow */
> 		if (nr_rates == KMALLOC_MAX_SIZE / sizeof(int))
> 			break;
> 
> is the best way to check.  This looks ugly to me.
> If we need to limit the number of rates, better to define some proper
> numbers as the upper limit.  And then, it should warn, not only
> breaking loop.

Thanks for looking into this.  Yeah, I agree using something like
MAX_NR_RATES is better.  Is 65535 okay or do we need a larger limit?

- xi

Re: [PATCH] ALSA: usb-audio: fix possible hang and overflow in parse_uac2_sample_rate_range()

[Takashi Iwai]

At Sun, 8 Jan 2012 07:45:09 -0500,
Xi Wang wrote:
> 
> On Jan 8, 2012, at 4:09 AM, Takashi Iwai wrote:
> > As of now, I have little time to evaluate, so I might have missed
> > something, but I wonder whether 
> > 
> > 		/* avoid overflow */
> > 		if (nr_rates == KMALLOC_MAX_SIZE / sizeof(int))
> > 			break;
> > 
> > is the best way to check.  This looks ugly to me.
> > If we need to limit the number of rates, better to define some proper
> > numbers as the upper limit.  And then, it should warn, not only
> > breaking loop.
> 
> Thanks for looking into this.  Yeah, I agree using something like
> MAX_NR_RATES is better.  Is 65535 okay or do we need a larger limit?

It's way too higher than any realistic situation ;)
I guess 1000 or such should suffice.


thanks,

Takashi

Re: [PATCH] ALSA: usb-audio: fix possible hang and overflow in parse_uac2_sample_rate_range()

[Takashi Iwai]

At Sun, 8 Jan 2012 07:45:09 -0500,
Xi Wang wrote:
> 
> On Jan 8, 2012, at 4:09 AM, Takashi Iwai wrote:
> > As of now, I have little time to evaluate, so I might have missed
> > something, but I wonder whether 
> > 
> > 		/* avoid overflow */
> > 		if (nr_rates == KMALLOC_MAX_SIZE / sizeof(int))
> > 			break;
> > 
> > is the best way to check.  This looks ugly to me.
> > If we need to limit the number of rates, better to define some proper
> > numbers as the upper limit.  And then, it should warn, not only
> > breaking loop.
> 
> Thanks for looking into this.  Yeah, I agree using something like
> MAX_NR_RATES is better.  Is 65535 okay or do we need a larger limit?

It's way too higher than any realistic situation ;)
I guess 1000 or such should suffice.


thanks,

Takashi

Re: [PATCH] ALSA: usb-audio: fix possible hang and overflow in parse_uac2_sample_rate_range()

[Xi Wang]

On 1/8/12 8:20 AM, Takashi Iwai wrote:
> It's way too higher than any realistic situation ;)
> I guess 1000 or such should suffice.

Let's limit it to 1024 then. ;-)

I will do a v2.  Thanks.

- xi

Re: [PATCH] ALSA: usb-audio: fix possible hang and overflow in parse_uac2_sample_rate_range()

[Xi Wang]

On 1/8/12 8:20 AM, Takashi Iwai wrote:
> It's way too higher than any realistic situation ;)
> I guess 1000 or such should suffice.

Let's limit it to 1024 then. ;-)

I will do a v2.  Thanks.

- xi

[PATCH v2] ALSA: usb-audio: fix possible hang and overflow in parse_uac2_sample_rate_range()

[Xi Wang]

A malicious USB device may feed in carefully crafted min/max/res values,
so that the inner loop in parse_uac2_sample_rate_range() could run for
a long time or even never terminate, e.g., given max = INT_MAX.

Also nr_rates could be a large integer, which causes an integer overflow
in the subsequent call to kmalloc() in parse_audio_format_rates_v2().
Thus, kmalloc() would allocate a smaller buffer than expected, leading
to a memory corruption.

To exploit the two vulnerabilities, an attacker needs physical access
to the machine to plug in a malicious USB device.

This patch makes two changes.

1) The type of "rate" is changed to unsigned int, so that the loop could
   stop once "rate" is larger than INT_MAX.

2) Limit nr_rates to 1024.

Suggested-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Xi Wang <xi.wang@gmail.com>
---
 sound/usb/format.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/sound/usb/format.c b/sound/usb/format.c
index 89421d1..e09aba1 100644
--- a/sound/usb/format.c
+++ b/sound/usb/format.c
@@ -209,6 +209,8 @@ static int parse_audio_format_rates_v1(struct snd_usb_audio *chip, struct audiof
 	return 0;
 }
 
+#define MAX_UAC2_NR_RATES 1024
+
 /*
  * Helper function to walk the array of sample rate triplets reported by
  * the device. The problem is that we need to parse whole array first to
@@ -226,7 +228,7 @@ static int parse_uac2_sample_rate_range(struct audioformat *fp, int nr_triplets,
 		int min = combine_quad(&data[2 + 12 * i]);
 		int max = combine_quad(&data[6 + 12 * i]);
 		int res = combine_quad(&data[10 + 12 * i]);
-		int rate;
+		unsigned int rate;
 
 		if ((max < 0) || (min < 0) || (res < 0) || (max < min))
 			continue;
@@ -253,6 +255,10 @@ static int parse_uac2_sample_rate_range(struct audioformat *fp, int nr_triplets,
 			fp->rates |= snd_pcm_rate_to_rate_bit(rate);
 
 			nr_rates++;
+			if (nr_rates >= MAX_UAC2_NR_RATES) {
+				snd_printk(KERN_ERR "invalid uac2 rates\n");
+				break;
+			}
 
 			/* avoid endless loop */
 			if (res == 0)
-- 
1.7.5.4

Re: [PATCH v2] ALSA: usb-audio: fix possible hang and overflow in parse_uac2_sample_rate_range()

[Takashi Iwai]

At Sun, 08 Jan 2012 09:02:52 -0500,
Xi Wang wrote:
> 
> A malicious USB device may feed in carefully crafted min/max/res values,
> so that the inner loop in parse_uac2_sample_rate_range() could run for
> a long time or even never terminate, e.g., given max = INT_MAX.
> 
> Also nr_rates could be a large integer, which causes an integer overflow
> in the subsequent call to kmalloc() in parse_audio_format_rates_v2().
> Thus, kmalloc() would allocate a smaller buffer than expected, leading
> to a memory corruption.
> 
> To exploit the two vulnerabilities, an attacker needs physical access
> to the machine to plug in a malicious USB device.
> 
> This patch makes two changes.
> 
> 1) The type of "rate" is changed to unsigned int, so that the loop could
>    stop once "rate" is larger than INT_MAX.
> 
> 2) Limit nr_rates to 1024.
> 
> Suggested-by: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Xi Wang <xi.wang@gmail.com>

Thanks, applied now.


Takashi

> ---
>  sound/usb/format.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/sound/usb/format.c b/sound/usb/format.c
> index 89421d1..e09aba1 100644
> --- a/sound/usb/format.c
> +++ b/sound/usb/format.c
> @@ -209,6 +209,8 @@ static int parse_audio_format_rates_v1(struct snd_usb_audio *chip, struct audiof
>  	return 0;
>  }
>  
> +#define MAX_UAC2_NR_RATES 1024
> +
>  /*
>   * Helper function to walk the array of sample rate triplets reported by
>   * the device. The problem is that we need to parse whole array first to
> @@ -226,7 +228,7 @@ static int parse_uac2_sample_rate_range(struct audioformat *fp, int nr_triplets,
>  		int min = combine_quad(&data[2 + 12 * i]);
>  		int max = combine_quad(&data[6 + 12 * i]);
>  		int res = combine_quad(&data[10 + 12 * i]);
> -		int rate;
> +		unsigned int rate;
>  
>  		if ((max < 0) || (min < 0) || (res < 0) || (max < min))
>  			continue;
> @@ -253,6 +255,10 @@ static int parse_uac2_sample_rate_range(struct audioformat *fp, int nr_triplets,
>  			fp->rates |= snd_pcm_rate_to_rate_bit(rate);
>  
>  			nr_rates++;
> +			if (nr_rates >= MAX_UAC2_NR_RATES) {
> +				snd_printk(KERN_ERR "invalid uac2 rates\n");
> +				break;
> +			}
>  
>  			/* avoid endless loop */
>  			if (res == 0)
> -- 
> 1.7.5.4
> 
> 

Re: [PATCH v2] ALSA: usb-audio: fix possible hang and overflow in parse_uac2_sample_rate_range()

[Takashi Iwai]

At Sun, 08 Jan 2012 09:02:52 -0500,
Xi Wang wrote:
> 
> A malicious USB device may feed in carefully crafted min/max/res values,
> so that the inner loop in parse_uac2_sample_rate_range() could run for
> a long time or even never terminate, e.g., given max = INT_MAX.
> 
> Also nr_rates could be a large integer, which causes an integer overflow
> in the subsequent call to kmalloc() in parse_audio_format_rates_v2().
> Thus, kmalloc() would allocate a smaller buffer than expected, leading
> to a memory corruption.
> 
> To exploit the two vulnerabilities, an attacker needs physical access
> to the machine to plug in a malicious USB device.
> 
> This patch makes two changes.
> 
> 1) The type of "rate" is changed to unsigned int, so that the loop could
>    stop once "rate" is larger than INT_MAX.
> 
> 2) Limit nr_rates to 1024.
> 
> Suggested-by: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Xi Wang <xi.wang@gmail.com>

Thanks, applied now.


Takashi

> ---
>  sound/usb/format.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/sound/usb/format.c b/sound/usb/format.c
> index 89421d1..e09aba1 100644
> --- a/sound/usb/format.c
> +++ b/sound/usb/format.c
> @@ -209,6 +209,8 @@ static int parse_audio_format_rates_v1(struct snd_usb_audio *chip, struct audiof
>  	return 0;
>  }
>  
> +#define MAX_UAC2_NR_RATES 1024
> +
>  /*
>   * Helper function to walk the array of sample rate triplets reported by
>   * the device. The problem is that we need to parse whole array first to
> @@ -226,7 +228,7 @@ static int parse_uac2_sample_rate_range(struct audioformat *fp, int nr_triplets,
>  		int min = combine_quad(&data[2 + 12 * i]);
>  		int max = combine_quad(&data[6 + 12 * i]);
>  		int res = combine_quad(&data[10 + 12 * i]);
> -		int rate;
> +		unsigned int rate;
>  
>  		if ((max < 0) || (min < 0) || (res < 0) || (max < min))
>  			continue;
> @@ -253,6 +255,10 @@ static int parse_uac2_sample_rate_range(struct audioformat *fp, int nr_triplets,
>  			fp->rates |= snd_pcm_rate_to_rate_bit(rate);
>  
>  			nr_rates++;
> +			if (nr_rates >= MAX_UAC2_NR_RATES) {
> +				snd_printk(KERN_ERR "invalid uac2 rates\n");
> +				break;
> +			}
>  
>  			/* avoid endless loop */
>  			if (res == 0)
> -- 
> 1.7.5.4
> 
>