linux-kernel - Re: [PATCH v3 02/12] usb: misc: cypress_cy7c63: update to use usb_control_msg

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]

Date:   Wed, 27 Jan 2021 20:10:45 +0530
From:   Anant Thazhemadam <anant.thazhemadam@...il.com>
To:     Johan Hovold <johan@...nel.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 02/12] usb: misc: cypress_cy7c63: update to use
 usb_control_msg_recv()


On 27/01/21 7:39 pm, Johan Hovold wrote:
> On Wed, Jan 27, 2021 at 12:03:53AM +0530, Anant Thazhemadam wrote:
>> The newer usb_control_msg_{send|recv}() API are an improvement on the
>> existing usb_control_msg() as it ensures that a short read/write is treated
> Short write has always been an error (I won't repeat for the remaining
> patches).
>
>> as an error, data can be used off the stack, and raw usb pipes need not be
>> created in the calling functions.
>> For this reason, the instance of usb_control_msg() has been replaced with
>> usb_control_msg_recv().
>>
>> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@...il.com>
>> ---
>>  drivers/usb/misc/cypress_cy7c63.c | 21 +++++----------------
>>  1 file changed, 5 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/usb/misc/cypress_cy7c63.c b/drivers/usb/misc/cypress_cy7c63.c
>> index 14faec51d7a5..76a320ef17a7 100644
>> --- a/drivers/usb/misc/cypress_cy7c63.c
>> +++ b/drivers/usb/misc/cypress_cy7c63.c
>> @@ -70,24 +70,15 @@ static int vendor_command(struct cypress *dev, unsigned char request,
>>  			  unsigned char address, unsigned char data)
>>  {
>>  	int retval = 0;
>> -	unsigned int pipe;
>> -	unsigned char *iobuf;
>> -
>> -	/* allocate some memory for the i/o buffer*/
>> -	iobuf = kzalloc(CYPRESS_MAX_REQSIZE, GFP_KERNEL);
>> -	if (!iobuf) {
>> -		retval = -ENOMEM;
>> -		goto error;
>> -	}
>> +	u8 iobuf[CYPRESS_MAX_REQSIZE] = {0};
>>  
>>  	dev_dbg(&dev->udev->dev, "Sending usb_control_msg (data: %d)\n", data);
>>  
>>  	/* prepare usb control message and send it upstream */
>> -	pipe = usb_rcvctrlpipe(dev->udev, 0);
>> -	retval = usb_control_msg(dev->udev, pipe, request,
>> -				 USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_OTHER,
>> -				 address, data, iobuf, CYPRESS_MAX_REQSIZE,
>> -				 USB_CTRL_GET_TIMEOUT);
>> +	retval = usb_control_msg_recv(dev->udev, 0, request,
>> +				      USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_OTHER,
>> +				      address, data, &iobuf, CYPRESS_MAX_REQSIZE,
>> +				      USB_CTRL_GET_TIMEOUT, GFP_KERNEL);
> Are you sure that the device always returns CYPRESS_MAX_REQSIZE here?
> Otherwise, this change may break the driver as it currently only uses
> the first two bytes of the received message, and only for some requests.
>
> Note that the driver appears uses the same helper function for
> CYPRESS_WRITE_PORT commands, which probably doesn't return 8 bytes in a
> reply.
>
> You could possibly add the missing short read check for the
> CYPRESS_READ_PORT case, but I'm afraid that the new helper are not a
> good fit here either.
>

Understood, but I think that change might be better proposed (for this, and cytherm, both)
separately from this series, so I'll just drop it from this series for now.

Thanks,
Anant