Received-SPF: pass (google.com: domain of linux-kernel+bounces-200722-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 4 Jun 2024 15:35:55 +0200
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
Cc: accessrunner-general@lists.sourceforge.net, linux-usb@vger.kernel.org,
	linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com,
	syzbot+00c18ee8497dd3be6ade@syzkaller.appspotmail.com
Subject: Re: [PATCH] usb: atm: cxacru: fix endpoint checking in cxacru_bind()
Message-ID: <2024060415-cloud-calcium-ca2f@gregkh>
References: <20240528183807.3832-1-n.zhandarovich@fintech.ru>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240528183807.3832-1-n.zhandarovich@fintech.ru>

On Tue, May 28, 2024 at 11:38:07AM -0700, Nikita Zhandarovich wrote:
> Syzbot is still reporting quite an old issue [1] that occurs due to
> incomplete checking of present usb endpoints. As such, wrong
> endpoints types may be used at urb sumbitting stage which in turn
> triggers a warning in usb_submit_urb().
> 
> Fix the issue by verifying that required endpoint types are present
> for both in and out endpoints, taking into account cmd endpoint type.
> 
> Unfortunately, this patch has not been tested on real hardware.
> 
> [1] Syzbot report:
> usb 1-1: BOGUS urb xfer, pipe 1 != type 3
> WARNING: CPU: 0 PID: 8667 at drivers/usb/core/urb.c:502 usb_submit_urb+0xed2/0x18a0 drivers/usb/core/urb.c:502
> Modules linked in:
> CPU: 0 PID: 8667 Comm: kworker/0:4 Not tainted 5.14.0-rc4-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Workqueue: usb_hub_wq hub_event
> RIP: 0010:usb_submit_urb+0xed2/0x18a0 drivers/usb/core/urb.c:502
> ...
> Call Trace:
>  cxacru_cm+0x3c0/0x8e0 drivers/usb/atm/cxacru.c:649
>  cxacru_card_status+0x22/0xd0 drivers/usb/atm/cxacru.c:760
>  cxacru_bind+0x7ac/0x11a0 drivers/usb/atm/cxacru.c:1209
>  usbatm_usb_probe+0x321/0x1ae0 drivers/usb/atm/usbatm.c:1055
>  cxacru_usb_probe+0xdf/0x1e0 drivers/usb/atm/cxacru.c:1363
>  usb_probe_interface+0x315/0x7f0 drivers/usb/core/driver.c:396
>  call_driver_probe drivers/base/dd.c:517 [inline]
>  really_probe+0x23c/0xcd0 drivers/base/dd.c:595
>  __driver_probe_device+0x338/0x4d0 drivers/base/dd.c:747
>  driver_probe_device+0x4c/0x1a0 drivers/base/dd.c:777
>  __device_attach_driver+0x20b/0x2f0 drivers/base/dd.c:894
>  bus_for_each_drv+0x15f/0x1e0 drivers/base/bus.c:427
>  __device_attach+0x228/0x4a0 drivers/base/dd.c:965
>  bus_probe_device+0x1e4/0x290 drivers/base/bus.c:487
>  device_add+0xc2f/0x2180 drivers/base/core.c:3354
>  usb_set_configuration+0x113a/0x1910 drivers/usb/core/message.c:2170
>  usb_generic_driver_probe+0xba/0x100 drivers/usb/core/generic.c:238
>  usb_probe_device+0xd9/0x2c0 drivers/usb/core/driver.c:293
> 
> Reported-and-tested-by: syzbot+00c18ee8497dd3be6ade@syzkaller.appspotmail.com
> Fixes: 902ffc3c707c ("USB: cxacru: Use a bulk/int URB to access the command endpoint")
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
> ---
> P.S. While the driver is orphaned, it might still make sense to
> suppress the syzbot report, seeing how ancient it is.
> P.P.S. checkpatch complains about outdated format of debug printing
> but I decided to keep it in tune with the rest of the driver. 
> 
>  drivers/usb/atm/cxacru.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c
> index 4ce7cba2b48a..8a8e94a601c6 100644
> --- a/drivers/usb/atm/cxacru.c
> +++ b/drivers/usb/atm/cxacru.c
> @@ -1131,7 +1131,8 @@ static int cxacru_bind(struct usbatm_data *usbatm_instance,
>  	struct cxacru_data *instance;
>  	struct usb_device *usb_dev = interface_to_usbdev(intf);
>  	struct usb_host_endpoint *cmd_ep = usb_dev->ep_in[CXACRU_EP_CMD];
> -	int ret;
> +	struct usb_endpoint_descriptor *in, *out;
> +	int ret = -1;

Why initialize this and then write over it?

Also, -1 is not a valid return value, so even if this was needed, it's
not correct :(


>  
>  	/* instance init */
>  	instance = kzalloc(sizeof(*instance), GFP_KERNEL);
> @@ -1177,6 +1178,19 @@ static int cxacru_bind(struct usbatm_data *usbatm_instance,
>  		goto fail;
>  	}
>  
> +	if (usb_endpoint_xfer_int(&cmd_ep->desc))
> +		ret = usb_find_common_endpoints(intf->cur_altsetting,
> +						NULL, NULL, &in, &out);
> +	else
> +		ret = usb_find_common_endpoints(intf->cur_altsetting,
> +						&in, &out, NULL, NULL);
> +
> +	if (ret) {
> +		usb_dbg(usbatm_instance, "cxacru_bind: interface has incorrect endpoints\n");

Shouldn't this be an error instead?

thanks,

greg k-h