[PATCH v4 2/3] usb: misc: ezusb: update to use usb_control_msg_send()

[Anant Thazhemadam]

The newer usb_control_msg_{send|recv}() API ensures that a short read 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_send() appropriately.

Signed-off-by: Anant Thazhemadam 
Reviewed-by: Johan Hovold 
---
 drivers/usb/misc/ezusb.c | 16 ++--
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/misc/ezusb.c b/drivers/usb/misc/ezusb.c
index f058d8029761..78aaee56c2b7 100644
--- a/drivers/usb/misc/ezusb.c
+++ b/drivers/usb/misc/ezusb.c
@@ -31,24 +31,12 @@ static const struct ezusb_fx_type ezusb_fx1 = {
 static int ezusb_writememory(struct usb_device *dev, int address,
unsigned char *data, int length, __u8 request)
 {
-   int result;
-   unsigned char *transfer_buffer;
-
if (!dev)
return -ENODEV;
 
-   transfer_buffer = kmemdup(data, length, GFP_KERNEL);
-   if (!transfer_buffer) {
-   dev_err(>dev, "%s - kmalloc(%d) failed.\n",
-   __func__, length);
-   return -ENOMEM;
-   }
-   result = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), request,
+   return usb_control_msg_send(dev, 0, request,
 USB_DIR_OUT | USB_TYPE_VENDOR | 
USB_RECIP_DEVICE,
-address, 0, transfer_buffer, length, 3000);
-
-   kfree(transfer_buffer);
-   return result;
+address, 0, data, length, 3000, GFP_KERNEL);
 }
 
 static int ezusb_set_reset(struct usb_device *dev, unsigned short cpucs_reg,
-- 
2.25.1

[PATCH v4 3/3] usb: misc: usbsevseg: update to use usb_control_msg_send()

[Anant Thazhemadam]

The newer usb_control_msg_{send|recv}() API ensures that a short read 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, instances of usb_control_msg() have been replaced with
usb_control_msg_send() appropriately.

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/usbsevseg.c | 60 ++--
 1 file changed, 17 insertions(+), 43 deletions(-)

diff --git a/drivers/usb/misc/usbsevseg.c b/drivers/usb/misc/usbsevseg.c
index 551074f5b7ad..4bc816bb09bb 100644
--- a/drivers/usb/misc/usbsevseg.c
+++ b/drivers/usb/misc/usbsevseg.c
@@ -74,15 +74,10 @@ static void update_display_powered(struct usb_sevsegdev 
*mydev)
if (mydev->shadow_power != 1)
return;
 
-   rc = usb_control_msg(mydev->udev,
-   usb_sndctrlpipe(mydev->udev, 0),
-   0x12,
-   0x48,
-   (80 * 0x100) + 10, /*  (power mode) */
-   (0x00 * 0x100) + (mydev->powered ? 1 : 0),
-   NULL,
-   0,
-   2000);
+   rc = usb_control_msg_send(mydev->udev, 0, 0x12, 0x48,
+ (80 * 0x100) + 10, /*  (power mode) */
+ (0x00 * 0x100) + (mydev->powered ? 1 : 0),
+ NULL, 0, 2000, GFP_KERNEL);
if (rc < 0)
dev_dbg(>udev->dev, "power retval = %d\n", rc);
 
@@ -99,15 +94,10 @@ static void update_display_mode(struct usb_sevsegdev *mydev)
if(mydev->shadow_power != 1)
return;
 
-   rc = usb_control_msg(mydev->udev,
-   usb_sndctrlpipe(mydev->udev, 0),
-   0x12,
-   0x48,
-   (82 * 0x100) + 10, /* (set mode) */
-   (mydev->mode_msb * 0x100) + mydev->mode_lsb,
-   NULL,
-   0,
-   2000);
+   rc = usb_control_msg_send(mydev->udev, 0, 0x12, 0x48,
+ (82 * 0x100) + 10, /* (set mode) */
+ (mydev->mode_msb * 0x100) + mydev->mode_lsb,
+ NULL, 0, 2000, GFP_NOIO);
 
if (rc < 0)
dev_dbg(>udev->dev, "mode retval = %d\n", rc);
@@ -117,48 +107,32 @@ static void update_display_visual(struct usb_sevsegdev 
*mydev, gfp_t mf)
 {
int rc;
int i;
-   unsigned char *buffer;
+   unsigned char buffer[MAXLEN] = {0};
u8 decimals = 0;
 
if(mydev->shadow_power != 1)
return;
 
-   buffer = kzalloc(MAXLEN, mf);
-   if (!buffer)
-   return;
-
/* The device is right to left, where as you write left to right */
for (i = 0; i < mydev->textlength; i++)
buffer[i] = mydev->text[mydev->textlength-1-i];
 
-   rc = usb_control_msg(mydev->udev,
-   usb_sndctrlpipe(mydev->udev, 0),
-   0x12,
-   0x48,
-   (85 * 0x100) + 10, /* (write text) */
-   (0 * 0x100) + mydev->textmode, /* mode  */
-   buffer,
-   mydev->textlength,
-   2000);
+   rc = usb_control_msg_send(mydev->udev, 0, 0x12, 0x48,
+ (85 * 0x100) + 10, /* (write text) */
+ (0 * 0x100) + mydev->textmode, /* mode  */
+ , mydev->textlength, 2000, mf);
 
if (rc < 0)
dev_dbg(>udev->dev, "write retval = %d\n", rc);
 
-   kfree(buffer);
-
/* The device is right to left, where as you write left to right */
for (i = 0; i < sizeof(mydev->decimals); i++)
decimals |= mydev->decimals[i] << i;
 
-   rc = usb_control_msg(mydev->udev,
-   usb_sndctrlpipe(mydev->udev, 0),
-   0x12,
-   0x48,
-   (86 * 0x100) + 10, /* (set decimal) */
-   (0 * 0x100) + decimals, /* decimals */
-   NULL,
-   0,
-   2000);
+   rc = usb_control_msg_send(mydev->udev, 0, 0x12, 0x48,
+ (86 * 0x100) + 10, /* (set decimal) */
+ (0 * 0x100) + decimals, /* decimals */
+ NULL, 0, 2000, mf);
 
if (rc < 0)
dev_dbg(>udev->dev, "decimal retval = %d\n", rc);
-- 
2.25.1

[PATCH v4 0/3] drivers: usb: misc: update to use usb_control_msg_{send|recv}() API

[Anant Thazhemadam]

The new usb_control_msg_{send|recv}() API provides a more convenient way
of using usb_control_msg() in some usecases. Using this, short reads are
considered as errors, data can be used off the stack, and the need for
the calling function to create a raw usb pipe is eliminated.
This patch series aims to update existing instances of usb_control_msg() 
in drivers/usb/misc/* to usb_control_msg_{send|recv}() appropriately
wherever it would be a good fit, and also update the return value
checking mechanisms in place (if any), as necessary so nothing breaks.

Changes in v4:

  * Drop all proposed changes to drivers (from v3) where the new API
doesn't fit appropriately.
  * Update commit messages.
  * Link to v3:
  
https://lore.kernel.org/linux-usb/ybf9exzii12oc...@hovoldconsulting.com/T/#m269ab33b52331c134bbbc77d13cb65c2194a6093

Changes in v3:

  * idmouse, emi26 and emi62 are left unchanged, and are not updated.
-> since control transfers in idmouse are without a data stage, there's no
   real advantage in using the new helper here.
-> in emi26, and emi62, FW_LOAD_SIZE = 1048 (> 1024). Thus, if we try to 
use the
   new helpers, it will result in either build warnings, or memory being 
allocated.

  * Link to v2:
  
https://lore.kernel.org/linux-usb/20201130013103.2580467-1-anant.thazhema...@gmail.com/T/


Changes in v2:

  * Buffer variables that were previously dynamically allocated are no
longer dynamically allocated unless they have a variable length
(since that threw a warning).

  * Link to v1:

https://lore.kernel.org/linux-usb/20201129160612.1908074-1-anant.thazhema...@gmail.com/
 



Anant Thazhemadam (3):
  usb: misc: ehset: update to use the usb_control_msg_{send|recv}() API
  usb: misc: ezusb: update to use usb_control_msg_send()
  usb: misc: usbsevseg: update to use usb_control_msg_send()

 drivers/usb/misc/ehset.c | 76 +++-
 drivers/usb/misc/ezusb.c | 16 +---
 drivers/usb/misc/usbsevseg.c | 60 
 3 files changed, 51 insertions(+), 101 deletions(-)

-- 
2.25.1

[PATCH v4 1/3] usb: misc: ehset: update to use the usb_control_msg_{send|recv}() API

[Anant Thazhemadam]

The newer usb_control_msg_{send|recv}() API ensures that a short read
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, instances of usb_control_msg() have been replaced with
usb_control_msg_{recv|send}() appropriately.

Now, we also test for a short device descriptor (which USB core
should already have fetched if you get to probe this driver), but which
wasn't verified again here before.

Signed-off-by: Anant Thazhemadam 
Reviewed-by: Peter Chen 
Reviewed-by: Johan Hovold 
---
 drivers/usb/misc/ehset.c | 76 +---
 1 file changed, 32 insertions(+), 44 deletions(-)

diff --git a/drivers/usb/misc/ehset.c b/drivers/usb/misc/ehset.c
index 2752e1f4f4d0..f87890f9cd26 100644
--- a/drivers/usb/misc/ehset.c
+++ b/drivers/usb/misc/ehset.c
@@ -24,68 +24,57 @@ static int ehset_probe(struct usb_interface *intf,
int ret = -EINVAL;
struct usb_device *dev = interface_to_usbdev(intf);
struct usb_device *hub_udev = dev->parent;
-   struct usb_device_descriptor *buf;
+   struct usb_device_descriptor buf;
u8 portnum = dev->portnum;
u16 test_pid = le16_to_cpu(dev->descriptor.idProduct);
 
switch (test_pid) {
case TEST_SE0_NAK_PID:
-   ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
-   USB_REQ_SET_FEATURE, USB_RT_PORT,
-   USB_PORT_FEAT_TEST,
-   (USB_TEST_SE0_NAK << 8) | portnum,
-   NULL, 0, 1000);
+   ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
+  USB_RT_PORT, USB_PORT_FEAT_TEST,
+  (USB_TEST_SE0_NAK << 8) | portnum,
+  NULL, 0, 1000, GFP_KERNEL);
break;
case TEST_J_PID:
-   ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
-   USB_REQ_SET_FEATURE, USB_RT_PORT,
-   USB_PORT_FEAT_TEST,
-   (USB_TEST_J << 8) | portnum,
-   NULL, 0, 1000);
+   ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
+  USB_RT_PORT, USB_PORT_FEAT_TEST,
+  (USB_TEST_J << 8) | portnum, NULL, 0,
+  1000, GFP_KERNEL);
break;
case TEST_K_PID:
-   ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
-   USB_REQ_SET_FEATURE, USB_RT_PORT,
-   USB_PORT_FEAT_TEST,
-   (USB_TEST_K << 8) | portnum,
-   NULL, 0, 1000);
+   ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
+  USB_RT_PORT, USB_PORT_FEAT_TEST,
+  (USB_TEST_K << 8) | portnum, NULL, 0,
+  1000, GFP_KERNEL);
break;
case TEST_PACKET_PID:
-   ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
-   USB_REQ_SET_FEATURE, USB_RT_PORT,
-   USB_PORT_FEAT_TEST,
-   (USB_TEST_PACKET << 8) | portnum,
-   NULL, 0, 1000);
+   ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
+  USB_RT_PORT, USB_PORT_FEAT_TEST,
+  (USB_TEST_PACKET << 8) | portnum,
+  NULL, 0, 1000, GFP_KERNEL);
break;
case TEST_HS_HOST_PORT_SUSPEND_RESUME:
/* Test: wait for 15secs -> suspend -> 15secs delay -> resume */
msleep(15 * 1000);
-   ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
-   USB_REQ_SET_FEATURE, USB_RT_PORT,
-   USB_PORT_FEAT_SUSPEND, portnum,
-   NULL, 0, 1000);
+   ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
+  USB_RT_PORT, USB_PORT_FEAT_SUSPEND,
+  portnum, NULL, 0, 1000, GFP_KERNEL);
if (ret < 0)
break;
 
msleep(15 * 1000);
-   ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
-

Re: [PATCH v3 01/12] usb: misc: appledisplay: update to use the usb_control_msg_{send|recv}() API

[Anant Thazhemadam]

On 27/01/21 7:28 pm, Johan Hovold wrote:
> On Wed, Jan 27, 2021 at 12:03:52AM +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 I mentioned in my comments on v2, a short write has always been
> treated as an error so you shouldn't imply that it wasn't here (and in
> the other commit messages).

The newer API ensures that a short send/recv is an error, as they either
complete the complete translation, or return an error, and remove the need
for explicit return value checking that was previously expected to detect the 
short
read/write (which wasn't present in a lot of places).
That's what I was trying to say. But if the commit message isn't representative 
of
that, I'll try and modify it.

Does this sound like a better commit message?

"The newer usb_control_msg_{send|recv}() API are an improvement on the
existing usb_control_msg().

The new API ensures either the full translation is completed,
or an error is returned. This ensures that all short send/recv are detected as
errors even if there is no explicit return value checking performed.

The new API also allows us to use data off the stack, and don't require raw usb
pipes to be created in the calling functions."


>> 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, instances of usb_control_msg() have been replaced with
>> usb_control_msg_{recv|send}(), and all return value checking
>> conditions have also been modified appropriately.
>>
>> Signed-off-by: Anant Thazhemadam 
>> ---
>>  drivers/usb/misc/appledisplay.c | 46 ++---
>>  1 file changed, 19 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/usb/misc/appledisplay.c 
>> b/drivers/usb/misc/appledisplay.c
>> index c8098e9b432e..117deb2fdc29 100644
>> --- a/drivers/usb/misc/appledisplay.c
>> +++ b/drivers/usb/misc/appledisplay.c
>> @@ -132,21 +132,17 @@ static int appledisplay_bl_update_status(struct 
>> backlight_device *bd)
>>  pdata->msgdata[0] = 0x10;
>>  pdata->msgdata[1] = bd->props.brightness;
>>  
>> -retval = usb_control_msg(
>> -pdata->udev,
>> -usb_sndctrlpipe(pdata->udev, 0),
>> -USB_REQ_SET_REPORT,
>> -USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
>> -ACD_USB_BRIGHTNESS,
>> -0,
>> -pdata->msgdata, 2,
> In this case, the buffer is already DMA-able (and is in fact only used
> for this purpose) so this patch introduces an extra allocation and
> memcpy for no really good reason.
>
>> -ACD_USB_TIMEOUT);
>> +retval = usb_control_msg_send(pdata->udev,
>> +  0,
>> +  USB_REQ_SET_REPORT,
>> +  USB_DIR_OUT | USB_TYPE_CLASS | 
>> USB_RECIP_INTERFACE,
>> +  ACD_USB_BRIGHTNESS,
>> +  0,
>> +  pdata->msgdata, 2,
>> +  ACD_USB_TIMEOUT, GFP_KERNEL);
>>  mutex_unlock(>sysfslock);
>>  
>> -if (retval < 0)
>> -return retval;
>> -else
>> -return 0;
>> +return retval;
>>  }
>>  
>>  static int appledisplay_bl_get_brightness(struct backlight_device *bd)
>> @@ -155,21 +151,17 @@ static int appledisplay_bl_get_brightness(struct 
>> backlight_device *bd)
>>  int retval, brightness;
>>  
>>  mutex_lock(>sysfslock);
>> -retval = usb_control_msg(
>> -pdata->udev,
>> -usb_rcvctrlpipe(pdata->udev, 0),
>> -USB_REQ_GET_REPORT,
>> -USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
>> -ACD_USB_BRIGHTNESS,
>> -0,
>> -pdata->msgdata, 2,
>> -ACD_USB_TIMEOUT);
>> -if (retval < 2) {
>> -if (retval >= 0)
>> -retval = -EMSGSIZE;
>> -} else {
>> +retval = usb_control_msg_recv(pdata->udev,
>> +  0,
>> +  USB_REQ_GET_REPORT,
>> +  USB_DIR_IN | USB_TYPE_CLASS | 
>> USB_RECIP_INTERFACE,
>> +  ACD_USB_BRIGHTNESS,
>> +  0,
>> +  pdata->msgdata, 2,
>> +  ACD_USB_TIMEOUT, GFP_KERNEL);
>> +if (retval == 0)
>>  brightness = pdata->msgdata[1];
>> -}
>> +
> Same here, this introduces an extra allocation and memcpy and the only
> thing you gain is essentially the removal of the two lines for handling
> a short read.
>
>>  mutex_unlock(>sysfslock);
>>  
>>  if (retval < 0)
> I'd consider dropping this one as well.


Yes, that's probably the better thing to do here.


Thanks,
Anant

Re: [PATCH v3 02/12] usb: misc: cypress_cy7c63: update to use usb_control_msg_recv()

[Anant Thazhemadam]

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 
>> ---
>>  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(>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, , 
>> 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

[RESEND PATCH v3 12/12] usb: misc: usbtest: update to use the usb_control_msg_{send|recv}() API

[Anant Thazhemadam]

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, instances of usb_control_msg() have been replaced with
usb_control_msg_{recv|send}() and the return value checking conditions have
also been modified appropriately.

Signed-off-by: Anant Thazhemadam 
---
Resending this patch since the subject line for the initial submission 
(sent as a part of the patch series) wasn't set correctly.

 drivers/usb/misc/usbtest.c | 69 --
 1 file changed, 29 insertions(+), 40 deletions(-)

diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index 150090ee4ec1..4337eff2a749 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -672,19 +672,15 @@ static int get_altsetting(struct usbtest_dev *dev)
struct usb_device   *udev = interface_to_usbdev(iface);
int retval;
 
-   retval = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
-   USB_REQ_GET_INTERFACE, USB_DIR_IN|USB_RECIP_INTERFACE,
-   0, iface->altsetting[0].desc.bInterfaceNumber,
-   dev->buf, 1, USB_CTRL_GET_TIMEOUT);
-   switch (retval) {
-   case 1:
-   return dev->buf[0];
-   case 0:
-   retval = -ERANGE;
-   fallthrough;
-   default:
+   retval = usb_control_msg_recv(udev, 0, USB_REQ_GET_INTERFACE,
+ USB_DIR_IN|USB_RECIP_INTERFACE,
+ 0, 
iface->altsetting[0].desc.bInterfaceNumber,
+ dev->buf, 1, USB_CTRL_GET_TIMEOUT, 
GFP_KERNEL);
+
+   if (retval < 0)
return retval;
-   }
+
+   return dev->buf[0];
 }
 
 static int set_altsetting(struct usbtest_dev *dev, int alternate)
@@ -872,14 +868,15 @@ static int ch9_postconfig(struct usbtest_dev *dev)
 * ... although some cheap devices (like one TI Hub I've got)
 * won't return config descriptors except before set_config.
 */
-   retval = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
-   USB_REQ_GET_CONFIGURATION,
-   USB_DIR_IN | USB_RECIP_DEVICE,
-   0, 0, dev->buf, 1, USB_CTRL_GET_TIMEOUT);
-   if (retval != 1 || dev->buf[0] != expected) {
+   retval = usb_control_msg_recv(udev, 0, 
USB_REQ_GET_CONFIGURATION,
+ USB_DIR_IN | USB_RECIP_DEVICE,  0,
+ 0, dev->buf, 1, 
USB_CTRL_GET_TIMEOUT,
+ GFP_KERNEL);
+
+   if (retval != 0 || dev->buf[0] != expected) {
dev_err(>dev, "get config --> %d %d (1 %d)\n",
retval, dev->buf[0], expected);
-   return (retval < 0) ? retval : -EDOM;
+   return retval;
}
}
 
@@ -1683,10 +1680,10 @@ static int test_halt(struct usbtest_dev *tdev, int ep, 
struct urb *urb)
return retval;
 
/* set halt (protocol test only), verify it worked */
-   retval = usb_control_msg(urb->dev, usb_sndctrlpipe(urb->dev, 0),
-   USB_REQ_SET_FEATURE, USB_RECIP_ENDPOINT,
-   USB_ENDPOINT_HALT, ep,
-   NULL, 0, USB_CTRL_SET_TIMEOUT);
+   retval = usb_control_msg_send(urb->dev, 0, USB_REQ_SET_FEATURE,
+ USB_RECIP_ENDPOINT, USB_ENDPOINT_HALT,
+ ep, NULL, 0, USB_CTRL_SET_TIMEOUT,
+ GFP_KERNEL);
if (retval < 0) {
ERROR(tdev, "ep %02x couldn't set halt, %d\n", ep, retval);
return retval;
@@ -1845,30 +1842,22 @@ static int ctrl_out(struct usbtest_dev *dev,
/* write patterned data */
for (j = 0; j < len; j++)
buf[j] = (u8)(i + j);
-   retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
-   0x5b, USB_DIR_OUT|USB_TYPE_VENDOR,
-   0, 0, buf, len, USB_CTRL_SET_TIMEOUT);
-   if (retval != len) {
+   retval = usb_control_msg_send(udev, 0, 0x5b,
+ USB_DIR_OUT | USB_TYPE_VENDOR, 0,
+ 0, buf, len, USB_CTRL_SET_TIMEOUT,
+ GFP_KERNEL);
+   if (retval < 0) {
what = "write";
-   

Re: [PATCH v3 12/12] [PATCH v3 12/12] usb: misc: usbtest: update to use the, usb_control_msg_{send|recv}() API

[Anant Thazhemadam]

Looks like the subject line got messed up for patch 12/12.
I'm sorry about that. Do I have to resend the patch?

Thanks,
Anant

[no subject]

[Anant Thazhemadam]

Subject: [PATCH v3 12/12] usb: misc: usbtest: update to use the
 usb_control_msg_{send|recv}() API

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, instances of usb_control_msg() have been replaced with
usb_control_msg_{recv|send}() and the return value checking conditions have
also been modified appropriately.

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/usbtest.c | 69 --
 1 file changed, 29 insertions(+), 40 deletions(-)

diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index 150090ee4ec1..4337eff2a749 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -672,19 +672,15 @@ static int get_altsetting(struct usbtest_dev *dev)
struct usb_device   *udev = interface_to_usbdev(iface);
int retval;
 
-   retval = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
-   USB_REQ_GET_INTERFACE, USB_DIR_IN|USB_RECIP_INTERFACE,
-   0, iface->altsetting[0].desc.bInterfaceNumber,
-   dev->buf, 1, USB_CTRL_GET_TIMEOUT);
-   switch (retval) {
-   case 1:
-   return dev->buf[0];
-   case 0:
-   retval = -ERANGE;
-   fallthrough;
-   default:
+   retval = usb_control_msg_recv(udev, 0, USB_REQ_GET_INTERFACE,
+ USB_DIR_IN|USB_RECIP_INTERFACE,
+ 0, 
iface->altsetting[0].desc.bInterfaceNumber,
+ dev->buf, 1, USB_CTRL_GET_TIMEOUT, 
GFP_KERNEL);
+
+   if (retval < 0)
return retval;
-   }
+
+   return dev->buf[0];
 }
 
 static int set_altsetting(struct usbtest_dev *dev, int alternate)
@@ -872,14 +868,15 @@ static int ch9_postconfig(struct usbtest_dev *dev)
 * ... although some cheap devices (like one TI Hub I've got)
 * won't return config descriptors except before set_config.
 */
-   retval = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
-   USB_REQ_GET_CONFIGURATION,
-   USB_DIR_IN | USB_RECIP_DEVICE,
-   0, 0, dev->buf, 1, USB_CTRL_GET_TIMEOUT);
-   if (retval != 1 || dev->buf[0] != expected) {
+   retval = usb_control_msg_recv(udev, 0, 
USB_REQ_GET_CONFIGURATION,
+ USB_DIR_IN | USB_RECIP_DEVICE,  0,
+ 0, dev->buf, 1, 
USB_CTRL_GET_TIMEOUT,
+ GFP_KERNEL);
+
+   if (retval != 0 || dev->buf[0] != expected) {
dev_err(>dev, "get config --> %d %d (1 %d)\n",
retval, dev->buf[0], expected);
-   return (retval < 0) ? retval : -EDOM;
+   return retval;
}
}
 
@@ -1683,10 +1680,10 @@ static int test_halt(struct usbtest_dev *tdev, int ep, 
struct urb *urb)
return retval;
 
/* set halt (protocol test only), verify it worked */
-   retval = usb_control_msg(urb->dev, usb_sndctrlpipe(urb->dev, 0),
-   USB_REQ_SET_FEATURE, USB_RECIP_ENDPOINT,
-   USB_ENDPOINT_HALT, ep,
-   NULL, 0, USB_CTRL_SET_TIMEOUT);
+   retval = usb_control_msg_send(urb->dev, 0, USB_REQ_SET_FEATURE,
+ USB_RECIP_ENDPOINT, USB_ENDPOINT_HALT,
+ ep, NULL, 0, USB_CTRL_SET_TIMEOUT,
+ GFP_KERNEL);
if (retval < 0) {
ERROR(tdev, "ep %02x couldn't set halt, %d\n", ep, retval);
return retval;
@@ -1845,30 +1842,22 @@ static int ctrl_out(struct usbtest_dev *dev,
/* write patterned data */
for (j = 0; j < len; j++)
buf[j] = (u8)(i + j);
-   retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
-   0x5b, USB_DIR_OUT|USB_TYPE_VENDOR,
-   0, 0, buf, len, USB_CTRL_SET_TIMEOUT);
-   if (retval != len) {
+   retval = usb_control_msg_send(udev, 0, 0x5b,
+ USB_DIR_OUT | USB_TYPE_VENDOR, 0,
+ 0, buf, len, USB_CTRL_SET_TIMEOUT,
+ GFP_KERNEL);
+   if (retval < 0) {
what = "write";
-   if 

[PATCH v3 11/12] usb: misc: usbsevseg: update to use usb_control_msg_send()

[Anant Thazhemadam]

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, instances of usb_control_msg() have been replaced with
usb_control_msg_send() appropriately.

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/usbsevseg.c | 60 ++--
 1 file changed, 17 insertions(+), 43 deletions(-)

diff --git a/drivers/usb/misc/usbsevseg.c b/drivers/usb/misc/usbsevseg.c
index 551074f5b7ad..ade4bc58d5f2 100644
--- a/drivers/usb/misc/usbsevseg.c
+++ b/drivers/usb/misc/usbsevseg.c
@@ -74,15 +74,10 @@ static void update_display_powered(struct usb_sevsegdev 
*mydev)
if (mydev->shadow_power != 1)
return;
 
-   rc = usb_control_msg(mydev->udev,
-   usb_sndctrlpipe(mydev->udev, 0),
-   0x12,
-   0x48,
-   (80 * 0x100) + 10, /*  (power mode) */
-   (0x00 * 0x100) + (mydev->powered ? 1 : 0),
-   NULL,
-   0,
-   2000);
+   rc = usb_control_msg_send(mydev->udev, 0, 0x12, 0x48,
+ (80 * 0x100) + 10, /*  (power mode) */
+ (0x00 * 0x100) + (mydev->powered ? 1 : 0),
+ NULL, 0, 2000, GFP_KERNEL);
if (rc < 0)
dev_dbg(>udev->dev, "power retval = %d\n", rc);
 
@@ -99,15 +94,10 @@ static void update_display_mode(struct usb_sevsegdev *mydev)
if(mydev->shadow_power != 1)
return;
 
-   rc = usb_control_msg(mydev->udev,
-   usb_sndctrlpipe(mydev->udev, 0),
-   0x12,
-   0x48,
-   (82 * 0x100) + 10, /* (set mode) */
-   (mydev->mode_msb * 0x100) + mydev->mode_lsb,
-   NULL,
-   0,
-   2000);
+   rc = usb_control_msg_send(mydev->udev, 0, 0x12, 0x48,
+ (82 * 0x100) + 10, /* (set mode) */
+ (mydev->mode_msb * 0x100) + mydev->mode_lsb,
+ NULL, 0, 2000, GFP_KERNEL);
 
if (rc < 0)
dev_dbg(>udev->dev, "mode retval = %d\n", rc);
@@ -117,48 +107,32 @@ static void update_display_visual(struct usb_sevsegdev 
*mydev, gfp_t mf)
 {
int rc;
int i;
-   unsigned char *buffer;
+   unsigned char buffer[MAXLEN] = {0};
u8 decimals = 0;
 
if(mydev->shadow_power != 1)
return;
 
-   buffer = kzalloc(MAXLEN, mf);
-   if (!buffer)
-   return;
-
/* The device is right to left, where as you write left to right */
for (i = 0; i < mydev->textlength; i++)
buffer[i] = mydev->text[mydev->textlength-1-i];
 
-   rc = usb_control_msg(mydev->udev,
-   usb_sndctrlpipe(mydev->udev, 0),
-   0x12,
-   0x48,
-   (85 * 0x100) + 10, /* (write text) */
-   (0 * 0x100) + mydev->textmode, /* mode  */
-   buffer,
-   mydev->textlength,
-   2000);
+   rc = usb_control_msg_send(mydev->udev, 0, 0x12, 0x48,
+ (85 * 0x100) + 10, /* (write text) */
+ (0 * 0x100) + mydev->textmode, /* mode  */
+ , mydev->textlength, 2000, mf);
 
if (rc < 0)
dev_dbg(>udev->dev, "write retval = %d\n", rc);
 
-   kfree(buffer);
-
/* The device is right to left, where as you write left to right */
for (i = 0; i < sizeof(mydev->decimals); i++)
decimals |= mydev->decimals[i] << i;
 
-   rc = usb_control_msg(mydev->udev,
-   usb_sndctrlpipe(mydev->udev, 0),
-   0x12,
-   0x48,
-   (86 * 0x100) + 10, /* (set decimal) */
-   (0 * 0x100) + decimals, /* decimals */
-   NULL,
-   0,
-   2000);
+   rc = usb_control_msg_send(mydev->udev, 0, 0x12, 0x48,
+ (86 * 0x100) + 10, /* (set decimal) */
+ (0 * 0x100) + decimals, /* decimals */
+ NULL, 0, 2000, mf);
 
if (rc < 0)
dev_dbg(>udev->dev, "decimal retval = %d\n", rc);
-- 
2.25.1

[PATCH v3 09/12] usb: misc: lvstest: update to use the usb_control_msg_{send|recv}() API

[Anant Thazhemadam]

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, instances of usb_control_msg() have been replaced with
usb_control_msg_{recv|send}() and the return value checking conditions have
also been modified appropriately.

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/lvstest.c | 38 --
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/misc/lvstest.c b/drivers/usb/misc/lvstest.c
index f8686139d6f3..daa1b2c9139f 100644
--- a/drivers/usb/misc/lvstest.c
+++ b/drivers/usb/misc/lvstest.c
@@ -85,17 +85,17 @@ static void destroy_lvs_device(struct usb_device *udev)
 static int lvs_rh_clear_port_feature(struct usb_device *hdev,
int port1, int feature)
 {
-   return usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
+   return usb_control_msg_send(hdev, 0,
USB_REQ_CLEAR_FEATURE, USB_RT_PORT, feature, port1,
-   NULL, 0, 1000);
+   NULL, 0, 1000, GFP_KERNEL);
 }
 
 static int lvs_rh_set_port_feature(struct usb_device *hdev,
int port1, int feature)
 {
-   return usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
+   return usb_control_msg_send(hdev, 0,
USB_REQ_SET_FEATURE, USB_RT_PORT, feature, port1,
-   NULL, 0, 1000);
+   NULL, 0, 1000, GFP_KERNEL);
 }
 
 static ssize_t u3_entry_store(struct device *dev,
@@ -257,13 +257,9 @@ static ssize_t get_dev_desc_store(struct device *dev,
 {
struct usb_interface *intf = to_usb_interface(dev);
struct usb_device *udev;
-   struct usb_device_descriptor *descriptor;
+   struct usb_device_descriptor descriptor;
int ret;
 
-   descriptor = kmalloc(sizeof(*descriptor), GFP_KERNEL);
-   if (!descriptor)
-   return -ENOMEM;
-
udev = create_lvs_device(intf);
if (!udev) {
dev_err(dev, "failed to create lvs device\n");
@@ -271,18 +267,16 @@ static ssize_t get_dev_desc_store(struct device *dev,
goto free_desc;
}
 
-   ret = usb_control_msg(udev, (PIPE_CONTROL << 30) | USB_DIR_IN,
-   USB_REQ_GET_DESCRIPTOR, USB_DIR_IN, USB_DT_DEVICE << 8,
-   0, descriptor, sizeof(*descriptor),
-   USB_CTRL_GET_TIMEOUT);
+   ret = usb_control_msg_recv(udev, 0, USB_REQ_GET_DESCRIPTOR,
+  USB_DIR_IN, USB_DT_DEVICE << 8,
+  0, , sizeof(descriptor),
+  USB_CTRL_GET_TIMEOUT, GFP_KERNEL);
if (ret < 0)
dev_err(dev, "can't read device descriptor %d\n", ret);
 
destroy_lvs_device(udev);
 
 free_desc:
-   kfree(descriptor);
-
if (ret < 0)
return ret;
 
@@ -336,10 +330,10 @@ static void lvs_rh_work(struct work_struct *work)
 
/* Examine each root port */
for (i = 1; i <= descriptor->bNbrPorts; i++) {
-   ret = usb_control_msg(hdev, usb_rcvctrlpipe(hdev, 0),
+   ret = usb_control_msg_recv(hdev, 0,
USB_REQ_GET_STATUS, USB_DIR_IN | USB_RT_PORT, 0, i,
-   port_status, sizeof(*port_status), 1000);
-   if (ret < 4)
+   port_status, sizeof(*port_status), 1000, GFP_KERNEL);
+   if (ret < 0)
continue;
 
portchange = le16_to_cpu(port_status->wPortChange);
@@ -420,13 +414,13 @@ static int lvs_rh_probe(struct usb_interface *intf,
usb_set_intfdata(intf, lvs);
 
/* how many number of ports this root hub has */
-   ret = usb_control_msg(hdev, usb_rcvctrlpipe(hdev, 0),
+   ret = usb_control_msg_recv(hdev, 0,
USB_REQ_GET_DESCRIPTOR, USB_DIR_IN | USB_RT_HUB,
USB_DT_SS_HUB << 8, 0, >descriptor,
-   USB_DT_SS_HUB_SIZE, USB_CTRL_GET_TIMEOUT);
-   if (ret < (USB_DT_HUB_NONVAR_SIZE + 2)) {
+   USB_DT_SS_HUB_SIZE, USB_CTRL_GET_TIMEOUT, GFP_KERNEL);
+   if (ret < 0) {
dev_err(>dev, "wrong root hub descriptor read %d\n", ret);
-   return ret < 0 ? ret : -EINVAL;
+   return ret;
}
 
/* submit urb to poll interrupt endpoint */
-- 
2.25.1

[PATCH v3 10/12] usb: misc: trancevibrator: update to use usb_control_msg_send()

[Anant Thazhemadam]

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_send() and the return value checking condition has also
been modified appropriately.

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/trancevibrator.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/misc/trancevibrator.c 
b/drivers/usb/misc/trancevibrator.c
index a3dfc77578ea..c50807b4f4ef 100644
--- a/drivers/usb/misc/trancevibrator.c
+++ b/drivers/usb/misc/trancevibrator.c
@@ -59,11 +59,11 @@ static ssize_t speed_store(struct device *dev, struct 
device_attribute *attr,
dev_dbg(>udev->dev, "speed = %d\n", tv->speed);
 
/* Set speed */
-   retval = usb_control_msg(tv->udev, usb_sndctrlpipe(tv->udev, 0),
+   retval = usb_control_msg_send(tv->udev, 0,
 0x01, /* vendor request: set speed */
 USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_OTHER,
 tv->speed, /* speed value */
-0, NULL, 0, USB_CTRL_GET_TIMEOUT);
+0, NULL, 0, USB_CTRL_GET_TIMEOUT, GFP_KERNEL);
if (retval) {
tv->speed = old;
dev_dbg(>udev->dev, "retval = %d\n", retval);
-- 
2.25.1

[PATCH v3 06/12] usb: misc: iowarrior: update to use the usb_control_msg_{send|recv}() API

[Anant Thazhemadam]

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, instances of usb_control_msg() have been replaced with
usb_control_msg_{recv|send}() appropriately.

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/iowarrior.c | 34 +-
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c
index efbd317f2f25..9d6a7548e537 100644
--- a/drivers/usb/misc/iowarrior.c
+++ b/drivers/usb/misc/iowarrior.c
@@ -109,12 +109,12 @@ static int usb_get_report(struct usb_device *dev,
  struct usb_host_interface *inter, unsigned char type,
  unsigned char id, void *buf, int size)
 {
-   return usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
-  USB_REQ_GET_REPORT,
-  USB_DIR_IN | USB_TYPE_CLASS |
-  USB_RECIP_INTERFACE, (type << 8) + id,
-  inter->desc.bInterfaceNumber, buf, size,
-  GET_TIMEOUT*HZ);
+   return usb_control_msg_recv(dev, 0,
+   USB_REQ_GET_REPORT,
+   USB_DIR_IN | USB_TYPE_CLASS |
+   USB_RECIP_INTERFACE, (type << 8) + id,
+   inter->desc.bInterfaceNumber, buf, size,
+   GET_TIMEOUT*HZ, GFP_KERNEL);
 }
 //#endif
 
@@ -123,13 +123,13 @@ static int usb_get_report(struct usb_device *dev,
 static int usb_set_report(struct usb_interface *intf, unsigned char type,
  unsigned char id, void *buf, int size)
 {
-   return usb_control_msg(interface_to_usbdev(intf),
-  usb_sndctrlpipe(interface_to_usbdev(intf), 0),
-  USB_REQ_SET_REPORT,
-  USB_TYPE_CLASS | USB_RECIP_INTERFACE,
-  (type << 8) + id,
-  intf->cur_altsetting->desc.bInterfaceNumber, buf,
-  size, HZ);
+   return usb_control_msg_send(interface_to_usbdev(intf),
+   0,
+   USB_REQ_SET_REPORT,
+   USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+   (type << 8) + id,
+   
intf->cur_altsetting->desc.bInterfaceNumber, buf,
+   size, HZ, GFP_KERNEL);
 }
 
 /*-*/
@@ -851,10 +851,10 @@ static int iowarrior_probe(struct usb_interface 
*interface,
 
/* Set the idle timeout to 0, if this is interface 0 */
if (dev->interface->cur_altsetting->desc.bInterfaceNumber == 0) {
-   usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
-   0x0A,
-   USB_TYPE_CLASS | USB_RECIP_INTERFACE, 0,
-   0, NULL, 0, USB_CTRL_SET_TIMEOUT);
+   usb_control_msg_send(udev, 0,
+0x0A,
+USB_TYPE_CLASS | USB_RECIP_INTERFACE, 0,
+0, NULL, 0, USB_CTRL_SET_TIMEOUT, 
GFP_KERNEL);
}
/* allow device read and ioctl */
dev->present = 1;
-- 
2.25.1

[PATCH v3 08/12] usb: misc: ldusb: update to use usb_control_msg_send()

[Anant Thazhemadam]

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_send() has been replaced
with usb_control_msg_send() appropriately.

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/ldusb.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/misc/ldusb.c b/drivers/usb/misc/ldusb.c
index 670e4d91e9ca..259ead4edecb 100644
--- a/drivers/usb/misc/ldusb.c
+++ b/drivers/usb/misc/ldusb.c
@@ -573,15 +573,13 @@ static ssize_t ld_usb_write(struct file *file, const char 
__user *buffer,
}
 
if (dev->interrupt_out_endpoint == NULL) {
-   /* try HID_REQ_SET_REPORT=9 on control_endpoint instead of 
interrupt_out_endpoint */
-   retval = usb_control_msg(interface_to_usbdev(dev->intf),
-
usb_sndctrlpipe(interface_to_usbdev(dev->intf), 0),
-9,
+   retval = usb_control_msg_send(interface_to_usbdev(dev->intf),
+0, 9,
 USB_TYPE_CLASS | USB_RECIP_INTERFACE | 
USB_DIR_OUT,
 1 << 8, 0,
 dev->interrupt_out_buffer,
 bytes_to_write,
-USB_CTRL_SET_TIMEOUT);
+USB_CTRL_SET_TIMEOUT, GFP_KERNEL);
if (retval < 0)
dev_err(>intf->dev,
"Couldn't submit HID_REQ_SET_REPORT %d\n",
-- 
2.25.1

[PATCH v3 07/12] usb: misc: isight_firmware: update to use usb_control_msg_send()

[Anant Thazhemadam]

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 instances of usb_control_msg() have been replaced with
usb_control_msg_send(), and return value checking has also been
appropriately enforced.

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/isight_firmware.c | 30 +-
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/misc/isight_firmware.c 
b/drivers/usb/misc/isight_firmware.c
index 4d30095d6ad2..1bd14a431f6c 100644
--- a/drivers/usb/misc/isight_firmware.c
+++ b/drivers/usb/misc/isight_firmware.c
@@ -37,13 +37,10 @@ static int isight_firmware_load(struct usb_interface *intf,
struct usb_device *dev = interface_to_usbdev(intf);
int llen, len, req, ret = 0;
const struct firmware *firmware;
-   unsigned char *buf = kmalloc(50, GFP_KERNEL);
+   unsigned char buf[50];
unsigned char data[4];
const u8 *ptr;
 
-   if (!buf)
-   return -ENOMEM;
-
if (request_firmware(, "isight.fw", >dev) != 0) {
printk(KERN_ERR "Unable to load isight firmware\n");
ret = -ENODEV;
@@ -53,11 +50,11 @@ static int isight_firmware_load(struct usb_interface *intf,
ptr = firmware->data;
 
buf[0] = 0x01;
-   if (usb_control_msg
-   (dev, usb_sndctrlpipe(dev, 0), 0xa0, 0x40, 0xe600, 0, buf, 1,
-300) != 1) {
+   ret = usb_control_msg_send(dev, 0, 0xa0, 0x40, 0xe600,
+  0, , 1, 300, GFP_KERNEL);
+   if (ret != 0) {
printk(KERN_ERR
-  "Failed to initialise isight firmware loader\n");
+   "Failed to initialise isight firmware loader\n");
ret = -ENODEV;
goto out;
}
@@ -82,15 +79,15 @@ static int isight_firmware_load(struct usb_interface *intf,
ret = -ENODEV;
goto out;
}
-   memcpy(buf, ptr, llen);
+   memcpy(, ptr, llen);
 
ptr += llen;
 
-   if (usb_control_msg
-   (dev, usb_sndctrlpipe(dev, 0), 0xa0, 0x40, req, 0,
-buf, llen, 300) != llen) {
+   ret = usb_control_msg_send(dev, 0, 0xa0, 0x40, req, 0,
+  , llen, 300, GFP_KERNEL);
+   if (ret != 0) {
printk(KERN_ERR
-  "Failed to load isight firmware\n");
+   "Failed to load isight firmware\n");
ret = -ENODEV;
goto out;
}
@@ -99,15 +96,14 @@ static int isight_firmware_load(struct usb_interface *intf,
}
 
buf[0] = 0x00;
-   if (usb_control_msg
-   (dev, usb_sndctrlpipe(dev, 0), 0xa0, 0x40, 0xe600, 0, buf, 1,
-300) != 1) {
+   ret = usb_control_msg_send(dev, 0, 0xa0, 0x40, 0xe600,
+  0, , 1, 300, GFP_KERNEL);
+   if (ret != 0) {
printk(KERN_ERR "isight firmware loading completion failed\n");
ret = -ENODEV;
}
 
 out:
-   kfree(buf);
release_firmware(firmware);
return ret;
 }
-- 
2.25.1

[PATCH v3 04/12] usb: misc: ehset: update to use the usb_control_msg_{send|recv}() API

[Anant Thazhemadam]

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, instances of usb_control_msg() have been replaced with
usb_control_msg_{recv|send}() appropriately.

Signed-off-by: Anant Thazhemadam 
Reviewed-by: Peter Chen 
---
 drivers/usb/misc/ehset.c | 76 +---
 1 file changed, 32 insertions(+), 44 deletions(-)

diff --git a/drivers/usb/misc/ehset.c b/drivers/usb/misc/ehset.c
index 2752e1f4f4d0..f87890f9cd26 100644
--- a/drivers/usb/misc/ehset.c
+++ b/drivers/usb/misc/ehset.c
@@ -24,68 +24,57 @@ static int ehset_probe(struct usb_interface *intf,
int ret = -EINVAL;
struct usb_device *dev = interface_to_usbdev(intf);
struct usb_device *hub_udev = dev->parent;
-   struct usb_device_descriptor *buf;
+   struct usb_device_descriptor buf;
u8 portnum = dev->portnum;
u16 test_pid = le16_to_cpu(dev->descriptor.idProduct);
 
switch (test_pid) {
case TEST_SE0_NAK_PID:
-   ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
-   USB_REQ_SET_FEATURE, USB_RT_PORT,
-   USB_PORT_FEAT_TEST,
-   (USB_TEST_SE0_NAK << 8) | portnum,
-   NULL, 0, 1000);
+   ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
+  USB_RT_PORT, USB_PORT_FEAT_TEST,
+  (USB_TEST_SE0_NAK << 8) | portnum,
+  NULL, 0, 1000, GFP_KERNEL);
break;
case TEST_J_PID:
-   ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
-   USB_REQ_SET_FEATURE, USB_RT_PORT,
-   USB_PORT_FEAT_TEST,
-   (USB_TEST_J << 8) | portnum,
-   NULL, 0, 1000);
+   ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
+  USB_RT_PORT, USB_PORT_FEAT_TEST,
+  (USB_TEST_J << 8) | portnum, NULL, 0,
+  1000, GFP_KERNEL);
break;
case TEST_K_PID:
-   ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
-   USB_REQ_SET_FEATURE, USB_RT_PORT,
-   USB_PORT_FEAT_TEST,
-   (USB_TEST_K << 8) | portnum,
-   NULL, 0, 1000);
+   ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
+  USB_RT_PORT, USB_PORT_FEAT_TEST,
+  (USB_TEST_K << 8) | portnum, NULL, 0,
+  1000, GFP_KERNEL);
break;
case TEST_PACKET_PID:
-   ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
-   USB_REQ_SET_FEATURE, USB_RT_PORT,
-   USB_PORT_FEAT_TEST,
-   (USB_TEST_PACKET << 8) | portnum,
-   NULL, 0, 1000);
+   ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
+  USB_RT_PORT, USB_PORT_FEAT_TEST,
+  (USB_TEST_PACKET << 8) | portnum,
+  NULL, 0, 1000, GFP_KERNEL);
break;
case TEST_HS_HOST_PORT_SUSPEND_RESUME:
/* Test: wait for 15secs -> suspend -> 15secs delay -> resume */
msleep(15 * 1000);
-   ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
-   USB_REQ_SET_FEATURE, USB_RT_PORT,
-   USB_PORT_FEAT_SUSPEND, portnum,
-   NULL, 0, 1000);
+   ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
+  USB_RT_PORT, USB_PORT_FEAT_SUSPEND,
+  portnum, NULL, 0, 1000, GFP_KERNEL);
if (ret < 0)
break;
 
msleep(15 * 1000);
-   ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
-   USB_REQ_CLEAR_FEATURE, USB_RT_PORT,
-   USB_PORT_FEAT_SUSPEND, portnum,
- 

[PATCH v3 05/12] usb: misc: ezusb: update to use usb_control_msg_send()

[Anant Thazhemadam]

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

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/ezusb.c | 16 ++--
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/misc/ezusb.c b/drivers/usb/misc/ezusb.c
index f058d8029761..78aaee56c2b7 100644
--- a/drivers/usb/misc/ezusb.c
+++ b/drivers/usb/misc/ezusb.c
@@ -31,24 +31,12 @@ static const struct ezusb_fx_type ezusb_fx1 = {
 static int ezusb_writememory(struct usb_device *dev, int address,
unsigned char *data, int length, __u8 request)
 {
-   int result;
-   unsigned char *transfer_buffer;
-
if (!dev)
return -ENODEV;
 
-   transfer_buffer = kmemdup(data, length, GFP_KERNEL);
-   if (!transfer_buffer) {
-   dev_err(>dev, "%s - kmalloc(%d) failed.\n",
-   __func__, length);
-   return -ENOMEM;
-   }
-   result = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), request,
+   return usb_control_msg_send(dev, 0, request,
 USB_DIR_OUT | USB_TYPE_VENDOR | 
USB_RECIP_DEVICE,
-address, 0, transfer_buffer, length, 3000);
-
-   kfree(transfer_buffer);
-   return result;
+address, 0, data, length, 3000, GFP_KERNEL);
 }
 
 static int ezusb_set_reset(struct usb_device *dev, unsigned short cpucs_reg,
-- 
2.25.1

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

[Anant Thazhemadam]

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 
---
 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(>udev->dev, "retval = %d\n", retval);
+   cytherm->brightness, , 8);
+   if (!retval)
+   dev_dbg(>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(>udev->dev, "retval = %d\n", retval);
-   
-   kfree(buffer);
-   
+   0x01, , 8);
+   if (!retval)
+   dev_dbg(>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(>udev->dev, "retval = %d\n", retval);
+   retval = vendor_command(cytherm->udev, READ_RAM, TEMP, 0, , 8);
+   if (!retval)
+   dev_dbg(>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(>udev->dev, "retval = %d\n", retval);
+   retval = vendor_command(cytherm->udev, READ_RAM, SIGN, 0, , 8);
+   if (!retval)
+   dev_dbg(>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

[PATCH v3 00/12] drivers: usb: misc: update to use usb_control_msg_{send|recv}() API

[Anant Thazhemadam]

The new usb_control_msg_{send|recv}() API provides an improved way of 
using usb_control_msg(). Using this, short reads/writes are considered
as errors, data can be used off the stack, and the need for the calling
function to create a raw usb pipe is eliminated.
This patch series aims to update existing instances of usb_control_msg() 
in drivers/usb/misc/* to usb_control_msg_{send|recv}() appropriately, and
also update the return value checking mechanisms in place (if any), as
necessary so nothing breaks.

Changes in v3:

  * idmouse, emi26 and emi62 are left unchanged, and are not updated.
-> since control transfers in idmouse are without a data stage, there's no
   real advantage in using the new helper here.
-> in emi26, and emi62, FW_LOAD_SIZE = 1048 (> 1024). Thus, if we try to 
use the
   new helpers, it will result in either build warnings, or memory being 
allocated.

  * Link to v2:
  
https://lore.kernel.org/linux-usb/20201130013103.2580467-1-anant.thazhema...@gmail.com/T/


Changes in v2:

  * Buffer variables that were previously dynamically allocated are no
longer dynamically allocated unless they have a variable length
(since that threw a warning).

  * Link to v1:

https://lore.kernel.org/linux-usb/20201129160612.1908074-1-anant.thazhema...@gmail.com/
 


Anant Thazhemadam (12):
  usb: misc: appledisplay: update to use the
usb_control_msg_{send|recv}() API
  usb: misc: cypress_cy7c63: update to use usb_control_msg_recv()
  usb: misc: cytherm: update to use usb_control_msg_recv()
  usb: misc: ehset: update to use the usb_control_msg_{send|recv}() API
  usb: misc: ezusb: update to use usb_control_msg_send()
  usb: misc: iowarrior: update to use the usb_control_msg_{send|recv}()
API
  usb: misc: isight_firmware: update to use usb_control_msg_send()
  usb: misc: ldusb: update to use usb_control_msg_send()
  usb: misc: lvstest: update to use the usb_control_msg_{send|recv}()
API
  usb: misc: trancevibrator: update to use usb_control_msg_send()
  usb: misc: usbsevseg: update to use usb_control_msg_send()
  usb: misc: usbtest: update to use the usb_control_msg_{send|recv}()
API

 drivers/usb/misc/appledisplay.c|  46 +--
 drivers/usb/misc/cypress_cy7c63.c  |  21 ++---
 drivers/usb/misc/cytherm.c | 128 ++---
 drivers/usb/misc/ehset.c   |  76 -
 drivers/usb/misc/ezusb.c   |  16 +---
 drivers/usb/misc/iowarrior.c   |  34 
 drivers/usb/misc/isight_firmware.c |  30 +++
 drivers/usb/misc/ldusb.c   |   8 +-
 drivers/usb/misc/lvstest.c |  38 -
 drivers/usb/misc/trancevibrator.c  |   4 +-
 drivers/usb/misc/usbsevseg.c   |  60 --
 drivers/usb/misc/usbtest.c |  69 +++-
 12 files changed, 198 insertions(+), 332 deletions(-)

-- 
2.25.1

[PATCH v3 01/12] usb: misc: appledisplay: update to use the usb_control_msg_{send|recv}() API

[Anant Thazhemadam]

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, instances of usb_control_msg() have been replaced with
usb_control_msg_{recv|send}(), and all return value checking
conditions have also been modified appropriately.

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/appledisplay.c | 46 ++---
 1 file changed, 19 insertions(+), 27 deletions(-)

diff --git a/drivers/usb/misc/appledisplay.c b/drivers/usb/misc/appledisplay.c
index c8098e9b432e..117deb2fdc29 100644
--- a/drivers/usb/misc/appledisplay.c
+++ b/drivers/usb/misc/appledisplay.c
@@ -132,21 +132,17 @@ static int appledisplay_bl_update_status(struct 
backlight_device *bd)
pdata->msgdata[0] = 0x10;
pdata->msgdata[1] = bd->props.brightness;
 
-   retval = usb_control_msg(
-   pdata->udev,
-   usb_sndctrlpipe(pdata->udev, 0),
-   USB_REQ_SET_REPORT,
-   USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
-   ACD_USB_BRIGHTNESS,
-   0,
-   pdata->msgdata, 2,
-   ACD_USB_TIMEOUT);
+   retval = usb_control_msg_send(pdata->udev,
+ 0,
+ USB_REQ_SET_REPORT,
+ USB_DIR_OUT | USB_TYPE_CLASS | 
USB_RECIP_INTERFACE,
+ ACD_USB_BRIGHTNESS,
+ 0,
+ pdata->msgdata, 2,
+ ACD_USB_TIMEOUT, GFP_KERNEL);
mutex_unlock(>sysfslock);
 
-   if (retval < 0)
-   return retval;
-   else
-   return 0;
+   return retval;
 }
 
 static int appledisplay_bl_get_brightness(struct backlight_device *bd)
@@ -155,21 +151,17 @@ static int appledisplay_bl_get_brightness(struct 
backlight_device *bd)
int retval, brightness;
 
mutex_lock(>sysfslock);
-   retval = usb_control_msg(
-   pdata->udev,
-   usb_rcvctrlpipe(pdata->udev, 0),
-   USB_REQ_GET_REPORT,
-   USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
-   ACD_USB_BRIGHTNESS,
-   0,
-   pdata->msgdata, 2,
-   ACD_USB_TIMEOUT);
-   if (retval < 2) {
-   if (retval >= 0)
-   retval = -EMSGSIZE;
-   } else {
+   retval = usb_control_msg_recv(pdata->udev,
+ 0,
+ USB_REQ_GET_REPORT,
+ USB_DIR_IN | USB_TYPE_CLASS | 
USB_RECIP_INTERFACE,
+ ACD_USB_BRIGHTNESS,
+ 0,
+ pdata->msgdata, 2,
+ ACD_USB_TIMEOUT, GFP_KERNEL);
+   if (retval == 0)
brightness = pdata->msgdata[1];
-   }
+
mutex_unlock(>sysfslock);
 
if (retval < 0)
-- 
2.25.1

[PATCH v3 02/12] usb: misc: cypress_cy7c63: update to use usb_control_msg_recv()

[Anant Thazhemadam]

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

Signed-off-by: Anant Thazhemadam 
---
 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(>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, , 
CYPRESS_MAX_REQSIZE,
+ USB_CTRL_GET_TIMEOUT, GFP_KERNEL);
 
/* store returned data (more READs to be added) */
switch (request) {
@@ -107,8 +98,6 @@ static int vendor_command(struct cypress *dev, unsigned char 
request,
break;
}
 
-   kfree(iobuf);
-error:
return retval;
 }
 
-- 
2.25.1

Re: [PATCH v2 05/15] usb: misc: emi26: update to use usb_control_msg_send()

[Anant Thazhemadam]

On 04/12/20 8:11 pm, Johan Hovold wrote:
> On Mon, Nov 30, 2020 at 06:58:47AM +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,
> Short writes have always been treated as an error. The new send helper
> only changes the return value from the transfer size to 0.
>
> And this driver never reads.
>
> Try to describe the motivation for changing this driver which is to
> avoid the explicit kmemdup().
>
Thank you. I will try and put a more appropriate commit message.
>> 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_send() appropriately.
>>
>> Signed-off-by: Anant Thazhemadam 
>> ---
>>  drivers/usb/misc/emi26.c | 31 ---
>>  1 file changed, 8 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/usb/misc/emi26.c b/drivers/usb/misc/emi26.c
>> index 24d841850e05..1dd024507f40 100644
>> --- a/drivers/usb/misc/emi26.c
>> +++ b/drivers/usb/misc/emi26.c
>> @@ -27,7 +27,7 @@
>>  #define INTERNAL_RAM(address)   (address <= MAX_INTERNAL_ADDRESS)
>>  
>>  static int emi26_writememory( struct usb_device *dev, int address,
>> -  const unsigned char *data, int length,
>> +  const void *data, int length,
> Why is this needed?
>
>>__u8 bRequest);
>>  static int emi26_set_reset(struct usb_device *dev, unsigned char reset_bit);
>>  static int emi26_load_firmware (struct usb_device *dev);
>> @@ -35,22 +35,12 @@ static int emi26_probe(struct usb_interface *intf, const 
>> struct usb_device_id *i
>>  static void emi26_disconnect(struct usb_interface *intf);
>>  
>>  /* thanks to drivers/usb/serial/keyspan_pda.c code */
>> -static int emi26_writememory (struct usb_device *dev, int address,
>> -  const unsigned char *data, int length,
>> +static int emi26_writememory(struct usb_device *dev, int address,
>> +  const void *data, int length,
>>__u8 request)
>>  {
>> -int result;
>> -unsigned char *buffer =  kmemdup(data, length, GFP_KERNEL);
>> -
>> -if (!buffer) {
>> -dev_err(>dev, "kmalloc(%d) failed.\n", length);
>> -return -ENOMEM;
>> -}
>> -/* Note: usb_control_msg returns negative value on error or length of 
>> the
>> - *   data that was written! */
>> -result = usb_control_msg (dev, usb_sndctrlpipe(dev, 0), request, 0x40, 
>> address, 0, buffer, length, 300);
>> -kfree (buffer);
>> -return result;
>> +return usb_control_msg_send(dev, 0, request, 0x40, address, 0,
>> +data, length, 300, GFP_KERNEL);
> So you're changing the return value on success from length to 0 here.
> Did you make sure that all callers can handle that?

All the callers presently contain only an error checking condition for a return 
value < 0,
which still applies even with this change. So this wouldn't raise any issues.

>>  }
>>  
>>  /* thanks to drivers/usb/serial/keyspan_pda.c code */
>> @@ -77,11 +67,7 @@ static int emi26_load_firmware (struct usb_device *dev)
>>  int err = -ENOMEM;
>>  int i;
>>  __u32 addr; /* Address to write */
>> -__u8 *buf;
>> -
>> -buf = kmalloc(FW_LOAD_SIZE, GFP_KERNEL);
>> -if (!buf)
>> -goto wraperr;
>> +__u8 buf[FW_LOAD_SIZE];
> As the build bots reported, you must not put large structures like this
> on the stack.

Understood. 
But I'm considering dropping this change (and the one proposed for emi62)
altogether in v3 - since these would end up requiring memory to dynamically 
allocated
twice for the same purpose.
However, if you still think the pros of updating this (and emi62) outweigh the 
cons,
please let me know, and I'll make sure to send in another version fixing it.


>>  
>>  err = request_ihex_firmware(_fw, "emi26/loader.fw", >dev);
>>  if (err)
>> @@ -133,11 +119,11 @@ static int emi26_load_firmware (struct usb_device *dev)
>>  
>>  /* intel hex records are terminated with type 0 element */
>>  while (rec && (i + be16_to_cpu(rec->len) < FW_LOAD_SIZE)) {
>> -memcpy(buf + i, rec->data, be16_to_cpu(rec->len));

Re: [PATCH v2 08/15] usb: misc: idmouse: update to use usb_control_msg_send()

[Anant Thazhemadam]

On 04/12/20 8:16 pm, Johan Hovold wrote:
> On Mon, Nov 30, 2020 at 07:00:31AM +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_send() appropriately.
>>
>> Signed-off-by: Anant Thazhemadam 
>> ---
>>  drivers/usb/misc/idmouse.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
> Especially for control transfers without a data stage there isn't
> really any benefit of the new helper.
>
> I'd just leave this one unchanged.
>
>> diff --git a/drivers/usb/misc/idmouse.c b/drivers/usb/misc/idmouse.c
>> index e9437a176518..52126441a633 100644
>> --- a/drivers/usb/misc/idmouse.c
>> +++ b/drivers/usb/misc/idmouse.c
>> @@ -56,8 +56,9 @@ static const struct usb_device_id idmouse_table[] = {
>>  #define FTIP_SCROLL  0x24
>>  
>>  #define ftip_command(dev, command, value, index) \
>> -usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0), command, \
>> -USB_TYPE_VENDOR | USB_RECIP_ENDPOINT | USB_DIR_OUT, value, index, NULL, 
>> 0, 1000)
>> +usb_control_msg_send(dev->udev, 0, command, \
>> +USB_TYPE_VENDOR | USB_RECIP_ENDPOINT | USB_DIR_OUT, \
>> +value, index, NULL, 0, 1000, GFP_KERNEL)
>>  
>>  MODULE_DEVICE_TABLE(usb, idmouse_table);
> Johan

Understood. I'll make sure this is left out in the v3.

Thanks,
Anant

Re: [f2fs-dev] [PATCH] fs: f2fs: fix potential shift-out-of-bounds error in sanity_check_raw_super()

[Anant Thazhemadam]

On 10/12/20 7:40 am, Chao Yu wrote:
> On 2020/12/10 10:00, Anant Thazhemadam wrote:
>>
>> On 10/12/20 7:16 am, Chao Yu wrote:
>>> Hi Anant,
>>>
>>> I've posted a patch a little earlier. :P
>>>
>>> https://lore.kernel.org/linux-f2fs-devel/20201209084936.31711-1-yuch...@huawei.com/T/#u
>>>
>> Ah well, that's alright, especially considering that your patch looks better.
>> Glad that bug has been fixed nonetheless. :)
>
> Anyway, thanks a lot for taking time to fix f2fs bug. :)
>
> I prefer to add your Signed-off-by into "f2fs: fix shift-out-of-bounds
> in sanity_check_raw_super()" if you don't mind.
>
>

Sure, I'd appreciate that. Thank you! :D

Thanks,
Anant

Re: [f2fs-dev] [PATCH] fs: f2fs: fix potential shift-out-of-bounds error in sanity_check_raw_super()

[Anant Thazhemadam]

On 10/12/20 7:16 am, Chao Yu wrote:
> Hi Anant,
>
> I've posted a patch a little earlier. :P
>
> https://lore.kernel.org/linux-f2fs-devel/20201209084936.31711-1-yuch...@huawei.com/T/#u
>
Ah well, that's alright, especially considering that your patch looks better.
Glad that bug has been fixed nonetheless. :)

Cheers,
Anant

[PATCH] fs: f2fs: fix potential shift-out-of-bounds error in sanity_check_raw_super()

[Anant Thazhemadam]

In sanity_check_raw_super(), if
1 << le32_to_cpu(raw_super->log_blocksize) != F2FS_BLKSIZE, then the
block size is deemed to be invalid.

syzbot triggered a shift-out-of-bounds bug by assigning a value of 59 to
le32_to_cpu(raw_super->log_blocksize).
Although the value assigned itself isn't of much significance, this goes
to show that even if the block size is invalid,
le32_to_cpu(raw_super->log_blocksize) can be potentially evaluated to a
value for which the shift exponent becomes too large for the unsigned
int.

Since 1 << le32_to_cpu(raw_super->log_blocksize) must be = 4096 for a
valid block size, le32_to_cpu(raw_super->log_blocksize) must equal 12.
Replacing the existing check with the more direct sanity check
resolves this bug.

Reported-by: syzbot+ca9a785f8ac472085...@syzkaller.appspotmail.com
Tested-by: syzbot+ca9a785f8ac472085...@syzkaller.appspotmail.com
Signed-off-by: Anant Thazhemadam 
---
 fs/f2fs/super.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 33808c397580..4bc7372af43f 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -2775,7 +2775,6 @@ static int sanity_check_raw_super(struct f2fs_sb_info 
*sbi,
block_t total_sections, blocks_per_seg;
struct f2fs_super_block *raw_super = (struct f2fs_super_block *)
(bh->b_data + F2FS_SUPER_OFFSET);
-   unsigned int blocksize;
size_t crc_offset = 0;
__u32 crc = 0;
 
@@ -2802,10 +2801,8 @@ static int sanity_check_raw_super(struct f2fs_sb_info 
*sbi,
}
 
/* Currently, support only 4KB block size */
-   blocksize = 1 << le32_to_cpu(raw_super->log_blocksize);
-   if (blocksize != F2FS_BLKSIZE) {
-   f2fs_info(sbi, "Invalid blocksize (%u), supports only 4KB",
- blocksize);
+   if (le32_to_cpu(raw_super->log_blocksize) != 12) {
+   f2fs_info(sbi, "Invalid blocksize. Only 4KB supported");
return -EFSCORRUPTED;
}
 
-- 
2.25.1

Re: [PATCH] fs: quota: fix array-index-out-of-bounds bug by passing correct argument to vfs_cleanup_quota_inode()

[Anant Thazhemadam]

On 09/12/20 2:37 pm, Jan Kara wrote:
> On Wed 09-12-20 01:13:38, Anant Thazhemadam wrote:
>> When dquot_resume() was last updated, the argument that got passed
>> to vfs_cleanup_quota_inode was incorrectly set.
>>
>> If type = -1 and dquot_load_quota_sb() returns a negative value,
>> then vfs_cleanup_quota_inode() gets called with -1 passed as an
>> argument, and this leads to an array-index-out-of-bounds bug.
>>
>> Fix this issue by correctly passing the arguments.
>>
>> Fixes: ae45f07d47cc ("quota: Simplify dquot_resume()")
>> Reported-by: syzbot+2643e825238d7aabb...@syzkaller.appspotmail.com
>> Tested-by: syzbot+2643e825238d7aabb...@syzkaller.appspotmail.com
>> Signed-off-by: Anant Thazhemadam 
> Thanks for the fix! I've just queued the very same fix I wrote yesterday to
> my tree. But yours has better changelog so let me pick your patch instead
> ;)

Glad to hear that. Thank you! :D

> For next time, how can we avoid collisions like this? Did you work on the fix
> based on the syzbot email sent to the list so if I actually reply to the
> syzbot email that I'm working on / already have a fix you'd see it?

I came across the bug on the syzbot dashboard, and not through the mailing list.
But even if I did come across this on the mailing list, there is the still a 
fair chance
that I could've come across this bug, and started working on it before replied 
to
the syzbot email, right?
I can't speak for everyone, but even if I see a bug on the mailing list, I go 
over to
the dashboard, and get the apt .config and reproducer from there, and try to 
work
on it; almost never checking that initial syzbot mail again.

However, iirc there have been previous discussions regarding this on
the mailing lists (although I'm not sure where I came across them :/ ).
For this reason I've Cc-ed Dmitry onto this reply, and hopefully he'll be able 
to direct
you to those conversations, and also validate any new ideas you might have.
I'd be more than happy to contribute too if I can add any value to the 
discussion
around that, and to whatever ideas you may have, since this is a issue that has
been around for quite a while now. :)

Hope this helps,
Anant

[PATCH] fs: quota: fix array-index-out-of-bounds bug by passing correct argument to vfs_cleanup_quota_inode()

[Anant Thazhemadam]

When dquot_resume() was last updated, the argument that got passed
to vfs_cleanup_quota_inode was incorrectly set.

If type = -1 and dquot_load_quota_sb() returns a negative value,
then vfs_cleanup_quota_inode() gets called with -1 passed as an
argument, and this leads to an array-index-out-of-bounds bug.

Fix this issue by correctly passing the arguments.

Fixes: ae45f07d47cc ("quota: Simplify dquot_resume()")
Reported-by: syzbot+2643e825238d7aabb...@syzkaller.appspotmail.com
Tested-by: syzbot+2643e825238d7aabb...@syzkaller.appspotmail.com
Signed-off-by: Anant Thazhemadam 
---
If type = -1 is passed as an argument to vfs_cleanup_quota_inode(),
it causes an array-index-out-of-bounds error since dqopt->files[-1]
can be potentially attempted to be accessed.
Before the bisected commit introduced this bug, vfs_load_quota_inode()
was being directly called in dquot_resume(), and subsequently 
vfs_cleanup_quota_inode() was called with the cnt value as argument.

 fs/quota/dquot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index bb02989d92b6..4f1373463766 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -2455,7 +2455,7 @@ int dquot_resume(struct super_block *sb, int type)
ret = dquot_load_quota_sb(sb, cnt, dqopt->info[cnt].dqi_fmt_id,
  flags);
if (ret < 0)
-   vfs_cleanup_quota_inode(sb, type);
+   vfs_cleanup_quota_inode(sb, cnt);
}
 
return ret;
-- 
2.25.1

[PATCH] media: usb: dvd-usb: fix uninit-value bug in dibusb_read_eeprom_byte()

[Anant Thazhemadam]

In dibusb_read_eeprom_byte(), if dibusb_i2c_msg() fails, val gets
assigned an value that's not properly initialized.
Using kzalloc() in place of kmalloc() for the buffer fixes this issue,
as the val can now be set to 0 in the event dibusb_i2c_msg() fails.

Reported-by: syzbot+e27b4fd589762b0b9...@syzkaller.appspotmail.com
Tested-by: syzbot+e27b4fd589762b0b9...@syzkaller.appspotmail.com
Signed-off-by: Anant Thazhemadam 
---
 drivers/media/usb/dvb-usb/dibusb-common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/usb/dvb-usb/dibusb-common.c 
b/drivers/media/usb/dvb-usb/dibusb-common.c
index 02b51d1a1b67..aff60c10cb0b 100644
--- a/drivers/media/usb/dvb-usb/dibusb-common.c
+++ b/drivers/media/usb/dvb-usb/dibusb-common.c
@@ -223,7 +223,7 @@ int dibusb_read_eeprom_byte(struct dvb_usb_device *d, u8 
offs, u8 *val)
u8 *buf;
int rc;
 
-   buf = kmalloc(2, GFP_KERNEL);
+   buf = kzalloc(2, GFP_KERNEL);
if (!buf)
return -ENOMEM;
 
-- 
2.25.1

[PATCH] net: wireless: validate key indexes for cfg80211_registered_device

[Anant Thazhemadam]

syzbot discovered a bug in which an OOB access was being made because
an unsuitable key_idx value was wrongly considered to be acceptable
while deleting a key in nl80211_del_key().

Since we don't know the cipher at the time of deletion, if
cfg80211_validate_key_settings() were to be called directly in
nl80211_del_key(), even valid keys would be wrongly determined invalid,
and deletion wouldn't occur correctly.
For this reason, a new function - cfg80211_valid_key_idx(), has been
created, to determine if the key_idx value provided is valid or not.
cfg80211_valid_key_idx() is directly called in 2 places -
nl80211_del_key(), and cfg80211_validate_key_settings().

Reported-by: syzbot+49d4cab497c2142ee...@syzkaller.appspotmail.com
Tested-by: syzbot+49d4cab497c2142ee...@syzkaller.appspotmail.com
Suggested-by: Johannes Berg 
Signed-off-by: Anant Thazhemadam 
---
For the bug that was getting triggered, pairwise was true, and 
the NL80211_EXT_FEATURE_BEACON_PROTECTION feature was set too.
The control logic for cfg80211_validate_key_settings() has been
designed keeping this also in mind.

 net/wireless/core.h|  2 ++
 net/wireless/nl80211.c |  7 ---
 net/wireless/util.c| 27 ---
 3 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/net/wireless/core.h b/net/wireless/core.h
index e3e9686859d4..7df91f940212 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -433,6 +433,8 @@ void cfg80211_sme_abandon_assoc(struct wireless_dev *wdev);
 
 /* internal helpers */
 bool cfg80211_supported_cipher_suite(struct wiphy *wiphy, u32 cipher);
+bool cfg80211_valid_key_idx(struct cfg80211_registered_device *rdev,
+   int key_idx, bool pairwise);
 int cfg80211_validate_key_settings(struct cfg80211_registered_device *rdev,
   struct key_params *params, int key_idx,
   bool pairwise, const u8 *mac_addr);
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index a77174b99b07..db36158911ae 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -4260,9 +4260,6 @@ static int nl80211_del_key(struct sk_buff *skb, struct 
genl_info *info)
if (err)
return err;
 
-   if (key.idx < 0)
-   return -EINVAL;
-
if (info->attrs[NL80211_ATTR_MAC])
mac_addr = nla_data(info->attrs[NL80211_ATTR_MAC]);
 
@@ -4278,6 +4275,10 @@ static int nl80211_del_key(struct sk_buff *skb, struct 
genl_info *info)
key.type != NL80211_KEYTYPE_GROUP)
return -EINVAL;
 
+   if (!cfg80211_valid_key_idx(rdev, key.idx,
+   key.type == NL80211_KEYTYPE_PAIRWISE))
+   return -EINVAL;
+
if (!rdev->ops->del_key)
return -EOPNOTSUPP;
 
diff --git a/net/wireless/util.c b/net/wireless/util.c
index f01746894a4e..07b17feb9b1e 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -272,18 +272,31 @@ bool cfg80211_supported_cipher_suite(struct wiphy *wiphy, 
u32 cipher)
return false;
 }
 
-int cfg80211_validate_key_settings(struct cfg80211_registered_device *rdev,
-  struct key_params *params, int key_idx,
-  bool pairwise, const u8 *mac_addr)
+bool cfg80211_valid_key_idx(struct cfg80211_registered_device *rdev,
+   int key_idx, bool pairwise)
 {
int max_key_idx = 5;
 
-   if (wiphy_ext_feature_isset(>wiphy,
-   NL80211_EXT_FEATURE_BEACON_PROTECTION) ||
-   wiphy_ext_feature_isset(>wiphy,
-   
NL80211_EXT_FEATURE_BEACON_PROTECTION_CLIENT))
+   if (pairwise)
+   max_key_idx = 3;
+
+   else if (wiphy_ext_feature_isset(>wiphy,
+NL80211_EXT_FEATURE_BEACON_PROTECTION) 
||
+wiphy_ext_feature_isset(>wiphy,
+
NL80211_EXT_FEATURE_BEACON_PROTECTION_CLIENT))
max_key_idx = 7;
+
if (key_idx < 0 || key_idx > max_key_idx)
+   return false;
+
+   return true;
+}
+
+int cfg80211_validate_key_settings(struct cfg80211_registered_device *rdev,
+  struct key_params *params, int key_idx,
+  bool pairwise, const u8 *mac_addr)
+{
+   if (!cfg80211_valid_key_idx(rdev, key_idx, pairwise))
return -EINVAL;
 
if (!pairwise && mac_addr && !(rdev->wiphy.flags & WIPHY_FLAG_IBSS_RSN))
-- 
2.25.1

Re: [PATCH] net: mac80211: cfg: enforce sanity checks for key_index in ieee80211_del_key()

[Anant Thazhemadam]

On 01/12/20 5:36 pm, Johannes Berg wrote:
> On Tue, 2020-12-01 at 17:26 +0530, Anant Thazhemadam wrote:
>> On 01/12/20 3:30 pm, Johannes Berg wrote:
>>> On Tue, 2020-12-01 at 15:26 +0530, Anant Thazhemadam wrote:
>>>> Currently, it is assumed that key_idx values that are passed to
>>>> ieee80211_del_key() are all valid indexes as is, and no sanity checks
>>>> are performed for it.
>>>> However, syzbot was able to trigger an array-index-out-of-bounds bug
>>>> by passing a key_idx value of 5, when the maximum permissible index
>>>> value is (NUM_DEFAULT_KEYS - 1).
>>>> Enforcing sanity checks helps in preventing this bug, or a similar
>>>> instance in the context of ieee80211_del_key() from occurring.
>>> I think we should do this more generally in cfg80211, like in
>>> nl80211_new_key() we do it via cfg80211_validate_key_settings().
>>>
>>> I suppose we cannot use the same function, but still, would be good to
>>> address this generally in nl80211 for all drivers.
>> Hello,
>>
>> This gave me the idea of trying to use cfg80211_validate_key_settings()
>> directly in ieee80211_del_key(). I did try that out, tested it, and this bug
>> doesn't seem to be getting triggered anymore.
>> If this is okay, then I can send in a v2 soon. :)
>>
>> If there is any reason that I'm missing as to why 
>> cfg80211_validate_key_settings()
>> cannot be used in this context, please let me know.
> If it works then I guess that's OK. I thought we didn't have all the
> right information, e.g. whether a key is pairwise or not?
>
> johannes
>
Well,
cfg80211_supported_cipher_suite(>wiphy, params->cipher) returned
false, and thus it worked for the syzbot reproducer.
Would it be a safer idea to enforce the conditions that I initially put (in
ieee80211_del_key()) directly in cfg80211_validate_key_settings() itself - by
updating max_key_index, and checking accordingly?

Thanks,
Anant

Re: [PATCH] net: mac80211: cfg: enforce sanity checks for key_index in ieee80211_del_key()

[Anant Thazhemadam]

On 01/12/20 3:30 pm, Johannes Berg wrote:
> On Tue, 2020-12-01 at 15:26 +0530, Anant Thazhemadam wrote:
>> Currently, it is assumed that key_idx values that are passed to
>> ieee80211_del_key() are all valid indexes as is, and no sanity checks
>> are performed for it.
>> However, syzbot was able to trigger an array-index-out-of-bounds bug
>> by passing a key_idx value of 5, when the maximum permissible index
>> value is (NUM_DEFAULT_KEYS - 1).
>> Enforcing sanity checks helps in preventing this bug, or a similar
>> instance in the context of ieee80211_del_key() from occurring.
> I think we should do this more generally in cfg80211, like in
> nl80211_new_key() we do it via cfg80211_validate_key_settings().
>
> I suppose we cannot use the same function, but still, would be good to
> address this generally in nl80211 for all drivers.

Hello,

This gave me the idea of trying to use cfg80211_validate_key_settings()
directly in ieee80211_del_key(). I did try that out, tested it, and this bug
doesn't seem to be getting triggered anymore.
If this is okay, then I can send in a v2 soon. :)

If there is any reason that I'm missing as to why 
cfg80211_validate_key_settings()
cannot be used in this context, please let me know.

Thanks,
Anant

[PATCH] net: mac80211: cfg: enforce sanity checks for key_index in ieee80211_del_key()

[Anant Thazhemadam]

Currently, it is assumed that key_idx values that are passed to
ieee80211_del_key() are all valid indexes as is, and no sanity checks
are performed for it.
However, syzbot was able to trigger an array-index-out-of-bounds bug
by passing a key_idx value of 5, when the maximum permissible index
value is (NUM_DEFAULT_KEYS - 1).
Enforcing sanity checks helps in preventing this bug, or a similar
instance in the context of ieee80211_del_key() from occurring.

Reported-by: syzbot+49d4cab497c2142ee...@syzkaller.appspotmail.com
Tested-by: syzbot+49d4cab497c2142ee...@syzkaller.appspotmail.com
Signed-off-by: Anant Thazhemadam 
---
 net/mac80211/cfg.c | 24 +---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 7276e66ae435..d349e33134e6 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -516,12 +516,30 @@ static int ieee80211_del_key(struct wiphy *wiphy, struct 
net_device *dev,
if (!sta)
goto out_unlock;
 
-   if (pairwise)
+   if (pairwise) {
+   if (key_idx >= NUM_DEFAULT_KEYS) {
+   ret = -EINVAL;
+   goto out_unlock;
+   }
key = key_mtx_dereference(local, sta->ptk[key_idx]);
-   else
+   } else {
+   if (key_idx >= (NUM_DEFAULT_KEYS +
+   NUM_DEFAULT_MGMT_KEYS +
+   NUM_DEFAULT_BEACON_KEYS)) {
+   ret = -EINVAL;
+   goto out_unlock;
+   }
key = key_mtx_dereference(local, sta->gtk[key_idx]);
-   } else
+   }
+   } else {
+   if (key_idx >= (NUM_DEFAULT_KEYS +
+   NUM_DEFAULT_MGMT_KEYS +
+   NUM_DEFAULT_BEACON_KEYS)) {
+   ret = -EINVAL;
+   goto out_unlock;
+   }
key = key_mtx_dereference(local, sdata->keys[key_idx]);
+   }
 
if (!key) {
ret = -ENOENT;
-- 
2.25.1

[PATCH v2 15/15] usb: misc: usbtest: update to use the usb_control_msg_{send|recv}() API

[Anant Thazhemadam]

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, instances of usb_control_msg() have been replaced with
usb_control_msg_{recv|send}() and the return value checking conditions have
also been modified appropriately.

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/usbtest.c | 69 --
 1 file changed, 29 insertions(+), 40 deletions(-)

diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index 150090ee4ec1..4337eff2a749 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -672,19 +672,15 @@ static int get_altsetting(struct usbtest_dev *dev)
struct usb_device   *udev = interface_to_usbdev(iface);
int retval;
 
-   retval = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
-   USB_REQ_GET_INTERFACE, USB_DIR_IN|USB_RECIP_INTERFACE,
-   0, iface->altsetting[0].desc.bInterfaceNumber,
-   dev->buf, 1, USB_CTRL_GET_TIMEOUT);
-   switch (retval) {
-   case 1:
-   return dev->buf[0];
-   case 0:
-   retval = -ERANGE;
-   fallthrough;
-   default:
+   retval = usb_control_msg_recv(udev, 0, USB_REQ_GET_INTERFACE,
+ USB_DIR_IN|USB_RECIP_INTERFACE,
+ 0, 
iface->altsetting[0].desc.bInterfaceNumber,
+ dev->buf, 1, USB_CTRL_GET_TIMEOUT, 
GFP_KERNEL);
+
+   if (retval < 0)
return retval;
-   }
+
+   return dev->buf[0];
 }
 
 static int set_altsetting(struct usbtest_dev *dev, int alternate)
@@ -872,14 +868,15 @@ static int ch9_postconfig(struct usbtest_dev *dev)
 * ... although some cheap devices (like one TI Hub I've got)
 * won't return config descriptors except before set_config.
 */
-   retval = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
-   USB_REQ_GET_CONFIGURATION,
-   USB_DIR_IN | USB_RECIP_DEVICE,
-   0, 0, dev->buf, 1, USB_CTRL_GET_TIMEOUT);
-   if (retval != 1 || dev->buf[0] != expected) {
+   retval = usb_control_msg_recv(udev, 0, 
USB_REQ_GET_CONFIGURATION,
+ USB_DIR_IN | USB_RECIP_DEVICE,  0,
+ 0, dev->buf, 1, 
USB_CTRL_GET_TIMEOUT,
+ GFP_KERNEL);
+
+   if (retval != 0 || dev->buf[0] != expected) {
dev_err(>dev, "get config --> %d %d (1 %d)\n",
retval, dev->buf[0], expected);
-   return (retval < 0) ? retval : -EDOM;
+   return retval;
}
}
 
@@ -1683,10 +1680,10 @@ static int test_halt(struct usbtest_dev *tdev, int ep, 
struct urb *urb)
return retval;
 
/* set halt (protocol test only), verify it worked */
-   retval = usb_control_msg(urb->dev, usb_sndctrlpipe(urb->dev, 0),
-   USB_REQ_SET_FEATURE, USB_RECIP_ENDPOINT,
-   USB_ENDPOINT_HALT, ep,
-   NULL, 0, USB_CTRL_SET_TIMEOUT);
+   retval = usb_control_msg_send(urb->dev, 0, USB_REQ_SET_FEATURE,
+ USB_RECIP_ENDPOINT, USB_ENDPOINT_HALT,
+ ep, NULL, 0, USB_CTRL_SET_TIMEOUT,
+ GFP_KERNEL);
if (retval < 0) {
ERROR(tdev, "ep %02x couldn't set halt, %d\n", ep, retval);
return retval;
@@ -1845,30 +1842,22 @@ static int ctrl_out(struct usbtest_dev *dev,
/* write patterned data */
for (j = 0; j < len; j++)
buf[j] = (u8)(i + j);
-   retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
-   0x5b, USB_DIR_OUT|USB_TYPE_VENDOR,
-   0, 0, buf, len, USB_CTRL_SET_TIMEOUT);
-   if (retval != len) {
+   retval = usb_control_msg_send(udev, 0, 0x5b,
+ USB_DIR_OUT | USB_TYPE_VENDOR, 0,
+ 0, buf, len, USB_CTRL_SET_TIMEOUT,
+ GFP_KERNEL);
+   if (retval < 0) {
what = "write";
-   if (retval >= 0) {
-   ERROR(dev, "ctrl_out, wlen %d (expected %d)\n",
-

[PATCH v2 14/15] usb: misc: usbsevseg: update to use usb_control_msg_send()

[Anant Thazhemadam]

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, instances of usb_control_msg() have been replaced with
usb_control_msg_send() appropriately.

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/usbsevseg.c | 60 ++--
 1 file changed, 17 insertions(+), 43 deletions(-)

diff --git a/drivers/usb/misc/usbsevseg.c b/drivers/usb/misc/usbsevseg.c
index 551074f5b7ad..ade4bc58d5f2 100644
--- a/drivers/usb/misc/usbsevseg.c
+++ b/drivers/usb/misc/usbsevseg.c
@@ -74,15 +74,10 @@ static void update_display_powered(struct usb_sevsegdev 
*mydev)
if (mydev->shadow_power != 1)
return;
 
-   rc = usb_control_msg(mydev->udev,
-   usb_sndctrlpipe(mydev->udev, 0),
-   0x12,
-   0x48,
-   (80 * 0x100) + 10, /*  (power mode) */
-   (0x00 * 0x100) + (mydev->powered ? 1 : 0),
-   NULL,
-   0,
-   2000);
+   rc = usb_control_msg_send(mydev->udev, 0, 0x12, 0x48,
+ (80 * 0x100) + 10, /*  (power mode) */
+ (0x00 * 0x100) + (mydev->powered ? 1 : 0),
+ NULL, 0, 2000, GFP_KERNEL);
if (rc < 0)
dev_dbg(>udev->dev, "power retval = %d\n", rc);
 
@@ -99,15 +94,10 @@ static void update_display_mode(struct usb_sevsegdev *mydev)
if(mydev->shadow_power != 1)
return;
 
-   rc = usb_control_msg(mydev->udev,
-   usb_sndctrlpipe(mydev->udev, 0),
-   0x12,
-   0x48,
-   (82 * 0x100) + 10, /* (set mode) */
-   (mydev->mode_msb * 0x100) + mydev->mode_lsb,
-   NULL,
-   0,
-   2000);
+   rc = usb_control_msg_send(mydev->udev, 0, 0x12, 0x48,
+ (82 * 0x100) + 10, /* (set mode) */
+ (mydev->mode_msb * 0x100) + mydev->mode_lsb,
+ NULL, 0, 2000, GFP_KERNEL);
 
if (rc < 0)
dev_dbg(>udev->dev, "mode retval = %d\n", rc);
@@ -117,48 +107,32 @@ static void update_display_visual(struct usb_sevsegdev 
*mydev, gfp_t mf)
 {
int rc;
int i;
-   unsigned char *buffer;
+   unsigned char buffer[MAXLEN] = {0};
u8 decimals = 0;
 
if(mydev->shadow_power != 1)
return;
 
-   buffer = kzalloc(MAXLEN, mf);
-   if (!buffer)
-   return;
-
/* The device is right to left, where as you write left to right */
for (i = 0; i < mydev->textlength; i++)
buffer[i] = mydev->text[mydev->textlength-1-i];
 
-   rc = usb_control_msg(mydev->udev,
-   usb_sndctrlpipe(mydev->udev, 0),
-   0x12,
-   0x48,
-   (85 * 0x100) + 10, /* (write text) */
-   (0 * 0x100) + mydev->textmode, /* mode  */
-   buffer,
-   mydev->textlength,
-   2000);
+   rc = usb_control_msg_send(mydev->udev, 0, 0x12, 0x48,
+ (85 * 0x100) + 10, /* (write text) */
+ (0 * 0x100) + mydev->textmode, /* mode  */
+ , mydev->textlength, 2000, mf);
 
if (rc < 0)
dev_dbg(>udev->dev, "write retval = %d\n", rc);
 
-   kfree(buffer);
-
/* The device is right to left, where as you write left to right */
for (i = 0; i < sizeof(mydev->decimals); i++)
decimals |= mydev->decimals[i] << i;
 
-   rc = usb_control_msg(mydev->udev,
-   usb_sndctrlpipe(mydev->udev, 0),
-   0x12,
-   0x48,
-   (86 * 0x100) + 10, /* (set decimal) */
-   (0 * 0x100) + decimals, /* decimals */
-   NULL,
-   0,
-   2000);
+   rc = usb_control_msg_send(mydev->udev, 0, 0x12, 0x48,
+ (86 * 0x100) + 10, /* (set decimal) */
+ (0 * 0x100) + decimals, /* decimals */
+ NULL, 0, 2000, mf);
 
if (rc < 0)
dev_dbg(>udev->dev, "decimal retval = %d\n", rc);
-- 
2.25.1

[PATCH v2 13/15] usb: misc: trancevibrator: update to use usb_control_msg_send()

[Anant Thazhemadam]

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_send() and the return value checking condition has also
been modified appropriately.

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/trancevibrator.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/misc/trancevibrator.c 
b/drivers/usb/misc/trancevibrator.c
index a3dfc77578ea..c50807b4f4ef 100644
--- a/drivers/usb/misc/trancevibrator.c
+++ b/drivers/usb/misc/trancevibrator.c
@@ -59,11 +59,11 @@ static ssize_t speed_store(struct device *dev, struct 
device_attribute *attr,
dev_dbg(>udev->dev, "speed = %d\n", tv->speed);
 
/* Set speed */
-   retval = usb_control_msg(tv->udev, usb_sndctrlpipe(tv->udev, 0),
+   retval = usb_control_msg_send(tv->udev, 0,
 0x01, /* vendor request: set speed */
 USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_OTHER,
 tv->speed, /* speed value */
-0, NULL, 0, USB_CTRL_GET_TIMEOUT);
+0, NULL, 0, USB_CTRL_GET_TIMEOUT, GFP_KERNEL);
if (retval) {
tv->speed = old;
dev_dbg(>udev->dev, "retval = %d\n", retval);
-- 
2.25.1

[PATCH v2 11/15] usb: misc: ldusb: update to use usb_control_msg_send()

[Anant Thazhemadam]

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_send() has been replaced
with usb_control_msg_send() appropriately.

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/ldusb.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/misc/ldusb.c b/drivers/usb/misc/ldusb.c
index 670e4d91e9ca..259ead4edecb 100644
--- a/drivers/usb/misc/ldusb.c
+++ b/drivers/usb/misc/ldusb.c
@@ -573,15 +573,13 @@ static ssize_t ld_usb_write(struct file *file, const char 
__user *buffer,
}
 
if (dev->interrupt_out_endpoint == NULL) {
-   /* try HID_REQ_SET_REPORT=9 on control_endpoint instead of 
interrupt_out_endpoint */
-   retval = usb_control_msg(interface_to_usbdev(dev->intf),
-
usb_sndctrlpipe(interface_to_usbdev(dev->intf), 0),
-9,
+   retval = usb_control_msg_send(interface_to_usbdev(dev->intf),
+0, 9,
 USB_TYPE_CLASS | USB_RECIP_INTERFACE | 
USB_DIR_OUT,
 1 << 8, 0,
 dev->interrupt_out_buffer,
 bytes_to_write,
-USB_CTRL_SET_TIMEOUT);
+USB_CTRL_SET_TIMEOUT, GFP_KERNEL);
if (retval < 0)
dev_err(>intf->dev,
"Couldn't submit HID_REQ_SET_REPORT %d\n",
-- 
2.25.1

[PATCH v2 12/15] usb: misc: lvstest: update to use the usb_control_msg_{send|recv}() API

[Anant Thazhemadam]

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, instances of usb_control_msg() have been replaced with
usb_control_msg_{recv|send}() and the return value checking conditions have
also been modified appropriately.

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/lvstest.c | 38 --
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/misc/lvstest.c b/drivers/usb/misc/lvstest.c
index f8686139d6f3..daa1b2c9139f 100644
--- a/drivers/usb/misc/lvstest.c
+++ b/drivers/usb/misc/lvstest.c
@@ -85,17 +85,17 @@ static void destroy_lvs_device(struct usb_device *udev)
 static int lvs_rh_clear_port_feature(struct usb_device *hdev,
int port1, int feature)
 {
-   return usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
+   return usb_control_msg_send(hdev, 0,
USB_REQ_CLEAR_FEATURE, USB_RT_PORT, feature, port1,
-   NULL, 0, 1000);
+   NULL, 0, 1000, GFP_KERNEL);
 }
 
 static int lvs_rh_set_port_feature(struct usb_device *hdev,
int port1, int feature)
 {
-   return usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
+   return usb_control_msg_send(hdev, 0,
USB_REQ_SET_FEATURE, USB_RT_PORT, feature, port1,
-   NULL, 0, 1000);
+   NULL, 0, 1000, GFP_KERNEL);
 }
 
 static ssize_t u3_entry_store(struct device *dev,
@@ -257,13 +257,9 @@ static ssize_t get_dev_desc_store(struct device *dev,
 {
struct usb_interface *intf = to_usb_interface(dev);
struct usb_device *udev;
-   struct usb_device_descriptor *descriptor;
+   struct usb_device_descriptor descriptor;
int ret;
 
-   descriptor = kmalloc(sizeof(*descriptor), GFP_KERNEL);
-   if (!descriptor)
-   return -ENOMEM;
-
udev = create_lvs_device(intf);
if (!udev) {
dev_err(dev, "failed to create lvs device\n");
@@ -271,18 +267,16 @@ static ssize_t get_dev_desc_store(struct device *dev,
goto free_desc;
}
 
-   ret = usb_control_msg(udev, (PIPE_CONTROL << 30) | USB_DIR_IN,
-   USB_REQ_GET_DESCRIPTOR, USB_DIR_IN, USB_DT_DEVICE << 8,
-   0, descriptor, sizeof(*descriptor),
-   USB_CTRL_GET_TIMEOUT);
+   ret = usb_control_msg_recv(udev, 0, USB_REQ_GET_DESCRIPTOR,
+  USB_DIR_IN, USB_DT_DEVICE << 8,
+  0, , sizeof(descriptor),
+  USB_CTRL_GET_TIMEOUT, GFP_KERNEL);
if (ret < 0)
dev_err(dev, "can't read device descriptor %d\n", ret);
 
destroy_lvs_device(udev);
 
 free_desc:
-   kfree(descriptor);
-
if (ret < 0)
return ret;
 
@@ -336,10 +330,10 @@ static void lvs_rh_work(struct work_struct *work)
 
/* Examine each root port */
for (i = 1; i <= descriptor->bNbrPorts; i++) {
-   ret = usb_control_msg(hdev, usb_rcvctrlpipe(hdev, 0),
+   ret = usb_control_msg_recv(hdev, 0,
USB_REQ_GET_STATUS, USB_DIR_IN | USB_RT_PORT, 0, i,
-   port_status, sizeof(*port_status), 1000);
-   if (ret < 4)
+   port_status, sizeof(*port_status), 1000, GFP_KERNEL);
+   if (ret < 0)
continue;
 
portchange = le16_to_cpu(port_status->wPortChange);
@@ -420,13 +414,13 @@ static int lvs_rh_probe(struct usb_interface *intf,
usb_set_intfdata(intf, lvs);
 
/* how many number of ports this root hub has */
-   ret = usb_control_msg(hdev, usb_rcvctrlpipe(hdev, 0),
+   ret = usb_control_msg_recv(hdev, 0,
USB_REQ_GET_DESCRIPTOR, USB_DIR_IN | USB_RT_HUB,
USB_DT_SS_HUB << 8, 0, >descriptor,
-   USB_DT_SS_HUB_SIZE, USB_CTRL_GET_TIMEOUT);
-   if (ret < (USB_DT_HUB_NONVAR_SIZE + 2)) {
+   USB_DT_SS_HUB_SIZE, USB_CTRL_GET_TIMEOUT, GFP_KERNEL);
+   if (ret < 0) {
dev_err(>dev, "wrong root hub descriptor read %d\n", ret);
-   return ret < 0 ? ret : -EINVAL;
+   return ret;
}
 
/* submit urb to poll interrupt endpoint */
-- 
2.25.1

[PATCH v2 10/15] usb: misc: isight_firmware: update to use usb_control_msg_send()

[Anant Thazhemadam]

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 instances of usb_control_msg() have been replaced with
usb_control_msg_send(), and return value checking has also been
appropriately enforced.

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/isight_firmware.c | 30 +-
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/misc/isight_firmware.c 
b/drivers/usb/misc/isight_firmware.c
index 4d30095d6ad2..1bd14a431f6c 100644
--- a/drivers/usb/misc/isight_firmware.c
+++ b/drivers/usb/misc/isight_firmware.c
@@ -37,13 +37,10 @@ static int isight_firmware_load(struct usb_interface *intf,
struct usb_device *dev = interface_to_usbdev(intf);
int llen, len, req, ret = 0;
const struct firmware *firmware;
-   unsigned char *buf = kmalloc(50, GFP_KERNEL);
+   unsigned char buf[50];
unsigned char data[4];
const u8 *ptr;
 
-   if (!buf)
-   return -ENOMEM;
-
if (request_firmware(, "isight.fw", >dev) != 0) {
printk(KERN_ERR "Unable to load isight firmware\n");
ret = -ENODEV;
@@ -53,11 +50,11 @@ static int isight_firmware_load(struct usb_interface *intf,
ptr = firmware->data;
 
buf[0] = 0x01;
-   if (usb_control_msg
-   (dev, usb_sndctrlpipe(dev, 0), 0xa0, 0x40, 0xe600, 0, buf, 1,
-300) != 1) {
+   ret = usb_control_msg_send(dev, 0, 0xa0, 0x40, 0xe600,
+  0, , 1, 300, GFP_KERNEL);
+   if (ret != 0) {
printk(KERN_ERR
-  "Failed to initialise isight firmware loader\n");
+   "Failed to initialise isight firmware loader\n");
ret = -ENODEV;
goto out;
}
@@ -82,15 +79,15 @@ static int isight_firmware_load(struct usb_interface *intf,
ret = -ENODEV;
goto out;
}
-   memcpy(buf, ptr, llen);
+   memcpy(, ptr, llen);
 
ptr += llen;
 
-   if (usb_control_msg
-   (dev, usb_sndctrlpipe(dev, 0), 0xa0, 0x40, req, 0,
-buf, llen, 300) != llen) {
+   ret = usb_control_msg_send(dev, 0, 0xa0, 0x40, req, 0,
+  , llen, 300, GFP_KERNEL);
+   if (ret != 0) {
printk(KERN_ERR
-  "Failed to load isight firmware\n");
+   "Failed to load isight firmware\n");
ret = -ENODEV;
goto out;
}
@@ -99,15 +96,14 @@ static int isight_firmware_load(struct usb_interface *intf,
}
 
buf[0] = 0x00;
-   if (usb_control_msg
-   (dev, usb_sndctrlpipe(dev, 0), 0xa0, 0x40, 0xe600, 0, buf, 1,
-300) != 1) {
+   ret = usb_control_msg_send(dev, 0, 0xa0, 0x40, 0xe600,
+  0, , 1, 300, GFP_KERNEL);
+   if (ret != 0) {
printk(KERN_ERR "isight firmware loading completion failed\n");
ret = -ENODEV;
}
 
 out:
-   kfree(buf);
release_firmware(firmware);
return ret;
 }
-- 
2.25.1

[PATCH v2 09/15] usb: misc: iowarrior: update to use the usb_control_msg_{send|recv}() API

[Anant Thazhemadam]

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, instances of usb_control_msg() have been replaced with
usb_control_msg_{recv|send}() appropriately.

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/iowarrior.c | 34 +-
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c
index 70ec29681526..53726fffe5df 100644
--- a/drivers/usb/misc/iowarrior.c
+++ b/drivers/usb/misc/iowarrior.c
@@ -109,12 +109,12 @@ static int usb_get_report(struct usb_device *dev,
  struct usb_host_interface *inter, unsigned char type,
  unsigned char id, void *buf, int size)
 {
-   return usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
-  USB_REQ_GET_REPORT,
-  USB_DIR_IN | USB_TYPE_CLASS |
-  USB_RECIP_INTERFACE, (type << 8) + id,
-  inter->desc.bInterfaceNumber, buf, size,
-  GET_TIMEOUT*HZ);
+   return usb_control_msg_recv(dev, 0,
+   USB_REQ_GET_REPORT,
+   USB_DIR_IN | USB_TYPE_CLASS |
+   USB_RECIP_INTERFACE, (type << 8) + id,
+   inter->desc.bInterfaceNumber, buf, size,
+   GET_TIMEOUT*HZ, GFP_KERNEL);
 }
 //#endif
 
@@ -123,13 +123,13 @@ static int usb_get_report(struct usb_device *dev,
 static int usb_set_report(struct usb_interface *intf, unsigned char type,
  unsigned char id, void *buf, int size)
 {
-   return usb_control_msg(interface_to_usbdev(intf),
-  usb_sndctrlpipe(interface_to_usbdev(intf), 0),
-  USB_REQ_SET_REPORT,
-  USB_TYPE_CLASS | USB_RECIP_INTERFACE,
-  (type << 8) + id,
-  intf->cur_altsetting->desc.bInterfaceNumber, buf,
-  size, HZ);
+   return usb_control_msg_send(interface_to_usbdev(intf),
+   0,
+   USB_REQ_SET_REPORT,
+   USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+   (type << 8) + id,
+   
intf->cur_altsetting->desc.bInterfaceNumber, buf,
+   size, HZ, GFP_KERNEL);
 }
 
 /*-*/
@@ -854,10 +854,10 @@ static int iowarrior_probe(struct usb_interface 
*interface,
 
/* Set the idle timeout to 0, if this is interface 0 */
if (dev->interface->cur_altsetting->desc.bInterfaceNumber == 0) {
-   usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
-   0x0A,
-   USB_TYPE_CLASS | USB_RECIP_INTERFACE, 0,
-   0, NULL, 0, USB_CTRL_SET_TIMEOUT);
+   usb_control_msg_send(udev, 0,
+0x0A,
+USB_TYPE_CLASS | USB_RECIP_INTERFACE, 0,
+0, NULL, 0, USB_CTRL_SET_TIMEOUT, 
GFP_KERNEL);
}
/* allow device read and ioctl */
dev->present = 1;
-- 
2.25.1

[PATCH v2 08/15] usb: misc: idmouse: update to use usb_control_msg_send()

[Anant Thazhemadam]

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

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/idmouse.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/misc/idmouse.c b/drivers/usb/misc/idmouse.c
index e9437a176518..52126441a633 100644
--- a/drivers/usb/misc/idmouse.c
+++ b/drivers/usb/misc/idmouse.c
@@ -56,8 +56,9 @@ static const struct usb_device_id idmouse_table[] = {
 #define FTIP_SCROLL  0x24
 
 #define ftip_command(dev, command, value, index) \
-   usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0), command, \
-   USB_TYPE_VENDOR | USB_RECIP_ENDPOINT | USB_DIR_OUT, value, index, NULL, 
0, 1000)
+   usb_control_msg_send(dev->udev, 0, command, \
+   USB_TYPE_VENDOR | USB_RECIP_ENDPOINT | USB_DIR_OUT, \
+   value, index, NULL, 0, 1000, GFP_KERNEL)
 
 MODULE_DEVICE_TABLE(usb, idmouse_table);
 
-- 
2.25.1

[PATCH v2 07/15] usb: misc: ezusb: update to use usb_control_msg_send()

[Anant Thazhemadam]

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

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/ezusb.c | 16 ++--
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/misc/ezusb.c b/drivers/usb/misc/ezusb.c
index f058d8029761..78aaee56c2b7 100644
--- a/drivers/usb/misc/ezusb.c
+++ b/drivers/usb/misc/ezusb.c
@@ -31,24 +31,12 @@ static const struct ezusb_fx_type ezusb_fx1 = {
 static int ezusb_writememory(struct usb_device *dev, int address,
unsigned char *data, int length, __u8 request)
 {
-   int result;
-   unsigned char *transfer_buffer;
-
if (!dev)
return -ENODEV;
 
-   transfer_buffer = kmemdup(data, length, GFP_KERNEL);
-   if (!transfer_buffer) {
-   dev_err(>dev, "%s - kmalloc(%d) failed.\n",
-   __func__, length);
-   return -ENOMEM;
-   }
-   result = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), request,
+   return usb_control_msg_send(dev, 0, request,
 USB_DIR_OUT | USB_TYPE_VENDOR | 
USB_RECIP_DEVICE,
-address, 0, transfer_buffer, length, 3000);
-
-   kfree(transfer_buffer);
-   return result;
+address, 0, data, length, 3000, GFP_KERNEL);
 }
 
 static int ezusb_set_reset(struct usb_device *dev, unsigned short cpucs_reg,
-- 
2.25.1

[PATCH v2 06/15] usb: misc: emi62: update to use usb_control_msg_send()

[Anant Thazhemadam]

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

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/emi62.c | 30 +++---
 1 file changed, 7 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/misc/emi62.c b/drivers/usb/misc/emi62.c
index 3eea60437f56..6ac4e72d53e0 100644
--- a/drivers/usb/misc/emi62.c
+++ b/drivers/usb/misc/emi62.c
@@ -36,7 +36,7 @@
 #define INTERNAL_RAM(address)   (address <= MAX_INTERNAL_ADDRESS)
 
 static int emi62_writememory(struct usb_device *dev, int address,
-const unsigned char *data, int length,
+const void *data, int length,
 __u8 bRequest);
 static int emi62_set_reset(struct usb_device *dev, unsigned char reset_bit);
 static int emi62_load_firmware (struct usb_device *dev);
@@ -45,21 +45,11 @@ static void emi62_disconnect(struct usb_interface *intf);
 
 /* thanks to drivers/usb/serial/keyspan_pda.c code */
 static int emi62_writememory(struct usb_device *dev, int address,
-const unsigned char *data, int length,
+const void *data, int length,
 __u8 request)
 {
-   int result;
-   unsigned char *buffer =  kmemdup(data, length, GFP_KERNEL);
-
-   if (!buffer) {
-   dev_err(>dev, "kmalloc(%d) failed.\n", length);
-   return -ENOMEM;
-   }
-   /* Note: usb_control_msg returns negative value on error or length of 
the
-*   data that was written! */
-   result = usb_control_msg (dev, usb_sndctrlpipe(dev, 0), request, 0x40, 
address, 0, buffer, length, 300);
-   kfree (buffer);
-   return result;
+   return usb_control_msg_send(dev, 0, request, 0x40, address,
+   0, data, length, 300, GFP_KERNEL);
 }
 
 /* thanks to drivers/usb/serial/keyspan_pda.c code */
@@ -85,12 +75,9 @@ static int emi62_load_firmware (struct usb_device *dev)
int err = -ENOMEM;
int i;
__u32 addr; /* Address to write */
-   __u8 *buf;
+   __u8 buf[FW_LOAD_SIZE];
 
dev_dbg(>dev, "load_firmware\n");
-   buf = kmalloc(FW_LOAD_SIZE, GFP_KERNEL);
-   if (!buf)
-   goto wraperr;
 
err = request_ihex_firmware(_fw, "emi62/loader.fw", >dev);
if (err)
@@ -140,11 +127,11 @@ static int emi62_load_firmware (struct usb_device *dev)
 
/* intel hex records are terminated with type 0 element */
while (rec && (i + be16_to_cpu(rec->len) < FW_LOAD_SIZE)) {
-   memcpy(buf + i, rec->data, be16_to_cpu(rec->len));
+   memcpy([i], rec->data, be16_to_cpu(rec->len));
i += be16_to_cpu(rec->len);
rec = ihex_next_binrec(rec);
}
-   err = emi62_writememory(dev, addr, buf, i, ANCHOR_LOAD_FPGA);
+   err = emi62_writememory(dev, addr, , i, ANCHOR_LOAD_FPGA);
if (err < 0)
goto wraperr;
} while (rec);
@@ -209,8 +196,6 @@ static int emi62_load_firmware (struct usb_device *dev)
release_firmware(bitstream_fw);
release_firmware(firmware_fw);
 
-   kfree(buf);
-
/* return 1 to fail the driver inialization
 * and give real driver change to load */
return 1;
@@ -223,7 +208,6 @@ static int emi62_load_firmware (struct usb_device *dev)
release_firmware(bitstream_fw);
release_firmware(firmware_fw);
 
-   kfree(buf);
dev_err(>dev, "Error\n");
return err;
 }
-- 
2.25.1

[PATCH v2 05/15] usb: misc: emi26: update to use usb_control_msg_send()

[Anant Thazhemadam]

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

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/emi26.c | 31 ---
 1 file changed, 8 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/misc/emi26.c b/drivers/usb/misc/emi26.c
index 24d841850e05..1dd024507f40 100644
--- a/drivers/usb/misc/emi26.c
+++ b/drivers/usb/misc/emi26.c
@@ -27,7 +27,7 @@
 #define INTERNAL_RAM(address)   (address <= MAX_INTERNAL_ADDRESS)
 
 static int emi26_writememory( struct usb_device *dev, int address,
- const unsigned char *data, int length,
+ const void *data, int length,
  __u8 bRequest);
 static int emi26_set_reset(struct usb_device *dev, unsigned char reset_bit);
 static int emi26_load_firmware (struct usb_device *dev);
@@ -35,22 +35,12 @@ static int emi26_probe(struct usb_interface *intf, const 
struct usb_device_id *i
 static void emi26_disconnect(struct usb_interface *intf);
 
 /* thanks to drivers/usb/serial/keyspan_pda.c code */
-static int emi26_writememory (struct usb_device *dev, int address,
- const unsigned char *data, int length,
+static int emi26_writememory(struct usb_device *dev, int address,
+ const void *data, int length,
  __u8 request)
 {
-   int result;
-   unsigned char *buffer =  kmemdup(data, length, GFP_KERNEL);
-
-   if (!buffer) {
-   dev_err(>dev, "kmalloc(%d) failed.\n", length);
-   return -ENOMEM;
-   }
-   /* Note: usb_control_msg returns negative value on error or length of 
the
-*   data that was written! */
-   result = usb_control_msg (dev, usb_sndctrlpipe(dev, 0), request, 0x40, 
address, 0, buffer, length, 300);
-   kfree (buffer);
-   return result;
+   return usb_control_msg_send(dev, 0, request, 0x40, address, 0,
+   data, length, 300, GFP_KERNEL);
 }
 
 /* thanks to drivers/usb/serial/keyspan_pda.c code */
@@ -77,11 +67,7 @@ static int emi26_load_firmware (struct usb_device *dev)
int err = -ENOMEM;
int i;
__u32 addr; /* Address to write */
-   __u8 *buf;
-
-   buf = kmalloc(FW_LOAD_SIZE, GFP_KERNEL);
-   if (!buf)
-   goto wraperr;
+   __u8 buf[FW_LOAD_SIZE];
 
err = request_ihex_firmware(_fw, "emi26/loader.fw", >dev);
if (err)
@@ -133,11 +119,11 @@ static int emi26_load_firmware (struct usb_device *dev)
 
/* intel hex records are terminated with type 0 element */
while (rec && (i + be16_to_cpu(rec->len) < FW_LOAD_SIZE)) {
-   memcpy(buf + i, rec->data, be16_to_cpu(rec->len));
+   memcpy([i], rec->data, be16_to_cpu(rec->len));
i += be16_to_cpu(rec->len);
rec = ihex_next_binrec(rec);
}
-   err = emi26_writememory(dev, addr, buf, i, ANCHOR_LOAD_FPGA);
+   err = emi26_writememory(dev, addr, , i, ANCHOR_LOAD_FPGA);
if (err < 0)
goto wraperr;
} while (rec);
@@ -211,7 +197,6 @@ static int emi26_load_firmware (struct usb_device *dev)
release_firmware(bitstream_fw);
release_firmware(firmware_fw);
 
-   kfree(buf);
return err;
 }
 
-- 
2.25.1

[PATCH v2 04/15] usb: misc: ehset: update to use the usb_control_msg_{send|recv}() API

[Anant Thazhemadam]

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, instances of usb_control_msg() have been replaced with
usb_control_msg_{recv|send}() appropriately.

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/ehset.c | 76 +---
 1 file changed, 32 insertions(+), 44 deletions(-)

diff --git a/drivers/usb/misc/ehset.c b/drivers/usb/misc/ehset.c
index 2752e1f4f4d0..f87890f9cd26 100644
--- a/drivers/usb/misc/ehset.c
+++ b/drivers/usb/misc/ehset.c
@@ -24,68 +24,57 @@ static int ehset_probe(struct usb_interface *intf,
int ret = -EINVAL;
struct usb_device *dev = interface_to_usbdev(intf);
struct usb_device *hub_udev = dev->parent;
-   struct usb_device_descriptor *buf;
+   struct usb_device_descriptor buf;
u8 portnum = dev->portnum;
u16 test_pid = le16_to_cpu(dev->descriptor.idProduct);
 
switch (test_pid) {
case TEST_SE0_NAK_PID:
-   ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
-   USB_REQ_SET_FEATURE, USB_RT_PORT,
-   USB_PORT_FEAT_TEST,
-   (USB_TEST_SE0_NAK << 8) | portnum,
-   NULL, 0, 1000);
+   ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
+  USB_RT_PORT, USB_PORT_FEAT_TEST,
+  (USB_TEST_SE0_NAK << 8) | portnum,
+  NULL, 0, 1000, GFP_KERNEL);
break;
case TEST_J_PID:
-   ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
-   USB_REQ_SET_FEATURE, USB_RT_PORT,
-   USB_PORT_FEAT_TEST,
-   (USB_TEST_J << 8) | portnum,
-   NULL, 0, 1000);
+   ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
+  USB_RT_PORT, USB_PORT_FEAT_TEST,
+  (USB_TEST_J << 8) | portnum, NULL, 0,
+  1000, GFP_KERNEL);
break;
case TEST_K_PID:
-   ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
-   USB_REQ_SET_FEATURE, USB_RT_PORT,
-   USB_PORT_FEAT_TEST,
-   (USB_TEST_K << 8) | portnum,
-   NULL, 0, 1000);
+   ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
+  USB_RT_PORT, USB_PORT_FEAT_TEST,
+  (USB_TEST_K << 8) | portnum, NULL, 0,
+  1000, GFP_KERNEL);
break;
case TEST_PACKET_PID:
-   ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
-   USB_REQ_SET_FEATURE, USB_RT_PORT,
-   USB_PORT_FEAT_TEST,
-   (USB_TEST_PACKET << 8) | portnum,
-   NULL, 0, 1000);
+   ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
+  USB_RT_PORT, USB_PORT_FEAT_TEST,
+  (USB_TEST_PACKET << 8) | portnum,
+  NULL, 0, 1000, GFP_KERNEL);
break;
case TEST_HS_HOST_PORT_SUSPEND_RESUME:
/* Test: wait for 15secs -> suspend -> 15secs delay -> resume */
msleep(15 * 1000);
-   ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
-   USB_REQ_SET_FEATURE, USB_RT_PORT,
-   USB_PORT_FEAT_SUSPEND, portnum,
-   NULL, 0, 1000);
+   ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
+  USB_RT_PORT, USB_PORT_FEAT_SUSPEND,
+  portnum, NULL, 0, 1000, GFP_KERNEL);
if (ret < 0)
break;
 
msleep(15 * 1000);
-   ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
-   USB_REQ_CLEAR_FEATURE, USB_RT_PORT,
-   USB_PORT_FEAT_SUSPEND, portnum,
-   N

[PATCH v2 03/15] usb: misc: cytherm: update to use usb_control_msg_recv()

[Anant Thazhemadam]

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 
---
 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(>udev->dev, "retval = %d\n", retval);
+   cytherm->brightness, , 8);
+   if (!retval)
+   dev_dbg(>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(>udev->dev, "retval = %d\n", retval);
-   
-   kfree(buffer);
-   
+   0x01, , 8);
+   if (!retval)
+   dev_dbg(>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(>udev->dev, "retval = %d\n", retval);
+   retval = vendor_command(cytherm->udev, READ_RAM, TEMP, 0, , 8);
+   if (!retval)
+   dev_dbg(>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(>udev->dev, "retval = %d\n", retval);
+   retval = vendor_command(cytherm->udev, READ_RAM, SIGN, 0, , 8);
+   if (!retval)
+   dev_dbg(>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

[PATCH v2 02/15] usb: misc: cypress_cy7c63: update to use usb_control_msg_recv()

[Anant Thazhemadam]

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

Signed-off-by: Anant Thazhemadam 
---
 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(>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, , 
CYPRESS_MAX_REQSIZE,
+ USB_CTRL_GET_TIMEOUT, GFP_KERNEL);
 
/* store returned data (more READs to be added) */
switch (request) {
@@ -107,8 +98,6 @@ static int vendor_command(struct cypress *dev, unsigned char 
request,
break;
}
 
-   kfree(iobuf);
-error:
return retval;
 }
 
-- 
2.25.1

[PATCH v2 01/15] usb: misc: appledisplay: update to use the usb_control_msg_{send|recv}() API

[Anant Thazhemadam]

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, instances of usb_control_msg() have been replaced with
usb_control_msg_{recv|send}(), and all return value checking
conditions have also been modified appropriately.

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/appledisplay.c | 46 ++---
 1 file changed, 19 insertions(+), 27 deletions(-)

diff --git a/drivers/usb/misc/appledisplay.c b/drivers/usb/misc/appledisplay.c
index c8098e9b432e..117deb2fdc29 100644
--- a/drivers/usb/misc/appledisplay.c
+++ b/drivers/usb/misc/appledisplay.c
@@ -132,21 +132,17 @@ static int appledisplay_bl_update_status(struct 
backlight_device *bd)
pdata->msgdata[0] = 0x10;
pdata->msgdata[1] = bd->props.brightness;
 
-   retval = usb_control_msg(
-   pdata->udev,
-   usb_sndctrlpipe(pdata->udev, 0),
-   USB_REQ_SET_REPORT,
-   USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
-   ACD_USB_BRIGHTNESS,
-   0,
-   pdata->msgdata, 2,
-   ACD_USB_TIMEOUT);
+   retval = usb_control_msg_send(pdata->udev,
+ 0,
+ USB_REQ_SET_REPORT,
+ USB_DIR_OUT | USB_TYPE_CLASS | 
USB_RECIP_INTERFACE,
+ ACD_USB_BRIGHTNESS,
+ 0,
+ pdata->msgdata, 2,
+ ACD_USB_TIMEOUT, GFP_KERNEL);
mutex_unlock(>sysfslock);
 
-   if (retval < 0)
-   return retval;
-   else
-   return 0;
+   return retval;
 }
 
 static int appledisplay_bl_get_brightness(struct backlight_device *bd)
@@ -155,21 +151,17 @@ static int appledisplay_bl_get_brightness(struct 
backlight_device *bd)
int retval, brightness;
 
mutex_lock(>sysfslock);
-   retval = usb_control_msg(
-   pdata->udev,
-   usb_rcvctrlpipe(pdata->udev, 0),
-   USB_REQ_GET_REPORT,
-   USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
-   ACD_USB_BRIGHTNESS,
-   0,
-   pdata->msgdata, 2,
-   ACD_USB_TIMEOUT);
-   if (retval < 2) {
-   if (retval >= 0)
-   retval = -EMSGSIZE;
-   } else {
+   retval = usb_control_msg_recv(pdata->udev,
+ 0,
+ USB_REQ_GET_REPORT,
+ USB_DIR_IN | USB_TYPE_CLASS | 
USB_RECIP_INTERFACE,
+ ACD_USB_BRIGHTNESS,
+ 0,
+ pdata->msgdata, 2,
+ ACD_USB_TIMEOUT, GFP_KERNEL);
+   if (retval == 0)
brightness = pdata->msgdata[1];
-   }
+
mutex_unlock(>sysfslock);
 
if (retval < 0)
-- 
2.25.1

[PATCH v2 00/15] drivers: usb: misc: update to use usb_control_msg_{send|recv}()

[Anant Thazhemadam]

The new usb_control_msg_{send|recv}() API provides an improved way of 
using usb_control_msg(). Using this, short reads/writes are considered
as errors, data can be used off the stack, and the need for the calling
function to create a raw usb pipe is eliminated.
This patch series aims to update existing instances of usb_control_msg() 
in drivers/usb/misc to usb_control_msg_{send|recv}() appropriately, and
also update the return value checking mechanisms in place (if any), as
necessary so nothing breaks.

I was however unable to update one instance of usb_control_msg() in 
drivers/usb/misc/apple-mfi-fastcharge.c.

The return value checking mechanism present here, is as follows.
if (retval) {
dev_dbg(>udev->dev, "retval = %d\n", retval);
return retval;
}

mfi->charge_type = val->intval;

return 0;

This implies that mfi->charge_type = val->intval only when number of
bytes transferred = 0, and the return value is directly returned 
otherwise. Since the new API doesn't return the number of bytes 
transferred, I wasn't quite sure how this instance could be updated.
In case this check is logically incorrect, a patch with a fix 
can be sent in as well.

Changes in v2:

  * Buffer variables that were previously dynamically allocated are no
longer dynamically allocated unless they have a variable length
(since that threw a warning).


Anant Thazhemadam (15):
  usb: misc: appledisplay: update to use the
usb_control_msg_{send|recv}() API
  usb: misc: cypress_cy7c63: update to use usb_control_msg_recv()
  usb: misc: cytherm: update to use usb_control_msg_recv()
  usb: misc: ehset: update to use the usb_control_msg_{send|recv}() API
  usb: misc: emi26: update to use usb_control_msg_send()
  usb: misc: emi62: update to use usb_control_msg_send()
  usb: misc: ezusb: update to use usb_control_msg_send()
  usb: misc: idmouse: update to use usb_control_msg_send()
  usb: misc: iowarrior: update to use the usb_control_msg_{send|recv}()
API
  usb: misc: isight_firmware: update to use usb_control_msg_send()
  usb: misc: ldusb: update to use usb_control_msg_send()
  usb: misc: lvstest: update to use the usb_control_msg_{send|recv}()
API
  usb: misc: trancevibrator: update to use usb_control_msg_send()
  usb: misc: usbsevseg: update to use usb_control_msg_send()
  usb: misc: usbtest: update to use the usb_control_msg_{send|recv}()
API

 drivers/usb/misc/appledisplay.c|  46 +--
 drivers/usb/misc/cypress_cy7c63.c  |  21 ++---
 drivers/usb/misc/cytherm.c | 128 ++---
 drivers/usb/misc/ehset.c   |  76 -
 drivers/usb/misc/emi26.c   |  31 ++-
 drivers/usb/misc/emi62.c   |  30 ++-
 drivers/usb/misc/ezusb.c   |  16 +---
 drivers/usb/misc/idmouse.c |   5 +-
 drivers/usb/misc/iowarrior.c   |  34 
 drivers/usb/misc/isight_firmware.c |  30 +++
 drivers/usb/misc/ldusb.c   |   8 +-
 drivers/usb/misc/lvstest.c |  38 -
 drivers/usb/misc/trancevibrator.c  |   4 +-
 drivers/usb/misc/usbsevseg.c   |  60 --
 drivers/usb/misc/usbtest.c |  69 +++-
 15 files changed, 216 insertions(+), 380 deletions(-)

-- 
2.25.1

Re: [PATCH 03/15] usb: misc: cytherm: update to use usb_control_msg_recv()

[Anant Thazhemadam]

On 29/11/20 9:46 pm, Greg Kroah-Hartman wrote:
> There's no more need to dynamically allocate the buffer variable here
> now, right?  It can be on the stack as the change you made above allows
> that to work properly, no need to allocate the buffer twice in a row
> (once here and once in the USB core).
>
> That would make these functions less complex, always a good thing.
>
> You should check this on the other patches in this series as well.

Understood, I will do that. I apologize for the oversight.

Thanks,
Anant

[PATCH 11/15] usb: misc: ldusb: update to use usb_control_msg_send()

[Anant Thazhemadam]

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_send() has been replaced
with usb_control_msg_send() appropriately.

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/ldusb.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/misc/ldusb.c b/drivers/usb/misc/ldusb.c
index 670e4d91e9ca..259ead4edecb 100644
--- a/drivers/usb/misc/ldusb.c
+++ b/drivers/usb/misc/ldusb.c
@@ -573,15 +573,13 @@ static ssize_t ld_usb_write(struct file *file, const char 
__user *buffer,
}
 
if (dev->interrupt_out_endpoint == NULL) {
-   /* try HID_REQ_SET_REPORT=9 on control_endpoint instead of 
interrupt_out_endpoint */
-   retval = usb_control_msg(interface_to_usbdev(dev->intf),
-
usb_sndctrlpipe(interface_to_usbdev(dev->intf), 0),
-9,
+   retval = usb_control_msg_send(interface_to_usbdev(dev->intf),
+0, 9,
 USB_TYPE_CLASS | USB_RECIP_INTERFACE | 
USB_DIR_OUT,
 1 << 8, 0,
 dev->interrupt_out_buffer,
 bytes_to_write,
-USB_CTRL_SET_TIMEOUT);
+USB_CTRL_SET_TIMEOUT, GFP_KERNEL);
if (retval < 0)
dev_err(>intf->dev,
"Couldn't submit HID_REQ_SET_REPORT %d\n",
-- 
2.25.1

[PATCH 13/15] usb: misc: trancevibrator: update to use usb_control_msg_send()

[Anant Thazhemadam]

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_send() and the return value checking condition has also
been modified appropriately.

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/trancevibrator.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/misc/trancevibrator.c 
b/drivers/usb/misc/trancevibrator.c
index a3dfc77578ea..2c36ee249b4b 100644
--- a/drivers/usb/misc/trancevibrator.c
+++ b/drivers/usb/misc/trancevibrator.c
@@ -59,12 +59,12 @@ static ssize_t speed_store(struct device *dev, struct 
device_attribute *attr,
dev_dbg(>udev->dev, "speed = %d\n", tv->speed);
 
/* Set speed */
-   retval = usb_control_msg(tv->udev, usb_sndctrlpipe(tv->udev, 0),
+   retval = usb_control_msg_send(tv->udev, 0,
 0x01, /* vendor request: set speed */
 USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_OTHER,
 tv->speed, /* speed value */
-0, NULL, 0, USB_CTRL_GET_TIMEOUT);
-   if (retval) {
+0, NULL, 0, USB_CTRL_GET_TIMEOUT, GFP_KERNEL);
+   if (retval == 0) {
tv->speed = old;
dev_dbg(>udev->dev, "retval = %d\n", retval);
return retval;
-- 
2.25.1

[PATCH 08/15] usb: misc: idmouse: update to use usb_control_msg_send()

[Anant Thazhemadam]

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

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/idmouse.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/misc/idmouse.c b/drivers/usb/misc/idmouse.c
index e9437a176518..52126441a633 100644
--- a/drivers/usb/misc/idmouse.c
+++ b/drivers/usb/misc/idmouse.c
@@ -56,8 +56,9 @@ static const struct usb_device_id idmouse_table[] = {
 #define FTIP_SCROLL  0x24
 
 #define ftip_command(dev, command, value, index) \
-   usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0), command, \
-   USB_TYPE_VENDOR | USB_RECIP_ENDPOINT | USB_DIR_OUT, value, index, NULL, 
0, 1000)
+   usb_control_msg_send(dev->udev, 0, command, \
+   USB_TYPE_VENDOR | USB_RECIP_ENDPOINT | USB_DIR_OUT, \
+   value, index, NULL, 0, 1000, GFP_KERNEL)
 
 MODULE_DEVICE_TABLE(usb, idmouse_table);
 
-- 
2.25.1

[PATCH 10/15] usb: misc: isight_firmware: update to use usb_control_msg_send()

[Anant Thazhemadam]

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 instances of usb_control_msg() have been replaced with
usb_control_msg_send(), and return value checking has also been
appropriately enforced.

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/isight_firmware.c | 22 +++---
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/misc/isight_firmware.c 
b/drivers/usb/misc/isight_firmware.c
index 4d30095d6ad2..d3fa6fa81e6c 100644
--- a/drivers/usb/misc/isight_firmware.c
+++ b/drivers/usb/misc/isight_firmware.c
@@ -53,11 +53,11 @@ static int isight_firmware_load(struct usb_interface *intf,
ptr = firmware->data;
 
buf[0] = 0x01;
-   if (usb_control_msg
-   (dev, usb_sndctrlpipe(dev, 0), 0xa0, 0x40, 0xe600, 0, buf, 1,
-300) != 1) {
+   ret = usb_control_msg_send(dev, 0, 0xa0, 0x40, 0xe600,
+  0, buf, 1, 300, GFP_KERNEL);
+   if (ret != 0) {
printk(KERN_ERR
-  "Failed to initialise isight firmware loader\n");
+   "Failed to initialise isight firmware loader\n");
ret = -ENODEV;
goto out;
}
@@ -86,11 +86,11 @@ static int isight_firmware_load(struct usb_interface *intf,
 
ptr += llen;
 
-   if (usb_control_msg
-   (dev, usb_sndctrlpipe(dev, 0), 0xa0, 0x40, req, 0,
-buf, llen, 300) != llen) {
+   ret = usb_control_msg_send(dev, 0, 0xa0, 0x40, req, 0,
+  buf, llen, 300, GFP_KERNEL);
+   if (ret != 0) {
printk(KERN_ERR
-  "Failed to load isight firmware\n");
+   "Failed to load isight firmware\n");
ret = -ENODEV;
goto out;
}
@@ -99,9 +99,9 @@ static int isight_firmware_load(struct usb_interface *intf,
}
 
buf[0] = 0x00;
-   if (usb_control_msg
-   (dev, usb_sndctrlpipe(dev, 0), 0xa0, 0x40, 0xe600, 0, buf, 1,
-300) != 1) {
+   ret = usb_control_msg_send(dev, 0, 0xa0, 0x40, 0xe600,
+  0, buf, 1, 300, GFP_KERNEL);
+   if (ret != 0) {
printk(KERN_ERR "isight firmware loading completion failed\n");
ret = -ENODEV;
}
-- 
2.25.1

[PATCH 12/15] usb: misc: lvstest: update to use the usb_control_msg_{send|recv}() API

[Anant Thazhemadam]

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, instances of usb_control_msg() have been replaced with
usb_control_msg_{recv|send}() and the return value checking conditions have
also been modified appropriately.

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/lvstest.c | 30 +++---
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/misc/lvstest.c b/drivers/usb/misc/lvstest.c
index f8686139d6f3..ab08483258d2 100644
--- a/drivers/usb/misc/lvstest.c
+++ b/drivers/usb/misc/lvstest.c
@@ -85,17 +85,17 @@ static void destroy_lvs_device(struct usb_device *udev)
 static int lvs_rh_clear_port_feature(struct usb_device *hdev,
int port1, int feature)
 {
-   return usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
+   return usb_control_msg_send(hdev, 0,
USB_REQ_CLEAR_FEATURE, USB_RT_PORT, feature, port1,
-   NULL, 0, 1000);
+   NULL, 0, 1000, GFP_KERNEL);
 }
 
 static int lvs_rh_set_port_feature(struct usb_device *hdev,
int port1, int feature)
 {
-   return usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
+   return usb_control_msg_send(hdev, 0,
USB_REQ_SET_FEATURE, USB_RT_PORT, feature, port1,
-   NULL, 0, 1000);
+   NULL, 0, 1000, GFP_KERNEL);
 }
 
 static ssize_t u3_entry_store(struct device *dev,
@@ -271,10 +271,10 @@ static ssize_t get_dev_desc_store(struct device *dev,
goto free_desc;
}
 
-   ret = usb_control_msg(udev, (PIPE_CONTROL << 30) | USB_DIR_IN,
-   USB_REQ_GET_DESCRIPTOR, USB_DIR_IN, USB_DT_DEVICE << 8,
-   0, descriptor, sizeof(*descriptor),
-   USB_CTRL_GET_TIMEOUT);
+   ret = usb_control_msg_recv(udev, 0, USB_REQ_GET_DESCRIPTOR,
+  USB_DIR_IN, USB_DT_DEVICE << 8,
+  0, descriptor, sizeof(*descriptor),
+  USB_CTRL_GET_TIMEOUT, GFP_KERNEL);
if (ret < 0)
dev_err(dev, "can't read device descriptor %d\n", ret);
 
@@ -336,10 +336,10 @@ static void lvs_rh_work(struct work_struct *work)
 
/* Examine each root port */
for (i = 1; i <= descriptor->bNbrPorts; i++) {
-   ret = usb_control_msg(hdev, usb_rcvctrlpipe(hdev, 0),
+   ret = usb_control_msg_recv(hdev, 0,
USB_REQ_GET_STATUS, USB_DIR_IN | USB_RT_PORT, 0, i,
-   port_status, sizeof(*port_status), 1000);
-   if (ret < 4)
+   port_status, sizeof(*port_status), 1000, GFP_KERNEL);
+   if (ret < 0)
continue;
 
portchange = le16_to_cpu(port_status->wPortChange);
@@ -420,13 +420,13 @@ static int lvs_rh_probe(struct usb_interface *intf,
usb_set_intfdata(intf, lvs);
 
/* how many number of ports this root hub has */
-   ret = usb_control_msg(hdev, usb_rcvctrlpipe(hdev, 0),
+   ret = usb_control_msg_recv(hdev, 0,
USB_REQ_GET_DESCRIPTOR, USB_DIR_IN | USB_RT_HUB,
USB_DT_SS_HUB << 8, 0, >descriptor,
-   USB_DT_SS_HUB_SIZE, USB_CTRL_GET_TIMEOUT);
-   if (ret < (USB_DT_HUB_NONVAR_SIZE + 2)) {
+   USB_DT_SS_HUB_SIZE, USB_CTRL_GET_TIMEOUT, GFP_KERNEL);
+   if (ret < 0) {
dev_err(>dev, "wrong root hub descriptor read %d\n", ret);
-   return ret < 0 ? ret : -EINVAL;
+   return ret;
}
 
/* submit urb to poll interrupt endpoint */
-- 
2.25.1

[PATCH 15/15] usb: misc: usbtest: update to use the usb_control_msg_{send|recv}() API

[Anant Thazhemadam]

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, instances of usb_control_msg() have been replaced with
usb_control_msg_{recv|send}() and the return value checking conditions have
also been modified appropriately.

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/usbtest.c | 63 +++---
 1 file changed, 31 insertions(+), 32 deletions(-)

diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index 150090ee4ec1..e44ec5261d82 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -672,10 +672,11 @@ static int get_altsetting(struct usbtest_dev *dev)
struct usb_device   *udev = interface_to_usbdev(iface);
int retval;
 
-   retval = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
-   USB_REQ_GET_INTERFACE, USB_DIR_IN|USB_RECIP_INTERFACE,
-   0, iface->altsetting[0].desc.bInterfaceNumber,
-   dev->buf, 1, USB_CTRL_GET_TIMEOUT);
+   retval = usb_control_msg_recv(udev, 0, USB_REQ_GET_INTERFACE,
+ USB_DIR_IN|USB_RECIP_INTERFACE,
+ 0, 
iface->altsetting[0].desc.bInterfaceNumber,
+ dev->buf, 1, USB_CTRL_GET_TIMEOUT, 
GFP_KERNEL);
+
switch (retval) {
case 1:
return dev->buf[0];
@@ -685,6 +686,11 @@ static int get_altsetting(struct usbtest_dev *dev)
default:
return retval;
}
+
+   if (retval < 0)
+   return retval;
+
+   return dev->buf[0];
 }
 
 static int set_altsetting(struct usbtest_dev *dev, int alternate)
@@ -872,14 +878,15 @@ static int ch9_postconfig(struct usbtest_dev *dev)
 * ... although some cheap devices (like one TI Hub I've got)
 * won't return config descriptors except before set_config.
 */
-   retval = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
-   USB_REQ_GET_CONFIGURATION,
-   USB_DIR_IN | USB_RECIP_DEVICE,
-   0, 0, dev->buf, 1, USB_CTRL_GET_TIMEOUT);
-   if (retval != 1 || dev->buf[0] != expected) {
+   retval = usb_control_msg_recv(udev, 0, 
USB_REQ_GET_CONFIGURATION,
+ USB_DIR_IN | USB_RECIP_DEVICE,  0,
+ 0, dev->buf, 1, 
USB_CTRL_GET_TIMEOUT,
+ GFP_KERNEL);
+
+   if (retval != 0 || dev->buf[0] != expected) {
dev_err(>dev, "get config --> %d %d (1 %d)\n",
retval, dev->buf[0], expected);
-   return (retval < 0) ? retval : -EDOM;
+   return retval;
}
}
 
@@ -1683,10 +1690,10 @@ static int test_halt(struct usbtest_dev *tdev, int ep, 
struct urb *urb)
return retval;
 
/* set halt (protocol test only), verify it worked */
-   retval = usb_control_msg(urb->dev, usb_sndctrlpipe(urb->dev, 0),
-   USB_REQ_SET_FEATURE, USB_RECIP_ENDPOINT,
-   USB_ENDPOINT_HALT, ep,
-   NULL, 0, USB_CTRL_SET_TIMEOUT);
+   retval = usb_control_msg_send(urb->dev, 0, USB_REQ_SET_FEATURE,
+ USB_RECIP_ENDPOINT, USB_ENDPOINT_HALT,
+ ep, NULL, 0, USB_CTRL_SET_TIMEOUT,
+ GFP_KERNEL);
if (retval < 0) {
ERROR(tdev, "ep %02x couldn't set halt, %d\n", ep, retval);
return retval;
@@ -1845,30 +1852,22 @@ static int ctrl_out(struct usbtest_dev *dev,
/* write patterned data */
for (j = 0; j < len; j++)
buf[j] = (u8)(i + j);
-   retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
-   0x5b, USB_DIR_OUT|USB_TYPE_VENDOR,
-   0, 0, buf, len, USB_CTRL_SET_TIMEOUT);
-   if (retval != len) {
+   retval = usb_control_msg_send(udev, 0, 0x5b,
+ USB_DIR_OUT | USB_TYPE_VENDOR, 0,
+ 0, buf, len, USB_CTRL_SET_TIMEOUT,
+ GFP_KERNEL);
+   if (retval < 0) {
what = "write";
-   if (retval >= 0) {
-   ERROR(dev, "ctrl_out, wlen %d (expected %d)\n&q

[PATCH 14/15] usb: misc: usbsevseg: update to use usb_control_msg_send()

[Anant Thazhemadam]

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, instances of usb_control_msg() have been replaced with
usb_control_msg_send() appropriately.

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/usbsevseg.c | 52 +++-
 1 file changed, 16 insertions(+), 36 deletions(-)

diff --git a/drivers/usb/misc/usbsevseg.c b/drivers/usb/misc/usbsevseg.c
index 551074f5b7ad..f0f6728a8b54 100644
--- a/drivers/usb/misc/usbsevseg.c
+++ b/drivers/usb/misc/usbsevseg.c
@@ -74,15 +74,10 @@ static void update_display_powered(struct usb_sevsegdev 
*mydev)
if (mydev->shadow_power != 1)
return;
 
-   rc = usb_control_msg(mydev->udev,
-   usb_sndctrlpipe(mydev->udev, 0),
-   0x12,
-   0x48,
-   (80 * 0x100) + 10, /*  (power mode) */
-   (0x00 * 0x100) + (mydev->powered ? 1 : 0),
-   NULL,
-   0,
-   2000);
+   rc = usb_control_msg_send(mydev->udev, 0, 0x12, 0x48,
+ (80 * 0x100) + 10, /*  (power mode) */
+ (0x00 * 0x100) + (mydev->powered ? 1 : 0),
+ NULL, 0, 2000, GFP_KERNEL);
if (rc < 0)
dev_dbg(>udev->dev, "power retval = %d\n", rc);
 
@@ -99,15 +94,10 @@ static void update_display_mode(struct usb_sevsegdev *mydev)
if(mydev->shadow_power != 1)
return;
 
-   rc = usb_control_msg(mydev->udev,
-   usb_sndctrlpipe(mydev->udev, 0),
-   0x12,
-   0x48,
-   (82 * 0x100) + 10, /* (set mode) */
-   (mydev->mode_msb * 0x100) + mydev->mode_lsb,
-   NULL,
-   0,
-   2000);
+   rc = usb_control_msg_send(mydev->udev, 0, 0x12, 0x48,
+ (82 * 0x100) + 10, /* (set mode) */
+ (mydev->mode_msb * 0x100) + mydev->mode_lsb,
+ NULL, 0, 2000, GFP_KERNEL);
 
if (rc < 0)
dev_dbg(>udev->dev, "mode retval = %d\n", rc);
@@ -131,15 +121,10 @@ static void update_display_visual(struct usb_sevsegdev 
*mydev, gfp_t mf)
for (i = 0; i < mydev->textlength; i++)
buffer[i] = mydev->text[mydev->textlength-1-i];
 
-   rc = usb_control_msg(mydev->udev,
-   usb_sndctrlpipe(mydev->udev, 0),
-   0x12,
-   0x48,
-   (85 * 0x100) + 10, /* (write text) */
-   (0 * 0x100) + mydev->textmode, /* mode  */
-   buffer,
-   mydev->textlength,
-   2000);
+   rc = usb_control_msg_send(mydev->udev, 0, 0x12, 0x48,
+ (85 * 0x100) + 10, /* (write text) */
+ (0 * 0x100) + mydev->textmode, /* mode  */
+ buffer, mydev->textlength, 2000, GFP_KERNEL);
 
if (rc < 0)
dev_dbg(>udev->dev, "write retval = %d\n", rc);
@@ -150,15 +135,10 @@ static void update_display_visual(struct usb_sevsegdev 
*mydev, gfp_t mf)
for (i = 0; i < sizeof(mydev->decimals); i++)
decimals |= mydev->decimals[i] << i;
 
-   rc = usb_control_msg(mydev->udev,
-   usb_sndctrlpipe(mydev->udev, 0),
-   0x12,
-   0x48,
-   (86 * 0x100) + 10, /* (set decimal) */
-   (0 * 0x100) + decimals, /* decimals */
-   NULL,
-   0,
-   2000);
+   rc = usb_control_msg_send(mydev->udev, 0, 0x12, 0x48,
+ (86 * 0x100) + 10, /* (set decimal) */
+ (0 * 0x100) + decimals, /* decimals */
+ NULL, 0, 2000, GFP_KERNEL);
 
if (rc < 0)
dev_dbg(>udev->dev, "decimal retval = %d\n", rc);
-- 
2.25.1

[PATCH 09/15] usb: misc: iowarrior: update to use the usb_control_msg_{send|recv}() API

[Anant Thazhemadam]

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, instances of usb_control_msg() have been replaced with
usb_control_msg_{recv|send}() appropriately.

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/iowarrior.c | 34 +-
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c
index 70ec29681526..53726fffe5df 100644
--- a/drivers/usb/misc/iowarrior.c
+++ b/drivers/usb/misc/iowarrior.c
@@ -109,12 +109,12 @@ static int usb_get_report(struct usb_device *dev,
  struct usb_host_interface *inter, unsigned char type,
  unsigned char id, void *buf, int size)
 {
-   return usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
-  USB_REQ_GET_REPORT,
-  USB_DIR_IN | USB_TYPE_CLASS |
-  USB_RECIP_INTERFACE, (type << 8) + id,
-  inter->desc.bInterfaceNumber, buf, size,
-  GET_TIMEOUT*HZ);
+   return usb_control_msg_recv(dev, 0,
+   USB_REQ_GET_REPORT,
+   USB_DIR_IN | USB_TYPE_CLASS |
+   USB_RECIP_INTERFACE, (type << 8) + id,
+   inter->desc.bInterfaceNumber, buf, size,
+   GET_TIMEOUT*HZ, GFP_KERNEL);
 }
 //#endif
 
@@ -123,13 +123,13 @@ static int usb_get_report(struct usb_device *dev,
 static int usb_set_report(struct usb_interface *intf, unsigned char type,
  unsigned char id, void *buf, int size)
 {
-   return usb_control_msg(interface_to_usbdev(intf),
-  usb_sndctrlpipe(interface_to_usbdev(intf), 0),
-  USB_REQ_SET_REPORT,
-  USB_TYPE_CLASS | USB_RECIP_INTERFACE,
-  (type << 8) + id,
-  intf->cur_altsetting->desc.bInterfaceNumber, buf,
-  size, HZ);
+   return usb_control_msg_send(interface_to_usbdev(intf),
+   0,
+   USB_REQ_SET_REPORT,
+   USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+   (type << 8) + id,
+   
intf->cur_altsetting->desc.bInterfaceNumber, buf,
+   size, HZ, GFP_KERNEL);
 }
 
 /*-*/
@@ -854,10 +854,10 @@ static int iowarrior_probe(struct usb_interface 
*interface,
 
/* Set the idle timeout to 0, if this is interface 0 */
if (dev->interface->cur_altsetting->desc.bInterfaceNumber == 0) {
-   usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
-   0x0A,
-   USB_TYPE_CLASS | USB_RECIP_INTERFACE, 0,
-   0, NULL, 0, USB_CTRL_SET_TIMEOUT);
+   usb_control_msg_send(udev, 0,
+0x0A,
+USB_TYPE_CLASS | USB_RECIP_INTERFACE, 0,
+0, NULL, 0, USB_CTRL_SET_TIMEOUT, 
GFP_KERNEL);
}
/* allow device read and ioctl */
dev->present = 1;
-- 
2.25.1

[PATCH 07/15] usb: misc: ezusb: update to use usb_control_msg_send()

[Anant Thazhemadam]

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

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/ezusb.c | 16 ++--
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/misc/ezusb.c b/drivers/usb/misc/ezusb.c
index f058d8029761..78aaee56c2b7 100644
--- a/drivers/usb/misc/ezusb.c
+++ b/drivers/usb/misc/ezusb.c
@@ -31,24 +31,12 @@ static const struct ezusb_fx_type ezusb_fx1 = {
 static int ezusb_writememory(struct usb_device *dev, int address,
unsigned char *data, int length, __u8 request)
 {
-   int result;
-   unsigned char *transfer_buffer;
-
if (!dev)
return -ENODEV;
 
-   transfer_buffer = kmemdup(data, length, GFP_KERNEL);
-   if (!transfer_buffer) {
-   dev_err(>dev, "%s - kmalloc(%d) failed.\n",
-   __func__, length);
-   return -ENOMEM;
-   }
-   result = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), request,
+   return usb_control_msg_send(dev, 0, request,
 USB_DIR_OUT | USB_TYPE_VENDOR | 
USB_RECIP_DEVICE,
-address, 0, transfer_buffer, length, 3000);
-
-   kfree(transfer_buffer);
-   return result;
+address, 0, data, length, 3000, GFP_KERNEL);
 }
 
 static int ezusb_set_reset(struct usb_device *dev, unsigned short cpucs_reg,
-- 
2.25.1

[PATCH 02/15] usb: misc: cypress_cy7c63: update to use usb_control_msg_recv()

[Anant Thazhemadam]

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

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/cypress_cy7c63.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/misc/cypress_cy7c63.c 
b/drivers/usb/misc/cypress_cy7c63.c
index 14faec51d7a5..fc09251fc87c 100644
--- a/drivers/usb/misc/cypress_cy7c63.c
+++ b/drivers/usb/misc/cypress_cy7c63.c
@@ -70,7 +70,6 @@ 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*/
@@ -83,11 +82,10 @@ static int vendor_command(struct cypress *dev, unsigned 
char request,
dev_dbg(>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);
 
/* store returned data (more READs to be added) */
switch (request) {
-- 
2.25.1

[PATCH 05/15] usb: misc: emi26: update to use usb_control_msg_send()

[Anant Thazhemadam]

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

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/emi26.c | 14 ++
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/misc/emi26.c b/drivers/usb/misc/emi26.c
index 24d841850e05..1093809d34ad 100644
--- a/drivers/usb/misc/emi26.c
+++ b/drivers/usb/misc/emi26.c
@@ -39,18 +39,8 @@ static int emi26_writememory (struct usb_device *dev, int 
address,
  const unsigned char *data, int length,
  __u8 request)
 {
-   int result;
-   unsigned char *buffer =  kmemdup(data, length, GFP_KERNEL);
-
-   if (!buffer) {
-   dev_err(>dev, "kmalloc(%d) failed.\n", length);
-   return -ENOMEM;
-   }
-   /* Note: usb_control_msg returns negative value on error or length of 
the
-*   data that was written! */
-   result = usb_control_msg (dev, usb_sndctrlpipe(dev, 0), request, 0x40, 
address, 0, buffer, length, 300);
-   kfree (buffer);
-   return result;
+   return usb_control_msg_send(dev, 0, request, 0x40, address, 0,
+   data, length, 300, GFP_KERNEL);
 }
 
 /* thanks to drivers/usb/serial/keyspan_pda.c code */
-- 
2.25.1

[PATCH 04/15] usb: misc: ehset: update to use the usb_control_msg_{send|recv}() API

[Anant Thazhemadam]

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, instances of usb_control_msg() have been replaced with
usb_control_msg_{recv|send}() appropriately.

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/ehset.c | 70 ++--
 1 file changed, 31 insertions(+), 39 deletions(-)

diff --git a/drivers/usb/misc/ehset.c b/drivers/usb/misc/ehset.c
index 2752e1f4f4d0..068f4fae2965 100644
--- a/drivers/usb/misc/ehset.c
+++ b/drivers/usb/misc/ehset.c
@@ -30,48 +30,42 @@ static int ehset_probe(struct usb_interface *intf,
 
switch (test_pid) {
case TEST_SE0_NAK_PID:
-   ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
-   USB_REQ_SET_FEATURE, USB_RT_PORT,
-   USB_PORT_FEAT_TEST,
-   (USB_TEST_SE0_NAK << 8) | portnum,
-   NULL, 0, 1000);
+   ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
+  USB_RT_PORT, USB_PORT_FEAT_TEST,
+  (USB_TEST_SE0_NAK << 8) | portnum,
+  NULL, 0, 1000, GFP_KERNEL);
break;
case TEST_J_PID:
-   ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
-   USB_REQ_SET_FEATURE, USB_RT_PORT,
-   USB_PORT_FEAT_TEST,
-   (USB_TEST_J << 8) | portnum,
-   NULL, 0, 1000);
+   ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
+  USB_RT_PORT, USB_PORT_FEAT_TEST,
+  (USB_TEST_J << 8) | portnum, NULL, 0,
+  1000, GFP_KERNEL);
break;
case TEST_K_PID:
-   ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
-   USB_REQ_SET_FEATURE, USB_RT_PORT,
-   USB_PORT_FEAT_TEST,
-   (USB_TEST_K << 8) | portnum,
-   NULL, 0, 1000);
+   ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
+  USB_RT_PORT, USB_PORT_FEAT_TEST,
+  (USB_TEST_K << 8) | portnum, NULL, 0,
+  1000, GFP_KERNEL);
break;
case TEST_PACKET_PID:
-   ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
-   USB_REQ_SET_FEATURE, USB_RT_PORT,
-   USB_PORT_FEAT_TEST,
-   (USB_TEST_PACKET << 8) | portnum,
-   NULL, 0, 1000);
+   ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
+  USB_RT_PORT, USB_PORT_FEAT_TEST,
+  (USB_TEST_PACKET << 8) | portnum,
+  NULL, 0, 1000, GFP_KERNEL);
break;
case TEST_HS_HOST_PORT_SUSPEND_RESUME:
/* Test: wait for 15secs -> suspend -> 15secs delay -> resume */
msleep(15 * 1000);
-   ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
-   USB_REQ_SET_FEATURE, USB_RT_PORT,
-   USB_PORT_FEAT_SUSPEND, portnum,
-   NULL, 0, 1000);
+   ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
+  USB_RT_PORT, USB_PORT_FEAT_SUSPEND,
+  portnum, NULL, 0, 1000, GFP_KERNEL);
if (ret < 0)
break;
 
msleep(15 * 1000);
-   ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
-   USB_REQ_CLEAR_FEATURE, USB_RT_PORT,
-   USB_PORT_FEAT_SUSPEND, portnum,
-   NULL, 0, 1000);
+   ret = usb_control_msg_send(hub_udev, 0, USB_REQ_CLEAR_FEATURE,
+  USB_RT_PORT, USB_PORT_FEAT_SUSPEND,
+  portnum, NULL, 0, 1000, GFP_KERNEL);
break;
case TEST_SINGLE_STEP_GET_DEV_DESC:
 

[PATCH 06/15] usb: misc: emi62: update to use usb_control_msg_send()

[Anant Thazhemadam]

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

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/emi62.c | 14 ++
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/misc/emi62.c b/drivers/usb/misc/emi62.c
index 3eea60437f56..de984e91085c 100644
--- a/drivers/usb/misc/emi62.c
+++ b/drivers/usb/misc/emi62.c
@@ -48,18 +48,8 @@ static int emi62_writememory(struct usb_device *dev, int 
address,
 const unsigned char *data, int length,
 __u8 request)
 {
-   int result;
-   unsigned char *buffer =  kmemdup(data, length, GFP_KERNEL);
-
-   if (!buffer) {
-   dev_err(>dev, "kmalloc(%d) failed.\n", length);
-   return -ENOMEM;
-   }
-   /* Note: usb_control_msg returns negative value on error or length of 
the
-*   data that was written! */
-   result = usb_control_msg (dev, usb_sndctrlpipe(dev, 0), request, 0x40, 
address, 0, buffer, length, 300);
-   kfree (buffer);
-   return result;
+   return usb_control_msg_send(dev, 0, request, 0x40, address,
+   0, data, length, 300, GFP_KERNEL);
 }
 
 /* thanks to drivers/usb/serial/keyspan_pda.c code */
-- 
2.25.1

[PATCH 03/15] usb: misc: cytherm: update to use usb_control_msg_recv()

[Anant Thazhemadam]

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 
---
 drivers/usb/misc/cytherm.c | 42 +++---
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/drivers/usb/misc/cytherm.c b/drivers/usb/misc/cytherm.c
index 3e3802aaefa3..75fa4d806a77 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);
 }
 
 
@@ -95,14 +95,14 @@ static ssize_t brightness_store(struct device *dev, struct 
device_attribute *att
/* Set brightness */
retval = vendor_command(cytherm->udev, WRITE_RAM, BRIGHTNESS, 
cytherm->brightness, buffer, 8);
-   if (retval)
-   dev_dbg(>udev->dev, "retval = %d\n", retval);
+   if (!retval)
+   dev_dbg(>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(>udev->dev, "retval = %d\n", retval);
-   
+   if (!retval)
+   dev_dbg(>udev->dev, "µC informed of change in 
brightness setting\n");
+
kfree(buffer);

return count;
@@ -130,14 +130,14 @@ static ssize_t temp_show(struct device *dev, struct 
device_attribute *attr, char
 
/* read temperature */
retval = vendor_command(cytherm->udev, READ_RAM, TEMP, 0, buffer, 8);
-   if (retval)
-   dev_dbg(>udev->dev, "retval = %d\n", retval);
+   if (!retval)
+   dev_dbg(>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(>udev->dev, "retval = %d\n", retval);
+   if (!retval)
+   dev_dbg(>udev->dev, "read sign successfully\n");
sign = buffer[1];
 
kfree(buffer);
@@ -165,8 +165,8 @@ static ssize_t button_show(struct device *dev, struct 
device_attribute *attr, ch
 
/* check button */
retval = vendor_command(cytherm->udev, READ_RAM, BUTTON, 0, buffer, 8);
-   if (retval)
-   dev_dbg(>udev->dev, "retval = %d\n", retval);
+   if (!retval)
+   dev_dbg(>udev->dev, "checked button successfully\n");

retval = buffer[1];
 
@@ -193,7 +193,7 @@ static ssize_t port0_show(struct device *dev, struct 
device_attribute *attr, cha
return 0;
 
retval = vendor_command(cytherm->udev, READ_PORT, 0, 0, buffer, 8);
-   if (retval)
+   if (!retval)
dev_dbg(>udev->dev, "retval = %d\n", retval);
 
retval = buffer[1];
@@ -226,7 +226,7 @@ static ssize_t port0_store(struct device *dev, struct 
device_attribute *attr, co

retval = vendor_command(cytherm->udev, WRITE_PORT, 0,
tmp, buffer, 8);
-   if (retval)
+   if (!retval)
dev_dbg(>udev->dev, "retval = %d\n", retval);
 
kfree(buffer);
@@ -248,7 +248,7 @@ static ssize_t port1_show(struct device *dev, struct 
device_attribute *attr, cha
return 0;
 
retval = vendor_command(cytherm->udev, READ_PORT, 1, 0, buffer, 8);
-   if (retval)
+   if (!retval)
dev_dbg(>udev->dev, "retval = %d\n", retval);

retval = buffer[1];
@

[PATCH 01/15] usb: misc: appledisplay: update to use the usb_control_msg_{send|recv}() API

[Anant Thazhemadam]

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, instances of usb_control_msg() have been replaced with
usb_control_msg_{recv|send}(), and all return value checking
conditions have also been modified appropriately.

Signed-off-by: Anant Thazhemadam 
---
 drivers/usb/misc/appledisplay.c | 46 ++---
 1 file changed, 19 insertions(+), 27 deletions(-)

diff --git a/drivers/usb/misc/appledisplay.c b/drivers/usb/misc/appledisplay.c
index c8098e9b432e..117deb2fdc29 100644
--- a/drivers/usb/misc/appledisplay.c
+++ b/drivers/usb/misc/appledisplay.c
@@ -132,21 +132,17 @@ static int appledisplay_bl_update_status(struct 
backlight_device *bd)
pdata->msgdata[0] = 0x10;
pdata->msgdata[1] = bd->props.brightness;
 
-   retval = usb_control_msg(
-   pdata->udev,
-   usb_sndctrlpipe(pdata->udev, 0),
-   USB_REQ_SET_REPORT,
-   USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
-   ACD_USB_BRIGHTNESS,
-   0,
-   pdata->msgdata, 2,
-   ACD_USB_TIMEOUT);
+   retval = usb_control_msg_send(pdata->udev,
+ 0,
+ USB_REQ_SET_REPORT,
+ USB_DIR_OUT | USB_TYPE_CLASS | 
USB_RECIP_INTERFACE,
+ ACD_USB_BRIGHTNESS,
+ 0,
+ pdata->msgdata, 2,
+ ACD_USB_TIMEOUT, GFP_KERNEL);
mutex_unlock(>sysfslock);
 
-   if (retval < 0)
-   return retval;
-   else
-   return 0;
+   return retval;
 }
 
 static int appledisplay_bl_get_brightness(struct backlight_device *bd)
@@ -155,21 +151,17 @@ static int appledisplay_bl_get_brightness(struct 
backlight_device *bd)
int retval, brightness;
 
mutex_lock(>sysfslock);
-   retval = usb_control_msg(
-   pdata->udev,
-   usb_rcvctrlpipe(pdata->udev, 0),
-   USB_REQ_GET_REPORT,
-   USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
-   ACD_USB_BRIGHTNESS,
-   0,
-   pdata->msgdata, 2,
-   ACD_USB_TIMEOUT);
-   if (retval < 2) {
-   if (retval >= 0)
-   retval = -EMSGSIZE;
-   } else {
+   retval = usb_control_msg_recv(pdata->udev,
+ 0,
+ USB_REQ_GET_REPORT,
+ USB_DIR_IN | USB_TYPE_CLASS | 
USB_RECIP_INTERFACE,
+ ACD_USB_BRIGHTNESS,
+ 0,
+ pdata->msgdata, 2,
+ ACD_USB_TIMEOUT, GFP_KERNEL);
+   if (retval == 0)
brightness = pdata->msgdata[1];
-   }
+
mutex_unlock(>sysfslock);
 
if (retval < 0)
-- 
2.25.1

[PATCH 00/15] drivers: usb: misc: update to use usb_control_msg_{send|recv}()

[Anant Thazhemadam]

The new usb_control_msg_{send|recv}() API provides an improved way of 
using usb_control_msg(). Using this, short reads/writes are considered
as errors, data can be used off the stack, and the need for the calling
function to create a raw usb pipe is eliminated.
This patch series aims to update existing instances of usb_control_msg() 
in drivers/usb/misc to usb_control_msg_{send|recv}() appropriately, and
also update the return value checking mechanisms in place (if any), as
necessary so nothing breaks.

I was however unable to update one instance of usb_control_msg() in 
drivers/usb/misc/apple-mfi-fastcharge.c.

The return value checking mechanism present here, is as follows.
if (retval) {
dev_dbg(>udev->dev, "retval = %d\n", retval);
return retval;
}

mfi->charge_type = val->intval;

return 0;

This implies that mfi->charge_type = val->intval only when number of
bytes transferred = 0, and the return value is directly returned 
otherwise. Since the new API doesn't return the number of bytes 
transferred, I wasn't quite sure how this instance could be updated.
In case this check is logically incorrect, a patch with a fix 
can be sent in as well.


Anant Thazhemadam (15):
  usb: misc: appledisplay: update to use the
usb_control_msg_{send|recv}() API
  usb: misc: cypress_cy7c63: update to use usb_control_msg_recv()
  usb: misc: cytherm: update to use usb_control_msg_recv()
  usb: misc: ehset: update to use the usb_control_msg_{send|recv}() API
  usb: misc: emi26: update to use usb_control_msg_send()
  usb: misc: emi62: update to use usb_control_msg_send()
  usb: misc: ezusb: update to use usb_control_msg_send()
  usb: misc: idmouse: update to use usb_control_msg_send()
  usb: misc: iowarrior: update to use the usb_control_msg_{send|recv}()
API
  usb: misc: isight_firmware: update to use usb_control_msg_send()
  usb: misc: ldusb: update to use usb_control_msg_send()
  usb: misc: lvstest: update to use the usb_control_msg_{send|recv}()
API
  usb: misc: trancevibrator: update to use usb_control_msg_send()
  usb: misc: usbsevseg: update to use usb_control_msg_send()
  usb: misc: usbtest: update to use the usb_control_msg_{send|recv}()
API

 drivers/usb/misc/appledisplay.c| 46 
 drivers/usb/misc/cypress_cy7c63.c  | 10 ++---
 drivers/usb/misc/cytherm.c | 42 +-
 drivers/usb/misc/ehset.c   | 70 +-
 drivers/usb/misc/emi26.c   | 14 +-
 drivers/usb/misc/emi62.c   | 14 +-
 drivers/usb/misc/ezusb.c   | 16 +--
 drivers/usb/misc/idmouse.c |  5 ++-
 drivers/usb/misc/iowarrior.c   | 34 +++
 drivers/usb/misc/isight_firmware.c | 22 +-
 drivers/usb/misc/ldusb.c   |  8 ++--
 drivers/usb/misc/lvstest.c | 30 ++---
 drivers/usb/misc/trancevibrator.c  |  6 +--
 drivers/usb/misc/usbsevseg.c   | 52 +++---
 drivers/usb/misc/usbtest.c | 63 +--
 15 files changed, 180 insertions(+), 252 deletions(-)

-- 
2.25.1

[PATCH] misc: vmw_vmci: fix kernel info-leak by initializing dbells in vmci_ctx_get_chkpt_doorbells()

[Anant Thazhemadam]

A kernel-infoleak was reported by syzbot, which was caused because
dbells was left uninitialized.
Using kzalloc() instead of kmalloc() fixes this issue.

Reported-by: syzbot+a79e17c39564bedf0...@syzkaller.appspotmail.com
Tested-by: syzbot+a79e17c39564bedf0...@syzkaller.appspotmail.com
Signed-off-by: Anant Thazhemadam 
---
 drivers/misc/vmw_vmci/vmci_context.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/vmw_vmci/vmci_context.c 
b/drivers/misc/vmw_vmci/vmci_context.c
index 16695366ec92..26ff49fdf0f7 100644
--- a/drivers/misc/vmw_vmci/vmci_context.c
+++ b/drivers/misc/vmw_vmci/vmci_context.c
@@ -743,7 +743,7 @@ static int vmci_ctx_get_chkpt_doorbells(struct vmci_ctx 
*context,
return VMCI_ERROR_MORE_DATA;
}
 
-   dbells = kmalloc(data_size, GFP_ATOMIC);
+   dbells = kzalloc(data_size, GFP_ATOMIC);
if (!dbells)
return VMCI_ERROR_NO_MEM;
 
-- 
2.25.1

Re: [RESEND PATCH v3] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API

[Anant Thazhemadam]

On 05/11/20 5:54 am, Jakub Kicinski wrote:
> On Mon,  2 Nov 2020 23:09:46 +0530 Anant Thazhemadam wrote:
>> Currently, __usbnet_{read|write}_cmd() use usb_control_msg().
>> However, this could lead to potential partial reads/writes being
>> considered valid, and since most of the callers of
>> usbnet_{read|write}_cmd() don't take partial reads/writes into account
>> (only checking for negative error number is done), and this can lead to
>> issues.
>>
>> However, the new usb_control_msg_{send|recv}() APIs don't allow partial
>> reads and writes.
>> Using the new APIs also relaxes the return value checking that must
>> be done after usbnet_{read|write}_cmd() is called.
>>
>> Signed-off-by: Anant Thazhemadam 
> So you're changing the semantics without updating the callers?
>
> I'm confused. 
>
> Is this supposed to be applied to some tree which already has the
> callers fixed?
>
> At a quick scan at least drivers/net/usb/plusb.c* would get confused 
> as it compares the return value to zero and 0 used to mean "nothing
> transferred", now it means "all good", no? 
>
> * I haven't looked at all the other callers

I see. I checked most of the callers that directly called the functions,
but it seems to have slipped my mind that these callers were also
wrappers, and to check the callers for these wrapper.
I apologize for the oversight.
I'll perform a more in-depth analysis of all the callers, fix this mistake,
and send in a patch series instead, that update all the callers too.
Would that be alright?

Thank you for your time.

Thanks,
Anant

[PATCH 0/2] prevent potential access of uninitialized members in can_rcv() and canfd_rcv()

[Anant Thazhemadam]

In both can_rcv(), and canfd_rcv(), when skb->len = 0, cfd->len 
(which is uninitialized) is accessed by pr_warn_once().

Performing the validation check for cfd->len separately, after the 
validation check for skb->len is done, resolves this issue in both 
instances, without compromising the degree of detail provided in the
log messages.

Anant Thazhemadam (2):
  can: af_can: prevent potential access of uninitialized member in
can_rcv()
  can: af_can: prevent potential access of uninitialized member in
canfd_rcv()

 net/can/af_can.c | 38 --
 1 file changed, 28 insertions(+), 10 deletions(-)

-- 
2.25.1

[PATCH 1/2] can: af_can: prevent potential access of uninitialized member in can_rcv()

[Anant Thazhemadam]

In can_rcv(), cfd->len is uninitialized when skb->len = 0, and this
uninitialized cfd->len is accessed nonetheless by pr_warn_once().

Fix this uninitialized variable access by checking cfd->len's validity
condition (cfd->len > CAN_MAX_DLEN) separately after the skb->len's
condition is checked, and appropriately modify the log messages that
are generated as well.
In case either of the required conditions fail, the skb is freed and
NET_RX_DROP is returned, same as before.

Fixes: 8cb68751c115 ("can: af_can: can_rcv(): replace WARN_ONCE by 
pr_warn_once")
Reported-by: syzbot+9bcb0c9409066696d...@syzkaller.appspotmail.com
Tested-by: Anant Thazhemadam 
Signed-off-by: Anant Thazhemadam 
---
 net/can/af_can.c | 19 ++-
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/net/can/af_can.c b/net/can/af_can.c
index ea29a6d97ef5..8ea01524f062 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -677,16 +677,25 @@ static int can_rcv(struct sk_buff *skb, struct net_device 
*dev,
 {
struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
 
-   if (unlikely(dev->type != ARPHRD_CAN || skb->len != CAN_MTU ||
-cfd->len > CAN_MAX_DLEN)) {
-   pr_warn_once("PF_CAN: dropped non conform CAN skbuf: dev type 
%d, len %d, datalen %d\n",
+   if (unlikely(dev->type != ARPHRD_CAN || skb->len != CAN_MTU)) {
+   pr_warn_once("PF_CAN: dropped non conform CAN skbuff: dev type 
%d, len %d\n",
+dev->type, skb->len);
+   goto free_skb;
+   }
+
+   /* This check is made separately since cfd->len would be uninitialized 
if skb->len = 0. */
+   if (unlikely(cfd->len > CAN_MAX_DLEN)) {
+   pr_warn_once("PF_CAN: dropped non conform CAN skbuff: dev type 
%d, len %d, datalen %d\n",
 dev->type, skb->len, cfd->len);
-   kfree_skb(skb);
-   return NET_RX_DROP;
+   goto free_skb;
}
 
can_receive(skb, dev);
return NET_RX_SUCCESS;
+
+free_skb:
+   kfree_skb(skb);
+   return NET_RX_DROP;
 }
 
 static int canfd_rcv(struct sk_buff *skb, struct net_device *dev,
-- 
2.25.1

[PATCH 2/2] can: af_can: prevent potential access of uninitialized member in canfd_rcv()

[Anant Thazhemadam]

In canfd_rcv(), cfd->len is uninitialized when skb->len = 0, and this
uninitialized cfd->len is accessed nonetheless by pr_warn_once().

Fix this uninitialized variable access by checking cfd->len's validity
condition (cfd->len > CANFD_MAX_DLEN) separately after the skb->len's
condition is checked, and appropriately modify the log messages that
are generated as well.
In case either of the required conditions fail, the skb is freed and
NET_RX_DROP is returned, same as before.

Fixes: d4689846881d ("can: af_can: canfd_rcv(): replace WARN_ONCE by 
pr_warn_once")
Reported-by: syzbot+9bcb0c9409066696d...@syzkaller.appspotmail.com
Tested-by: Anant Thazhemadam 
Signed-off-by: Anant Thazhemadam 
---
This patch was locally tested using the reproducer and .config file
generated by syzbot.

 net/can/af_can.c | 19 ++-
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/net/can/af_can.c b/net/can/af_can.c
index 8ea01524f062..d759334f8843 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -703,16 +703,25 @@ static int canfd_rcv(struct sk_buff *skb, struct 
net_device *dev,
 {
struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
 
-   if (unlikely(dev->type != ARPHRD_CAN || skb->len != CANFD_MTU ||
-cfd->len > CANFD_MAX_DLEN)) {
-   pr_warn_once("PF_CAN: dropped non conform CAN FD skbuf: dev 
type %d, len %d, datalen %d\n",
+   if (unlikely(dev->type != ARPHRD_CAN || skb->len != CANFD_MTU)) {
+   pr_warn_once("PF_CAN: dropped non conform CAN FD skbuff: dev 
type %d, len %d\n",
+dev->type, skb->len);
+   goto free_skb;
+   }
+
+   /* This check is made separately since cfd->len would be uninitialized 
if skb->len = 0. */
+   if (unlikely(cfd->len > CANFD_MAX_DLEN)) {
+   pr_warn_once("PF_CAN: dropped non conform CAN FD skbuff: dev 
type %d, len %d, datalen %d\n",
 dev->type, skb->len, cfd->len);
-   kfree_skb(skb);
-   return NET_RX_DROP;
+   goto free_skb;
}
 
can_receive(skb, dev);
return NET_RX_SUCCESS;
+
+free_skb:
+   kfree_skb(skb);
+   return NET_RX_DROP;
 }
 
 /* af_can protocol functions */
-- 
2.25.1

[RESEND PATCH v3] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API

[Anant Thazhemadam]

Currently, __usbnet_{read|write}_cmd() use usb_control_msg().
However, this could lead to potential partial reads/writes being
considered valid, and since most of the callers of
usbnet_{read|write}_cmd() don't take partial reads/writes into account
(only checking for negative error number is done), and this can lead to
issues.

However, the new usb_control_msg_{send|recv}() APIs don't allow partial
reads and writes.
Using the new APIs also relaxes the return value checking that must
be done after usbnet_{read|write}_cmd() is called.

Signed-off-by: Anant Thazhemadam 
---
Changes in v3:
* Aligned continuation lines after the opening brackets
Changes in v2:
* Fix build error


 drivers/net/usb/usbnet.c | 52 
 1 file changed, 10 insertions(+), 42 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index bf6c58240bd4..b2df3417a41c 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1982,64 +1982,32 @@ EXPORT_SYMBOL(usbnet_link_change);
 static int __usbnet_read_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
 u16 value, u16 index, void *data, u16 size)
 {
-   void *buf = NULL;
-   int err = -ENOMEM;
 
netdev_dbg(dev->net, "usbnet_read_cmd cmd=0x%02x reqtype=%02x"
   " value=0x%04x index=0x%04x size=%d\n",
   cmd, reqtype, value, index, size);
 
-   if (size) {
-   buf = kmalloc(size, GFP_KERNEL);
-   if (!buf)
-   goto out;
-   }
-
-   err = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
- cmd, reqtype, value, index, buf, size,
- USB_CTRL_GET_TIMEOUT);
-   if (err > 0 && err <= size) {
-if (data)
-memcpy(data, buf, err);
-else
-netdev_dbg(dev->net,
-"Huh? Data requested but thrown away.\n");
-}
-   kfree(buf);
-out:
-   return err;
+   return usb_control_msg_recv(dev->udev, 0,
+   cmd, reqtype, value, index, data, size,
+   USB_CTRL_GET_TIMEOUT, GFP_KERNEL);
 }
 
 static int __usbnet_write_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
  u16 value, u16 index, const void *data,
  u16 size)
 {
-   void *buf = NULL;
-   int err = -ENOMEM;
-
netdev_dbg(dev->net, "usbnet_write_cmd cmd=0x%02x reqtype=%02x"
   " value=0x%04x index=0x%04x size=%d\n",
   cmd, reqtype, value, index, size);
 
-   if (data) {
-   buf = kmemdup(data, size, GFP_KERNEL);
-   if (!buf)
-   goto out;
-   } else {
-if (size) {
-WARN_ON_ONCE(1);
-err = -EINVAL;
-goto out;
-}
-}
-
-   err = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
- cmd, reqtype, value, index, buf, size,
- USB_CTRL_SET_TIMEOUT);
-   kfree(buf);
+   if (size && !data) {
+   WARN_ON_ONCE(1);
+   return -EINVAL;
+   }
 
-out:
-   return err;
+   return usb_control_msg_send(dev->udev, 0,
+   cmd, reqtype, value, index, data, size,
+   USB_CTRL_SET_TIMEOUT, GFP_KERNEL);
 }
 
 /*
-- 
2.25.1

Re: [PATCH v3] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API

[Anant Thazhemadam]

On 02/11/20 3:10 pm, Oliver Neukum wrote:
> Am Sonntag, den 01.11.2020, 03:05 +0530 schrieb Anant Thazhemadam:
>> Currently, __usbnet_{read|write}_cmd() use usb_control_msg().
>> However, this could lead to potential partial reads/writes being
>> considered valid, and since most of the callers of
>> usbnet_{read|write}_cmd() don't take partial reads/writes into account
>> (only checking for negative error number is done), and this can lead to
>> issues.
>>
> Hi,
>
> plesae send this as a post of its own. We cannot take a new set
> as a reply to an older set.
>
>   Regards
>   Oliver
>

Got it. I will do that.

Thanks,
Anant

Re: [PATCH] net: can: prevent potential access of uninitialized value in canfd_rcv()

[Anant Thazhemadam]

On 02-11-2020 12:40, Marc Kleine-Budde wrote:
> On 11/2/20 4:13 AM, Anant Thazhemadam wrote:
>> In canfd_rcv(), cfd->len is uninitialized when skb->len = 0, and this
>> uninitialized cfd->len is accessed nonetheless by pr_warn_once().
>>
>> Fix this uninitialized variable access by checking cfd->len's validity
>> condition (cfd->len > CANFD_MAX_DLEN) separately after the skb->len's
>> condition is checked, and appropriately modify the log messages that
>> are generated as well.
>> In case either of the required conditions fail, the skb is freed and
>> NET_RX_DROP is returned, same as before.
>>
>> Reported-by: syzbot+9bcb0c9409066696d...@syzkaller.appspotmail.com
>> Tested-by: Anant Thazhemadam 
>> Signed-off-by: Anant Thazhemadam 
>> ---
>> This patch was locally tested using the reproducer and .config file 
>> generated by syzbot.
>>
>>  net/can/af_can.c | 19 ++-
>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/can/af_can.c b/net/can/af_can.c
>> index ea29a6d97ef5..1b9f2e50f065 100644
>> --- a/net/can/af_can.c
>> +++ b/net/can/af_can.c
>> @@ -694,16 +694,25 @@ static int canfd_rcv(struct sk_buff *skb, struct 
>> net_device *dev,
> Can you create a similar patch for "can_rcv()"?

Yes, I can. Would it be alright if that was part of the v2 itself (since it's 
similar changes)?
Or would I have to split them up into 2 different patches and send it as a 
2-patch series
(since the changes made are in different functions)?

>
>>  {
>>  struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
>>  
>> -if (unlikely(dev->type != ARPHRD_CAN || skb->len != CANFD_MTU ||
>> - cfd->len > CANFD_MAX_DLEN)) {
>> -pr_warn_once("PF_CAN: dropped non conform CAN FD skbuf: dev 
>> type %d, len %d, datalen %d\n",
>> +if (unlikely(dev->type != ARPHRD_CAN || skb->len != CANFD_MTU)) {
>> +pr_warn_once("PF_CAN: dropped non conform CAN FD skbuff: dev 
>> type %d, len %d\n",
>> + dev->type, skb->len);
>> +goto free_skb;
>> +}
>> +
>> +// This check is made separately since cfd->len would be uninitialized 
>> if skb->len = 0.
> Please don't use C++ comment style in the kernel.

Noted. I'll have this fixed in the v2.

>
>> +else if (unlikely(cfd->len > CANFD_MAX_DLEN)) {
> Please move the "else" right after the closing curly bracket: "} else if () {"
> or convert it into an "if () {"

Noted.

>
>> +pr_warn_once("PF_CAN: dropped non conform CAN FD skbuff: dev 
>> type %d, len %d, datalen %d\n",
>>   dev->type, skb->len, cfd->len);
>> -kfree_skb(skb);
>> -return NET_RX_DROP;
>> +goto free_skb;
>>  }
>>  
>>  can_receive(skb, dev);
>>  return NET_RX_SUCCESS;
>> +
>> +free_skb:
>> +kfree_skb(skb);
>> +return NET_RX_DROP;
>>  }
>>  
>>  /* af_can protocol functions */
>>
> regards,
> Marc

Thank you for your time.

Thanks,
Anant

[PATCH] net: can: prevent potential access of uninitialized value in canfd_rcv()

[Anant Thazhemadam]

In canfd_rcv(), cfd->len is uninitialized when skb->len = 0, and this
uninitialized cfd->len is accessed nonetheless by pr_warn_once().

Fix this uninitialized variable access by checking cfd->len's validity
condition (cfd->len > CANFD_MAX_DLEN) separately after the skb->len's
condition is checked, and appropriately modify the log messages that
are generated as well.
In case either of the required conditions fail, the skb is freed and
NET_RX_DROP is returned, same as before.

Reported-by: syzbot+9bcb0c9409066696d...@syzkaller.appspotmail.com
Tested-by: Anant Thazhemadam 
Signed-off-by: Anant Thazhemadam 
---
This patch was locally tested using the reproducer and .config file 
generated by syzbot.

 net/can/af_can.c | 19 ++-
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/net/can/af_can.c b/net/can/af_can.c
index ea29a6d97ef5..1b9f2e50f065 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -694,16 +694,25 @@ static int canfd_rcv(struct sk_buff *skb, struct 
net_device *dev,
 {
struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
 
-   if (unlikely(dev->type != ARPHRD_CAN || skb->len != CANFD_MTU ||
-cfd->len > CANFD_MAX_DLEN)) {
-   pr_warn_once("PF_CAN: dropped non conform CAN FD skbuf: dev 
type %d, len %d, datalen %d\n",
+   if (unlikely(dev->type != ARPHRD_CAN || skb->len != CANFD_MTU)) {
+   pr_warn_once("PF_CAN: dropped non conform CAN FD skbuff: dev 
type %d, len %d\n",
+dev->type, skb->len);
+   goto free_skb;
+   }
+
+   // This check is made separately since cfd->len would be uninitialized 
if skb->len = 0.
+   else if (unlikely(cfd->len > CANFD_MAX_DLEN)) {
+   pr_warn_once("PF_CAN: dropped non conform CAN FD skbuff: dev 
type %d, len %d, datalen %d\n",
 dev->type, skb->len, cfd->len);
-   kfree_skb(skb);
-   return NET_RX_DROP;
+   goto free_skb;
}
 
can_receive(skb, dev);
return NET_RX_SUCCESS;
+
+free_skb:
+   kfree_skb(skb);
+   return NET_RX_DROP;
 }
 
 /* af_can protocol functions */
-- 
2.25.1

[PATCH v3] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API

[Anant Thazhemadam]

Currently, __usbnet_{read|write}_cmd() use usb_control_msg().
However, this could lead to potential partial reads/writes being
considered valid, and since most of the callers of
usbnet_{read|write}_cmd() don't take partial reads/writes into account
(only checking for negative error number is done), and this can lead to
issues.

However, the new usb_control_msg_{send|recv}() APIs don't allow partial
reads and writes.
Using the new APIs also relaxes the return value checking that must
be done after usbnet_{read|write}_cmd() is called.

Signed-off-by: Anant Thazhemadam 
---
Changes in v3:
* Aligned continuation lines after the opening brackets
Changes in v2:
* Fix build error

 drivers/net/usb/usbnet.c | 52 
 1 file changed, 10 insertions(+), 42 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index bf6c58240bd4..b2df3417a41c 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1982,64 +1982,32 @@ EXPORT_SYMBOL(usbnet_link_change);
 static int __usbnet_read_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
 u16 value, u16 index, void *data, u16 size)
 {
-   void *buf = NULL;
-   int err = -ENOMEM;
 
netdev_dbg(dev->net, "usbnet_read_cmd cmd=0x%02x reqtype=%02x"
   " value=0x%04x index=0x%04x size=%d\n",
   cmd, reqtype, value, index, size);
 
-   if (size) {
-   buf = kmalloc(size, GFP_KERNEL);
-   if (!buf)
-   goto out;
-   }
-
-   err = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
- cmd, reqtype, value, index, buf, size,
- USB_CTRL_GET_TIMEOUT);
-   if (err > 0 && err <= size) {
-if (data)
-memcpy(data, buf, err);
-else
-netdev_dbg(dev->net,
-"Huh? Data requested but thrown away.\n");
-}
-   kfree(buf);
-out:
-   return err;
+   return usb_control_msg_recv(dev->udev, 0,
+   cmd, reqtype, value, index, data, size,
+   USB_CTRL_GET_TIMEOUT, GFP_KERNEL);
 }
 
 static int __usbnet_write_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
  u16 value, u16 index, const void *data,
  u16 size)
 {
-   void *buf = NULL;
-   int err = -ENOMEM;
-
netdev_dbg(dev->net, "usbnet_write_cmd cmd=0x%02x reqtype=%02x"
   " value=0x%04x index=0x%04x size=%d\n",
   cmd, reqtype, value, index, size);
 
-   if (data) {
-   buf = kmemdup(data, size, GFP_KERNEL);
-   if (!buf)
-   goto out;
-   } else {
-if (size) {
-WARN_ON_ONCE(1);
-err = -EINVAL;
-goto out;
-}
-}
-
-   err = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
- cmd, reqtype, value, index, buf, size,
- USB_CTRL_SET_TIMEOUT);
-   kfree(buf);
+   if (size && !data) {
+   WARN_ON_ONCE(1);
+   return -EINVAL;
+   }
 
-out:
-   return err;
+   return usb_control_msg_send(dev->udev, 0,
+   cmd, reqtype, value, index, data, size,
+   USB_CTRL_SET_TIMEOUT, GFP_KERNEL);
 }
 
 /*
-- 
2.25.1

Re: [PATCH v2] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API

[Anant Thazhemadam]

On 01/11/20 2:41 am, Jakub Kicinski wrote:
> On Thu, 29 Oct 2020 18:52:56 +0530 Anant Thazhemadam wrote:
>> +return usb_control_msg_recv(dev->udev, 0,
>> +  cmd, reqtype, value, index, data, size,
>> +  USB_CTRL_GET_TIMEOUT, GFP_KERNEL);
> Please align continuation lines after the opening bracket.

I will do that, and send in a v3 right away.

Thanks,
Anant

Re: [PATCH v2] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API

[Anant Thazhemadam]

On 29/10/20 6:52 pm, Anant Thazhemadam wrote:
> Currently, __usbnet_{read|write}_cmd() use usb_control_msg(),
> and thus consider potential partial reads/writes being done to 
> be perfectly valid.
> Quite a few callers of usbnet_{read|write}_cmd() don't enforce
> checking for partial reads/writes into account either, automatically
> assuming that a complete read/write occurs.
>
> However, the new usb_control_msg_{send|recv}() APIs don't allow partial
> reads and writes.
> Using the new APIs also relaxes the return value checking that must
> be done after usbnet_{read|write}_cmd() is called.
>
> Signed-off-by: Anant Thazhemadam  
> <mailto:anant.thazhema...@gmail.com>
> ---
> Changes in v2:
>   * Fix build error
>
> This patch has been compile and build tested with a .config file that
> was generated using make allyesconfig, and the build error has been 
> fixed.
> Unfortunately, I wasn't able to get my hands on a usbnet adapter for testing,
> and would appreciate it if someone could do that.
>
>  drivers/net/usb/usbnet.c | 52 
>  1 file changed, 10 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index bf6c58240bd4..2f7c7b7f4047 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -1982,64 +1982,32 @@ EXPORT_SYMBOL(usbnet_link_change);
>  static int __usbnet_read_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
>u16 value, u16 index, void *data, u16 size)
>  {
> - void *buf = NULL;
> - int err = -ENOMEM;
>  
>   netdev_dbg(dev->net, "usbnet_read_cmd cmd=0x%02x reqtype=%02x"
>  " value=0x%04x index=0x%04x size=%d\n",
>  cmd, reqtype, value, index, size);
>  
> - if (size) {
> - buf = kmalloc(size, GFP_KERNEL);
> - if (!buf)
> - goto out;
> - }
> -
> - err = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
> -   cmd, reqtype, value, index, buf, size,
> -   USB_CTRL_GET_TIMEOUT);
> - if (err > 0 && err <= size) {
> -if (data)
> -memcpy(data, buf, err);
> -else
> -netdev_dbg(dev->net,
> -"Huh? Data requested but thrown away.\n");
> -}
> - kfree(buf);
> -out:
> - return err;
> + return usb_control_msg_recv(dev->udev, 0,
> +   cmd, reqtype, value, index, data, size,
> +   USB_CTRL_GET_TIMEOUT, GFP_KERNEL);
>  }
>  
>  static int __usbnet_write_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
> u16 value, u16 index, const void *data,
> u16 size)
>  {
> - void *buf = NULL;
> - int err = -ENOMEM;
> -
>   netdev_dbg(dev->net, "usbnet_write_cmd cmd=0x%02x reqtype=%02x"
>  " value=0x%04x index=0x%04x size=%d\n",
>  cmd, reqtype, value, index, size);
>  
> - if (data) {
> - buf = kmemdup(data, size, GFP_KERNEL);
> - if (!buf)
> - goto out;
> - } else {
> -if (size) {
> -WARN_ON_ONCE(1);
> -err = -EINVAL;
> -goto out;
> -}
> -}
> -
> - err = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
> -   cmd, reqtype, value, index, buf, size,
> -   USB_CTRL_SET_TIMEOUT);
> - kfree(buf);
> + if (size && !data) {
> + WARN_ON_ONCE(1);
> + return -EINVAL;
> + }
>  
> -out:
> - return err;
> + return usb_control_msg_send(dev->udev, 0,
> + cmd, reqtype, value, index, data, size,
> + USB_CTRL_SET_TIMEOUT, GFP_KERNEL);
>  }
>  
>  /*

I had a v2 prepared and ready but was told to wait for a week before sending it 
in,
since usb_control_msg_{send|recv}() that were being used were not present in the
networking tree at the time, and all the trees would be converged by then.
So, just to be on the safer side, I waited for two weeks.
I checked the net tree, and found the APIs there too (defined in usb.h).

However the build seems to fail here,
    
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org/thread/O2BERGN7SYYC6LNOOKNUGPS2IJLDWYT7/

I'm not entirely sure at this point why this is happening, and would appreciate 
it if
someone could take the time to tell me if and how this might be an issue with my
patch.

Thanks,
Anant

[PATCH v2] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API

[Anant Thazhemadam]

Currently, __usbnet_{read|write}_cmd() use usb_control_msg(),
and thus consider potential partial reads/writes being done to 
be perfectly valid.
Quite a few callers of usbnet_{read|write}_cmd() don't enforce
checking for partial reads/writes into account either, automatically
assuming that a complete read/write occurs.

However, the new usb_control_msg_{send|recv}() APIs don't allow partial
reads and writes.
Using the new APIs also relaxes the return value checking that must
be done after usbnet_{read|write}_cmd() is called.

Signed-off-by: Anant Thazhemadam 
---
Changes in v2:
* Fix build error

This patch has been compile and build tested with a .config file that
was generated using make allyesconfig, and the build error has been 
fixed.
Unfortunately, I wasn't able to get my hands on a usbnet adapter for testing,
and would appreciate it if someone could do that.

 drivers/net/usb/usbnet.c | 52 
 1 file changed, 10 insertions(+), 42 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index bf6c58240bd4..2f7c7b7f4047 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1982,64 +1982,32 @@ EXPORT_SYMBOL(usbnet_link_change);
 static int __usbnet_read_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
 u16 value, u16 index, void *data, u16 size)
 {
-   void *buf = NULL;
-   int err = -ENOMEM;
 
netdev_dbg(dev->net, "usbnet_read_cmd cmd=0x%02x reqtype=%02x"
   " value=0x%04x index=0x%04x size=%d\n",
   cmd, reqtype, value, index, size);
 
-   if (size) {
-   buf = kmalloc(size, GFP_KERNEL);
-   if (!buf)
-   goto out;
-   }
-
-   err = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
- cmd, reqtype, value, index, buf, size,
- USB_CTRL_GET_TIMEOUT);
-   if (err > 0 && err <= size) {
-if (data)
-memcpy(data, buf, err);
-else
-netdev_dbg(dev->net,
-"Huh? Data requested but thrown away.\n");
-}
-   kfree(buf);
-out:
-   return err;
+   return usb_control_msg_recv(dev->udev, 0,
+ cmd, reqtype, value, index, data, size,
+ USB_CTRL_GET_TIMEOUT, GFP_KERNEL);
 }
 
 static int __usbnet_write_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
  u16 value, u16 index, const void *data,
  u16 size)
 {
-   void *buf = NULL;
-   int err = -ENOMEM;
-
netdev_dbg(dev->net, "usbnet_write_cmd cmd=0x%02x reqtype=%02x"
   " value=0x%04x index=0x%04x size=%d\n",
   cmd, reqtype, value, index, size);
 
-   if (data) {
-   buf = kmemdup(data, size, GFP_KERNEL);
-   if (!buf)
-   goto out;
-   } else {
-if (size) {
-WARN_ON_ONCE(1);
-err = -EINVAL;
-goto out;
-}
-}
-
-   err = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
- cmd, reqtype, value, index, buf, size,
- USB_CTRL_SET_TIMEOUT);
-   kfree(buf);
+   if (size && !data) {
+   WARN_ON_ONCE(1);
+   return -EINVAL;
+   }
 
-out:
-   return err;
+   return usb_control_msg_send(dev->udev, 0,
+   cmd, reqtype, value, index, data, size,
+   USB_CTRL_SET_TIMEOUT, GFP_KERNEL);
 }
 
 /*
-- 
2.25.1

[tip: perf/urgent] staging: comedi: check validity of wMaxPacketSize of usb endpoints found

[tip-bot2 for Anant Thazhemadam]

The following commit has been merged into the perf/urgent branch of tip:

Commit-ID: 38df15cb4ce149ce3648d2a9ccc0140afa71fc02
Gitweb:
https://git.kernel.org/tip/38df15cb4ce149ce3648d2a9ccc0140afa71fc02
Author:Anant Thazhemadam 
AuthorDate:Sat, 10 Oct 2020 13:59:32 +05:30
Committer: Greg Kroah-Hartman 
CommitterDate: Sat, 17 Oct 2020 08:31:21 +02:00

staging: comedi: check validity of wMaxPacketSize of usb endpoints found

commit e1f13c879a7c21bd207dc6242455e8e3a1e88b40 upstream.

While finding usb endpoints in vmk80xx_find_usb_endpoints(), check if
wMaxPacketSize = 0 for the endpoints found.

Some devices have isochronous endpoints that have wMaxPacketSize = 0
(as required by the USB-2 spec).
However, since this doesn't apply here, wMaxPacketSize = 0 can be
considered to be invalid.

Reported-by: syzbot+009f546aa1370056b...@syzkaller.appspotmail.com
Tested-by: syzbot+009f546aa1370056b...@syzkaller.appspotmail.com
Signed-off-by: Anant Thazhemadam 
Cc: stable 
Link: 
https://lore.kernel.org/r/20201010082933.5417-1-anant.thazhema...@gmail.com
Signed-off-by: Greg Kroah-Hartman 
Signed-off-by: Greg Kroah-Hartman 

---
 drivers/staging/comedi/drivers/vmk80xx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/comedi/drivers/vmk80xx.c 
b/drivers/staging/comedi/drivers/vmk80xx.c
index 65dc6c5..7956abc 100644
--- a/drivers/staging/comedi/drivers/vmk80xx.c
+++ b/drivers/staging/comedi/drivers/vmk80xx.c
@@ -667,6 +667,9 @@ static int vmk80xx_find_usb_endpoints(struct comedi_device 
*dev)
if (!devpriv->ep_rx || !devpriv->ep_tx)
return -ENODEV;
 
+   if (!usb_endpoint_maxp(devpriv->ep_rx) || 
!usb_endpoint_maxp(devpriv->ep_tx))
+   return -EINVAL;
+
return 0;
 }
 

[PATCH v5] bluetooth: hci_h5: fix memory leak in h5_close

[Anant Thazhemadam]

When h5_close() is called, h5 is directly freed when !hu->serdev.
However, h5->rx_skb is not freed, which causes a memory leak.

Freeing h5->rx_skb and setting it to NULL, fixes this memory leak.

Fixes: ce945552fde4 ("Bluetooth: hci_h5: Add support for serdev enumerated 
devices")
Reported-by: syzbot+6ce141c55b2f7aafd...@syzkaller.appspotmail.com
Tested-by: syzbot+6ce141c55b2f7aafd...@syzkaller.appspotmail.com
Signed-off-by: Anant Thazhemadam 
---
Changes in v5:
* Set h5->rx_skb = NULL unconditionally - to improve code
  readability
* Update commit message accordingly

Changes in v4:
* Free h5->rx_skb even when hu->serdev
(Suggested by Hans de Goede )
* If hu->serdev, then assign h5->rx_skb = NULL

Changes in v3:
* Free h5->rx_skb when !hu->serdev, and fix the memory leak
* Do not incorrectly and unnecessarily call serdev_device_close()

Changes in v2:
* Fixed the Fixes tag


 drivers/bluetooth/hci_h5.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
index e41854e0d79a..0ef253136b06 100644
--- a/drivers/bluetooth/hci_h5.c
+++ b/drivers/bluetooth/hci_h5.c
@@ -245,6 +245,9 @@ static int h5_close(struct hci_uart *hu)
skb_queue_purge(>rel);
skb_queue_purge(>unrel);
 
+   kfree_skb(h5->rx_skb);
+   h5->rx_skb = NULL;
+
if (h5->vnd && h5->vnd->close)
h5->vnd->close(h5);
 
-- 
2.25.1

Re: [PATCH v4] bluetooth: hci_h5: fix memory leak in h5_close

[Anant Thazhemadam]

Hi,

On 16/10/20 4:58 pm, Hans de Goede wrote:
> Hi,
>
> On 10/7/20 5:48 AM, Anant Thazhemadam wrote:
>> If h5_close is called when !hu->serdev, h5 is directly freed.
>> However, h5->rx_skb is not freed, which causes a memory leak.
>>
>> Freeing h5->rx_skb fixes this memory leak.
>>
>> In case hu->serdev exists, h5->rx_skb is then set to NULL,
>> since we do not want to risk a potential NULL pointer 
>> dereference.
>>
>> Fixes: ce945552fde4 ("Bluetooth: hci_h5: Add support for serdev enumerated 
>> devices")
>> Reported-by: syzbot+6ce141c55b2f7aafd...@syzkaller.appspotmail.com
>> Tested-by: syzbot+6ce141c55b2f7aafd...@syzkaller.appspotmail.com
>> Signed-off-by: Anant Thazhemadam h5_close v4
>> ---
>> Changes in v4:
>>  * Free h5->rx_skb even when hu->serdev
>>  (Suggested by Hans de Goede )
>>  * If hu->serdev, then assign h5->rx_skb = NULL
>>
>> Changes in v3:
>>  * Free h5->rx_skb when !hu->serdev, and fix the memory leak
>>  * Do not incorrectly and unnecessarily call serdev_device_close()
>>
>> Changes in v2:
>>  * Fixed the Fixes tag
>>
>>  drivers/bluetooth/hci_h5.c | 4 
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
>> index e41854e0d79a..39f9553caa5c 100644
>> --- a/drivers/bluetooth/hci_h5.c
>> +++ b/drivers/bluetooth/hci_h5.c
>> @@ -245,11 +245,15 @@ static int h5_close(struct hci_uart *hu)
>>  skb_queue_purge(>rel);
>>  skb_queue_purge(>unrel);
>>  
>> +kfree_skb(h5->rx_skb);
>> +
>>  if (h5->vnd && h5->vnd->close)
>>  h5->vnd->close(h5);
>>  
>>  if (!hu->serdev)
>>  kfree(h5);
>> +else
>> +h5->rx_skb = NULL;
> Please just do this unconditionally directly after
> the kfree_skb()

Could you also please tell me why this might be necessary?
The pointer value stored at h5->rx_skb would be freed anyways when we free h5 
(since rx_skb is
essentially a member of the structure that h5 points to).
Also since we're performing the *if* check, the *else* condition wouldn't 
exactly be taxing either,
right?
Is there some performance metric that I'm missing where unconditionally setting 
it to NULL
in this manner would be better? (I couldn't find any resources that had any 
similar analysis
performed :/ )
Or is this in interest of code readability?

Also, how about we introduce a h5 = NULL, after freeing h5 when !hu->serdev?

Thank you for your time.

Thanks,
Anant

[PATCH v2] fs: gfs2: add validation checks for size of superblock

[Anant Thazhemadam]

In gfs2_check_sb(), no validation checks are performed with regards to
the size of the superblock.
syzkaller detected a slab-out-of-bounds bug that was primarily caused
because the block size for a superblock was set to zero.
A valid size for a superblock is a power of 2 between 512 and PAGE_SIZE.
Performing validation checks and ensuring that the size of the superblock
is valid fixes this bug.

Reported-by: syzbot+af90d47a37376844e...@syzkaller.appspotmail.com
Tested-by: syzbot+af90d47a37376844e...@syzkaller.appspotmail.com
Suggested-by: Andrew Price 
Signed-off-by: Anant Thazhemadam 
---

Changes in v2:

* Completely dropped the changes proposed in v1. Instead,
  validity checks for superblock size have been introduced. 
  (Suggested by Andrew Price)

* Addded a "Suggested-by" tag accrediting the patch idea to
  Andrew. If there's any issue with that, please let me know.

* Changed the commit header and commit message appropriately.

* Updated "Reported-by" and "Tested-by" tags to the same instance
  of the bug that was detected earlier (non consequential change).


 fs/gfs2/ops_fstype.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index 6d18d2c91add..f0605fae2c4c 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -169,6 +169,13 @@ static int gfs2_check_sb(struct gfs2_sbd *sdp, int silent)
return -EINVAL;
}
 
+   /* Check if the size of the block is valid - a power of 2 between 512 
and  PAGE_SIZE */
+   if (sb->sb_bsize < 512 || sb->sb_bsize > PAGE_SIZE || (sb->sb_bsize & 
(sb->sb_bsize - 1))) {
+   if (!silent)
+   pr_warn("Invalid superblock size\n");
+   return -EINVAL;
+   }
+
/*  If format numbers match exactly, we're done.  */
 
if (sb->sb_fs_format == GFS2_FORMAT_FS &&
-- 
2.25.1

Re: [Cluster-devel] [PATCH] fs: gfs2: prevent OOB access in gfs2_read_sb()

[Anant Thazhemadam]

On 14/10/20 6:34 pm, Andrew Price wrote:
> On 13/10/2020 16:26, Anant Thazhemadam wrote:
>> In gfs2_read_sb(), if the condition
>> (d != sdp->sd_heightsize[x - 1] || m)
>> isn't satisfied (in the first 11 iterations), the loop continues,
>> and begins to perform out-of-bounds access.
>> Fix this out-of-bounds access by introducing a condition in the for loop
>> that ensures that no more than GFS2_MAX_META_HEIGHT + 1 elements are
>> accessed.
>>
>> In addition to this, if the above condition is satisfied when
>> x = GFS2_MAX_META_HEIGHT (which = 10), and the flow of control breaks
>> out of the loop, then an out-of-bounds access is performed again while
>> assigning sdp->sd_heightsize[x] = ~0 (since x would be 11 now.), and
>> also the assertion that spd->sd_max_height <= GFS2_MAX_META_HEIGHT would
>> fail.
>> Fix this out-of-bounds access and ensure that the assertion doesn't fail
>> by introducing another variable "index", which keeps track of the last
>> valid value of x (pre-increment) that can be used.
>
> That's not quite the right approach. Your analysis below is correct: the 
> problem stems from the block size in the superblock being zeroed by the 
> fuzzer. So the correct fix would be to add a validation check for sb_bsize 
> (gfs2_check_sb() is lacking somewhat). Valid values are powers of 2 between 
> 512 and the page size.
>

I see. Thanks for the review! I'll send in a v2 that implements this check soon 
enough.

Thanks,
Anant

Re: [Cluster-devel] KASAN: slab-out-of-bounds Write in gfs2_fill_super

[Anant Thazhemadam]

On 30/09/20 7:52 pm, Andrew Price wrote:
> On 30/09/2020 13:39, syzbot wrote:
>> Hello,
>>
>> syzbot found the following issue on:
>>
>> HEAD commit:    fb0155a0 Merge tag 'nfs-for-5.9-3' of git://git.linux-nfs...
>> git tree:   upstream
>> console output: https://syzkaller.appspot.com/x/log.txt?x=13458c0f90
>> kernel config:  https://syzkaller.appspot.com/x/.config?x=adebb40048274f92
>> dashboard link: https://syzkaller.appspot.com/bug?extid=af90d47a37376844e731
>> compiler:   clang version 10.0.0 (https://github.com/llvm/llvm-project/ 
>> c2443155a0fb245c8f17f2c1c72b6ea391e86e81)
>> syz repro:  https://syzkaller.appspot.com/x/repro.syz?x=15c307d390
>> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=1353d58d90
>>
>> Bisection is inconclusive: the issue happens on the oldest tested release.
>>
>> bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=106acbbb90
>> final oops: https://syzkaller.appspot.com/x/report.txt?x=126acbbb90
>> console output: https://syzkaller.appspot.com/x/log.txt?x=146acbbb90
>>
>> IMPORTANT: if you fix the issue, please add the following tag to the commit:
>> Reported-by: syzbot+af90d47a37376844e...@syzkaller.appspotmail.com
>>
>> gfs2: fsid=loop0: Trying to join cluster "lock_nolock", "loop0"
>> gfs2: fsid=loop0: Now mounting FS...
>> ==
>> BUG: KASAN: slab-out-of-bounds in gfs2_read_sb fs/gfs2/ops_fstype.c:342 
>> [inline]
>> BUG: KASAN: slab-out-of-bounds in init_sb fs/gfs2/ops_fstype.c:479 [inline]
>> BUG: KASAN: slab-out-of-bounds in gfs2_fill_super+0x1db5/0x3fe0 
>> fs/gfs2/ops_fstype.c:1096
>> Write of size 8 at addr 88809073d548 by task syz-executor940/6853
>
> Bug filed for this:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1883929
>
> Andy
>
>> CPU: 1 PID: 6853 Comm: syz-executor940 Not tainted 5.9.0-rc7-syzkaller #0
>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS 
>> Google 01/01/2011
>> Call Trace:
>>   __dump_stack lib/dump_stack.c:77 [inline]
>>   dump_stack+0x1d6/0x29e lib/dump_stack.c:118
>>   print_address_description+0x66/0x620 mm/kasan/report.c:383
>>   __kasan_report mm/kasan/report.c:513 [inline]
>>   kasan_report+0x132/0x1d0 mm/kasan/report.c:530
>>   gfs2_read_sb fs/gfs2/ops_fstype.c:342 [inline]
>>   init_sb fs/gfs2/ops_fstype.c:479 [inline]
>>   gfs2_fill_super+0x1db5/0x3fe0 fs/gfs2/ops_fstype.c:1096
>>   get_tree_bdev+0x3e9/0x5f0 fs/super.c:1342
>>   gfs2_get_tree+0x4c/0x1f0 fs/gfs2/ops_fstype.c:1201
>>   vfs_get_tree+0x88/0x270 fs/super.c:1547
>>   do_new_mount fs/namespace.c:2875 [inline]
>>   path_mount+0x179d/0x29e0 fs/namespace.c:3192
>>   do_mount fs/namespace.c:3205 [inline]
>>   __do_sys_mount fs/namespace.c:3413 [inline]
>>   __se_sys_mount+0x126/0x180 fs/namespace.c:3390
>>   do_syscall_64+0x31/0x70 arch/x86/entry/common.c:46
>>   entry_SYSCALL_64_after_hwframe+0x44/0xa9
>> RIP: 0033:0x446dba
>> Code: b8 08 00 00 00 0f 05 48 3d 01 f0 ff ff 0f 83 fd ad fb ff c3 66 2e 0f 
>> 1f 84 00 00 00 00 00 66 90 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 
>> 0f 83 da ad fb ff c3 66 0f 1f 84 00 00 00 00 00
>> RSP: 002b:7fff4c56e748 EFLAGS: 0293 ORIG_RAX: 00a5
>> RAX: ffda RBX: 7fff4c56e7a0 RCX: 00446dba
>> RDX: 2000 RSI: 2100 RDI: 7fff4c56e760
>> RBP: 7fff4c56e760 R08: 7fff4c56e7a0 R09: 7fff0015
>> R10: 0220 R11: 0293 R12: 0001
>> R13: 0004 R14: 0003 R15: 0003
>>
>> Allocated by task 6853:
>>   kasan_save_stack mm/kasan/common.c:48 [inline]
>>   kasan_set_track mm/kasan/common.c:56 [inline]
>>   __kasan_kmalloc+0x100/0x130 mm/kasan/common.c:461
>>   kmem_cache_alloc_trace+0x1e4/0x2e0 mm/slab.c:3554
>>   kmalloc include/linux/slab.h:554 [inline]
>>   kzalloc include/linux/slab.h:666 [inline]
>>   init_sbd fs/gfs2/ops_fstype.c:77 [inline]
>>   gfs2_fill_super+0xb6/0x3fe0 fs/gfs2/ops_fstype.c:1018
>>   get_tree_bdev+0x3e9/0x5f0 fs/super.c:1342
>>   gfs2_get_tree+0x4c/0x1f0 fs/gfs2/ops_fstype.c:1201
>>   vfs_get_tree+0x88/0x270 fs/super.c:1547
>>   do_new_mount fs/namespace.c:2875 [inline]
>>   path_mount+0x179d/0x29e0 fs/namespace.c:3192
>>   do_mount fs/namespace.c:3205 [inline]
>>   __do_sys_mount fs/namespace.c:3413 [inline]
>>   __se_sys_mount+0x126/0x180 fs/namespace.c:3390
>>   do_syscall_64+0x31/0x70 arch/x86/entry/common.c:46
>>   entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>
>> The buggy address belongs to the object at 88809073c000
>>   which belongs to the cache kmalloc-8k of size 8192
>> The buggy address is located 5448 bytes inside of
>>   8192-byte region [88809073c000, 88809073e000)
>> The buggy address belongs to the page:
>> page:bd4b0b2d refcount:1 mapcount:0 mapping: 
>> index:0x0 pfn:0x9073c
>> head:bd4b0b2d order:2 compound_mapcount:0 compound_pincount:0
>> flags: 

[PATCH] fs: gfs2: prevent OOB access in gfs2_read_sb()

[Anant Thazhemadam]

In gfs2_read_sb(), if the condition
(d != sdp->sd_heightsize[x - 1] || m)
isn't satisfied (in the first 11 iterations), the loop continues,
and begins to perform out-of-bounds access.
Fix this out-of-bounds access by introducing a condition in the for loop
that ensures that no more than GFS2_MAX_META_HEIGHT + 1 elements are
accessed.

In addition to this, if the above condition is satisfied when
x = GFS2_MAX_META_HEIGHT (which = 10), and the flow of control breaks
out of the loop, then an out-of-bounds access is performed again while
assigning sdp->sd_heightsize[x] = ~0 (since x would be 11 now.), and
also the assertion that spd->sd_max_height <= GFS2_MAX_META_HEIGHT would
fail.
Fix this out-of-bounds access and ensure that the assertion doesn't fail
by introducing another variable "index", which keeps track of the last
valid value of x (pre-increment) that can be used.

Reported-by: syzbot+a5e2482a693e6b1e4...@syzkaller.appspotmail.com
Tested-by: syzbot+a5e2482a693e6b1e4...@syzkaller.appspotmail.com
Signed-off-by: Anant Thazhemadam 
---

I have one question here (potentially a place where I suspect this
patch could have a fatal flaw and might need some rework).

sdp->sd_max_height = x;
sdp->sd_heightsize[x] = ~0;

Were these lines written with the logic that the value of x would be
equal to (sdp->sd_heightsize[]'s last index filled in by the loop) + 1?
Or, is the expected value of x at these lines equal to 
(sdp->sd_heightsize[]'s last index as filled in by the loop)?
I would appreciate it if someone could clarify for me, how this would 
hold against the second potential out-of-bounds access I mentioned in my
commit message.

An additional comment (which I feel is of some significance) on this.
Reproducing the crash locally, I could infer that sdp->sd_fsb2bb_shift
sdp->sd_sb.sb_bsize, sdp->sd_sb.sb_bsize_shift, and sdp->sd_inptrs
were all 0.
This by extension also means that in gfs2_read_sb(), all the attributes
whose values were determined by performing some sort of calculation 
involving any one of these variables all resulted in either 0 or a 
negative value.
Simply doing the math will also show how this was also the primary reason
this OOB access occured in the first place. 
However, I still feel that gfs2_read_sb() could do with this bit of checking, 
since it fundamentally prevents OOB accesses from occuring in gfs2_read_sb()
in all scenarios.
Anyways, coming back to my initial point. Can having values like that be 
considered unacceptable and as something that needs to be handled (at 
gfs2_fill_super() maybe?) or is this non-anomalous behaviour and okay?

 fs/gfs2/ops_fstype.c | 15 ---
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index 6d18d2c91add..66ee8fb06ab9 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -281,7 +281,7 @@ static int gfs2_read_sb(struct gfs2_sbd *sdp, int silent)
 {
u32 hash_blocks, ind_blocks, leaf_blocks;
u32 tmp_blocks;
-   unsigned int x;
+   unsigned int x, index;
int error;
 
error = gfs2_read_super(sdp, GFS2_SB_ADDR >> sdp->sd_fsb2bb_shift, 
silent);
@@ -329,20 +329,21 @@ static int gfs2_read_sb(struct gfs2_sbd *sdp, int silent)
sdp->sd_heightsize[0] = sdp->sd_sb.sb_bsize -
sizeof(struct gfs2_dinode);
sdp->sd_heightsize[1] = sdp->sd_sb.sb_bsize * sdp->sd_diptrs;
-   for (x = 2;; x++) {
+   for (x = 2; x <= GFS2_MAX_META_HEIGHT; x++) {
u64 space, d;
u32 m;
 
-   space = sdp->sd_heightsize[x - 1] * sdp->sd_inptrs;
+   index = x;
+   space = sdp->sd_heightsize[index - 1] * sdp->sd_inptrs;
d = space;
m = do_div(d, sdp->sd_inptrs);
 
-   if (d != sdp->sd_heightsize[x - 1] || m)
+   if (d != sdp->sd_heightsize[index - 1] || m)
break;
-   sdp->sd_heightsize[x] = space;
+   sdp->sd_heightsize[index] = space;
}
-   sdp->sd_max_height = x;
-   sdp->sd_heightsize[x] = ~0;
+   sdp->sd_max_height = index;
+   sdp->sd_heightsize[index] = ~0;
gfs2_assert(sdp, sdp->sd_max_height <= GFS2_MAX_META_HEIGHT);
 
sdp->sd_max_dents_per_leaf = (sdp->sd_sb.sb_bsize -
-- 
2.25.1

Re: [PATCH net] net: 9p: initialize sun_server.sun_path to have addr's value only when addr is valid

[Anant Thazhemadam]

On 12-10-2020 13:29, Dominique Martinet wrote:
> Anant Thazhemadam wrote on Mon, Oct 12, 2020:
>> In p9_fd_create_unix, checking is performed to see if the addr (passed
>> as an argument) is NULL or not.
>> However, no check is performed to see if addr is a valid address, i.e.,
>> it doesn't entirely consist of only 0's.
>> The initialization of sun_server.sun_path to be equal to this faulty
>> addr value leads to an uninitialized variable, as detected by KMSAN.
>> Checking for this (faulty addr) and returning a negative error number
>> appropriately, resolves this issue.
> I'm not sure I agree a fully zeroed address is faulty but I agree we can
> probably refuse it given userspace can't pass useful abstract addresses
> here.


Understood. It's  probably a better that I modify the commit message a little 
and
send a v2 so it becomes more accurate.

> Just one nitpick but this is otherwise fine - good catch!

Thank you!

>
>> Reported-by: syzbot+75d51fe5bf4ebe988...@syzkaller.appspotmail.com
>> Tested-by: syzbot+75d51fe5bf4ebe988...@syzkaller.appspotmail.com
>> Signed-off-by: Anant Thazhemadam 
>> ---
>>  net/9p/trans_fd.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
>> index c0762a302162..8f528e783a6c 100644
>> --- a/net/9p/trans_fd.c
>> +++ b/net/9p/trans_fd.c
>> @@ -1023,7 +1023,7 @@ p9_fd_create_unix(struct p9_client *client, const char 
>> *addr, char *args)
>>  
>>  csocket = NULL;
>>  
>> -if (addr == NULL)
>> +if (!addr || !strlen(addr))
> Since we don't care about the actual length here, how about checking for
> addr[0] directly?
> That'll spare a strlen() call in the valid case.
>
You mentioned how a fully zeroed address isn't exactly faulty. By extension, 
wouldn't that
mean that an address that simply begins with a 0 isn't faulty as well?
This is an interesting point, because if the condition is modified to checking 
for addr[0] directly,
addresses that simply begin with 0 (but have more non-zero content following) 
wouldn't be
copied over either, right?
In the end, it comes down to what you define as a "valid" value that sun_path 
can have.
We've already agreed that a fully zeroed address wouldn't qualify as a valid 
value for sun_path.
Are addresses that aren't fully zeroed, but only begin with a 0 also to be 
considered as an
unacceptable value for sun_path?

Thanks,
Anant

[PATCH net] net: 9p: initialize sun_server.sun_path to have addr's value only when addr is valid

[Anant Thazhemadam]

In p9_fd_create_unix, checking is performed to see if the addr (passed
as an argument) is NULL or not.
However, no check is performed to see if addr is a valid address, i.e.,
it doesn't entirely consist of only 0's.
The initialization of sun_server.sun_path to be equal to this faulty
addr value leads to an uninitialized variable, as detected by KMSAN.
Checking for this (faulty addr) and returning a negative error number
appropriately, resolves this issue.

Reported-by: syzbot+75d51fe5bf4ebe988...@syzkaller.appspotmail.com
Tested-by: syzbot+75d51fe5bf4ebe988...@syzkaller.appspotmail.com
Signed-off-by: Anant Thazhemadam 
---
 net/9p/trans_fd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
index c0762a302162..8f528e783a6c 100644
--- a/net/9p/trans_fd.c
+++ b/net/9p/trans_fd.c
@@ -1023,7 +1023,7 @@ p9_fd_create_unix(struct p9_client *client, const char 
*addr, char *args)
 
csocket = NULL;
 
-   if (addr == NULL)
+   if (!addr || !strlen(addr))
return -EINVAL;
 
if (strlen(addr) >= UNIX_PATH_MAX) {
-- 
2.25.1

[PATCH v2] net: usb: rtl8150: don't incorrectly assign random MAC addresses

[Anant Thazhemadam]

In set_ethernet_addr(), if get_registers() succeeds, the ethernet address
that was read must be copied over. Otherwise, a random ethernet address
must be assigned.

get_registers() returns 0 if successful, and negative error number
otherwise. However, in set_ethernet_addr(), this return value is
incorrectly checked.

Since this return value will never be equal to sizeof(node_id), a
random MAC address will always be generated and assigned to the
device; even in cases when get_registers() is successful.

Correctly modifying the condition that checks if get_registers() was
successful or not fixes this problem, and copies the ethernet address
appropriately.

Fixes: f45a4248ea4c ("net: usb: rtl8150: set random MAC address when 
set_ethernet_addr() fails")
Signed-off-by: Anant Thazhemadam 
---
Changes in v2:
* Fixed the format of the Fixes tag
* Modified the commit message to better describe the issue being 
  fixed

+CCing Stephen and linux-next, since the commit fixed isn't in the networking
tree, but is present in linux-next.

 drivers/net/usb/rtl8150.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
index f020401adf04..bf8a60533f3e 100644
--- a/drivers/net/usb/rtl8150.c
+++ b/drivers/net/usb/rtl8150.c
@@ -261,7 +261,7 @@ static void set_ethernet_addr(rtl8150_t *dev)
 
ret = get_registers(dev, IDR, sizeof(node_id), node_id);
 
-   if (ret == sizeof(node_id)) {
+   if (!ret) {
ether_addr_copy(dev->netdev->dev_addr, node_id);
} else {
eth_hw_addr_random(dev->netdev);
-- 
2.25.1

Re: [PATCH] net: usb: rtl8150: don't incorrectly assign random MAC addresses

[Anant Thazhemadam]

Hi,

On 10/10/20 10:29 pm, Jakub Kicinski wrote:
> On Sat, 10 Oct 2020 12:14:59 +0530 Anant Thazhemadam wrote:
>> get_registers() directly returns the return value of
>> usb_control_msg_recv() - 0 if successful, and negative error number 
>> otherwise.
> Are you expecting Greg to take this as a part of some USB subsystem
> changes? I don't see usb_control_msg_recv() in my tree, and the
> semantics of usb_control_msg() are not what you described.

No, I'm not. usb_control_msg_recv() is an API that was recently
introduced, and get_registers() in rtl8150.c was also modified to
use it in order to prevent partial reads.

By your tree, I assume you mean
    https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git/
(it was the only one I could find).

I don't see the commit that this patch is supposed to fix in your
tree either... :/

Nonetheless, this commit fixes an issue that was applied to the
networking tree, and has made its way into linux-next as well, if
I'm not mistaken.

>> However, in set_ethernet_addr(), this return value is incorrectly 
>> checked.
>>
>> Since this return value will never be equal to sizeof(node_id), a 
>> random MAC address will always be generated and assigned to the 
>> device; even in cases when get_registers() is successful.
>>
>> Correctly modifying the condition that checks if get_registers() was 
>> successful or not fixes this problem, and copies the ethernet address
>> appropriately.
>>
>> Fixes: f45a4248ea4c ("set random MAC address when set_ethernet_addr() fails")
>> Signed-off-by: Anant Thazhemadam 
> The fixes tag does not follow the standard format:
>
> Fixes tag: Fixes: f45a4248ea4c ("set random MAC address when 
> set_ethernet_addr() fails")
> Has these problem(s):
>   - Subject does not match target commit subject
> Just use
>   git log -1 --format='Fixes: %h ("%s")'
>
>
> Please put the relevant maintainer in the To: field of the email, and
> even better - also mark the patch as [PATCH net], since it's a
> networking fix.

The script I've been using for sending patches in had been configured to CC
the maintainer(s) and respective mailing list(s). Sorry about that.

I will put the relevant maintainer in the To: field, fix the Fixes tag, and
mark the patch as [PATCH net] as well and send in a v2.

Thank you for your time.

Thanks,
Anant

Re: [PATCH] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API

[Anant Thazhemadam]

On 10/10/20 10:33 pm, Jakub Kicinski wrote:
> On Sat, 10 Oct 2020 12:26:23 +0530 Anant Thazhemadam wrote:
>> GPF_KERNEL
> You haven't even built this, let alone tested :/

I'm really sorry about this.
Turns out, my .config wasn't set generated by make allyesconfig, and thus
this regression went undetected.

I can submit a v2 that doesn't break the build, and which is build tested
properly.
However, I'm not clear on how else I could locally run unit tests pertaining to 
this
patch. I understand the critical requirement for testing changes and would 
really
appreciate it if someone could direct me towards some resource or another that
could help me figure that out too.

Once again, I'm really sorry for this oversight.
Thank you for your time.

Thanks,
Anant

Re: [PATCH] net: usb: rtl8150: don't incorrectly assign random MAC addresses

[Anant Thazhemadam]

On 10/10/20 11:46 pm, Jakub Kicinski wrote:
> On Sat, 10 Oct 2020 23:34:51 +0530 Anant Thazhemadam wrote:
>> On 10/10/20 10:29 pm, Jakub Kicinski wrote:
>>> On Sat, 10 Oct 2020 12:14:59 +0530 Anant Thazhemadam wrote:  
>>>> get_registers() directly returns the return value of
>>>> usb_control_msg_recv() - 0 if successful, and negative error number 
>>>> otherwise.  
>>> Are you expecting Greg to take this as a part of some USB subsystem
>>> changes? I don't see usb_control_msg_recv() in my tree, and the
>>> semantics of usb_control_msg() are not what you described.  
>> No, I'm not. usb_control_msg_recv() is an API that was recently
>> introduced, and get_registers() in rtl8150.c was also modified to
>> use it in order to prevent partial reads.
>>
>> By your tree, I assume you mean
>>     https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git/
>> (it was the only one I could find).
>>
>> I don't see the commit that this patch is supposed to fix in your
>> tree either... :/
>>
>> Nonetheless, this commit fixes an issue that was applied to the
>> networking tree, and has made its way into linux-next as well, if
>> I'm not mistaken.
> I mean the networking tree, what's the commit ID in linux-next?
>
> Your fixes tag points to f45a4248ea4c, but looks like the code was
> quite correct at that point.


Ah, my apologies. You're right. It doesn't look like those helpers have made
their way into the networking tree yet.

(This gets mentioned here as well,
    https://www.mail-archive.com/netdev@vger.kernel.org/msg357843.html)

The commit ID pointed to by the fixes tag is correct.
The change introduced by said commit looks right, but is logically incorrect.

get_registers() directly returns the return value of usb_control_msg_recv(),
and usb_control_msg_recv() returns 0 on success and negative error number
otherwise.

(You can find more about the new helpers here
    
https://lore.kernel.org/alsa-devel/20200914153756.3412156-1-gre...@linuxfoundation.org/
 )

The commit ID mentioned introduces a change that is supposed to copy over
the ethernet only when get_registers() succeeds, i.e., a complete read occurs,
and generate and set a random ethernet address otherwise (reading the
commit message should give some more insight).

The condition that checks if get_registers() succeeds (as specified in 
f45a4248ea4c)
was,
    ret == sizeof(node_id)
where ret is the return value of get_registers().

However, ret will never equal sizeof(node_id), since ret can only be equal to 0
or a negative number.

Thus, even in case where get_registers() succeeds, a randomly generated MAC
address would get copied over, instead of copying the appropriate ethernet
address, which is logically incorrect and not optimal.

Hence, we need to modify this to check if (ret == 0), and copy over the correct
ethernet address in that case, instead of randomly generating one and assigning
that.
Hope this helps.

Thanks,
Anant

[PATCH v3] staging: comedi: check validity of wMaxPacketSize of usb endpoints found

[Anant Thazhemadam]

While finding usb endpoints in vmk80xx_find_usb_endpoints(), check if 
wMaxPacketSize = 0 for the endpoints found.

Some devices have isochronous endpoints that have wMaxPacketSize = 0
(as required by the USB-2 spec).
However, since this doesn't apply here, wMaxPacketSize = 0 can be
considered to be invalid.

Reported-by: syzbot+009f546aa1370056b...@syzkaller.appspotmail.com
Tested-by: syzbot+009f546aa1370056b...@syzkaller.appspotmail.com
Signed-off-by: Anant Thazhemadam 
---
Changes in v3:
* Correctly list version information

Changes in v2:
* Fix coding style issue

The error (as detected by syzbot) is generated in 
vmk80xx_write_packet() (which is called in vmk80xx_reset_device()) when
it tries to assign devpriv->usb_tx_buf[0] = cmd.

This NULL pointer dereference issue arises because
size = usb_endpoint_maxp(devpriv->ep_tx) = 0.

This can be traced back to vmk80xx_find_usb_endpoints(), where the usb 
endpoints are found, and assigned accordingly.
(For some more insight, in vmk80xx_find_usb_endpoints(), 
if one of intf->cur_altsetting->iface_desc->endpoints' desc value = 0, 
and consequently this endpoint is assigned to devpriv->ep_tx,
this issue gets triggered.)

Checking if the wMaxPacketSize of an endpoint is invalid and returning
an error value accordingly, seems to fix the error.

We could also alternatively perform this checking (if the size is 0 or not) 
in vmk80xx_reset_device() itself, but it only seemed like covering up the issue
at that place, rather than fixing it, so I wasn't sure that was any better.

However, if I'm not wrong, this might end up causing the probe to fail, and I'm 
not sure if that's the right thing to do in cases like this, and if it isn't I'd
like some input on what exactly is the required course of action in cases like 
this.

 drivers/staging/comedi/drivers/vmk80xx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/comedi/drivers/vmk80xx.c 
b/drivers/staging/comedi/drivers/vmk80xx.c
index 65dc6c51037e..cb0a965d3c37 100644
--- a/drivers/staging/comedi/drivers/vmk80xx.c
+++ b/drivers/staging/comedi/drivers/vmk80xx.c
@@ -667,6 +667,9 @@ static int vmk80xx_find_usb_endpoints(struct comedi_device 
*dev)
if (!devpriv->ep_rx || !devpriv->ep_tx)
return -ENODEV;
 
+   if (!usb_endpoint_maxp(devpriv->ep_rx) || 
!usb_endpoint_maxp(devpriv->ep_tx))
+   return -EINVAL;
+
return 0;
 }
 
-- 
2.25.1

Re: [PATCH] staging: comedi: check validity of wMaxPacketSize of usb endpoints found

[Anant Thazhemadam]

On 10/10/20 1:39 pm, Greg Kroah-Hartman wrote:
> On Sat, Oct 10, 2020 at 07:29:13AM +0530, Anant Thazhemadam wrote:
>> Hi,
>>
>> On 10-10-2020 12:30, Greg Kroah-Hartman wrote:
>>> On Fri, Oct 09, 2020 at 09:50:29PM +0530, Anant Thazhemadam wrote:
>>>> While finding usb endpoints in vmk80xx_find_usb_endpoints(), check if 
>>>> wMaxPacketSize = 0 for the endpoints found.
>>>>
>>>> Some devices have isochronous endpoints that have wMaxPacketSize = 0
>>>> (as required by the USB-2 spec).
>>>> However, since this doesn't apply here, wMaxPacketSize = 0 can be
>>>> considered to be invalid.
>>>>
>>>> Reported-by: syzbot+009f546aa1370056b...@syzkaller.appspotmail.com
>>>> Tested-by: syzbot+009f546aa1370056b...@syzkaller.appspotmail.com
>>>> Signed-off-by: Anant Thazhemadam 
>>>> ---
>>> You sent 2 patches with the same subject, which one is the "latest" one?
>> This patch (that you have replied to) is the "latest" one.
>>
>>> Please always version your patches and put below the --- line what
>>> changed from the previous version, so that maintainers have a chance to
>>> know which to accept...
>> The other patch (with the same subject line) wasn't supposed to be sent out.
>> I realized there was a coding style error in that while sending, and 
>> cancelled
>> sending it, but it got sent nonetheless.
>> I would have included a v2 tag in this patch itself, but I didn't realize 
>> that the
>> previous one got sent until afterwards. :(
>> I'm sorry for that.
>>
>>> Can you fix this up and send a v3?
>> Shouldn't I resend this patch as a v2 instead? Since there wouldn't be any
>> changes from v2 (this patch) to v3 otherwise (unless of course, somebody 
>> could
>> suggest some more changes that could be made to this patch itself).
> The change would be that you are correctly listing the version
> information, so it would be v3 :)
>
Understood, thank you. I will send in a v3 as advised. :)

Thanks,
Anant

Re: [PATCH] staging: comedi: check validity of wMaxPacketSize of usb endpoints found

[Anant Thazhemadam]

Hi,

On 10-10-2020 12:30, Greg Kroah-Hartman wrote:
> On Fri, Oct 09, 2020 at 09:50:29PM +0530, Anant Thazhemadam wrote:
>> While finding usb endpoints in vmk80xx_find_usb_endpoints(), check if 
>> wMaxPacketSize = 0 for the endpoints found.
>>
>> Some devices have isochronous endpoints that have wMaxPacketSize = 0
>> (as required by the USB-2 spec).
>> However, since this doesn't apply here, wMaxPacketSize = 0 can be
>> considered to be invalid.
>>
>> Reported-by: syzbot+009f546aa1370056b...@syzkaller.appspotmail.com
>> Tested-by: syzbot+009f546aa1370056b...@syzkaller.appspotmail.com
>> Signed-off-by: Anant Thazhemadam 
>> ---
> You sent 2 patches with the same subject, which one is the "latest" one?

This patch (that you have replied to) is the "latest" one.

> Please always version your patches and put below the --- line what
> changed from the previous version, so that maintainers have a chance to
> know which to accept...

The other patch (with the same subject line) wasn't supposed to be sent out.
I realized there was a coding style error in that while sending, and cancelled
sending it, but it got sent nonetheless.
I would have included a v2 tag in this patch itself, but I didn't realize that 
the
previous one got sent until afterwards. :(
I'm sorry for that.

> Can you fix this up and send a v3?

Shouldn't I resend this patch as a v2 instead? Since there wouldn't be any
changes from v2 (this patch) to v3 otherwise (unless of course, somebody could
suggest some more changes that could be made to this patch itself).

Thanks,
Anant

[PATCH] net: usb: usbnet: update __usbnet_{read|write}_cmd() to use new API

[Anant Thazhemadam]

Currently, __usbnet_{read|write}_cmd() use usb_control_msg().
However, this could lead to potential partial reads/writes being
considered valid, and since most of the callers of
usbnet_{read|write}_cmd() don't take partial reads/writes into account
(only checking for negative error number is done), and this can lead to
issues.

However, the new usb_control_msg_{send|recv}() APIs don't allow partial
reads and writes.
Using the new APIs also relaxes the return value checking that must
be done after usbnet_{read|write}_cmd() is called.

Signed-off-by: Anant Thazhemadam 
---
Since not all callers of usbnet_{read|write}_cmd() check if a complete 
read/write happened, partial reads can go unnoticed.

This issue was briefly mentioned here.
https://lore.kernel.org/linux-usb/156564.25764.4.ca...@suse.com/

Using the new API in place of the old one doesn't break anything.
This is mainly because usb_control_msg_{send|recv}() returns 0 on success
and a negative error number on failure (which includes partial reads/writes).

Thus, the error checking condition provided by the present callers of 
usbnet_{read|write}_cmd() for failure (return value < 0 is considered as an 
error) will hold. 
And similarly, the condition checked by some callers for 'success' 
(return value >= 0 && return value < length/size) will also hold.

However, if I have missed out on any caller that this might cause problems with,
please let me know, and I will fix that up as well.

 drivers/net/usb/usbnet.c | 52 
 1 file changed, 10 insertions(+), 42 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index bf6c58240bd4..dd9fe530a374 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1982,64 +1982,32 @@ EXPORT_SYMBOL(usbnet_link_change);
 static int __usbnet_read_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
 u16 value, u16 index, void *data, u16 size)
 {
-   void *buf = NULL;
-   int err = -ENOMEM;
 
netdev_dbg(dev->net, "usbnet_read_cmd cmd=0x%02x reqtype=%02x"
   " value=0x%04x index=0x%04x size=%d\n",
   cmd, reqtype, value, index, size);
 
-   if (size) {
-   buf = kmalloc(size, GFP_KERNEL);
-   if (!buf)
-   goto out;
-   }
-
-   err = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
- cmd, reqtype, value, index, buf, size,
- USB_CTRL_GET_TIMEOUT);
-   if (err > 0 && err <= size) {
-if (data)
-memcpy(data, buf, err);
-else
-netdev_dbg(dev->net,
-"Huh? Data requested but thrown away.\n");
-}
-   kfree(buf);
-out:
-   return err;
+   return usb_control_msg_recv(dev->udev, 0,
+ cmd, reqtype, value, index, data, size,
+ USB_CTRL_GET_TIMEOUT, GFP_KERNEL);
 }
 
 static int __usbnet_write_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
  u16 value, u16 index, const void *data,
  u16 size)
 {
-   void *buf = NULL;
-   int err = -ENOMEM;
-
netdev_dbg(dev->net, "usbnet_write_cmd cmd=0x%02x reqtype=%02x"
   " value=0x%04x index=0x%04x size=%d\n",
   cmd, reqtype, value, index, size);
 
-   if (data) {
-   buf = kmemdup(data, size, GFP_KERNEL);
-   if (!buf)
-   goto out;
-   } else {
-if (size) {
-WARN_ON_ONCE(1);
-err = -EINVAL;
-goto out;
-}
-}
-
-   err = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
- cmd, reqtype, value, index, buf, size,
- USB_CTRL_SET_TIMEOUT);
-   kfree(buf);
+   if (size && !data) {
+   WARN_ON_ONCE(1);
+   return -EINVAL;
+   }
 
-out:
-   return err;
+   return usb_control_msg_send(dev->udev, 0,
+   cmd, reqtype, value, index, data, size,
+   USB_CTRL_SET_TIMEOUT, GPF_KERNEL);
 }
 
 /*
-- 
2.25.1

[PATCH] net: usb: rtl8150: don't incorrectly assign random MAC addresses

[Anant Thazhemadam]

get_registers() directly returns the return value of
usb_control_msg_recv() - 0 if successful, and negative error number 
otherwise.
However, in set_ethernet_addr(), this return value is incorrectly 
checked.

Since this return value will never be equal to sizeof(node_id), a 
random MAC address will always be generated and assigned to the 
device; even in cases when get_registers() is successful.

Correctly modifying the condition that checks if get_registers() was 
successful or not fixes this problem, and copies the ethernet address
appropriately.

Fixes: f45a4248ea4c ("set random MAC address when set_ethernet_addr() fails")
Signed-off-by: Anant Thazhemadam 
---
 drivers/net/usb/rtl8150.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
index f020401adf04..bf8a60533f3e 100644
--- a/drivers/net/usb/rtl8150.c
+++ b/drivers/net/usb/rtl8150.c
@@ -261,7 +261,7 @@ static void set_ethernet_addr(rtl8150_t *dev)
 
ret = get_registers(dev, IDR, sizeof(node_id), node_id);
 
-   if (ret == sizeof(node_id)) {
+   if (!ret) {
ether_addr_copy(dev->netdev->dev_addr, node_id);
} else {
eth_hw_addr_random(dev->netdev);
-- 
2.25.1

Re: [PATCH] staging: comedi: check validity of wMaxPacketSize of usb endpoints found

[Anant Thazhemadam]

On 09/10/20 9:46 pm, Anant Thazhemadam wrote:
> While finding usb endpoints in vmk80xx_find_usb_endpoints(), check if 
> wMaxPacketSize = 0 for the endpoints found.
>
> Some devices have isochronous endpoints that have wMaxPacketSize = 0
> (as required by the USB-2 spec).
> However, since this doesn't apply here, wMaxPacketSize = 0 can be
> considered to be invalid.
>
> Reported-by: syzbot+009f546aa1370056b...@syzkaller.appspotmail.com
> Tested-by: syzbot+009f546aa1370056b...@syzkaller.appspotmail.com
> Signed-off-by: Anant Thazhemadam 
> ---
> The error (as detected by syzbot) is generated in 
> vmk80xx_write_packet() (which is called in vmk80xx_reset_device()) when
> it tries to assign devpriv->usb_tx_buf[0] = cmd.
>
> This NULL pointer dereference issue arises because
> size = usb_endpoint_maxp(devpriv->ep_tx) = 0.
>
> This can be traced back to vmk80xx_find_usb_endpoints(), where the usb 
> endpoints are found, and assigned accordingly.
> (For some more insight, in vmk80xx_find_usb_endpoints(), 
> if one of intf->cur_altsetting->iface_desc->endpoints' desc value = 0, 
> and consequently this endpoint is assigned to devpriv->ep_tx,
> this issue gets triggered.)
>
> Checking if the wMaxPacketSize of an endpoint is invalid and returning
> an error value accordingly, seems to fix the error.
>
> We could also alternatively perform this checking (if the size is 0 or not) 
> in vmk80xx_reset_device() itself, but it only seemed like covering up the 
> issue
> at that place, rather than fixing it, so I wasn't sure that was any better.
>
> However, if I'm not wrong, this might end up causing the probe to fail, and 
> I'm 
> not sure if that's the right thing to do in cases like this, and if it isn't 
> I'd
> like some input on what exactly is the required course of action in cases 
> like this.
>
>  drivers/staging/comedi/drivers/vmk80xx.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/staging/comedi/drivers/vmk80xx.c 
> b/drivers/staging/comedi/drivers/vmk80xx.c
> index 65dc6c51037e..cb0a965d3c37 100644
> --- a/drivers/staging/comedi/drivers/vmk80xx.c
> +++ b/drivers/staging/comedi/drivers/vmk80xx.c
> @@ -667,6 +667,9 @@ static int vmk80xx_find_usb_endpoints(struct 
> comedi_device *dev)
>   if (!devpriv->ep_rx || !devpriv->ep_tx)
>   return -ENODEV;
>  
> + if(!usb_endpoint_maxp(devpriv->ep_rx) || 
> !usb_endpoint_maxp(devpriv->ep_tx))
> + return -EINVAL;
> +
>   return 0;
>  }
>  

The patch in this mail has a coding style issue (that I thought I had fixed), 
and was sent out by
mistake.
Please ignore this mail. Apologies.

Thanks,
Anant

[PATCH] staging: comedi: check validity of wMaxPacketSize of usb endpoints found

[Anant Thazhemadam]

While finding usb endpoints in vmk80xx_find_usb_endpoints(), check if 
wMaxPacketSize = 0 for the endpoints found.

Some devices have isochronous endpoints that have wMaxPacketSize = 0
(as required by the USB-2 spec).
However, since this doesn't apply here, wMaxPacketSize = 0 can be
considered to be invalid.

Reported-by: syzbot+009f546aa1370056b...@syzkaller.appspotmail.com
Tested-by: syzbot+009f546aa1370056b...@syzkaller.appspotmail.com
Signed-off-by: Anant Thazhemadam 
---
The error (as detected by syzbot) is generated in 
vmk80xx_write_packet() (which is called in vmk80xx_reset_device()) when
it tries to assign devpriv->usb_tx_buf[0] = cmd.

This NULL pointer dereference issue arises because
size = usb_endpoint_maxp(devpriv->ep_tx) = 0.

This can be traced back to vmk80xx_find_usb_endpoints(), where the usb 
endpoints are found, and assigned accordingly.
(For some more insight, in vmk80xx_find_usb_endpoints(), 
if one of intf->cur_altsetting->iface_desc->endpoints' desc value = 0, 
and consequently this endpoint is assigned to devpriv->ep_tx,
this issue gets triggered.)

Checking if the wMaxPacketSize of an endpoint is invalid and returning
an error value accordingly, seems to fix the error.

We could also alternatively perform this checking (if the size is 0 or not) 
in vmk80xx_reset_device() itself, but it only seemed like covering up the issue
at that place, rather than fixing it, so I wasn't sure that was any better.

However, if I'm not wrong, this might end up causing the probe to fail, and I'm 
not sure if that's the right thing to do in cases like this, and if it isn't I'd
like some input on what exactly is the required course of action in cases like 
this.

 drivers/staging/comedi/drivers/vmk80xx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/comedi/drivers/vmk80xx.c 
b/drivers/staging/comedi/drivers/vmk80xx.c
index 65dc6c51037e..cb0a965d3c37 100644
--- a/drivers/staging/comedi/drivers/vmk80xx.c
+++ b/drivers/staging/comedi/drivers/vmk80xx.c
@@ -667,6 +667,9 @@ static int vmk80xx_find_usb_endpoints(struct comedi_device 
*dev)
if (!devpriv->ep_rx || !devpriv->ep_tx)
return -ENODEV;
 
+   if (!usb_endpoint_maxp(devpriv->ep_rx) || 
!usb_endpoint_maxp(devpriv->ep_tx))
+   return -EINVAL;
+
return 0;
 }
 
-- 
2.25.1

[PATCH] staging: comedi: check validity of wMaxPacketSize of usb endpoints found

[Anant Thazhemadam]

While finding usb endpoints in vmk80xx_find_usb_endpoints(), check if 
wMaxPacketSize = 0 for the endpoints found.

Some devices have isochronous endpoints that have wMaxPacketSize = 0
(as required by the USB-2 spec).
However, since this doesn't apply here, wMaxPacketSize = 0 can be
considered to be invalid.

Reported-by: syzbot+009f546aa1370056b...@syzkaller.appspotmail.com
Tested-by: syzbot+009f546aa1370056b...@syzkaller.appspotmail.com
Signed-off-by: Anant Thazhemadam 
---
The error (as detected by syzbot) is generated in 
vmk80xx_write_packet() (which is called in vmk80xx_reset_device()) when
it tries to assign devpriv->usb_tx_buf[0] = cmd.

This NULL pointer dereference issue arises because
size = usb_endpoint_maxp(devpriv->ep_tx) = 0.

This can be traced back to vmk80xx_find_usb_endpoints(), where the usb 
endpoints are found, and assigned accordingly.
(For some more insight, in vmk80xx_find_usb_endpoints(), 
if one of intf->cur_altsetting->iface_desc->endpoints' desc value = 0, 
and consequently this endpoint is assigned to devpriv->ep_tx,
this issue gets triggered.)

Checking if the wMaxPacketSize of an endpoint is invalid and returning
an error value accordingly, seems to fix the error.

We could also alternatively perform this checking (if the size is 0 or not) 
in vmk80xx_reset_device() itself, but it only seemed like covering up the issue
at that place, rather than fixing it, so I wasn't sure that was any better.

However, if I'm not wrong, this might end up causing the probe to fail, and I'm 
not sure if that's the right thing to do in cases like this, and if it isn't I'd
like some input on what exactly is the required course of action in cases like 
this.

 drivers/staging/comedi/drivers/vmk80xx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/comedi/drivers/vmk80xx.c 
b/drivers/staging/comedi/drivers/vmk80xx.c
index 65dc6c51037e..cb0a965d3c37 100644
--- a/drivers/staging/comedi/drivers/vmk80xx.c
+++ b/drivers/staging/comedi/drivers/vmk80xx.c
@@ -667,6 +667,9 @@ static int vmk80xx_find_usb_endpoints(struct comedi_device 
*dev)
if (!devpriv->ep_rx || !devpriv->ep_tx)
return -ENODEV;
 
+   if(!usb_endpoint_maxp(devpriv->ep_rx) || 
!usb_endpoint_maxp(devpriv->ep_tx))
+   return -EINVAL;
+
return 0;
 }
 
-- 
2.25.1