lkml.org 
[lkml]   [2019]   [Aug]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] Fix an OOB bug in uac_mixer_unit_bmControls
On Sat, 17 Aug 2019 18:47:05 +0200,
Hui Peng wrote:
>
> No, there was not triggering. I found it accidentally when I was going through
> the code.
>
> Yeah, you are right. it is handled in the last check. Is it defined in the
> spec that the descriptor needs to have 4/6/2 additional bytes for the
> bmControl field?, if so, it is easier to understand the using the code in the
> way in my first patch.
>
> If you think this is unnecessary, we can skip this patch.

Well, the patch essentially open-codes what the helper function does
adds one more check unnecessarily, so I don't think it worth.
Let's skip it.


thanks,

Takashi

> On Sat, Aug 17, 2019 at 12:18 PM Takashi Iwai <tiwai@suse.de> wrote:
>
> On Sat, 17 Aug 2019 17:57:38 +0200,
> Hui Peng wrote:
> >
> > 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?
>
> Yes, this isn't for the mixer unit but for the processing unit.
> They have different definitions.
>
> Now back to the original report: I read the code again but fail to see
> where is OOB access.  Let's see the function:
>
> static int uac_mixer_unit_get_channels(struct mixer_build *state,
>                                        struct uac_mixer_unit_descriptor
> *desc)
> {
>         int mu_channels;
>         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:
>         case UAC_VERSION_2:
>         default:
>                 if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 1)
>                         return 0; /* no bmControls -> skip */
>                 mu_channels = uac_mixer_unit_bNrChannels(desc);
>                 break;
>         case UAC_VERSION_3:
>                 mu_channels = get_cluster_channels_v3(state,
>                                 uac3_mixer_unit_wClusterDescrID(desc));
>                 break;
>         }
>
>         if (!mu_channels)
>                 return 0;
>
> ... until this point, mu_channels is calculated but no actual access
> happens.  Then:
>
>         c = uac_mixer_unit_bmControls(desc, state->mixer->protocol);
>
> ... this returns the *address* of the bmControls bitmap.  At this
> point, it's not accessed yet.  Now:
>
>         if (c - (void *)desc + (mu_channels - 1) / 8 >= desc->bLength)
>                 return 0; /* no bmControls -> skip */
>
> ... here we check whether the actual bitmap address plus the max
> bitmap size overflows bLength.  And if it overflows, returns 0,
> indicating no bitmap available.
>
> So the code doesn't access but checks properly beforehand as far as I
> understand.  Is the actual OOB access triggered by some program?
>
> thanks,
>
> Takashi
>
> >
> > static inline __u8 *uac_processing_unit_bmControls(struct
> uac_processing_unit_descriptor *desc,
> >                                                    int protocol)
> > {
> >         switch (protocol) {
> >         case UAC_VERSION_1:
> >                 return &desc->baSourceID[desc->bNrInPins + 5];
> >         case UAC_VERSION_2:
> >                 return &desc->baSourceID[desc->bNrInPins + 6];
> >         case UAC_VERSION_3:
> >                 return &desc->baSourceID[desc->bNrInPins + 2];
> >         default:
> >                 return NULL;
> >         }
> > }
> >
> > On Sat, Aug 17, 2019 at 2:55 AM Takashi Iwai <tiwai@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@gmail.com>
> >     > Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
> >     > Signed-off-by: Hui Peng <benquike@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
> >     >
> >     >
> >
> >
>
>

\
 
 \ /
  Last update: 2019-08-17 20:03    [W:0.050 / U:0.488 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site