[alsa-devel] [PATCH] ALSA: usb-audio: Check mixer unit bitmap yet more strictly

[Amadeusz Sławiński]

On Thu, 22 Aug 2019 10:35:01 +0200
Takashi Iwai <tiwai at suse.de> wrote:

> On Thu, 22 Aug 2019 10:31:48 +0200,
> Amadeusz Sławiński wrote:
> > 
> > On Thu, 22 Aug 2019 09:32:03 +0200
> > Takashi Iwai <tiwai at suse.de> wrote:
> >   
> > > The bmControls (for UAC1) or bmMixerControls (for UAC2/3) bitmap has a
> > > variable size depending on both input and output pins.  Its size is to
> > > fit with input * output bits.  The problem is that the input size
> > > can't be determined simply from the unit descriptor itself but it
> > > needs to parse the whole connected sources.  Although the
> > > uac_mixer_unit_get_channels() tries to check some possible overflow of
> > > this bitmap, it's incomplete due to the lack of the  evaluation of
> > > input pins.
> > > 
> > > For covering possible overflows, this patch adds the bitmap overflow
> > > check in the loop of input pins in parse_audio_mixer_unit().
> > > 
> > > Fixes: 0bfe5e434e66 ("ALSA: usb-audio: Check mixer unit descriptors
> > > more strictly") Cc: <stable at vger.kernel.org>
> > > Signed-off-by: Takashi Iwai <tiwai at suse.de>
> > > ---
> > >  sound/usb/mixer.c | 34 ++++++++++++++++++++++++++--------
> > >  1 file changed, 26 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> > > index b5927c3d5bc0..27ee68a507a2 100644
> > > --- a/sound/usb/mixer.c
> > > +++ b/sound/usb/mixer.c
> > > @@ -739,7 +739,6 @@ 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;
> > > @@ -762,13 +761,6 @@ static int uac_mixer_unit_get_channels(struct
> > > mixer_build *state, break;
> > >  	}
> > >  
> > > -	if (!mu_channels)
> > > -		return 0;
> > > -
> > > -	c = uac_mixer_unit_bmControls(desc, state->mixer->protocol);
> > > -	if (c - (void *)desc + (mu_channels - 1) / 8 >=
> > > desc->bLength)
> > > -		return 0; /* no bmControls -> skip */
> > > -
> > >  	return mu_channels;
> > >  }
> > >  
> > > @@ -2009,6 +2001,29 @@ static int parse_audio_feature_unit(struct
> > > mixer_build *state, int unitid,
> > >   * Mixer Unit
> > >   */
> > >  
> > > +/* check whether the given in/out overflows bmMixerControls matrix */
> > > +static bool mixer_bitmap_overflow(struct uac_mixer_unit_descriptor
> > > *desc,
> > > +				  int protocol, int num_ins, int
> > > num_outs) +{
> > > +	u8 *hdr = (u8 *)desc;
> > > +	u8 *c = uac_mixer_unit_bmControls(desc, protocol);
> > > +	size_t rest; /* remaining bytes after bmMixerControls */
> > > +
> > > +	switch (protocol) {
> > > +	case UAC_VERSION_1:
> > > +	default:  
> > 
> > Won't this trigger implicit fall through warning?  
> 
> Ah indeed, I seem to have built with a little bit older kernel.
> Thanks for catching it.
> 
> The revised patch is below.
> 

Oh... didn't even notice the missing breaks ;)
I was asking about:
> +	case UAC_VERSION_1:
> +	default:
Unless default is handled specially it will probably warn on
UAC_VERSION_1 line.

> 
> Takashi
> 
> -- 8< --
> From: Takashi Iwai <tiwai at suse.de>
> Subject: [PATCH v2] ALSA: usb-audio: Check mixer unit bitmap yet more strictly
> 
> The bmControls (for UAC1) or bmMixerControls (for UAC2/3) bitmap has a
> variable size depending on both input and output pins.  Its size is to
> fit with input * output bits.  The problem is that the input size
> can't be determined simply from the unit descriptor itself but it
> needs to parse the whole connected sources.  Although the
> uac_mixer_unit_get_channels() tries to check some possible overflow of
> this bitmap, it's incomplete due to the lack of the  evaluation of
> input pins.
> 
> For covering possible overflows, this patch adds the bitmap overflow
> check in the loop of input pins in parse_audio_mixer_unit().
> 
> Fixes: 0bfe5e434e66 ("ALSA: usb-audio: Check mixer unit descriptors more strictly")
> Cc: <stable at vger.kernel.org>
> Signed-off-by: Takashi Iwai <tiwai at suse.de>
> ---
> V1->v2: Fix the missing break in mixer_bitmap_overflow().
> 
>  sound/usb/mixer.c | 36 ++++++++++++++++++++++++++++--------
>  1 file changed, 28 insertions(+), 8 deletions(-)
> 
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index b5927c3d5bc0..eceab19766db 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -739,7 +739,6 @@ 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;
> @@ -762,13 +761,6 @@ static int uac_mixer_unit_get_channels(struct mixer_build *state,
>  		break;
>  	}
>  
> -	if (!mu_channels)
> -		return 0;
> -
> -	c = uac_mixer_unit_bmControls(desc, state->mixer->protocol);
> -	if (c - (void *)desc + (mu_channels - 1) / 8 >= desc->bLength)
> -		return 0; /* no bmControls -> skip */
> -
>  	return mu_channels;
>  }
>  
> @@ -2009,6 +2001,31 @@ static int parse_audio_feature_unit(struct mixer_build *state, int unitid,
>   * Mixer Unit
>   */
>  
> +/* check whether the given in/out overflows bmMixerControls matrix */
> +static bool mixer_bitmap_overflow(struct uac_mixer_unit_descriptor *desc,
> +				  int protocol, int num_ins, int num_outs)
> +{
> +	u8 *hdr = (u8 *)desc;
> +	u8 *c = uac_mixer_unit_bmControls(desc, protocol);
> +	size_t rest; /* remaining bytes after bmMixerControls */
> +
> +	switch (protocol) {
> +	case UAC_VERSION_1:
> +	default:
> +		rest = 1; /* iMixer */
> +		break;
> +	case UAC_VERSION_2:
> +		rest = 2; /* bmControls + iMixer */
> +		break;
> +	case UAC_VERSION_3:
> +		rest = 6; /* bmControls + wMixerDescrStr */
> +		break;
> +	}
> +
> +	/* overflow? */
> +	return c + (num_ins * num_outs + 7) / 8 + rest > hdr + hdr[0];
> +}
> +
>  /*
>   * build a mixer unit control
>   *
> @@ -2137,6 +2154,9 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
>  		if (err < 0)
>  			return err;
>  		num_ins += iterm.channels;
> +		if (mixer_bitmap_overflow(desc, state->mixer->protocol,
> +					  num_ins, num_outs))
> +			break;
>  		for (; ich < num_ins; ich++) {
>  			int och, ich_has_controls = 0;
>