Received-SPF: pass (google.com: domain of linux-kernel+bounces-118693-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 26 Mar 2024 10:36:40 +0100
From: Greg KH <gregkh@linuxfoundation.org>
To: Norihiko Hama <Norihiko.Hama@alpsalpine.com>
Cc: balbi@kernel.org, linux-usb@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] usb: gadget: f_ncm: Fix UAF ncm object at re-bind after
 usb ep transport error
Message-ID: <2024032629-frolic-obtain-ad55@gregkh>
References: <20240325094543.5362-1-Norihiko.Hama@alpsalpine.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240325094543.5362-1-Norihiko.Hama@alpsalpine.com>

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@alpsalpine.com>
> ---
>  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