Re: [alsa-devel] [PATCH 1/4] ALSA: usb-audio: More validations of descriptor units

On Thu, 22 Aug 2019 10:51:18 +0200, Amadeusz Sławiński wrote:

On Thu, 22 Aug 2019 09:32:04 +0200 Takashi Iwai tiwai@suse.de wrote:

...

Introduce a new helper to validate each audio descriptor unit before and check the unit before actually accessing it. This should harden against the OOB access cases with malformed descriptors that have been recently frequently reported by fuzzers.

The existing descriptor checks are still kept although they become superfluous after this patch. They'll be cleaned up eventually later.

Signed-off-by: Takashi Iwai tiwai@suse.de

sound/usb/Makefile | 3 +- sound/usb/helper.h | 4 + sound/usb/mixer.c | 10 ++ sound/usb/power.c | 2 + sound/usb/quirks.c | 3 + sound/usb/stream.c | 25 ++-- sound/usb/validate.c | 332 +++++++++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 366 insertions(+), 13 deletions(-) create mode 100644 sound/usb/validate.c

diff --git a/sound/usb/Makefile b/sound/usb/Makefile index e1ce257ab705..d27a21b0ff9c 100644 --- a/sound/usb/Makefile +++ b/sound/usb/Makefile @@ -16,7 +16,8 @@ snd-usb-audio-objs := card.o \ power.o \ proc.o \ quirks.o \

• 	stream.o
  

• 	stream.o \
  

• 	validate.o
  
  

snd-usb-audio-$(CONFIG_SND_USB_AUDIO_USE_MEDIA_CONTROLLER) += media.o

diff --git a/sound/usb/helper.h b/sound/usb/helper.h index 6afb70156ec4..5e8a18b4e7b9 100644 --- a/sound/usb/helper.h +++ b/sound/usb/helper.h @@ -31,4 +31,8 @@ static inline int snd_usb_ctrl_intf(struct snd_usb_audio *chip) return get_iface_desc(chip->ctrl_intf)->bInterfaceNumber; }

+/* in validate.c */ +bool snd_usb_validate_audio_desc(void *p, int protocol); +bool snd_usb_validate_midi_desc(void *p);

#endif /* __USBAUDIO_HELPER_H */ diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index 27ee68a507a2..b4be226e07f3 100644 --- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -785,6 +785,8 @@ static int __check_input_term(struct mixer_build *state, int id, p1 = find_audio_control_unit(state, id); if (!p1) break;

• if (!snd_usb_validate_audio_desc(p1, protocol))
  

• 	break; /* bad descriptor */
  
  

  hdr = p1; term->id = id;

@@ -2773,6 +2775,11 @@ static int parse_audio_unit(struct mixer_build *state, int unitid) return -EINVAL; }

• if (!snd_usb_validate_audio_desc(p1, protocol)) {

• usb_audio_dbg(state->chip, "invalid unit %d\n", unitid);
  

• return 0; /* skip invalid unit */
  

• }
• if (protocol == UAC_VERSION_1 || protocol == UAC_VERSION_2) { switch (p1[2]) { case UAC_INPUT_TERMINAL:

@@ -3143,6 +3150,9 @@ static int snd_usb_mixer_controls(struct usb_mixer_interface *mixer) while ((p = snd_usb_find_csint_desc(mixer->hostif->extra, mixer->hostif->extralen, p, UAC_OUTPUT_TERMINAL)) != NULL) {

• if (!snd_usb_validate_audio_desc(p, mixer->protocol))
  

• 	continue; /* skip invalid descriptor */
  

• if (mixer->protocol == UAC_VERSION_1) { struct uac1_output_terminal_descriptor *desc = p;

diff --git a/sound/usb/power.c b/sound/usb/power.c index bd303a1ba1b7..606a2cb23eab 100644 --- a/sound/usb/power.c +++ b/sound/usb/power.c @@ -31,6 +31,8 @@ snd_usb_find_power_domain(struct usb_host_interface *ctrl_iface, struct uac3_power_domain_descriptor *pd_desc = p; int i;

• if (!snd_usb_validate_audio_desc(p, UAC_VERSION_3))
  

• 	continue;
  

  for (i = 0; i < pd_desc->bNrEntities; i++) { if (pd_desc->baEntityID[i] == id) { pd->pd_id = pd_desc->bPowerDomainID;

diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c index 78858918cbc1..7e9735aa7ac9 100644 --- a/sound/usb/quirks.c +++ b/sound/usb/quirks.c @@ -248,6 +248,9 @@ static int create_yamaha_midi_quirk(struct snd_usb_audio *chip, NULL, USB_MS_MIDI_OUT_JACK); if (!injd && !outjd) return -ENODEV;

• if (!snd_usb_validate_midi_desc(injd) ||

•    !snd_usb_validate_midi_desc(outjd))
  

• return -ENODEV;
  

  if (injd && (injd->bLength < 5 || (injd->bJackType != USB_MS_EMBEDDED && injd->bJackType != USB_MS_EXTERNAL)))

diff --git a/sound/usb/stream.c b/sound/usb/stream.c index e852c7fd6109..a0649c8ae460 100644 --- a/sound/usb/stream.c +++ b/sound/usb/stream.c @@ -627,16 +627,14 @@ static int parse_uac_endpoint_attributes(struct snd_usb_audio *chip, */ static void * snd_usb_find_input_terminal_descriptor(struct usb_host_interface *ctrl_iface,

• 		       int terminal_id, bool uac23)
  

• 		       int terminal_id, int protocol)
  

{ struct uac2_input_terminal_descriptor *term = NULL;

• size_t minlen = uac23 ? sizeof(struct uac2_input_terminal_descriptor) :

• sizeof(struct uac_input_terminal_descriptor);
  
  

  while ((term = snd_usb_find_csint_desc(ctrl_iface->extra, ctrl_iface->extralen, term, UAC_INPUT_TERMINAL))) {

• if (term->bLength < minlen)
  

• if (!snd_usb_validate_audio_desc(term, protocol))
  continue;
  

  if (term->bTerminalID == terminal_id) return term;

@@ -647,7 +645,7 @@ snd_usb_find_input_terminal_descriptor(struct usb_host_interface *ctrl_iface,

static void * snd_usb_find_output_terminal_descriptor(struct usb_host_interface *ctrl_iface,

• 			int terminal_id)
  

• 			int terminal_id, int protocol)
  

{ /* OK to use with both UAC2 and UAC3 */ struct uac2_output_terminal_descriptor *term = NULL; @@ -655,8 +653,9 @@ snd_usb_find_output_terminal_descriptor(struct usb_host_interface *ctrl_iface, while ((term = snd_usb_find_csint_desc(ctrl_iface->extra, ctrl_iface->extralen, term, UAC_OUTPUT_TERMINAL))) {

• if (term->bLength >= sizeof(*term) &&
  

•     term->bTerminalID == terminal_id)
  

• if (!snd_usb_validate_audio_desc(term, protocol))
  

• 	continue;
  

• if (term->bTerminalID == terminal_id)
  return term;
  

  }

@@ -731,7 +730,7 @@ snd_usb_get_audioformat_uac12(struct snd_usb_audio *chip,

iterm = snd_usb_find_input_terminal_descriptor(chip->ctrl_intf,
					       as->bTerminalLink,

• 					       false);
  

• 					       protocol);
  

  if (iterm) { num_channels = iterm->bNrChannels; chconfig = le16_to_cpu(iterm->wChannelConfig);

@@ -767,7 +766,7 @@ snd_usb_get_audioformat_uac12(struct snd_usb_audio *chip, */ input_term = snd_usb_find_input_terminal_descriptor(chip->ctrl_intf, as->bTerminalLink,

• 						    true);
  

• 						    protocol);
  

  if (input_term) { clock = input_term->bCSourceID; if (!chconfig && (num_channels == input_term->bNrChannels))

@@ -776,7 +775,8 @@ snd_usb_get_audioformat_uac12(struct snd_usb_audio *chip, }

output_term = snd_usb_find_output_terminal_descriptor(chip->ctrl_intf,

• 						      as->bTerminalLink);
  

• 						      as->bTerminalLink,
  

• 						      protocol);
  

  if (output_term) { clock = output_term->bCSourceID; goto found_clock;

@@ -1002,14 +1002,15 @@ snd_usb_get_audioformat_uac3(struct snd_usb_audio *chip, */ input_term = snd_usb_find_input_terminal_descriptor(chip->ctrl_intf, as->bTerminalLink,

• 					    true);
  

• 					    UAC_VERSION_3);
  

  if (input_term) { clock = input_term->bCSourceID; goto found_clock; }

  output_term = snd_usb_find_output_terminal_descriptor(chip->ctrl_intf,

• 					     as->bTerminalLink);
  

• 					      as->bTerminalLink,
  

• 					      UAC_VERSION_3);
  

  if (output_term) { clock = output_term->bCSourceID; goto found_clock;

diff --git a/sound/usb/validate.c b/sound/usb/validate.c new file mode 100644 index 000000000000..3c8f73a0eb12 --- /dev/null +++ b/sound/usb/validate.c @@ -0,0 +1,332 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +// +// Validation of USB-audio class descriptors +//

+#include <linux/init.h> +#include <linux/usb.h> +#include <linux/usb/audio.h> +#include <linux/usb/audio-v2.h> +#include <linux/usb/audio-v3.h> +#include <linux/usb/midi.h> +#include "usbaudio.h" +#include "helper.h"

+struct usb_desc_validator {

• unsigned char protocol;
• unsigned char type;
• bool (*func)(const void *p, const struct usb_desc_validator *v);
• size_t size;

+};

+#define UAC_VERSION_ALL (unsigned char)(-1)

+/* UAC1 only */ +static bool validate_uac1_header(const void *p,

• 		 const struct usb_desc_validator *v)
  

+{

• const struct uac1_ac_header_descriptor *d = p;
• return d->bLength >= sizeof(*d) &&

• d->bLength >= sizeof(*d) + d->bInCollection;
  

If my logic is not failing me in the morning, can't this be simplified to just: return d->bLength >= sizeof(*d) + d->bInCollection; ?

No, it's not same. The point of the first check is assure that the descriptor has enough size for reading its bInCollection field.

...

+}

+/* for mixer unit; covering all UACs */ +static bool validate_mixer_unit(const void *p,

• 		const struct usb_desc_validator *v)
  

+{

• const struct uac_mixer_unit_descriptor *d = p;
• size_t len;
• if (d->bLength < sizeof(*d) || !d->bNrInPins)

• return false;
  

• len = sizeof(*d) + d->bNrInPins;
• /* We can't determine the bitmap size only from this unit descriptor,

• * so just check with the remaining length.
  

• * The actual bitmap is checked at mixer unit parser.
  

• */
  

• switch (v->protocol) {
• case UAC_VERSION_1:
• default:

Implicit fall through?

Not really. The fall-through notion isn't needed for this kind of use cases. It's needed only for the case like

switch (foo) { case 0: do_something(); case 1: do_more(); break; }

thanks,

Takashi