[alsa-devel] [PATCH 0/3] ALSA: usb-audio: Fix NULL dereferences via invalid EP accesses
Hi,
this is a small patchset to address the possible Oops by accessing the invalid EP that was borked by a malformed firmware. One patch is a clean up while two are fixes.
Takashi
===
Takashi Iwai (3): ALSA: usb-audio: Fix NULL dereference in create_fixed_stream_quirk() ALSA: usb-audio: Minor code cleanup in create_fixed_stream_quirk() ALSA: usb-audio: Add sanity checks for endpoint accesses
sound/usb/clock.c | 2 ++ sound/usb/endpoint.c | 3 +++ sound/usb/mixer_quirks.c | 4 ++++ sound/usb/pcm.c | 2 ++ sound/usb/quirks.c | 22 ++++++++++++++-------- 5 files changed, 25 insertions(+), 8 deletions(-)
create_fixed_stream_quirk() may cause a NULL-pointer dereference by accessing the non-existing endpoint when a USB device with a malformed USB descriptor is used.
This patch avoids it simply by adding a sanity check of bNumEndpoints before the accesses.
Bugzilla: https://bugzilla.suse.com/show_bug.cgi?id=971125 Cc: stable@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/quirks.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c index 5b03296a5bd9..529c35cceaa6 100644 --- a/sound/usb/quirks.c +++ b/sound/usb/quirks.c @@ -180,6 +180,12 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip, } alts = &iface->altsetting[fp->altset_idx]; altsd = get_iface_desc(alts); + if (altsd->bNumEndpoints < 1) { + kfree(fp); + kfree(rate_table); + return -EINVAL; + } + fp->protocol = altsd->bInterfaceProtocol;
if (fp->datainterval == 0)
Just a minor code cleanup: unify the error paths.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/quirks.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c index 529c35cceaa6..a889d433cbdf 100644 --- a/sound/usb/quirks.c +++ b/sound/usb/quirks.c @@ -167,23 +167,18 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip, stream = (fp->endpoint & USB_DIR_IN) ? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK; err = snd_usb_add_audio_stream(chip, stream, fp); - if (err < 0) { - kfree(fp); - kfree(rate_table); - return err; - } + if (err < 0) + goto error; if (fp->iface != get_iface_desc(&iface->altsetting[0])->bInterfaceNumber || fp->altset_idx >= iface->num_altsetting) { - kfree(fp); - kfree(rate_table); - return -EINVAL; + err = -EINVAL; + goto error; } alts = &iface->altsetting[fp->altset_idx]; altsd = get_iface_desc(alts); if (altsd->bNumEndpoints < 1) { - kfree(fp); - kfree(rate_table); - return -EINVAL; + err = -EINVAL; + goto error; }
fp->protocol = altsd->bInterfaceProtocol; @@ -196,6 +191,11 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip, snd_usb_init_pitch(chip, fp->iface, alts, fp); snd_usb_init_sample_rate(chip, fp->iface, alts, fp, fp->rate_max); return 0; + + error: + kfree(fp); + kfree(rate_table); + return err; }
static int create_auto_pcm_quirk(struct snd_usb_audio *chip,
Add some sanity check codes before actually accessing the endpoint via get_endpoint() in order to avoid the invalid access through a malformed USB descriptor. Mostly just checking bNumEndpoints, but in one place (snd_microii_spdif_default_get()), the validity of iface and altsetting index is checked as well.
Bugzilla: https://bugzilla.suse.com/show_bug.cgi?id=971125 Cc: stable@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/clock.c | 2 ++ sound/usb/endpoint.c | 3 +++ sound/usb/mixer_quirks.c | 4 ++++ sound/usb/pcm.c | 2 ++ 4 files changed, 11 insertions(+)
diff --git a/sound/usb/clock.c b/sound/usb/clock.c index 2ed260b10f6d..7ccbcaf6a147 100644 --- a/sound/usb/clock.c +++ b/sound/usb/clock.c @@ -285,6 +285,8 @@ static int set_sample_rate_v1(struct snd_usb_audio *chip, int iface, unsigned char data[3]; int err, crate;
+ if (get_iface_desc(alts)->bNumEndpoints < 1) + return -EINVAL; ep = get_endpoint(alts, 0)->bEndpointAddress;
/* if endpoint doesn't have sampling rate control, bail out */ diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index 7b1cb365ffab..c07a7eda42a2 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -438,6 +438,9 @@ exit_clear: * * New endpoints will be added to chip->ep_list and must be freed by * calling snd_usb_endpoint_free(). + * + * For SND_USB_ENDPOINT_TYPE_SYNC, the caller needs to guarantee that + * bNumEndpoints > 1 beforehand. */ struct snd_usb_endpoint *snd_usb_add_endpoint(struct snd_usb_audio *chip, struct usb_host_interface *alts, diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c index 279025650568..f6c3bf79af9a 100644 --- a/sound/usb/mixer_quirks.c +++ b/sound/usb/mixer_quirks.c @@ -1519,7 +1519,11 @@ static int snd_microii_spdif_default_get(struct snd_kcontrol *kcontrol,
/* use known values for that card: interface#1 altsetting#1 */ iface = usb_ifnum_to_if(chip->dev, 1); + if (!iface || iface->num_altsetting < 2) + return -EINVAL; alts = &iface->altsetting[1]; + if (get_iface_desc(alts)->bNumEndpoints < 1) + return -EINVAL; ep = get_endpoint(alts, 0)->bEndpointAddress;
err = snd_usb_ctl_msg(chip->dev, diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 9245f52d43bd..44d178ee9177 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -159,6 +159,8 @@ static int init_pitch_v1(struct snd_usb_audio *chip, int iface, unsigned char data[1]; int err;
+ if (get_iface_desc(alts)->bNumEndpoints < 1) + return -EINVAL; ep = get_endpoint(alts, 0)->bEndpointAddress;
data[0] = 1;
participants (1)
-
Takashi Iwai