linux-kernel - Re: [PATCH] usb: fixing some clang warnings inside usb host drivers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0804469c-664a-219d-bb6a-b4e5f133edd9@i-love.sakura.ne.jp>
Date:   Sun, 19 Dec 2021 16:59:44 +0900
From:   Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     Julio Faracco <jcfaracco@...il.com>, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org, gregkh@...uxfoundation.org,
        axboe@...nel.dk, tglx@...utronix.de, damien.lemoal@....com,
        dkadashev@...il.com, paul.gortmaker@...driver.com,
        zhouyanjie@...yeetech.com, niklas.cassel@....com,
        macro@...am.me.uk, caihuoqing@...du.com
Subject: Re: [PATCH] usb: fixing some clang warnings inside usb host drivers

On 2021/12/19 11:50, Alan Stern wrote:
> You should read this code in usb_submit_urb():
> 
> 	max = usb_endpoint_maxp(&ep->desc);
> 	if (max <= 0) {
> 		dev_dbg(&dev->dev,
> 			"bogus endpoint ep%d%s in %s (bad maxpacket %d)\n",
> 			usb_endpoint_num(&ep->desc), is_out ? "out" : "in",
> 			__func__, max);
> 		return -EMSGSIZE;
> 	}
> 
> As far as I know, every code path leading to qtd_fill() has to pass this 
> test.

Excuse me, but surely qtd_fill() is using the result from usb_maxpacket()

----------------------------------------
static struct list_head *
qh_urb_transaction (
	struct ehci_hcd		*ehci,
	struct urb		*urb,
	struct list_head	*head,
	gfp_t			flags
) {
(...snipped...)
	maxpacket = usb_maxpacket(urb->dev, urb->pipe, !is_input);

	/*
	 * buffer gets wrapped in one or more qtds;
	 * last one may be "short" (including zero len)
	 * and may serve as a control status ack
	 */
	for (;;) {
		int this_qtd_len;

		this_qtd_len = qtd_fill(ehci, qtd, buf, this_sg_len, token,
				maxpacket);
		this_sg_len -= this_qtd_len;
		len -= this_qtd_len;
		buf += this_qtd_len;
(...snipped...)
}
----------------------------------------

and usb_maxpacket() may return 0 ?

----------------------------------------
static inline __u16
usb_maxpacket(struct usb_device *udev, int pipe, int is_out)
{
	struct usb_host_endpoint	*ep;
	unsigned			epnum = usb_pipeendpoint(pipe);

	if (is_out) {
		WARN_ON(usb_pipein(pipe));
		ep = udev->ep_out[epnum];
	} else {
		WARN_ON(usb_pipeout(pipe));
		ep = udev->ep_in[epnum];
	}
	if (!ep)
		return 0;

	/* NOTE:  only 0x07ff bits are for packet size... */
	return usb_endpoint_maxp(&ep->desc);
}
----------------------------------------

If we don't need to care about the possibility of returning 0 (including
all possible race conditions taken into account), please explain it as a
comment block.