linux-kernel - Re: [PATCH] net: hso: register netdev later to avoid a race condition

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170428193629.4f72caed@aktux>
Date:   Fri, 28 Apr 2017 19:36:29 +0200
From:   Andreas Kemnade <andreas@...nade.info>
To:     Johan Hovold <johan@...nel.org>
Cc:     davem@...emloft.net, joe@...ches.com, gregkh@...uxfoundation.org,
        peter@...leysoftware.com, hns@...delico.com,
        linux-usb@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: hso: register netdev later to avoid a race
 condition

On Thu, 27 Apr 2017 10:44:01 +0200
Johan Hovold <johan@...nel.org> wrote:

> On Wed, Apr 26, 2017 at 07:26:40PM +0200, Andreas Kemnade wrote:
> > If the netdev is accessed before the urbs are initialized,
> > there will be NULL pointer dereferences. That is avoided by
> > registering it when it is fully initialized.
> 
> > Reported-by: H. Nikolaus Schaller <hns@...delico.com>
> > Signed-off-by: Andreas Kemnade <andreas@...nade.info>
> > ---
> >  drivers/net/usb/hso.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> > index 93411a3..00067a0 100644
> > --- a/drivers/net/usb/hso.c
> > +++ b/drivers/net/usb/hso.c
> > @@ -2534,13 +2534,6 @@ static struct hso_device *hso_create_net_device(struct usb_interface *interface,
> >  	SET_NETDEV_DEV(net, &interface->dev);
> >  	SET_NETDEV_DEVTYPE(net, &hso_type);
> >  
> > -	/* registering our net device */
> > -	result = register_netdev(net);
> > -	if (result) {
> > -		dev_err(&interface->dev, "Failed to register device\n");
> > -		goto exit;
> > -	}
> > -
> >  	/* start allocating */
> >  	for (i = 0; i < MUX_BULK_RX_BUF_COUNT; i++) {
> >  		hso_net->mux_bulk_rx_urb_pool[i] = usb_alloc_urb(0, GFP_KERNEL);
> > @@ -2560,6 +2553,13 @@ static struct hso_device *hso_create_net_device(struct usb_interface *interface,
> >  
> >  	add_net_device(hso_dev);
> >  
> > +	/* registering our net device */
> > +	result = register_netdev(net);
> > +	if (result) {
> > +		dev_err(&interface->dev, "Failed to register device\n");
> > +		goto exit;
> 
> This all looks good, but you should consider cleaning up the error
> handling of this function as a follow-up as we should not be
> deregistering netdevs that have never been registered (e.g. if a
> required endpoint is missing or if registration fails for some reason).
> 
> But just to be clear, this problem existed also before this change.
> 
Just to check wether I am understanding this correctly. In your opinion
this patch is good for now. And later when it is applied, there should
be an additional error handling cleanup patch.

Regards,
Andreas

Content of type "application/pgp-signature" skipped