[alsa-devel] [PATCH 0/4] ALSA: usb-audio: More strict validation and cleanups
Hi,
this patchset introduces a more comprehensive validation code for USB-audio descriptor units, mainly for mixer stuff, for yet more hardening against the malformed descriptors. The rest are rather cleanups.
Takashi
===
Takashi Iwai (4): ALSA: usb-audio: More validations of descriptor units ALSA: usb-audio: Simplify parse_audio_unit() ALSA: usb-audio: Unify the release of usb_mixer_elem_info objects ALSA: usb-audio: Remove superfluous bLength checks
sound/usb/Makefile | 3 +- sound/usb/clock.c | 14 +-- sound/usb/helper.h | 4 + sound/usb/mixer.c | 237 ++++++++++++------------------------ sound/usb/power.c | 2 + sound/usb/quirks.c | 3 + sound/usb/stream.c | 25 ++-- sound/usb/validate.c | 332 +++++++++++++++++++++++++++++++++++++++++++++++++++ 8 files changed, 439 insertions(+), 181 deletions(-) create mode 100644 sound/usb/validate.c
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@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@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: + rest = 1; /* iMixer */ + case UAC_VERSION_2: + rest = 2; /* bmControls + iMixer */ + 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 +2152,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;
On Thu, 22 Aug 2019 09:32:03 +0200 Takashi Iwai tiwai@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@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@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, intnum_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?
rest = 1; /* iMixer */- case UAC_VERSION_2:
rest = 2; /* bmControls + iMixer */- 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 +2152,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;
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@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@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@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, intnum_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.
Takashi
-- 8< -- From: Takashi Iwai tiwai@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@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@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;
On Thu, 22 Aug 2019 10:35:01 +0200 Takashi Iwai tiwai@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@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@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@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, intnum_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@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@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@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;
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; +} + +/* 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: + len += 2 + 1; /* wChannelConfig, iChannelNames */ + /* bmControls[n*m] */ + len += 1; /* iMixer */ + break; + case UAC_VERSION_2: + len += 4 + 1; /* bmChannelConfig, iChannelNames */ + /* bmMixerControls[n*m] */ + len += 1 + 1; /* bmControls, iMixer */ + break; + case UAC_VERSION_3: + len += 2; /* wClusterDescrID */ + /* bmMixerControls[n*m] */ + break; + } + return d->bLength >= len; +} + +/* both for processing and extension units; covering all UACs */ +static bool validate_processing_unit(const void *p, + const struct usb_desc_validator *v) +{ + const struct uac_processing_unit_descriptor *d = p; + const unsigned char *hdr = p; + size_t len, m; + + if (d->bLength < sizeof(*d)) + return false; + len = d->bLength < sizeof(*d) + d->bNrInPins; + if (d->bLength < len) + return false; + switch (v->protocol) { + case UAC_VERSION_1: + default: + /* bNrChannels, wChannelConfig, iChannelNames, bControlSize */ + len += 1 + 2 + 1 + 1; + if (d->bLength < len) /* bControlSize */ + return false; + m = hdr[len]; + len += 1 + m + 1; /* bControlSize, bmControls, iProcessing */ + break; + case UAC_VERSION_2: + /* bNrChannels, bmChannelConfig, iChannelNames */ + len += 1 + 4 + 1; + if (v->type == UAC2_PROCESSING_UNIT_V2) + len += 2; /* bmControls -- 2 bytes for PU */ + else + len += 1; /* bmControls -- 1 byte for EU */ + len += 1; /* iProcessing */ + break; + case UAC_VERSION_3: + /* wProcessingDescrStr, bmControls */ + len += 2 + 4; + break; + } + if (d->bLength < len) + return false; + + switch (v->protocol) { + case UAC_VERSION_1: + default: + if (v->type == UAC1_EXTENSION_UNIT) + return true; /* OK */ + switch (d->wProcessType) { + case UAC_PROCESS_UP_DOWNMIX: + case UAC_PROCESS_DOLBY_PROLOGIC: + if (d->bLength < len + 1) /* bNrModes */ + return false; + m = hdr[len]; + len += 1 + m * 2; /* bNrModes, waModes(n) */ + break; + default: + break; + } + break; + case UAC_VERSION_2: + if (v->type == UAC2_EXTENSION_UNIT_V2) + return true; /* OK */ + switch (d->wProcessType) { + case UAC2_PROCESS_UP_DOWNMIX: + case UAC2_PROCESS_DOLBY_PROLOCIC: /* SiC! */ + if (d->bLength < len + 1) /* bNrModes */ + return false; + m = hdr[len]; + len += 1 + m * 4; /* bNrModes, daModes(n) */ + break; + default: + break; + } + break; + case UAC_VERSION_3: + if (v->type == UAC3_EXTENSION_UNIT) { + len += 2; /* wClusterDescrID */ + break; + } + switch (d->wProcessType) { + case UAC3_PROCESS_UP_DOWNMIX: + if (d->bLength < len + 1) /* bNrModes */ + return false; + m = hdr[len]; + len += 1 + m * 2; /* bNrModes, waClusterDescrID(n) */ + break; + case UAC3_PROCESS_MULTI_FUNCTION: + len += 2 + 4; /* wClusterDescrID, bmAlgorighms */ + break; + default: + break; + } + break; + } + if (d->bLength < len) + return false; + + return true; +} + +/* both for selector and clock selector units; covering all UACs */ +static bool validate_selector_unit(const void *p, + const struct usb_desc_validator *v) +{ + const struct uac_selector_unit_descriptor *d = p; + size_t len; + + if (d->bLength < sizeof(*d)) + return false; + len = sizeof(*d) + d->bNrInPins; + switch (v->protocol) { + case UAC_VERSION_1: + default: + len += 1; /* iSelector */ + break; + case UAC_VERSION_2: + len += 1 + 1; /* bmControls, iSelector */ + break; + case UAC_VERSION_3: + len += 4 + 2; /* bmControls, wSelectorDescrStr */ + break; + } + return d->bLength >= len; +} + +static bool validate_uac1_feature_unit(const void *p, + const struct usb_desc_validator *v) +{ + const struct uac_feature_unit_descriptor *d = p; + + if (d->bLength < sizeof(*d) || !d->bControlSize) + return false; + /* at least bmaControls(0) for master channel + iFeature */ + return d->bLength >= sizeof(*d) + d->bControlSize + 1; +} + +static bool validate_uac2_feature_unit(const void *p, + const struct usb_desc_validator *v) +{ + const struct uac2_feature_unit_descriptor *d = p; + + if (d->bLength < sizeof(*d)) + return false; + /* at least bmaControls(0) for master channel + iFeature */ + return d->bLength >= sizeof(*d) + 4 + 1; +} + +static bool validate_uac3_feature_unit(const void *p, + const struct usb_desc_validator *v) +{ + const struct uac3_feature_unit_descriptor *d = p; + + if (d->bLength < sizeof(*d)) + return false; + /* at least bmaControls(0) for master channel + wFeatureDescrStr */ + return d->bLength >= sizeof(*d) + 4 + 2; +} + +static bool validate_midi_out_jack(const void *p, + const struct usb_desc_validator *v) +{ + const struct usb_midi_out_jack_descriptor *d = p; + + return d->bLength >= sizeof(*d) && + d->bLength >= sizeof(*d) + d->bNrInputPins * 2; +} + +#define FIXED(p, t, s) { .protocol = (p), .type = (t), .size = sizeof(s) } +#define FUNC(p, t, f) { .protocol = (p), .type = (t), .func = (f) } + +static struct usb_desc_validator audio_validators[] = { + /* UAC1 */ + FUNC(UAC_VERSION_1, UAC_HEADER, validate_uac1_header), + FIXED(UAC_VERSION_1, UAC_INPUT_TERMINAL, + struct uac_input_terminal_descriptor), + FIXED(UAC_VERSION_1, UAC_OUTPUT_TERMINAL, + struct uac1_output_terminal_descriptor), + FUNC(UAC_VERSION_1, UAC_MIXER_UNIT, validate_mixer_unit), + FUNC(UAC_VERSION_1, UAC_SELECTOR_UNIT, validate_selector_unit), + FUNC(UAC_VERSION_1, UAC_FEATURE_UNIT, validate_uac1_feature_unit), + FUNC(UAC_VERSION_1, UAC1_PROCESSING_UNIT, validate_processing_unit), + FUNC(UAC_VERSION_1, UAC1_EXTENSION_UNIT, validate_processing_unit), + + /* UAC2 */ + FIXED(UAC_VERSION_2, UAC_HEADER, struct uac2_ac_header_descriptor), + FIXED(UAC_VERSION_2, UAC_INPUT_TERMINAL, + struct uac2_input_terminal_descriptor), + FIXED(UAC_VERSION_2, UAC_OUTPUT_TERMINAL, + struct uac2_output_terminal_descriptor), + FUNC(UAC_VERSION_2, UAC_MIXER_UNIT, validate_mixer_unit), + FUNC(UAC_VERSION_2, UAC_SELECTOR_UNIT, validate_selector_unit), + FUNC(UAC_VERSION_2, UAC_FEATURE_UNIT, validate_uac2_feature_unit), + /* UAC_VERSION_2, UAC2_EFFECT_UNIT: not implemented yet */ + FUNC(UAC_VERSION_2, UAC2_PROCESSING_UNIT_V2, validate_processing_unit), + FUNC(UAC_VERSION_2, UAC2_EXTENSION_UNIT_V2, validate_processing_unit), + FIXED(UAC_VERSION_2, UAC2_CLOCK_SOURCE, + struct uac_clock_source_descriptor), + FUNC(UAC_VERSION_2, UAC2_CLOCK_SELECTOR, validate_selector_unit), + FIXED(UAC_VERSION_2, UAC2_CLOCK_MULTIPLIER, + struct uac_clock_multiplier_descriptor), + /* UAC_VERSION_2, UAC2_SAMPLE_RATE_CONVERTER: not implemented yet */ + + /* UAC3 */ + FIXED(UAC_VERSION_2, UAC_HEADER, struct uac3_ac_header_descriptor), + FIXED(UAC_VERSION_3, UAC_INPUT_TERMINAL, + struct uac3_input_terminal_descriptor), + FIXED(UAC_VERSION_3, UAC_OUTPUT_TERMINAL, + struct uac3_output_terminal_descriptor), + /* UAC_VERSION_3, UAC3_EXTENDED_TERMINAL: not implemented yet */ + FUNC(UAC_VERSION_3, UAC3_MIXER_UNIT, validate_mixer_unit), + FUNC(UAC_VERSION_3, UAC3_SELECTOR_UNIT, validate_selector_unit), + FUNC(UAC_VERSION_3, UAC_FEATURE_UNIT, validate_uac3_feature_unit), + /* UAC_VERSION_3, UAC3_EFFECT_UNIT: not implemented yet */ + FUNC(UAC_VERSION_3, UAC3_PROCESSING_UNIT, validate_processing_unit), + FUNC(UAC_VERSION_3, UAC3_EXTENSION_UNIT, validate_processing_unit), + FIXED(UAC_VERSION_3, UAC3_CLOCK_SOURCE, + struct uac3_clock_source_descriptor), + FUNC(UAC_VERSION_3, UAC3_CLOCK_SELECTOR, validate_selector_unit), + FIXED(UAC_VERSION_3, UAC3_CLOCK_MULTIPLIER, + struct uac3_clock_multiplier_descriptor), + /* UAC_VERSION_3, UAC3_SAMPLE_RATE_CONVERTER: not implemented yet */ + /* UAC_VERSION_3, UAC3_CONNECTORS: not implemented yet */ + { } /* terminator */ +}; + +static struct usb_desc_validator midi_validators[] = { + FIXED(UAC_VERSION_ALL, USB_MS_HEADER, + struct usb_ms_header_descriptor), + FIXED(UAC_VERSION_ALL, USB_MS_MIDI_IN_JACK, + struct usb_midi_in_jack_descriptor), + FUNC(UAC_VERSION_ALL, USB_MS_MIDI_OUT_JACK, + validate_midi_out_jack), + { } /* terminator */ +}; + + +/* Validate the given unit descriptor, return true if it's OK */ +static bool validate_desc(unsigned char *hdr, int protocol, + const struct usb_desc_validator *v) +{ + if (hdr[1] != USB_DT_CS_INTERFACE) + return true; /* don't care */ + + for (; v->type; v++) { + if (v->type == hdr[2] && + (v->protocol == UAC_VERSION_ALL || + v->protocol == protocol)) { + if (v->func) + return v->func(hdr, v); + /* check for the fixed size */ + return hdr[0] >= v->size; + } + } + + return true; /* not matching, skip validation */ +} + +bool snd_usb_validate_audio_desc(void *p, int protocol) +{ + return validate_desc(p, protocol, audio_validators); +} + +bool snd_usb_validate_midi_desc(void *p) +{ + return validate_desc(p, UAC_VERSION_1, midi_validators); +} +
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.osnd-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;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;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;@@ -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);@@ -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);@@ -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);@@ -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);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; ?
+}
+/* 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?
len += 2 + 1; /* wChannelConfig, iChannelNames *//* bmControls[n*m] */len += 1; /* iMixer */break;- case UAC_VERSION_2:
len += 4 + 1; /* bmChannelConfig, iChannelNames *//* bmMixerControls[n*m] */len += 1 + 1; /* bmControls, iMixer */break;- case UAC_VERSION_3:
len += 2; /* wClusterDescrID *//* bmMixerControls[n*m] */break;- }
- return d->bLength >= len;
+}
+/* both for processing and extension units; covering all UACs */ +static bool validate_processing_unit(const void *p,
const struct usb_desc_validator *v)+{
- const struct uac_processing_unit_descriptor *d = p;
- const unsigned char *hdr = p;
- size_t len, m;
- if (d->bLength < sizeof(*d))
return false;- len = d->bLength < sizeof(*d) + d->bNrInPins;
- if (d->bLength < len)
return false;- switch (v->protocol) {
- case UAC_VERSION_1:
- default:
Implicit fall through?
/* bNrChannels, wChannelConfig, iChannelNames, bControlSize */len += 1 + 2 + 1 + 1;if (d->bLength < len) /* bControlSize */return false;m = hdr[len];len += 1 + m + 1; /* bControlSize, bmControls, iProcessing */break;- case UAC_VERSION_2:
/* bNrChannels, bmChannelConfig, iChannelNames */len += 1 + 4 + 1;if (v->type == UAC2_PROCESSING_UNIT_V2)len += 2; /* bmControls -- 2 bytes for PU */elselen += 1; /* bmControls -- 1 byte for EU */len += 1; /* iProcessing */break;- case UAC_VERSION_3:
/* wProcessingDescrStr, bmControls */len += 2 + 4;break;- }
- if (d->bLength < len)
return false;- switch (v->protocol) {
- case UAC_VERSION_1:
- default:
Implicit fall through?
if (v->type == UAC1_EXTENSION_UNIT)return true; /* OK */switch (d->wProcessType) {case UAC_PROCESS_UP_DOWNMIX:case UAC_PROCESS_DOLBY_PROLOGIC:
Implicit fall through?
if (d->bLength < len + 1) /* bNrModes */return false;m = hdr[len];len += 1 + m * 2; /* bNrModes, waModes(n) */break;default:break;}break;- case UAC_VERSION_2:
if (v->type == UAC2_EXTENSION_UNIT_V2)return true; /* OK */switch (d->wProcessType) {case UAC2_PROCESS_UP_DOWNMIX:case UAC2_PROCESS_DOLBY_PROLOCIC: /* SiC! */
Implicit fall through?
if (d->bLength < len + 1) /* bNrModes */return false;m = hdr[len];len += 1 + m * 4; /* bNrModes, daModes(n) */break;default:break;}break;- case UAC_VERSION_3:
if (v->type == UAC3_EXTENSION_UNIT) {len += 2; /* wClusterDescrID */break;}switch (d->wProcessType) {case UAC3_PROCESS_UP_DOWNMIX:if (d->bLength < len + 1) /* bNrModes */return false;m = hdr[len];len += 1 + m * 2; /* bNrModes, waClusterDescrID(n) */break;case UAC3_PROCESS_MULTI_FUNCTION:len += 2 + 4; /* wClusterDescrID, bmAlgorighms */break;default:break;}break;- }
- if (d->bLength < len)
return false;- return true;
+}
+/* both for selector and clock selector units; covering all UACs */ +static bool validate_selector_unit(const void *p,
const struct usb_desc_validator *v)+{
- const struct uac_selector_unit_descriptor *d = p;
- size_t len;
- if (d->bLength < sizeof(*d))
return false;- len = sizeof(*d) + d->bNrInPins;
- switch (v->protocol) {
- case UAC_VERSION_1:
- default:
Implicit fall through?
len += 1; /* iSelector */break;- case UAC_VERSION_2:
len += 1 + 1; /* bmControls, iSelector */break;- case UAC_VERSION_3:
len += 4 + 2; /* bmControls, wSelectorDescrStr */break;- }
- return d->bLength >= len;
+}
+static bool validate_uac1_feature_unit(const void *p,
const struct usb_desc_validator *v)+{
- const struct uac_feature_unit_descriptor *d = p;
- if (d->bLength < sizeof(*d) || !d->bControlSize)
return false;- /* at least bmaControls(0) for master channel + iFeature */
- return d->bLength >= sizeof(*d) + d->bControlSize + 1;
+}
+static bool validate_uac2_feature_unit(const void *p,
const struct usb_desc_validator *v)+{
- const struct uac2_feature_unit_descriptor *d = p;
- if (d->bLength < sizeof(*d))
return false;- /* at least bmaControls(0) for master channel + iFeature */
- return d->bLength >= sizeof(*d) + 4 + 1;
+}
+static bool validate_uac3_feature_unit(const void *p,
const struct usb_desc_validator *v)+{
- const struct uac3_feature_unit_descriptor *d = p;
- if (d->bLength < sizeof(*d))
return false;- /* at least bmaControls(0) for master channel + wFeatureDescrStr */
- return d->bLength >= sizeof(*d) + 4 + 2;
+}
+static bool validate_midi_out_jack(const void *p,
const struct usb_desc_validator *v)+{
- const struct usb_midi_out_jack_descriptor *d = p;
- return d->bLength >= sizeof(*d) &&
d->bLength >= sizeof(*d) + d->bNrInputPins * 2;
This can also be simplified.
+}
+#define FIXED(p, t, s) { .protocol = (p), .type = (t), .size = sizeof(s) } +#define FUNC(p, t, f) { .protocol = (p), .type = (t), .func = (f) }
+static struct usb_desc_validator audio_validators[] = {
- /* UAC1 */
- FUNC(UAC_VERSION_1, UAC_HEADER, validate_uac1_header),
- FIXED(UAC_VERSION_1, UAC_INPUT_TERMINAL,
struct uac_input_terminal_descriptor),- FIXED(UAC_VERSION_1, UAC_OUTPUT_TERMINAL,
struct uac1_output_terminal_descriptor),- FUNC(UAC_VERSION_1, UAC_MIXER_UNIT, validate_mixer_unit),
- FUNC(UAC_VERSION_1, UAC_SELECTOR_UNIT, validate_selector_unit),
- FUNC(UAC_VERSION_1, UAC_FEATURE_UNIT, validate_uac1_feature_unit),
- FUNC(UAC_VERSION_1, UAC1_PROCESSING_UNIT, validate_processing_unit),
- FUNC(UAC_VERSION_1, UAC1_EXTENSION_UNIT, validate_processing_unit),
- /* UAC2 */
- FIXED(UAC_VERSION_2, UAC_HEADER, struct uac2_ac_header_descriptor),
- FIXED(UAC_VERSION_2, UAC_INPUT_TERMINAL,
struct uac2_input_terminal_descriptor),- FIXED(UAC_VERSION_2, UAC_OUTPUT_TERMINAL,
struct uac2_output_terminal_descriptor),- FUNC(UAC_VERSION_2, UAC_MIXER_UNIT, validate_mixer_unit),
- FUNC(UAC_VERSION_2, UAC_SELECTOR_UNIT, validate_selector_unit),
- FUNC(UAC_VERSION_2, UAC_FEATURE_UNIT, validate_uac2_feature_unit),
- /* UAC_VERSION_2, UAC2_EFFECT_UNIT: not implemented yet */
- FUNC(UAC_VERSION_2, UAC2_PROCESSING_UNIT_V2, validate_processing_unit),
- FUNC(UAC_VERSION_2, UAC2_EXTENSION_UNIT_V2, validate_processing_unit),
- FIXED(UAC_VERSION_2, UAC2_CLOCK_SOURCE,
struct uac_clock_source_descriptor),- FUNC(UAC_VERSION_2, UAC2_CLOCK_SELECTOR, validate_selector_unit),
- FIXED(UAC_VERSION_2, UAC2_CLOCK_MULTIPLIER,
struct uac_clock_multiplier_descriptor),- /* UAC_VERSION_2, UAC2_SAMPLE_RATE_CONVERTER: not implemented yet */
- /* UAC3 */
- FIXED(UAC_VERSION_2, UAC_HEADER, struct uac3_ac_header_descriptor),
- FIXED(UAC_VERSION_3, UAC_INPUT_TERMINAL,
struct uac3_input_terminal_descriptor),- FIXED(UAC_VERSION_3, UAC_OUTPUT_TERMINAL,
struct uac3_output_terminal_descriptor),- /* UAC_VERSION_3, UAC3_EXTENDED_TERMINAL: not implemented yet */
- FUNC(UAC_VERSION_3, UAC3_MIXER_UNIT, validate_mixer_unit),
- FUNC(UAC_VERSION_3, UAC3_SELECTOR_UNIT, validate_selector_unit),
- FUNC(UAC_VERSION_3, UAC_FEATURE_UNIT, validate_uac3_feature_unit),
- /* UAC_VERSION_3, UAC3_EFFECT_UNIT: not implemented yet */
- FUNC(UAC_VERSION_3, UAC3_PROCESSING_UNIT, validate_processing_unit),
- FUNC(UAC_VERSION_3, UAC3_EXTENSION_UNIT, validate_processing_unit),
- FIXED(UAC_VERSION_3, UAC3_CLOCK_SOURCE,
struct uac3_clock_source_descriptor),- FUNC(UAC_VERSION_3, UAC3_CLOCK_SELECTOR, validate_selector_unit),
- FIXED(UAC_VERSION_3, UAC3_CLOCK_MULTIPLIER,
struct uac3_clock_multiplier_descriptor),- /* UAC_VERSION_3, UAC3_SAMPLE_RATE_CONVERTER: not implemented yet */
- /* UAC_VERSION_3, UAC3_CONNECTORS: not implemented yet */
- { } /* terminator */
+};
+static struct usb_desc_validator midi_validators[] = {
- FIXED(UAC_VERSION_ALL, USB_MS_HEADER,
struct usb_ms_header_descriptor),- FIXED(UAC_VERSION_ALL, USB_MS_MIDI_IN_JACK,
struct usb_midi_in_jack_descriptor),- FUNC(UAC_VERSION_ALL, USB_MS_MIDI_OUT_JACK,
validate_midi_out_jack),- { } /* terminator */
+};
+/* Validate the given unit descriptor, return true if it's OK */ +static bool validate_desc(unsigned char *hdr, int protocol,
const struct usb_desc_validator *v)+{
- if (hdr[1] != USB_DT_CS_INTERFACE)
return true; /* don't care */- for (; v->type; v++) {
if (v->type == hdr[2] &&(v->protocol == UAC_VERSION_ALL ||v->protocol == protocol)) {if (v->func)return v->func(hdr, v);/* check for the fixed size */return hdr[0] >= v->size;}- }
- return true; /* not matching, skip validation */
+}
+bool snd_usb_validate_audio_desc(void *p, int protocol) +{
- return validate_desc(p, protocol, audio_validators);
+}
+bool snd_usb_validate_midi_desc(void *p) +{
- return validate_desc(p, UAC_VERSION_1, midi_validators);
+}
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.osnd-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;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;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;@@ -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);@@ -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);@@ -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);@@ -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);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
Minor code refactoring by combining the UAC version and the type in the switch-case flow, so that we reduce the indentation and redundancy. One good bonus is that the duplicated definition of the same type value (e.g. UAC2_EFFECT_UNIT) can be handled more cleanly.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/mixer.c | 95 +++++++++++++++++++++++-------------------------------- 1 file changed, 39 insertions(+), 56 deletions(-)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index b4be226e07f3..2628697353bb 100644 --- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -2780,62 +2780,45 @@ static int parse_audio_unit(struct mixer_build *state, int unitid) return 0; /* skip invalid unit */ }
- if (protocol == UAC_VERSION_1 || protocol == UAC_VERSION_2) { - switch (p1[2]) { - case UAC_INPUT_TERMINAL: - return parse_audio_input_terminal(state, unitid, p1); - case UAC_MIXER_UNIT: - return parse_audio_mixer_unit(state, unitid, p1); - case UAC2_CLOCK_SOURCE: - return parse_clock_source_unit(state, unitid, p1); - case UAC_SELECTOR_UNIT: - case UAC2_CLOCK_SELECTOR: - return parse_audio_selector_unit(state, unitid, p1); - case UAC_FEATURE_UNIT: - return parse_audio_feature_unit(state, unitid, p1); - case UAC1_PROCESSING_UNIT: - /* UAC2_EFFECT_UNIT has the same value */ - if (protocol == UAC_VERSION_1) - return parse_audio_processing_unit(state, unitid, p1); - else - return 0; /* FIXME - effect units not implemented yet */ - case UAC1_EXTENSION_UNIT: - /* UAC2_PROCESSING_UNIT_V2 has the same value */ - if (protocol == UAC_VERSION_1) - return parse_audio_extension_unit(state, unitid, p1); - else /* UAC_VERSION_2 */ - return parse_audio_processing_unit(state, unitid, p1); - case UAC2_EXTENSION_UNIT_V2: - return parse_audio_extension_unit(state, unitid, p1); - default: - usb_audio_err(state->chip, - "unit %u: unexpected type 0x%02x\n", unitid, p1[2]); - return -EINVAL; - } - } else { /* UAC_VERSION_3 */ - switch (p1[2]) { - case UAC_INPUT_TERMINAL: - return parse_audio_input_terminal(state, unitid, p1); - case UAC3_MIXER_UNIT: - return parse_audio_mixer_unit(state, unitid, p1); - case UAC3_CLOCK_SOURCE: - return parse_clock_source_unit(state, unitid, p1); - case UAC3_SELECTOR_UNIT: - case UAC3_CLOCK_SELECTOR: - return parse_audio_selector_unit(state, unitid, p1); - case UAC3_FEATURE_UNIT: - return parse_audio_feature_unit(state, unitid, p1); - case UAC3_EFFECT_UNIT: - return 0; /* FIXME - effect units not implemented yet */ - case UAC3_PROCESSING_UNIT: - return parse_audio_processing_unit(state, unitid, p1); - case UAC3_EXTENSION_UNIT: - return parse_audio_extension_unit(state, unitid, p1); - default: - usb_audio_err(state->chip, - "unit %u: unexpected type 0x%02x\n", unitid, p1[2]); - return -EINVAL; - } +#define PTYPE(a, b) ((a) << 8 | (b)) + switch (PTYPE(protocol, p1[2])) { + case PTYPE(UAC_VERSION_1, UAC_INPUT_TERMINAL): + case PTYPE(UAC_VERSION_2, UAC_INPUT_TERMINAL): + case PTYPE(UAC_VERSION_3, UAC_INPUT_TERMINAL): + return parse_audio_input_terminal(state, unitid, p1); + case PTYPE(UAC_VERSION_1, UAC_MIXER_UNIT): + case PTYPE(UAC_VERSION_2, UAC_MIXER_UNIT): + case PTYPE(UAC_VERSION_3, UAC3_MIXER_UNIT): + return parse_audio_mixer_unit(state, unitid, p1); + case PTYPE(UAC_VERSION_2, UAC2_CLOCK_SOURCE): + case PTYPE(UAC_VERSION_3, UAC3_CLOCK_SOURCE): + return parse_clock_source_unit(state, unitid, p1); + case PTYPE(UAC_VERSION_1, UAC_SELECTOR_UNIT): + case PTYPE(UAC_VERSION_2, UAC_SELECTOR_UNIT): + case PTYPE(UAC_VERSION_3, UAC3_SELECTOR_UNIT): + case PTYPE(UAC_VERSION_2, UAC2_CLOCK_SELECTOR): + case PTYPE(UAC_VERSION_3, UAC3_CLOCK_SELECTOR): + return parse_audio_selector_unit(state, unitid, p1); + case PTYPE(UAC_VERSION_1, UAC_FEATURE_UNIT): + case PTYPE(UAC_VERSION_2, UAC_FEATURE_UNIT): + case PTYPE(UAC_VERSION_3, UAC3_FEATURE_UNIT): + return parse_audio_feature_unit(state, unitid, p1); + case PTYPE(UAC_VERSION_1, UAC1_PROCESSING_UNIT): + case PTYPE(UAC_VERSION_2, UAC2_PROCESSING_UNIT_V2): + case PTYPE(UAC_VERSION_3, UAC3_PROCESSING_UNIT): + return parse_audio_processing_unit(state, unitid, p1); + case PTYPE(UAC_VERSION_1, UAC1_EXTENSION_UNIT): + case PTYPE(UAC_VERSION_2, UAC2_EXTENSION_UNIT_V2): + case PTYPE(UAC_VERSION_3, UAC3_EXTENSION_UNIT): + return parse_audio_extension_unit(state, unitid, p1); + case PTYPE(UAC_VERSION_2, UAC2_EFFECT_UNIT): + case PTYPE(UAC_VERSION_3, UAC3_EFFECT_UNIT): + return 0; /* FIXME - effect units not implemented yet */ + default: + usb_audio_err(state->chip, + "unit %u: unexpected type 0x%02x\n", + unitid, p1[2]); + return -EINVAL; } }
Instead of the direct kfree() calls, introduce a new local helper to release the usb_mixer_elem_info object. This will be extended to do more than a single kfree() in the later patches.
Also, use the standard goto instead of multiple calls in parse_audio_selector_unit() error paths.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/mixer.c | 48 ++++++++++++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 20 deletions(-)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index 2628697353bb..7f498528be48 100644 --- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -1026,10 +1026,15 @@ static struct usb_feature_control_info audio_feature_info[] = { { UAC2_FU_PHASE_INVERTER, "Phase Inverter Control", USB_MIXER_BOOLEAN, -1 }, };
+static void usb_mixer_elem_info_free(struct usb_mixer_elem_info *cval) +{ + kfree(cval); +} + /* private_free callback */ void snd_usb_mixer_elem_free(struct snd_kcontrol *kctl) { - kfree(kctl->private_data); + usb_mixer_elem_info_free(kctl->private_data); kctl->private_data = NULL; }
@@ -1552,7 +1557,7 @@ static void __build_feature_ctl(struct usb_mixer_interface *mixer,
ctl_info = get_feature_control_info(control); if (!ctl_info) { - kfree(cval); + usb_mixer_elem_info_free(cval); return; } if (mixer->protocol == UAC_VERSION_1) @@ -1585,7 +1590,7 @@ static void __build_feature_ctl(struct usb_mixer_interface *mixer,
if (!kctl) { usb_audio_err(mixer->chip, "cannot malloc kcontrol\n"); - kfree(cval); + usb_mixer_elem_info_free(cval); return; } kctl->private_free = snd_usb_mixer_elem_free; @@ -1755,7 +1760,7 @@ static void build_connector_control(struct usb_mixer_interface *mixer, kctl = snd_ctl_new1(&usb_connector_ctl_ro, cval); if (!kctl) { usb_audio_err(mixer->chip, "cannot malloc kcontrol\n"); - kfree(cval); + usb_mixer_elem_info_free(cval); return; } get_connector_control_name(mixer, term, is_input, kctl->id.name, @@ -1808,7 +1813,7 @@ static int parse_clock_source_unit(struct mixer_build *state, int unitid, kctl = snd_ctl_new1(&usb_bool_master_control_ctl_ro, cval);
if (!kctl) { - kfree(cval); + usb_mixer_elem_info_free(cval); return -ENOMEM; }
@@ -2068,7 +2073,7 @@ static void build_mixer_unit_ctl(struct mixer_build *state, kctl = snd_ctl_new1(&usb_feature_unit_ctl, cval); if (!kctl) { usb_audio_err(state->chip, "cannot malloc kcontrol\n"); - kfree(cval); + usb_mixer_elem_info_free(cval); return; } kctl->private_free = snd_usb_mixer_elem_free; @@ -2466,7 +2471,7 @@ static int build_audio_procunit(struct mixer_build *state, int unitid,
kctl = snd_ctl_new1(&mixer_procunit_ctl, cval); if (!kctl) { - kfree(cval); + usb_mixer_elem_info_free(cval); return -ENOMEM; } kctl->private_free = snd_usb_mixer_elem_free; @@ -2604,7 +2609,7 @@ static void usb_mixer_selector_elem_free(struct snd_kcontrol *kctl) if (kctl->private_data) { struct usb_mixer_elem_info *cval = kctl->private_data; num_ins = cval->max; - kfree(cval); + usb_mixer_elem_info_free(cval); kctl->private_data = NULL; } if (kctl->private_value) { @@ -2676,10 +2681,10 @@ static int parse_audio_selector_unit(struct mixer_build *state, int unitid, break; }
- namelist = kmalloc_array(desc->bNrInPins, sizeof(char *), GFP_KERNEL); + namelist = kcalloc(desc->bNrInPins, sizeof(char *), GFP_KERNEL); if (!namelist) { - kfree(cval); - return -ENOMEM; + err = -ENOMEM; + goto error_cval; } #define MAX_ITEM_NAME_LEN 64 for (i = 0; i < desc->bNrInPins; i++) { @@ -2687,11 +2692,8 @@ static int parse_audio_selector_unit(struct mixer_build *state, int unitid, len = 0; namelist[i] = kmalloc(MAX_ITEM_NAME_LEN, GFP_KERNEL); if (!namelist[i]) { - while (i--) - kfree(namelist[i]); - kfree(namelist); - kfree(cval); - return -ENOMEM; + err = -ENOMEM; + goto error_name; } len = check_mapped_selector_name(state, unitid, i, namelist[i], MAX_ITEM_NAME_LEN); @@ -2705,10 +2707,8 @@ static int parse_audio_selector_unit(struct mixer_build *state, int unitid, kctl = snd_ctl_new1(&mixer_selectunit_ctl, cval); if (! kctl) { usb_audio_err(state->chip, "cannot malloc kcontrol\n"); - for (i = 0; i < desc->bNrInPins; i++) - kfree(namelist[i]); - kfree(namelist); - kfree(cval); + err = -ENOMEM; + goto error_name; return -ENOMEM; } kctl->private_value = (unsigned long)namelist; @@ -2755,6 +2755,14 @@ static int parse_audio_selector_unit(struct mixer_build *state, int unitid, usb_audio_dbg(state->chip, "[%d] SU [%s] items = %d\n", cval->head.id, kctl->id.name, desc->bNrInPins); return snd_usb_mixer_add_control(&cval->head, kctl); + + error_name: + for (i = 0; i < desc->bNrInPins; i++) + kfree(namelist[i]); + kfree(namelist); + error_cval: + usb_mixer_elem_info_free(cval); + return err; }
/*
Now that we got the more comprehensive validation code for USB-audio descriptors, the check of overflow in each descriptor unit parser became superfluous. Drop some of the obvious cases.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/clock.c | 14 ++++------ sound/usb/mixer.c | 84 ------------------------------------------------------- 2 files changed, 6 insertions(+), 92 deletions(-)
diff --git a/sound/usb/clock.c b/sound/usb/clock.c index 72e9bdf76115..6b8c14f9b5d4 100644 --- a/sound/usb/clock.c +++ b/sound/usb/clock.c @@ -38,39 +38,37 @@ static void *find_uac_clock_desc(struct usb_host_interface *iface, int id, static bool validate_clock_source_v2(void *p, int id) { struct uac_clock_source_descriptor *cs = p; - return cs->bLength == sizeof(*cs) && cs->bClockID == id; + return cs->bClockID == id; }
static bool validate_clock_source_v3(void *p, int id) { struct uac3_clock_source_descriptor *cs = p; - return cs->bLength == sizeof(*cs) && cs->bClockID == id; + return cs->bClockID == id; }
static bool validate_clock_selector_v2(void *p, int id) { struct uac_clock_selector_descriptor *cs = p; - return cs->bLength >= sizeof(*cs) && cs->bClockID == id && - cs->bLength == 7 + cs->bNrInPins; + return cs->bClockID == id; }
static bool validate_clock_selector_v3(void *p, int id) { struct uac3_clock_selector_descriptor *cs = p; - return cs->bLength >= sizeof(*cs) && cs->bClockID == id && - cs->bLength == 11 + cs->bNrInPins; + return cs->bClockID == id; }
static bool validate_clock_multiplier_v2(void *p, int id) { struct uac_clock_multiplier_descriptor *cs = p; - return cs->bLength == sizeof(*cs) && cs->bClockID == id; + return cs->bClockID == id; }
static bool validate_clock_multiplier_v3(void *p, int id) { struct uac3_clock_multiplier_descriptor *cs = p; - return cs->bLength == sizeof(*cs) && cs->bClockID == id; + return cs->bClockID == id; }
#define DEFINE_FIND_HELPER(name, obj, validator, type) \ diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index 7f498528be48..61b240835574 100644 --- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -740,13 +740,6 @@ static int uac_mixer_unit_get_channels(struct mixer_build *state, { int mu_channels;
- 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: @@ -1781,13 +1774,6 @@ static int parse_clock_source_unit(struct mixer_build *state, int unitid, if (state->mixer->protocol != UAC_VERSION_2) return -EINVAL;
- if (hdr->bLength != sizeof(*hdr)) { - usb_audio_dbg(state->chip, - "Bogus clock source descriptor length of %d, ignoring.\n", - hdr->bLength); - return 0; - } - /* * The only property of this unit we are interested in is the * clock source validity. If that isn't readable, just bail out. @@ -1846,62 +1832,20 @@ static int parse_audio_feature_unit(struct mixer_build *state, int unitid, __u8 *bmaControls;
if (state->mixer->protocol == UAC_VERSION_1) { - if (hdr->bLength < 7) { - usb_audio_err(state->chip, - "unit %u: invalid UAC_FEATURE_UNIT descriptor\n", - unitid); - return -EINVAL; - } csize = hdr->bControlSize; - if (!csize) { - usb_audio_dbg(state->chip, - "unit %u: invalid bControlSize == 0\n", - unitid); - return -EINVAL; - } channels = (hdr->bLength - 7) / csize - 1; bmaControls = hdr->bmaControls; - if (hdr->bLength < 7 + csize) { - usb_audio_err(state->chip, - "unit %u: invalid UAC_FEATURE_UNIT descriptor\n", - unitid); - return -EINVAL; - } } else if (state->mixer->protocol == UAC_VERSION_2) { struct uac2_feature_unit_descriptor *ftr = _ftr; - if (hdr->bLength < 6) { - usb_audio_err(state->chip, - "unit %u: invalid UAC_FEATURE_UNIT descriptor\n", - unitid); - return -EINVAL; - } csize = 4; channels = (hdr->bLength - 6) / 4 - 1; bmaControls = ftr->bmaControls; - if (hdr->bLength < 6 + csize) { - usb_audio_err(state->chip, - "unit %u: invalid UAC_FEATURE_UNIT descriptor\n", - unitid); - return -EINVAL; - } } else { /* UAC_VERSION_3 */ struct uac3_feature_unit_descriptor *ftr = _ftr;
- if (hdr->bLength < 7) { - usb_audio_err(state->chip, - "unit %u: invalid UAC3_FEATURE_UNIT descriptor\n", - unitid); - return -EINVAL; - } csize = 4; channels = (ftr->bLength - 7) / 4 - 1; bmaControls = ftr->bmaControls; - if (hdr->bLength < 7 + csize) { - usb_audio_err(state->chip, - "unit %u: invalid UAC3_FEATURE_UNIT descriptor\n", - unitid); - return -EINVAL; - } }
/* parse the source unit */ @@ -2099,15 +2043,11 @@ static int parse_audio_input_terminal(struct mixer_build *state, int unitid,
if (state->mixer->protocol == UAC_VERSION_2) { struct uac2_input_terminal_descriptor *d_v2 = raw_desc; - if (d_v2->bLength < sizeof(*d_v2)) - return -EINVAL; control = UAC2_TE_CONNECTOR; term_id = d_v2->bTerminalID; bmctls = le16_to_cpu(d_v2->bmControls); } else if (state->mixer->protocol == UAC_VERSION_3) { struct uac3_input_terminal_descriptor *d_v3 = raw_desc; - if (d_v3->bLength < sizeof(*d_v3)) - return -EINVAL; control = UAC3_TE_INSERTION; term_id = d_v3->bTerminalID; bmctls = le32_to_cpu(d_v3->bmControls); @@ -2369,18 +2309,7 @@ static int build_audio_procunit(struct mixer_build *state, int unitid, const char *name = extension_unit ? "Extension Unit" : "Processing Unit";
- if (desc->bLength < 13) { - usb_audio_err(state->chip, "invalid %s descriptor (id %d)\n", name, unitid); - return -EINVAL; - } - num_ins = desc->bNrInPins; - if (desc->bLength < 13 + num_ins || - desc->bLength < num_ins + uac_processing_unit_bControlSize(desc, state->mixer->protocol)) { - usb_audio_err(state->chip, "invalid %s descriptor (id %d)\n", name, unitid); - return -EINVAL; - } - for (i = 0; i < num_ins; i++) { err = parse_audio_unit(state, desc->baSourceID[i]); if (err < 0) @@ -2635,13 +2564,6 @@ static int parse_audio_selector_unit(struct mixer_build *state, int unitid, const struct usbmix_name_map *map; char **namelist;
- if (desc->bLength < 5 || !desc->bNrInPins || - desc->bLength < 5 + desc->bNrInPins) { - usb_audio_err(state->chip, - "invalid SELECTOR UNIT descriptor %d\n", unitid); - return -EINVAL; - } - for (i = 0; i < desc->bNrInPins; i++) { err = parse_audio_unit(state, desc->baSourceID[i]); if (err < 0) @@ -3147,8 +3069,6 @@ static int snd_usb_mixer_controls(struct usb_mixer_interface *mixer) if (mixer->protocol == UAC_VERSION_1) { struct uac1_output_terminal_descriptor *desc = p;
- if (desc->bLength < sizeof(*desc)) - continue; /* invalid descriptor? */ /* mark terminal ID as visited */ set_bit(desc->bTerminalID, state.unitbitmap); state.oterm.id = desc->bTerminalID; @@ -3160,8 +3080,6 @@ static int snd_usb_mixer_controls(struct usb_mixer_interface *mixer) } else if (mixer->protocol == UAC_VERSION_2) { struct uac2_output_terminal_descriptor *desc = p;
- if (desc->bLength < sizeof(*desc)) - continue; /* invalid descriptor? */ /* mark terminal ID as visited */ set_bit(desc->bTerminalID, state.unitbitmap); state.oterm.id = desc->bTerminalID; @@ -3187,8 +3105,6 @@ static int snd_usb_mixer_controls(struct usb_mixer_interface *mixer) } else { /* UAC_VERSION_3 */ struct uac3_output_terminal_descriptor *desc = p;
- if (desc->bLength < sizeof(*desc)) - continue; /* invalid descriptor? */ /* mark terminal ID as visited */ set_bit(desc->bTerminalID, state.unitbitmap); state.oterm.id = desc->bTerminalID;
participants (2)
-
Amadeusz Sławiński -
Takashi Iwai