| Message ID | 20191220093134.1248-1-johan@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: usb-audio: fix set_format altsetting sanity check | expand |
On Fri, 20 Dec 2019 10:31:34 +0100, Johan Hovold wrote: > > Make sure to check the return value of usb_altnum_to_altsetting() to > avoid dereferencing a NULL pointer when the requested alternate settings > is missing. > > The format altsetting number may come from a quirk table and there does > not seem to be any other validation of it (the corresponding index is > checked however). > > Fixes: b099b9693d23 ("ALSA: usb-audio: Avoid superfluous usb_set_interface() calls") > Cc: stable <stable@vger.kernel.org> # 4.18 > Signed-off-by: Johan Hovold <johan@kernel.org> > --- > sound/usb/pcm.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c > index 9c8930bb00c8..73dd9d21bb42 100644 > --- a/sound/usb/pcm.c > +++ b/sound/usb/pcm.c > @@ -506,9 +506,9 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt) > if (WARN_ON(!iface)) > return -EINVAL; > alts = usb_altnum_to_altsetting(iface, fmt->altsetting); > - altsd = get_iface_desc(alts); > - if (WARN_ON(altsd->bAlternateSetting != fmt->altsetting)) > + if (WARN_ON(!alts)) > return -EINVAL; Do we need WARN_ON() here? If this may hit on syzbot, it'll stop at this point because of panic_on_warn. thanks, Takashi > + altsd = get_iface_desc(alts); > > if (fmt == subs->cur_audiofmt) > return 0; > -- > 2.24.1 >
On Fri, Dec 20, 2019 at 10:46:50AM +0100, Takashi Iwai wrote: > On Fri, 20 Dec 2019 10:31:34 +0100, > Johan Hovold wrote: > > > > Make sure to check the return value of usb_altnum_to_altsetting() to > > avoid dereferencing a NULL pointer when the requested alternate settings > > is missing. > > > > The format altsetting number may come from a quirk table and there does > > not seem to be any other validation of it (the corresponding index is > > checked however). > > > > Fixes: b099b9693d23 ("ALSA: usb-audio: Avoid superfluous usb_set_interface() calls") > > Cc: stable <stable@vger.kernel.org> # 4.18 > > Signed-off-by: Johan Hovold <johan@kernel.org> > > --- > > sound/usb/pcm.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c > > index 9c8930bb00c8..73dd9d21bb42 100644 > > --- a/sound/usb/pcm.c > > +++ b/sound/usb/pcm.c > > @@ -506,9 +506,9 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt) > > if (WARN_ON(!iface)) > > return -EINVAL; > > alts = usb_altnum_to_altsetting(iface, fmt->altsetting); > > - altsd = get_iface_desc(alts); > > - if (WARN_ON(altsd->bAlternateSetting != fmt->altsetting)) > > + if (WARN_ON(!alts)) > > return -EINVAL; > > Do we need WARN_ON() here? If this may hit on syzbot, it'll stop at > this point because of panic_on_warn. Yeah, I considered that too and decided to leave it in. Just like for the WARN_ON(iface), those numbers should be verified at probe. I tried tracking where fmt->altsetting comes from, and it seems like a sanity check needs to be added at least to create_fixed_stream_quirk() where, for example, fmt->iface, fmt->altset_idx and the number of endpoints are verified. If there are other paths that can end up setting these fields to invalid values, we want that WARN_ON() in there so we can fix those. Johan
On Fri, 20 Dec 2019 11:23:15 +0100, Johan Hovold wrote: > > On Fri, Dec 20, 2019 at 10:46:50AM +0100, Takashi Iwai wrote: > > On Fri, 20 Dec 2019 10:31:34 +0100, > > Johan Hovold wrote: > > > > > > Make sure to check the return value of usb_altnum_to_altsetting() to > > > avoid dereferencing a NULL pointer when the requested alternate settings > > > is missing. > > > > > > The format altsetting number may come from a quirk table and there does > > > not seem to be any other validation of it (the corresponding index is > > > checked however). > > > > > > Fixes: b099b9693d23 ("ALSA: usb-audio: Avoid superfluous usb_set_interface() calls") > > > Cc: stable <stable@vger.kernel.org> # 4.18 > > > Signed-off-by: Johan Hovold <johan@kernel.org> > > > --- > > > sound/usb/pcm.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c > > > index 9c8930bb00c8..73dd9d21bb42 100644 > > > --- a/sound/usb/pcm.c > > > +++ b/sound/usb/pcm.c > > > @@ -506,9 +506,9 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt) > > > if (WARN_ON(!iface)) > > > return -EINVAL; > > > alts = usb_altnum_to_altsetting(iface, fmt->altsetting); > > > - altsd = get_iface_desc(alts); > > > - if (WARN_ON(altsd->bAlternateSetting != fmt->altsetting)) > > > + if (WARN_ON(!alts)) > > > return -EINVAL; > > > > Do we need WARN_ON() here? If this may hit on syzbot, it'll stop at > > this point because of panic_on_warn. > > Yeah, I considered that too and decided to leave it in. Just like for > the WARN_ON(iface), those numbers should be verified at probe. > > I tried tracking where fmt->altsetting comes from, and it seems like > a sanity check needs to be added at least to create_fixed_stream_quirk() > where, for example, fmt->iface, fmt->altset_idx and the number of > endpoints are verified. > > If there are other paths that can end up setting these fields to invalid > values, we want that WARN_ON() in there so we can fix those. Fair enough. I applied now as-is. Thanks! Takashi
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 9c8930bb00c8..73dd9d21bb42 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -506,9 +506,9 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt) if (WARN_ON(!iface)) return -EINVAL; alts = usb_altnum_to_altsetting(iface, fmt->altsetting); - altsd = get_iface_desc(alts); - if (WARN_ON(altsd->bAlternateSetting != fmt->altsetting)) + if (WARN_ON(!alts)) return -EINVAL; + altsd = get_iface_desc(alts); if (fmt == subs->cur_audiofmt) return 0;
Make sure to check the return value of usb_altnum_to_altsetting() to avoid dereferencing a NULL pointer when the requested alternate settings is missing. The format altsetting number may come from a quirk table and there does not seem to be any other validation of it (the corresponding index is checked however). Fixes: b099b9693d23 ("ALSA: usb-audio: Avoid superfluous usb_set_interface() calls") Cc: stable <stable@vger.kernel.org> # 4.18 Signed-off-by: Johan Hovold <johan@kernel.org> --- sound/usb/pcm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)