Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 04 May 2021 10:27:34 +0200
Message-ID: <s5hczu6rjih.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Jaroslav Kysela <perex@perex.cz>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 37/69] ALSA: usx2y: check for failure of usb_alloc_urb()
In-Reply-To: <aa185e3d-981f-ca5a-ea40-d266e62b82fe@perex.cz>
References: <20210503115736.2104747-1-gregkh@linuxfoundation.org>
        <20210503115736.2104747-38-gregkh@linuxfoundation.org>
        <aa185e3d-981f-ca5a-ea40-d266e62b82fe@perex.cz>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3
 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Mon, 03 May 2021 22:33:52 +0200,
Jaroslav Kysela wrote:
> 
> Dne 03. 05. 21 v 13:57 Greg Kroah-Hartman napsal(a):
> > While it is almost impossible to hit an error calling usb_alloc_urb(),
> > to make systems like syzbot which does error injection, and some static
> > analysis tools happy, properly handle errors on this path by unwinding
> > the previously allocated urbs and freeing them.
> 
> Perhaps, I miss something, but this revert and add the cleanup to the wrong
> place makes things worse:
> 
> The usb_stream_free() is called when init_urbs() fails (returns an error), so
> all urbs are freed there and pointers are reset to NULL. Your code frees urbs
> twice on an allocation error.
> 
> The reverted code does the job better.

Right, the suggested patch will cause the double-free, and the
reverted code is fine in this regard.

In anyway, the cleanup of this driver code is on my post-15.3 TODO
list.  So, Greg, could you drop the revert and the additional fix at
this round for usx2y driver?


Thanks!

Takashi

> 
> 						Jaroslav
> 
> > Cc: Takashi Iwai <tiwai@suse.de>
> > Cc: Jaroslav Kysela <perex@perex.cz>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  sound/usb/usx2y/usb_stream.c | 21 +++++++++++++++++++--
> >  1 file changed, 19 insertions(+), 2 deletions(-)
> > 
> > diff --git a/sound/usb/usx2y/usb_stream.c b/sound/usb/usx2y/usb_stream.c
> > index 6bba17bf689a..1091ea96a29a 100644
> > --- a/sound/usb/usx2y/usb_stream.c
> > +++ b/sound/usb/usx2y/usb_stream.c
> > @@ -88,18 +88,35 @@ static int init_urbs(struct usb_stream_kernel *sk, unsigned use_packsize,
> >  					sizeof(struct usb_stream_packet) *
> >  					s->inpackets;
> >  	int			u;
> > +	int			i;
> > +	int			err = -ENOMEM;
> >  
> >  	for (u = 0; u < USB_STREAM_NURBS; ++u) {
> > +		sk->outurb[u] = NULL;
> >  		sk->inurb[u] = usb_alloc_urb(sk->n_o_ps, GFP_KERNEL);
> > +		if (!sk->inurb[u])
> > +			goto error;
> >  		sk->outurb[u] = usb_alloc_urb(sk->n_o_ps, GFP_KERNEL);
> > +		if (!sk->outurb[u])
> > +			goto error;
> >  	}
> > +	u--;
> >  
> >  	if (init_pipe_urbs(sk, use_packsize, sk->inurb, indata, dev, in_pipe) ||
> >  	    init_pipe_urbs(sk, use_packsize, sk->outurb, sk->write_page, dev,
> > -			   out_pipe))
> > -		return -EINVAL;
> > +			   out_pipe)) {
> > +		err = -EINVAL;
> > +		goto error;
> > +	}
> >  
> >  	return 0;
> > +
> > +error:
> > +	for (i = 0; i <= u; ++i) {
> > +		usb_free_urb(sk->inurb[i]);
> > +		usb_free_urb(sk->outurb[i]);
> > +	}
> > +	return err;
> >  
> 
> -- 
> Jaroslav Kysela <perex@perex.cz>
> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
>