Re: [PATCH] Fix an OOB bug in uac_mixer_unit_bmControls

From: Takashi Iwai
Date: Sat Aug 17 2019 - 13:59:52 EST

Next message: Oleksij Rempel: "Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation"
Previous message: pr-tracker-bot: "Re: [PULL REQUEST] i2c for 5.3"
In reply to: Takashi Iwai: "Re: [PATCH] Fix an OOB bug in uac_mixer_unit_bmControls"
Next in thread: Hui Peng: "[PATCH] Fix an OOB bug in uac_mixer_unit_bmControls"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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
> >Â Â Â>
> >Â Â Â>
> >
> >
>
>

Next message: Oleksij Rempel: "Re: [PATCH v2 4/6] dt: bindings: add mt7621-pll dt binding documentation"
Previous message: pr-tracker-bot: "Re: [PULL REQUEST] i2c for 5.3"
In reply to: Takashi Iwai: "Re: [PATCH] Fix an OOB bug in uac_mixer_unit_bmControls"
Next in thread: Hui Peng: "[PATCH] Fix an OOB bug in uac_mixer_unit_bmControls"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]