Re: [PATCH 1/2] usb: gadget: u_ether: Do not make UDC parent of the net device

[Ondřej Jirman]

Hi,

On Fri, Nov 04, 2022 at 02:10:30PM +0100, Sascha Hauer wrote:
> ing-List: linux-kernel@xxxxxxxxxxxxxxx
> 
> The UDC is not a suitable parent of the net device as the UDC can
> change or vanish during the lifecycle of the ethernet gadget. This
> can be illustrated with the following:
> 
> mkdir -p /sys/kernel/config/usb_gadget/mygadget
> cd /sys/kernel/config/usb_gadget/mygadget
> mkdir -p configs/c.1/strings/0x409
> echo "C1:Composite Device" > configs/c.1/strings/0x409/configuration
> mkdir -p functions/ecm.usb0
> ln -s functions/ecm.usb0 configs/c.1/
> echo "dummy_udc.0" > UDC
> rmmod dummy_hcd
> 
> The 'rmmod' removes the UDC from the just created gadget, leaving
> the still existing net device with a no longer existing parent.

I have an even simpler reproducer on Pinephone Pro/RK3399 SoC. All it takes to
trigger the use after free and kernel panic in my case is to plug in a USB dock
to make DWC3 DRD switch to host mode and then unplug it to make it switch back to
peripheral mode.

This triggers a call to dwc3_gadget_exit and then to dwc3_gadget_init later on

https://elixir.bootlin.com/linux/latest/source/drivers/usb/dwc3/gadget.c#L4546

Then symlink to the net device becames broken in sysfs:

  https://megous.com/dl/tmp/3d8061f1749a7b2b.png

And after this happens, there's a kernel panic when removing the rndis gadget
configuration from configfs: https://paste.mozilla.org/Z5DFP9BV
(and possibly other issues, but the kernel panic is the most noticable :))

Applaying this patch makes the issue go away. So there definitely seems to
be some device lifetime issue somewhere in there.

kind regards,
	o.

> Accessing the ethernet device with commands like:
> 
> ip --details link show usb0
> 
> will result in a KASAN splat:
> 
> ==================================================================
> BUG: KASAN: use-after-free in if_nlmsg_size+0x3e8/0x528
> Read of size 4 at addr c5c84754 by task ip/357
> 
> CPU: 3 PID: 357 Comm: ip Not tainted 6.1.0-rc3-00013-gd14953726b24-dirty #324
> Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
>  unwind_backtrace from show_stack+0x10/0x14
>  show_stack from dump_stack_lvl+0x58/0x70
>  dump_stack_lvl from print_report+0x134/0x4d4
>  print_report from kasan_report+0x78/0x10c
>  kasan_report from if_nlmsg_size+0x3e8/0x528
>  if_nlmsg_size from rtnl_getlink+0x2b4/0x4d0
>  rtnl_getlink from rtnetlink_rcv_msg+0x1f4/0x674
>  rtnetlink_rcv_msg from netlink_rcv_skb+0xb4/0x1f8
>  netlink_rcv_skb from netlink_unicast+0x294/0x478
>  netlink_unicast from netlink_sendmsg+0x328/0x640
>  netlink_sendmsg from ____sys_sendmsg+0x2a4/0x3b4
>  ____sys_sendmsg from ___sys_sendmsg+0xc8/0x12c
>  ___sys_sendmsg from sys_sendmsg+0xa0/0x120
>  sys_sendmsg from ret_fast_syscall+0x0/0x1c
> 
> Solve this by not setting the parent of the ethernet device.
> 
> Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> ---
>  drivers/usb/gadget/function/u_ether.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
> index e06022873df16..8f12f3f8f6eeb 100644
> --- a/drivers/usb/gadget/function/u_ether.c
> +++ b/drivers/usb/gadget/function/u_ether.c
> @@ -798,7 +798,6 @@ struct eth_dev *gether_setup_name(struct usb_gadget *g,
>  	net->max_mtu = GETHER_MAX_MTU_SIZE;
>  
>  	dev->gadget = g;
> -	SET_NETDEV_DEV(net, &g->dev);
>  	SET_NETDEV_DEVTYPE(net, &gadget_type);
>  
>  	status = register_netdev(net);
> @@ -873,8 +872,6 @@ int gether_register_netdev(struct net_device *net)
>  	struct usb_gadget *g;
>  	int status;
>  
> -	if (!net->dev.parent)
> -		return -EINVAL;
>  	dev = netdev_priv(net);
>  	g = dev->gadget;
>  
> @@ -905,7 +902,6 @@ void gether_set_gadget(struct net_device *net, struct usb_gadget *g)
>  
>  	dev = netdev_priv(net);
>  	dev->gadget = g;
> -	SET_NETDEV_DEV(net, &g->dev);
>  }
>  EXPORT_SYMBOL_GPL(gether_set_gadget);
>  
> -- 
> 2.30.2
>