LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] Fix an OOB bug in parse_audio_mixer_unit
@ 2019-08-14  2:36 Hui Peng
  2019-08-14  6:36 ` Takashi Iwai
  0 siblings, 1 reply; 6+ messages in thread
From: Hui Peng @ 2019-08-14  2:36 UTC (permalink / raw)
  To: security
  Cc: Hui Peng, Mathias Payer, Jaroslav Kysela, Takashi Iwai,
	Thomas Gleixner, Allison Randal, YueHaibing, Wenwen Wang,
	alsa-devel, linux-kernel

The `uac_mixer_unit_descriptor` shown as below is read from the
device side. In `parse_audio_mixer_unit`, `baSourceID` field is
accessed from index 0 to `bNrInPins` - 1, the current implementation
assumes that descriptor is always valid (the length  of descriptor
is no shorter than 5 + `bNrInPins`). If a descriptor read from
the device side is invalid, it may trigger out-of-bound memory
access.

```
struct uac_mixer_unit_descriptor {
	__u8 bLength;
	__u8 bDescriptorType;
	__u8 bDescriptorSubtype;
	__u8 bUnitID;
	__u8 bNrInPins;
	__u8 baSourceID[];
}
```

This patch fixes the bug by add a sanity check on the length of
the descriptor.

Signed-off-by: Hui Peng <benquike@gmail.com>
Reported-by: Hui Peng <benquike@gmail.com>
Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
---
 sound/usb/mixer.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 7498b5191b68..38202ce67237 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -2091,6 +2091,15 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
 	struct usb_audio_term iterm;
 	int input_pins, num_ins, num_outs;
 	int pin, ich, err;
+	int desc_len = (int) ((unsigned long) state->buffer +
+			state->buflen - (unsigned long) raw_desc);
+
+	if (desc_len < sizeof(*desc) + desc->bNrInPins) {
+		usb_audio_err(state->chip,
+			      "descriptor %d too short\n",
+			      unitid);
+		return -EINVAL;
+	}
 
 	err = uac_mixer_unit_get_channels(state, desc);
 	if (err < 0) {
-- 
2.22.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Fix an OOB bug in parse_audio_mixer_unit
  2019-08-14  2:36 [PATCH] Fix an OOB bug in parse_audio_mixer_unit Hui Peng
@ 2019-08-14  6:36 ` Takashi Iwai
  2019-08-14  9:09   ` Dan Carpenter
       [not found]   ` <CAKpmkkUv=arsdJiexaM-UVhXEwfGN=zreny9P_kDNhQUij8=FQ@mail.gmail.com>
  0 siblings, 2 replies; 6+ messages in thread
From: Takashi Iwai @ 2019-08-14  6:36 UTC (permalink / raw)
  To: Hui Peng
  Cc: security, alsa-devel, YueHaibing, Thomas Gleixner,
	Allison Randal, Mathias Payer, Jaroslav Kysela, Takashi Iwai,
	Wenwen Wang, linux-kernel

On Wed, 14 Aug 2019 04:36:24 +0200,
Hui Peng wrote:
> 
> The `uac_mixer_unit_descriptor` shown as below is read from the
> device side. In `parse_audio_mixer_unit`, `baSourceID` field is
> accessed from index 0 to `bNrInPins` - 1, the current implementation
> assumes that descriptor is always valid (the length  of descriptor
> is no shorter than 5 + `bNrInPins`). If a descriptor read from
> the device side is invalid, it may trigger out-of-bound memory
> access.
> 
> ```
> struct uac_mixer_unit_descriptor {
> 	__u8 bLength;
> 	__u8 bDescriptorType;
> 	__u8 bDescriptorSubtype;
> 	__u8 bUnitID;
> 	__u8 bNrInPins;
> 	__u8 baSourceID[];
> }
> ```
> 
> This patch fixes the bug by add a sanity check on the length of
> the descriptor.
> 
> Signed-off-by: Hui Peng <benquike@gmail.com>
> Reported-by: Hui Peng <benquike@gmail.com>
> Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
> ---
>  sound/usb/mixer.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index 7498b5191b68..38202ce67237 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -2091,6 +2091,15 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
>  	struct usb_audio_term iterm;
>  	int input_pins, num_ins, num_outs;
>  	int pin, ich, err;
> +	int desc_len = (int) ((unsigned long) state->buffer +
> +			state->buflen - (unsigned long) raw_desc);
> +
> +	if (desc_len < sizeof(*desc) + desc->bNrInPins) {
> +		usb_audio_err(state->chip,
> +			      "descriptor %d too short\n",
> +			      unitid);
> +		return -EINVAL;
> +	}
>  
>  	err = uac_mixer_unit_get_channels(state, desc);
>  	if (err < 0) {

Hm, what is the desc->bLength value in the error case?

Basically the buffer boundary is already checked against bLength in
snd_usb_find_desc() which is called from obtaining the raw_desc in the
caller of this function (parse_audio_unit()).

So, if any, we need to check bLength for the possible overflow like
below.


thanks,

Takashi

--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -744,6 +744,8 @@ static int uac_mixer_unit_get_channels(struct mixer_build *state,
 		return -EINVAL;
 	if (!desc->bNrInPins)
 		return -EINVAL;
+	if (desc->bLength < sizeof(*desc) + desc->bNrInPins)
+		return -EINVAL;
 
 	switch (state->mixer->protocol) {
 	case UAC_VERSION_1:

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Fix an OOB bug in parse_audio_mixer_unit
  2019-08-14  6:36 ` Takashi Iwai
@ 2019-08-14  9:09   ` Dan Carpenter
  2019-08-14 15:14     ` Takashi Iwai
       [not found]   ` <CAKpmkkUv=arsdJiexaM-UVhXEwfGN=zreny9P_kDNhQUij8=FQ@mail.gmail.com>
  1 sibling, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2019-08-14  9:09 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: Hui Peng, security, alsa-devel, YueHaibing, Thomas Gleixner,
	Allison Randal, Mathias Payer, Jaroslav Kysela, Takashi Iwai,
	Wenwen Wang, linux-kernel

On Wed, Aug 14, 2019 at 08:36:42AM +0200, Takashi Iwai wrote:
> On Wed, 14 Aug 2019 04:36:24 +0200,
> Hui Peng wrote:
> > 
> > The `uac_mixer_unit_descriptor` shown as below is read from the
> > device side. In `parse_audio_mixer_unit`, `baSourceID` field is
> > accessed from index 0 to `bNrInPins` - 1, the current implementation
> > assumes that descriptor is always valid (the length  of descriptor
> > is no shorter than 5 + `bNrInPins`). If a descriptor read from
> > the device side is invalid, it may trigger out-of-bound memory
> > access.
> > 
> > ```
> > struct uac_mixer_unit_descriptor {
> > 	__u8 bLength;
> > 	__u8 bDescriptorType;
> > 	__u8 bDescriptorSubtype;
> > 	__u8 bUnitID;
> > 	__u8 bNrInPins;
> > 	__u8 baSourceID[];
> > }
> > ```
> > 
> > This patch fixes the bug by add a sanity check on the length of
> > the descriptor.
> > 
> > Signed-off-by: Hui Peng <benquike@gmail.com>
> > Reported-by: Hui Peng <benquike@gmail.com>
> > Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
> > ---
> >  sound/usb/mixer.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> > index 7498b5191b68..38202ce67237 100644
> > --- a/sound/usb/mixer.c
> > +++ b/sound/usb/mixer.c
> > @@ -2091,6 +2091,15 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
> >  	struct usb_audio_term iterm;
> >  	int input_pins, num_ins, num_outs;
> >  	int pin, ich, err;
> > +	int desc_len = (int) ((unsigned long) state->buffer +
> > +			state->buflen - (unsigned long) raw_desc);
> > +
> > +	if (desc_len < sizeof(*desc) + desc->bNrInPins) {
> > +		usb_audio_err(state->chip,
> > +			      "descriptor %d too short\n",
> > +			      unitid);
> > +		return -EINVAL;
> > +	}
> >  
> >  	err = uac_mixer_unit_get_channels(state, desc);
> >  	if (err < 0) {
> 
> Hm, what is the desc->bLength value in the error case?
> 
> Basically the buffer boundary is already checked against bLength in
> snd_usb_find_desc() which is called from obtaining the raw_desc in the
> caller of this function (parse_audio_unit()).
> 
> So, if any, we need to check bLength for the possible overflow like
> below.
> 
> 
> thanks,
> 
> Takashi
> 
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -744,6 +744,8 @@ static int uac_mixer_unit_get_channels(struct mixer_build *state,
>  		return -EINVAL;
>  	if (!desc->bNrInPins)
>  		return -EINVAL;
> +	if (desc->bLength < sizeof(*desc) + desc->bNrInPins)
> +		return -EINVAL;

VERSION 1 and 2 already have a different check:

	if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 1)
		return 0; /* no bmControls -> skip */

So something is possibly off by one.  It's just version 3 which doesn't
have a check.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Fix an OOB bug in parse_audio_mixer_unit
  2019-08-14  9:09   ` Dan Carpenter
@ 2019-08-14 15:14     ` Takashi Iwai
  0 siblings, 0 replies; 6+ messages in thread
From: Takashi Iwai @ 2019-08-14 15:14 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Hui Peng, security, alsa-devel, YueHaibing, Thomas Gleixner,
	Allison Randal, Mathias Payer, Jaroslav Kysela, Takashi Iwai,
	Wenwen Wang, linux-kernel

On Wed, 14 Aug 2019 11:09:21 +0200,
Dan Carpenter wrote:
> 
> On Wed, Aug 14, 2019 at 08:36:42AM +0200, Takashi Iwai wrote:
> > On Wed, 14 Aug 2019 04:36:24 +0200,
> > Hui Peng wrote:
> > > 
> > > The `uac_mixer_unit_descriptor` shown as below is read from the
> > > device side. In `parse_audio_mixer_unit`, `baSourceID` field is
> > > accessed from index 0 to `bNrInPins` - 1, the current implementation
> > > assumes that descriptor is always valid (the length  of descriptor
> > > is no shorter than 5 + `bNrInPins`). If a descriptor read from
> > > the device side is invalid, it may trigger out-of-bound memory
> > > access.
> > > 
> > > ```
> > > struct uac_mixer_unit_descriptor {
> > > 	__u8 bLength;
> > > 	__u8 bDescriptorType;
> > > 	__u8 bDescriptorSubtype;
> > > 	__u8 bUnitID;
> > > 	__u8 bNrInPins;
> > > 	__u8 baSourceID[];
> > > }
> > > ```
> > > 
> > > This patch fixes the bug by add a sanity check on the length of
> > > the descriptor.
> > > 
> > > Signed-off-by: Hui Peng <benquike@gmail.com>
> > > Reported-by: Hui Peng <benquike@gmail.com>
> > > Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
> > > ---
> > >  sound/usb/mixer.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> > > index 7498b5191b68..38202ce67237 100644
> > > --- a/sound/usb/mixer.c
> > > +++ b/sound/usb/mixer.c
> > > @@ -2091,6 +2091,15 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
> > >  	struct usb_audio_term iterm;
> > >  	int input_pins, num_ins, num_outs;
> > >  	int pin, ich, err;
> > > +	int desc_len = (int) ((unsigned long) state->buffer +
> > > +			state->buflen - (unsigned long) raw_desc);
> > > +
> > > +	if (desc_len < sizeof(*desc) + desc->bNrInPins) {
> > > +		usb_audio_err(state->chip,
> > > +			      "descriptor %d too short\n",
> > > +			      unitid);
> > > +		return -EINVAL;
> > > +	}
> > >  
> > >  	err = uac_mixer_unit_get_channels(state, desc);
> > >  	if (err < 0) {
> > 
> > Hm, what is the desc->bLength value in the error case?
> > 
> > Basically the buffer boundary is already checked against bLength in
> > snd_usb_find_desc() which is called from obtaining the raw_desc in the
> > caller of this function (parse_audio_unit()).
> > 
> > So, if any, we need to check bLength for the possible overflow like
> > below.
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > --- a/sound/usb/mixer.c
> > +++ b/sound/usb/mixer.c
> > @@ -744,6 +744,8 @@ static int uac_mixer_unit_get_channels(struct mixer_build *state,
> >  		return -EINVAL;
> >  	if (!desc->bNrInPins)
> >  		return -EINVAL;
> > +	if (desc->bLength < sizeof(*desc) + desc->bNrInPins)
> > +		return -EINVAL;
> 
> VERSION 1 and 2 already have a different check:
> 
> 	if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 1)
> 		return 0; /* no bmControls -> skip */
>
> So something is possibly off by one.  It's just version 3 which doesn't
> have a check.
> 

No, both are sensible checks.  The first check is about the minimal
size that doesn't contain bmControls bitmap which is optional on some
devices, while the latter checks about the presence of bmControls
field.  Note that the latter returns zero, which means no error, while
the former returns an error.


thanks,

Takashi

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Fix an OOB bug in parse_audio_mixer_unit
       [not found]   ` <CAKpmkkUv=arsdJiexaM-UVhXEwfGN=zreny9P_kDNhQUij8=FQ@mail.gmail.com>
@ 2019-08-14 16:33     ` Takashi Iwai
       [not found]       ` <CAKpmkkVzT5H0RTAu_Fa=9_gjf5v7k3qzPnnJvPpBp3BaP7G0ag@mail.gmail.com>
  0 siblings, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2019-08-14 16:33 UTC (permalink / raw)
  To: 彭辉
  Cc: security, alsa-devel, YueHaibing, Thomas Gleixner,
	Allison Randal, Mathias Payer, Jaroslav Kysela, Takashi Iwai,
	Wenwen Wang, linux-kernel

On Wed, 14 Aug 2019 18:28:39 +0200,
彭辉 wrote:
> 
> Hi, Takashi:
> Here the problem is that `desc->bLength` is controlled by the device side,
> so  `desc->bLength` may not represent the real length of the descriptor.
> That is why I use pointer arithmetic operations to derive the real size of the
> buffer
> in my patch.

But bLength is checked before calling this, i.e. it's already assured
that bLength fits within the buffer limit.  So, the result calls don't
have to care about the buffer limit itself, and they can just
concentrate on overflow over bLength.


thanks,

Takashi

> 
> On Wed, Aug 14, 2019 at 2:36 AM Takashi Iwai <tiwai@suse.de> wrote:
> 
>     On Wed, 14 Aug 2019 04:36:24 +0200,
>     Hui Peng wrote:
>     >
>     > The `uac_mixer_unit_descriptor` shown as below is read from the
>     > device side. In `parse_audio_mixer_unit`, `baSourceID` field is
>     > accessed from index 0 to `bNrInPins` - 1, the current implementation
>     > assumes that descriptor is always valid (the length  of descriptor
>     > is no shorter than 5 + `bNrInPins`). If a descriptor read from
>     > the device side is invalid, it may trigger out-of-bound memory
>     > access.
>     >
>     > ```
>     > struct uac_mixer_unit_descriptor {
>     >       __u8 bLength;
>     >       __u8 bDescriptorType;
>     >       __u8 bDescriptorSubtype;
>     >       __u8 bUnitID;
>     >       __u8 bNrInPins;
>     >       __u8 baSourceID[];
>     > }
>     > ```
>     >
>     > This patch fixes the bug by add a sanity check on the length of
>     > the descriptor.
>     >
>     > Signed-off-by: Hui Peng <benquike@gmail.com>
>     > Reported-by: Hui Peng <benquike@gmail.com>
>     > Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
>     > ---
>     >  sound/usb/mixer.c | 9 +++++++++
>     >  1 file changed, 9 insertions(+)
>     >
>     > diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
>     > index 7498b5191b68..38202ce67237 100644
>     > --- a/sound/usb/mixer.c
>     > +++ b/sound/usb/mixer.c
>     > @@ -2091,6 +2091,15 @@ static int parse_audio_mixer_unit(struct
>     mixer_build *state, int unitid,
>     >       struct usb_audio_term iterm;
>     >       int input_pins, num_ins, num_outs;
>     >       int pin, ich, err;
>     > +     int desc_len = (int) ((unsigned long) state->buffer +
>     > +                     state->buflen - (unsigned long) raw_desc);
>     > +
>     > +     if (desc_len < sizeof(*desc) + desc->bNrInPins) {
>     > +             usb_audio_err(state->chip,
>     > +                           "descriptor %d too short\n",
>     > +                           unitid);
>     > +             return -EINVAL;
>     > +     }
>     > 
>     >       err = uac_mixer_unit_get_channels(state, desc);
>     >       if (err < 0) {
>    
>     Hm, what is the desc->bLength value in the error case?
>    
>     Basically the buffer boundary is already checked against bLength in
>     snd_usb_find_desc() which is called from obtaining the raw_desc in the
>     caller of this function (parse_audio_unit()).
>    
>     So, if any, we need to check bLength for the possible overflow like
>     below.
> 
>     thanks,
>    
>     Takashi
>    
>     --- a/sound/usb/mixer.c
>     +++ b/sound/usb/mixer.c
>     @@ -744,6 +744,8 @@ static int uac_mixer_unit_get_channels(struct
>     mixer_build *state,
>                     return -EINVAL;
>             if (!desc->bNrInPins)
>                     return -EINVAL;
>     +       if (desc->bLength < sizeof(*desc) + desc->bNrInPins)
>     +               return -EINVAL;
>    
>             switch (state->mixer->protocol) {
>             case UAC_VERSION_1:
> 
> --
> May the Lord Richly Bless you and yours !
> 
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Fix an OOB bug in parse_audio_mixer_unit
       [not found]       ` <CAKpmkkVzT5H0RTAu_Fa=9_gjf5v7k3qzPnnJvPpBp3BaP7G0ag@mail.gmail.com>
@ 2019-08-14 18:21         ` Takashi Iwai
  0 siblings, 0 replies; 6+ messages in thread
From: Takashi Iwai @ 2019-08-14 18:21 UTC (permalink / raw)
  To: 彭辉
  Cc: security, alsa-devel, YueHaibing, Thomas Gleixner,
	Allison Randal, Mathias Payer, Jaroslav Kysela, Wenwen Wang,
	linux-kernel

On Wed, 14 Aug 2019 18:52:07 +0200,
彭辉 wrote:
> 
> Hi, Takashi:
> 
> Thanks for the guide.
> The new patch is confirmed and attached.

Thanks, applied now.


Takashi

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-14  2:36 [PATCH] Fix an OOB bug in parse_audio_mixer_unit Hui Peng
2019-08-14  6:36 ` Takashi Iwai
2019-08-14  9:09   ` Dan Carpenter
2019-08-14 15:14     ` Takashi Iwai
     [not found]   ` <CAKpmkkUv=arsdJiexaM-UVhXEwfGN=zreny9P_kDNhQUij8=FQ@mail.gmail.com>
2019-08-14 16:33     ` Takashi Iwai
     [not found]       ` <CAKpmkkVzT5H0RTAu_Fa=9_gjf5v7k3qzPnnJvPpBp3BaP7G0ag@mail.gmail.com>
2019-08-14 18:21         ` Takashi Iwai

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox