Re: [PATCH] Fix an OOB bug in parse_audio_mixer_unit

[Takashi Iwai]

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@xxxxxxx> 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@xxxxxxxxx>
>     > Reported-by: Hui Peng <benquike@xxxxxxxxx>
>     > Reported-by: Mathias Payer <mathias.payer@xxxxxxxxxxxxx>
>     > ---
>     >Â 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 !
> 
>