diff mbox series

[v3,03/12] usb: misc: cytherm: update to use usb_control_msg_recv()

Message ID 20210126183403.911653-4-anant.thazhemadam@gmail.com
State New
Headers show
Series drivers: usb: misc: update to use usb_control_msg_{send|recv}() API | expand

Commit Message

Anant Thazhemadam Jan. 26, 2021, 6:33 p.m. UTC 
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
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().

The return value checking enforced by callers of the updated function have
also been appropriately updated.

Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
---
 drivers/usb/misc/cytherm.c | 128 +++++++++++++------------------------
 1 file changed, 43 insertions(+), 85 deletions(-)

Comments

Johan Hovold Jan. 27, 2021, 2:21 p.m. UTC | #1 
On Wed, Jan 27, 2021 at 12:03:54AM +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

> 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().

> 

> The return value checking enforced by callers of the updated function have

> also been appropriately updated.

> 

> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>

> ---

>  drivers/usb/misc/cytherm.c | 128 +++++++++++++------------------------

>  1 file changed, 43 insertions(+), 85 deletions(-)

> 

> diff --git a/drivers/usb/misc/cytherm.c b/drivers/usb/misc/cytherm.c

> index 3e3802aaefa3..2ca36ea5b76a 100644

> --- a/drivers/usb/misc/cytherm.c

> +++ b/drivers/usb/misc/cytherm.c

> @@ -51,12 +51,12 @@ static int vendor_command(struct usb_device *dev, unsigned char request,

>  			  unsigned char value, unsigned char index,

>  			  void *buf, int size)

>  {

> -	return usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),

> -			       request, 

> -			       USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_OTHER,

> -			       value, 

> -			       index, buf, size,

> -			       USB_CTRL_GET_TIMEOUT);

> +	return usb_control_msg_recv(dev, 0,

> +				    request,

> +				    USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_OTHER,

> +				    value,

> +				    index, buf, size,

> +				    USB_CTRL_GET_TIMEOUT, GFP_KERNEL);

>  }

>  

>  

> @@ -78,33 +78,27 @@ static ssize_t brightness_store(struct device *dev, struct device_attribute *att

>  	struct usb_interface *intf = to_usb_interface(dev);

>  	struct usb_cytherm *cytherm = usb_get_intfdata(intf);

>  

> -	unsigned char *buffer;

> +	unsigned char buffer[8];

>  	int retval;

> -   

> -	buffer = kmalloc(8, GFP_KERNEL);

> -	if (!buffer)

> -		return 0;

>  

>  	cytherm->brightness = simple_strtoul(buf, NULL, 10);

> -   

> +

>  	if (cytherm->brightness > 0xFF)

>  		cytherm->brightness = 0xFF;

>  	else if (cytherm->brightness < 0)

>  		cytherm->brightness = 0;

> -   

> +

>  	/* Set brightness */

>  	retval = vendor_command(cytherm->udev, WRITE_RAM, BRIGHTNESS, 

> -				cytherm->brightness, buffer, 8);

> -	if (retval)

> -		dev_dbg(&cytherm->udev->dev, "retval = %d\n", retval);

> +				cytherm->brightness, &buffer, 8);

> +	if (!retval)

> +		dev_dbg(&cytherm->udev->dev, "brightness set correctly\n");

>  	/* Inform µC that we have changed the brightness setting */

>  	retval = vendor_command(cytherm->udev, WRITE_RAM, BRIGHTNESS_SEM,

> -				0x01, buffer, 8);

> -	if (retval)

> -		dev_dbg(&cytherm->udev->dev, "retval = %d\n", retval);

> -   

> -	kfree(buffer);

> -   

> +				0x01, &buffer, 8);

> +	if (!retval)

> +		dev_dbg(&cytherm->udev->dev, "µC informed of change in brightness setting\n");

> +

>  	return count;

>  }


This driver looks like it could have the same origin as the one touched
by the previous patch, and likewise this patch suffers from a similar
problem in that the driver always provides an 8-byte buffer but appears
to expect short reads (which would no be treated as errors).

You could consider adding the missing short read sanity checks, but
I'm afraid the new helpers are not a good fit here either.

Johan
diff mbox series

Patch

diff --git a/drivers/usb/misc/cytherm.c b/drivers/usb/misc/cytherm.c
index 3e3802aaefa3..2ca36ea5b76a 100644
--- a/drivers/usb/misc/cytherm.c
+++ b/drivers/usb/misc/cytherm.c
@@ -51,12 +51,12 @@  static int vendor_command(struct usb_device *dev, unsigned char request,
 			  unsigned char value, unsigned char index,
 			  void *buf, int size)
 {
-	return usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
-			       request, 
-			       USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_OTHER,
-			       value, 
-			       index, buf, size,
-			       USB_CTRL_GET_TIMEOUT);
+	return usb_control_msg_recv(dev, 0,
+				    request,
+				    USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_OTHER,
+				    value,
+				    index, buf, size,
+				    USB_CTRL_GET_TIMEOUT, GFP_KERNEL);
 }
 
 
@@ -78,33 +78,27 @@  static ssize_t brightness_store(struct device *dev, struct device_attribute *att
 	struct usb_interface *intf = to_usb_interface(dev);
 	struct usb_cytherm *cytherm = usb_get_intfdata(intf);
 
-	unsigned char *buffer;
+	unsigned char buffer[8];
 	int retval;
-   
-	buffer = kmalloc(8, GFP_KERNEL);
-	if (!buffer)
-		return 0;
 
 	cytherm->brightness = simple_strtoul(buf, NULL, 10);
-   
+
 	if (cytherm->brightness > 0xFF)
 		cytherm->brightness = 0xFF;
 	else if (cytherm->brightness < 0)
 		cytherm->brightness = 0;
-   
+
 	/* Set brightness */
 	retval = vendor_command(cytherm->udev, WRITE_RAM, BRIGHTNESS, 
-				cytherm->brightness, buffer, 8);
-	if (retval)
-		dev_dbg(&cytherm->udev->dev, "retval = %d\n", retval);
+				cytherm->brightness, &buffer, 8);
+	if (!retval)
+		dev_dbg(&cytherm->udev->dev, "brightness set correctly\n");
 	/* Inform µC that we have changed the brightness setting */
 	retval = vendor_command(cytherm->udev, WRITE_RAM, BRIGHTNESS_SEM,
-				0x01, buffer, 8);
-	if (retval)
-		dev_dbg(&cytherm->udev->dev, "retval = %d\n", retval);
-   
-	kfree(buffer);
-   
+				0x01, &buffer, 8);
+	if (!retval)
+		dev_dbg(&cytherm->udev->dev, "µC informed of change in brightness setting\n");
+
 	return count;
 }
 static DEVICE_ATTR_RW(brightness);
@@ -120,28 +114,22 @@  static ssize_t temp_show(struct device *dev, struct device_attribute *attr, char
 	struct usb_cytherm *cytherm = usb_get_intfdata(intf);
 
 	int retval;
-	unsigned char *buffer;
+	unsigned char buffer[8];
 
 	int temp, sign;
    
-	buffer = kmalloc(8, GFP_KERNEL);
-	if (!buffer)
-		return 0;
-
 	/* read temperature */
-	retval = vendor_command(cytherm->udev, READ_RAM, TEMP, 0, buffer, 8);
-	if (retval)
-		dev_dbg(&cytherm->udev->dev, "retval = %d\n", retval);
+	retval = vendor_command(cytherm->udev, READ_RAM, TEMP, 0, &buffer, 8);
+	if (!retval)
+		dev_dbg(&cytherm->udev->dev, "read temperature successfully\n");
 	temp = buffer[1];
    
 	/* read sign */
-	retval = vendor_command(cytherm->udev, READ_RAM, SIGN, 0, buffer, 8);
-	if (retval)
-		dev_dbg(&cytherm->udev->dev, "retval = %d\n", retval);
+	retval = vendor_command(cytherm->udev, READ_RAM, SIGN, 0, &buffer, 8);
+	if (!retval)
+		dev_dbg(&cytherm->udev->dev, "read sign successfully\n");
 	sign = buffer[1];
 
-	kfree(buffer);
-   
 	return sprintf(buf, "%c%i.%i", sign ? '-' : '+', temp >> 1,
 		       5*(temp - ((temp >> 1) << 1)));
 }
@@ -157,21 +145,15 @@  static ssize_t button_show(struct device *dev, struct device_attribute *attr, ch
 	struct usb_cytherm *cytherm = usb_get_intfdata(intf);
 
 	int retval;
-	unsigned char *buffer;
-
-	buffer = kmalloc(8, GFP_KERNEL);
-	if (!buffer)
-		return 0;
+	unsigned char buffer[8];
 
 	/* check button */
-	retval = vendor_command(cytherm->udev, READ_RAM, BUTTON, 0, buffer, 8);
-	if (retval)
-		dev_dbg(&cytherm->udev->dev, "retval = %d\n", retval);
+	retval = vendor_command(cytherm->udev, READ_RAM, BUTTON, 0, &buffer, 8);
+	if (!retval)
+		dev_dbg(&cytherm->udev->dev, "checked button successfully\n");
    
 	retval = buffer[1];
 
-	kfree(buffer);
-
 	if (retval)
 		return sprintf(buf, "1");
 	else
@@ -186,20 +168,14 @@  static ssize_t port0_show(struct device *dev, struct device_attribute *attr, cha
 	struct usb_cytherm *cytherm = usb_get_intfdata(intf);
 
 	int retval;
-	unsigned char *buffer;
-
-	buffer = kmalloc(8, GFP_KERNEL);
-	if (!buffer)
-		return 0;
+	unsigned char buffer[8];
 
-	retval = vendor_command(cytherm->udev, READ_PORT, 0, 0, buffer, 8);
-	if (retval)
+	retval = vendor_command(cytherm->udev, READ_PORT, 0, 0, &buffer, 8);
+	if (!retval)
 		dev_dbg(&cytherm->udev->dev, "retval = %d\n", retval);
 
 	retval = buffer[1];
 
-	kfree(buffer);
-
 	return sprintf(buf, "%d", retval);
 }
 
@@ -209,28 +185,22 @@  static ssize_t port0_store(struct device *dev, struct device_attribute *attr, co
 	struct usb_interface *intf = to_usb_interface(dev);
 	struct usb_cytherm *cytherm = usb_get_intfdata(intf);
 
-	unsigned char *buffer;
+	unsigned char buffer[8];
 	int retval;
 	int tmp;
-   
-	buffer = kmalloc(8, GFP_KERNEL);
-	if (!buffer)
-		return 0;
 
 	tmp = simple_strtoul(buf, NULL, 10);
-   
+
 	if (tmp > 0xFF)
 		tmp = 0xFF;
 	else if (tmp < 0)
 		tmp = 0;
-   
+
 	retval = vendor_command(cytherm->udev, WRITE_PORT, 0,
-				tmp, buffer, 8);
-	if (retval)
+				tmp, &buffer, 8);
+	if (!retval)
 		dev_dbg(&cytherm->udev->dev, "retval = %d\n", retval);
 
-	kfree(buffer);
-
 	return count;
 }
 static DEVICE_ATTR_RW(port0);
@@ -241,19 +211,13 @@  static ssize_t port1_show(struct device *dev, struct device_attribute *attr, cha
 	struct usb_cytherm *cytherm = usb_get_intfdata(intf);
 
 	int retval;
-	unsigned char *buffer;
+	unsigned char buffer[8];
 
-	buffer = kmalloc(8, GFP_KERNEL);
-	if (!buffer)
-		return 0;
-
-	retval = vendor_command(cytherm->udev, READ_PORT, 1, 0, buffer, 8);
-	if (retval)
+	retval = vendor_command(cytherm->udev, READ_PORT, 1, 0, &buffer, 8);
+	if (!retval)
 		dev_dbg(&cytherm->udev->dev, "retval = %d\n", retval);
-   
-	retval = buffer[1];
 
-	kfree(buffer);
+	retval = buffer[1];
 
 	return sprintf(buf, "%d", retval);
 }
@@ -264,13 +228,9 @@  static ssize_t port1_store(struct device *dev, struct device_attribute *attr, co
 	struct usb_interface *intf = to_usb_interface(dev);
 	struct usb_cytherm *cytherm = usb_get_intfdata(intf);
 
-	unsigned char *buffer;
+	unsigned char buffer[8];
 	int retval;
 	int tmp;
-   
-	buffer = kmalloc(8, GFP_KERNEL);
-	if (!buffer)
-		return 0;
 
 	tmp = simple_strtoul(buf, NULL, 10);
    
@@ -278,14 +238,12 @@  static ssize_t port1_store(struct device *dev, struct device_attribute *attr, co
 		tmp = 0xFF;
 	else if (tmp < 0)
 		tmp = 0;
-   
+
 	retval = vendor_command(cytherm->udev, WRITE_PORT, 1,
-				tmp, buffer, 8);
-	if (retval)
+				tmp, &buffer, 8);
+	if (!retval)
 		dev_dbg(&cytherm->udev->dev, "retval = %d\n", retval);
 
-	kfree(buffer);
-
 	return count;
 }
 static DEVICE_ATTR_RW(port1);