Re: [PATCH] usb: gadget: f_ncm: Fix UAF ncm object at re-bind after usb ep transport error

[Greg KH]

On Mon, Mar 25, 2024 at 06:45:43PM +0900, Norihiko Hama wrote:
> When ncm function is working and then stop usb0 interface for link down,
> eth_stop() is called. At this piont, accidentally if usb transport error
> should happen in usb_ep_enable(), 'in_ep' and/or 'out_ep' may not be enabled.
> 
> After that, ncm_disable() is called to disable for ncm unbind
> but gether_disconnect() is never called since 'in_ep' is not enabled.
> 
> As the result, ncm object is released in ncm unbind
> but 'dev->port_usb' associated to 'ncm->port' is not NULL.
> 
> And when ncm bind again to recover netdev, ncm object is reallocated
> but usb0 interface is already associated to previous released ncm object.
> 
> Therefore, once usb0 interface is up and eth_start_xmit() is called,
> released ncm object is dereferrenced and it might cause use-after-free memory.
> 
> [function unlink via configfs]
> usb0: eth_stop dev->port_usb=ffffff9b179c3200
> --> error happens in usb_ep_enable().
> NCM: ncm_disable: ncm=ffffff9b179c3200
> --> no gether_disconnect() since ncm->port.in_ep->enabled is false.
> NCM: ncm_unbind: ncm unbind ncm=ffffff9b179c3200
> NCM: ncm_free: ncm free ncm=ffffff9b179c3200   <-- released ncm
> 
> [function link via configfs]
> NCM: ncm_alloc: ncm alloc ncm=ffffff9ac4f8a000
> NCM: ncm_bind: ncm bind ncm=ffffff9ac4f8a000
> NCM: ncm_set_alt: ncm=ffffff9ac4f8a000 alt=0
> usb0: eth_open dev->port_usb=ffffff9b179c3200  <-- previous released ncm
> usb0: eth_start dev->port_usb=ffffff9b179c3200 <--
> 
> Unable to handle kernel paging request at virtual address dead00000000014f
> 
> This patch addresses the issue by checking if 'ncm->netdev' is not NULL at
> ncm_disable() to call gether_disconnect() to deassociate 'dev->port_usb'.
> It's more reasonable to check 'ncm->netdev' to call gether_connect/disconnect
> rather than check 'ncm->port.in_ep->enabled' since it might not be enabled
> but the gether connection might be established.
> 
> Signed-off-by: Norihiko Hama <Norihiko.Hama@xxxxxxxxxxxxxx>
> ---
>  drivers/usb/gadget/function/f_ncm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
> index bd095ae569ed..23960cd16463 100644
> --- a/drivers/usb/gadget/function/f_ncm.c
> +++ b/drivers/usb/gadget/function/f_ncm.c
> @@ -888,7 +888,7 @@ static int ncm_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>  		if (alt > 1)
>  			goto fail;
>  
> -		if (ncm->port.in_ep->enabled) {
> +		if (ncm->netdev) {
>  			DBG(cdev, "reset ncm\n");
>  			ncm->netdev = NULL;
>  			gether_disconnect(&ncm->port);
> @@ -1365,7 +1365,7 @@ static void ncm_disable(struct usb_function *f)
>  
>  	DBG(cdev, "ncm deactivated\n");
>  
> -	if (ncm->port.in_ep->enabled) {
> +	if (ncm->netdev) {
>  		ncm->netdev = NULL;
>  		gether_disconnect(&ncm->port);
>  	}
> -- 
> 2.17.1
> 

What commit id does this change fix?

thanks,

greg k-h