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@xxxxxxxxxxxxxxx> # 4.18 > > Signed-off-by: Johan Hovold <johan@xxxxxxxxxx> > > --- > > 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 _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx https://mailman.alsa-project.org/mailman/listinfo/alsa-devel