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@xxxxxxx> 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@xxxxxxx>
> > ---
> > 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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
