[alsa-devel] [PATCH] Fix an OOB bug in uac_mixer_unit_bmControls

[Hui Peng]

Looking around, there are other suspicious codes. E.g., in the following
function, it seems to be the same as `uac_mixer_unit_bmControls`, but it is
accessing `desc->bNrInPins + 5`, in case of UAC_VERSION_1.
Is this intended?

static inline __u8
<https://elixir.bootlin.com/linux/latest/ident/__u8>
*uac_processing_unit_bmControls
<https://elixir.bootlin.com/linux/latest/ident/uac_processing_unit_bmControls>(struct
uac_processing_unit_descriptor
<https://elixir.bootlin.com/linux/latest/ident/uac_processing_unit_descriptor>
*desc,
						   int protocol <https://elixir.bootlin.com/linux/latest/ident/protocol>){
	switch (protocol <https://elixir.bootlin.com/linux/latest/ident/protocol>) {
	case UAC_VERSION_1
<https://elixir.bootlin.com/linux/latest/ident/UAC_VERSION_1>:
		return &desc->baSourceID[desc->bNrInPins + 5];
	case UAC_VERSION_2
<https://elixir.bootlin.com/linux/latest/ident/UAC_VERSION_2>:
		return &desc->baSourceID[desc->bNrInPins + 6];
	case UAC_VERSION_3
<https://elixir.bootlin.com/linux/latest/ident/UAC_VERSION_3>:
		return &desc->baSourceID[desc->bNrInPins + 2];
	default:
		return NULL;
	}}



On Sat, Aug 17, 2019 at 2:55 AM Takashi Iwai <tiwai at suse.de> wrote:

> On Sat, 17 Aug 2019 06:32:07 +0200,
> Hui Peng wrote:
> >
> > `uac_mixer_unit_get_channels` calls `uac_mixer_unit_bmControls`
> > to get pointer to bmControls field. The current implementation of
> > `uac_mixer_unit_get_channels` does properly check the size of
> > uac_mixer_unit_descriptor descriptor and may allow OOB access
> > in `uac_mixer_unit_bmControls`.
> >
> > Reported-by: Hui Peng <benquike at gmail.com>
> > Reported-by: Mathias Payer <mathias.payer at nebelwelt.net>
> > Signed-off-by: Hui Peng <benquike at gmail.com>
>
> Ah a good catch.
>
> One easier fix in this case would be to get the offset from
> uac_mixer_unit_bmControls(), e.g.
>
>         /* calculate the offset of bmControls field */
>         size_t bmc_offset = uac_mixer_unit_bmControls(NULL, protocol) -
> NULL;
>         ....
>         if (desc->bLength < bmc_offset)
>                 return 0;
>
> thanks,
>
> Takashi
>
>
> > ---
> >  sound/usb/mixer.c | 25 ++++++++++++++++++-------
> >  1 file changed, 18 insertions(+), 7 deletions(-)
> >
> > diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> > index b5927c3d5bc0..00e6274a63c3 100644
> > --- a/sound/usb/mixer.c
> > +++ b/sound/usb/mixer.c
> > @@ -738,28 +738,39 @@ static int get_cluster_channels_v3(struct
> mixer_build *state, unsigned int clust
> >  static int uac_mixer_unit_get_channels(struct mixer_build *state,
> >                                      struct uac_mixer_unit_descriptor
> *desc)
> >  {
> > -     int mu_channels;
> > +     int mu_channels = 0;
> >       void *c;
> >
> > -     if (desc->bLength < sizeof(*desc))
> > -             return -EINVAL;
> >       if (!desc->bNrInPins)
> >               return -EINVAL;
> > -     if (desc->bLength < sizeof(*desc) + desc->bNrInPins)
> > -             return -EINVAL;
> >
> >       switch (state->mixer->protocol) {
> >       case UAC_VERSION_1:
> > +             // limit derived from uac_mixer_unit_bmControls
> > +             if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 4)
> > +                     return 0;
> > +
> > +             mu_channels = uac_mixer_unit_bNrChannels(desc);
> > +             break;
> > +
> >       case UAC_VERSION_2:
> > -     default:
> > -             if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 1)
> > +             // limit derived from uac_mixer_unit_bmControls
> > +             if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 6)
> >                       return 0; /* no bmControls -> skip */
> > +
> >               mu_channels = uac_mixer_unit_bNrChannels(desc);
> >               break;
> >       case UAC_VERSION_3:
> > +             // limit derived from uac_mixer_unit_bmControls
> > +             if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 2)
> > +                     return 0; /* no bmControls -> skip */
> > +
> >               mu_channels = get_cluster_channels_v3(state,
> >                               uac3_mixer_unit_wClusterDescrID(desc));
> >               break;
> > +
> > +     default:
> > +             break;
> >       }
> >
> >       if (!mu_channels)
> > --
> > 2.22.1
> >
> >
>