linux-kernel - Re: [PATCH v2] Input: bcm5974 - check endpoint type before starting traffic

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] [day] [month] [year] [list]

Message-ID: <973e87e1-13a5-40d2-9917-ed1984fb9edd@gmail.com>
Date:   Sat, 14 Oct 2023 09:57:55 +0200
From:   Javier Carrasco <javier.carrasco.cruz@...il.com>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     John Horan <knasher@...il.com>,
        Henrik Rydberg <rydberg@...math.org>,
        linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
        syzbot+348331f63b034f89b622@...kaller.appspotmail.com
Subject: Re: [PATCH v2] Input: bcm5974 - check endpoint type before starting
 traffic

Hi Dmitry,

On 14.10.23 05:03, Dmitry Torokhov wrote:
> Hi Javier,
> 
> On Thu, Oct 12, 2023 at 06:51:49PM +0200, Javier Carrasco wrote:
>>  
>> +static bool bcm5974_ep_is_int_in(struct usb_host_interface *iface, int addr)
>> +{
>> +	struct usb_endpoint_descriptor *endpoint;
>> +	int i;
>> +
>> +	for (i = 0; i < iface->desc.bNumEndpoints; i++) {
>> +		endpoint = &iface->endpoint[i].desc;
>> +		if (endpoint->bEndpointAddress == addr) {
>> +			if (usb_endpoint_is_int_in(endpoint))
>> +				return true;
>> +		}
>> +	}
>> +	return false;
>> +}
> 
> This essentially reimplements usb_find_endpoint() in a sense, so can we
> instead do:
> 
> 	ep = usb_find_endpoint(iface, addr);
> 	if (!ep || !usb_endpoint_is_int_in(ep)) {
> 		dev_err(...);
> 		return ...;
> 	}
> 
Thanks for your feedback. usb_find_endpoint is a static function from
the usb core and in principle is not available here, but your code
snippet seems to reinterpret usb_check_int_endpoints() for a single
address. I would suggest using usb_check_int_endpoints and pass both
addresses at the same time (if both exist, of course).
> 
> Also it looks like the handling of button endpoint is interleaved with
> the trackpad endpoint, I wonder if it would not be better if we have a
> separate "if (cfg->tp_type == TYPE1)" where we would do the check,
> allocate URB, and did all the rest of set up for button transfers.
Using usb_check_int_endpoints would solve this immediately.
> 
> Thanks.
> 
Thanks and best regards,
Javier Carrasco