Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Message-ID: <973e87e1-13a5-40d2-9917-ed1984fb9edd@gmail.com>
Date:   Sat, 14 Oct 2023 09:57:55 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] Input: bcm5974 - check endpoint type before starting
 traffic
To:     Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc:     John Horan <knasher@gmail.com>,
        Henrik Rydberg <rydberg@bitmath.org>,
        linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
        syzbot+348331f63b034f89b622@syzkaller.appspotmail.com
References: <20231007-topic-bcm5974_bulk-v2-1-021131c83efb@gmail.com>
 <ZSoFDLv8_CG1SGN6@google.com>
Content-Language: en-US
From:   Javier Carrasco <javier.carrasco.cruz@gmail.com>
In-Reply-To: <ZSoFDLv8_CG1SGN6@google.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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