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@xxxxxxx> 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@xxxxxxx> 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@xxxxxxxxx>
> > > Reported-by: Mathias Payer <mathias.payer@xxxxxxxxxxxxx>
> > > Signed-off-by: Hui Peng <benquike@xxxxxxxxx>
> >
> > 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
> > >
> > >
> >
> >
>
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
