From: Takashi Iwai <tiwai@suse.de> To: Xi Wang <xi.wang@gmail.com> Cc: Jaroslav Kysela <perex@perex.cz>, Clemens Ladisch <clemens@ladisch.de>, Daniel Mack <zonque@gmail.com>, Wolfgang Breyha <wbreyha@gmx.net>, alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] ALSA: usb-audio: fix possible hang and overflow in parse_uac2_sample_rate_range() Date: Sun, 08 Jan 2012 10:09:11 +0100 [thread overview] Message-ID: <s5hehvamuh4.wl%tiwai@suse.de> (raw) In-Reply-To: <1325698749-5353-1-git-send-email-xi.wang@gmail.com> 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 >
WARNING: multiple messages have this Message-ID (diff)
From: Takashi Iwai <tiwai@suse.de> To: Xi Wang <xi.wang@gmail.com> Cc: alsa-devel@alsa-project.org, Clemens Ladisch <clemens@ladisch.de>, linux-kernel@vger.kernel.org, Daniel Mack <zonque@gmail.com>, Wolfgang Breyha <wbreyha@gmx.net> Subject: Re: [PATCH] ALSA: usb-audio: fix possible hang and overflow in parse_uac2_sample_rate_range() Date: Sun, 08 Jan 2012 10:09:11 +0100 [thread overview] Message-ID: <s5hehvamuh4.wl%tiwai@suse.de> (raw) In-Reply-To: <1325698749-5353-1-git-send-email-xi.wang@gmail.com> 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 >
next prev parent reply other threads:[~2012-01-08 9:09 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2012-01-04 17:39 [PATCH] ALSA: usb-audio: fix possible hang and overflow in parse_uac2_sample_rate_range() Xi Wang 2012-01-08 9:09 ` Takashi Iwai [this message] 2012-01-08 9:09 ` Takashi Iwai 2012-01-08 12:45 ` Xi Wang 2012-01-08 12:45 ` Xi Wang 2012-01-08 13:20 ` Takashi Iwai 2012-01-08 13:20 ` Takashi Iwai 2012-01-08 13:55 ` Xi Wang 2012-01-08 13:55 ` Xi Wang 2012-01-08 14:02 ` [PATCH v2] " Xi Wang 2012-01-08 15:04 ` Takashi Iwai 2012-01-08 15:04 ` Takashi Iwai
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=s5hehvamuh4.wl%tiwai@suse.de \ --to=tiwai@suse.de \ --cc=alsa-devel@alsa-project.org \ --cc=clemens@ladisch.de \ --cc=linux-kernel@vger.kernel.org \ --cc=perex@perex.cz \ --cc=wbreyha@gmx.net \ --cc=xi.wang@gmail.com \ --cc=zonque@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.