Date:   Mon, 20 Dec 2021 00:19:53 -0800
From:   Dmitry Torokhov <dmitry.torokhov@gmail.com>
To:     Dongliang Mu <mudongliangabcd@gmail.com>
Cc:     Greg Kroah-Hartman <gregkh@suse.de>,
        syzbot+9780d2b05ac158d32284@syzkaller.appspotmail.com,
        Dmitry Torokhov <dtor@insightbb.com>,
        linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] driver: powermate: kill urb in the disconnect function
Message-ID: <YcA8qX3irdSSJnN+@google.com>
References: <20211219060241.4125076-1-mudongliangabcd@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211219060241.4125076-1-mudongliangabcd@gmail.com>
Precedence: bulk

Hi Dongliang,

On Sun, Dec 19, 2021 at 02:02:15PM +0800, Dongliang Mu wrote:
> In powermate_disconnect, powermate_pulse_led will invoke
> powermate_sync_state and submit one urb with pm as its context.
> If powermate disconnect before the execution of complete handler,
> the pm will become a dangling pointer and lead to UAF.
> 
> Fix this by calling usb_kill_urb(pm->config) in the disconnect function.
> Note that, the error handling error does not need to take care of this.

Unfortunately this analysis is not correct and adding usb_kill_urb()
does not really help, as while input device is registered there may be
new events coming into the driver that may cause config URB be submitted
again.

What actually happens is that call to usb_free_urb(pm->config) below
tries to properly shutdown and destroy URB, but unfortunately
powermate_config_complete() handler only uses urb->status to print error
message, and resubmits the URB that is about to be freed again.

The URB completion handler needs to check for -ESHUTDOWN (-108) and
other terminal error codes before attempting to resubmit the URB.

> 
> Reported-by: syzbot+9780d2b05ac158d32284@syzkaller.appspotmail.com
> Fixes: ba0acb5ee318901 ("Input: move USB miscellaneous devices under drivers/input/misc")
> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> ---
>  drivers/input/misc/powermate.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/input/misc/powermate.c b/drivers/input/misc/powermate.c
> index c4e0e1886061..903993469fde 100644
> --- a/drivers/input/misc/powermate.c
> +++ b/drivers/input/misc/powermate.c
> @@ -424,6 +424,7 @@ static void powermate_disconnect(struct usb_interface *intf)
>  	if (pm) {
>  		pm->requires_update = 0;
>  		usb_kill_urb(pm->irq);
> +		usb_kill_urb(pm->config);
>  		input_unregister_device(pm->input);
>  		usb_free_urb(pm->irq);
>  		usb_free_urb(pm->config);
> -- 
> 2.25.1
> 

Thanks.

-- 
Dmitry