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: <Y4JEGYMtIWX9clxo@lunn.ch>
Date:   Sat, 26 Nov 2022 17:51:37 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Vincent Mailhol <mailhol.vincent@...adoo.fr>
Cc:     linux-can@...r.kernel.org, Marc Kleine-Budde <mkl@...gutronix.de>,
        linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        netdev@...r.kernel.org, linux-usb@...r.kernel.org,
        Saeed Mahameed <saeed@...nel.org>,
        Jiri Pirko <jiri@...dia.com>,
        Lukas Magel <lukas.magel@...teo.net>
Subject: Re: [PATCH v4 2/6] can: etas_es58x: add devlink support

> @@ -2196,11 +2198,12 @@ static struct es58x_device *es58x_init_es58x_dev(struct usb_interface *intf,
>  		ops = &es581_4_ops;
>  	}
>  
> -	es58x_dev = devm_kzalloc(dev, es58x_sizeof_es58x_device(param),
> -				 GFP_KERNEL);
> -	if (!es58x_dev)
> +	devlink = devlink_alloc(&es58x_dl_ops, es58x_sizeof_es58x_device(param),
> +				dev);
> +	if (!devlink)
>  		return ERR_PTR(-ENOMEM);
>  
> +	es58x_dev = devlink_priv(devlink);

That is 'interesting'. Makes me wonder about lifetimes of different
objects. Previously your es58x_dev structure would disappear when the
driver is released, or an explicit call to devm_kfree(). Now it
disappears when devlink_free() is called. Any danger of use after free
here?

USB devices always make me wonder about life times rules since they
are probably the mode dynamic sort of device the kernel has the
handle, them just abruptly disappearing.

>  	es58x_dev->param = param;
>  	es58x_dev->ops = ops;
>  	es58x_dev->dev = dev;
> @@ -2247,6 +2250,8 @@ static int es58x_probe(struct usb_interface *intf,
>  	if (ret)
>  		return ret;
>  
> +	devlink_register(priv_to_devlink(es58x_dev));
> +
>  	for (ch_idx = 0; ch_idx < es58x_dev->num_can_ch; ch_idx++) {
>  		ret = es58x_init_netdev(es58x_dev, ch_idx);
>  		if (ret) {
> @@ -2272,8 +2277,10 @@ static void es58x_disconnect(struct usb_interface *intf)
>  	dev_info(&intf->dev, "Disconnecting %s %s\n",
>  		 es58x_dev->udev->manufacturer, es58x_dev->udev->product);
>  
> +	devlink_unregister(priv_to_devlink(es58x_dev));
>  	es58x_free_netdevs(es58x_dev);
>  	es58x_free_urbs(es58x_dev);
> +	devlink_free(priv_to_devlink(es58x_dev));
>  	usb_set_intfdata(intf, NULL);

Should devlink_free() be after usb_set_inftdata()?

       Andrew

Powered by blists - more mailing lists