Skip to contentSkip to conversation

i2c: cp2615: check for allocation failure in cp2615_i2c_recv()

PATCH Applied Dan Carpenter 3 participants 7 messages May 12, 2021 – Jun 20, 2021

Subsystems (3)
Kernel janitors I2C subsystem host drivers CP2615 I2C driver
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 i2c: cp2615: check for allocation failure in cp2615_i2c_recv()

We need to add a check for if the kzalloc() fails.

Fixes 4a7695429ead ("i2c: cp2615: add i2c driver for Silicon Labs' CP2615 Digital Audio Bridge")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/i2c/busses/i2c-cp2615.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
drivers/i2c/busses/i2c-cp2615.c +10 -4
index 78cfecd1ea76..3ded28632e4c 100644
@@ -138,17 +138,23 @@ cp2615_i2c_send(struct usb_interface *usbif, struct cp2615_i2c_transfer *i2c_w)
 static int
 cp2615_i2c_recv(struct usb_interface *usbif, unsigned char tag, void *buf)
 {
-	struct cp2615_iop_msg *msg = kzalloc(sizeof(*msg), GFP_KERNEL);
-	struct cp2615_i2c_transfer_result *i2c_r = (struct cp2615_i2c_transfer_result *)&msg->data;
 	struct usb_device *usbdev = interface_to_usbdev(usbif);
-	int res = usb_bulk_msg(usbdev, usb_rcvbulkpipe(usbdev, IOP_EP_IN),
-			       msg, sizeof(struct cp2615_iop_msg), NULL, 0);
+	struct cp2615_iop_msg *msg;
+	struct cp2615_i2c_transfer_result *i2c_r;
+	int res;
+
+	msg = kzalloc(sizeof(*msg), GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
 
+	res = usb_bulk_msg(usbdev, usb_rcvbulkpipe(usbdev, IOP_EP_IN), msg,
+			   sizeof(struct cp2615_iop_msg), NULL, 0);
 	if (res < 0) {
 		kfree(msg);
 		return res;
 	}
 
+	i2c_r = (struct cp2615_i2c_transfer_result *)&msg->data;
 	if (msg->msg != htons(iop_I2cTransferResult) || i2c_r->tag != tag) {
 		kfree(msg);
 		return -EIO;
Signature · 1 line
2.30.2
maintainer
#1 lore raw

Dan Carpenter <[email protected]> ezt írta (időpont: 2021. máj. 12., Sze, 12:07):

Quoted 2 lines

We need to add a check for if the kzalloc() fails.

That is correct, I missed that :/

Quoted 12 lines
Fixes 4a7695429ead ("i2c: cp2615: add i2c driver for Silicon Labs' CP2615 Digital Audio Bridge")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/i2c/busses/i2c-cp2615.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
drivers/i2c/busses/i2c-cp2615.c +0 -0
index 78cfecd1ea76..3ded28632e4c 100644

drivers/i2c/busses/i2c-cp2615.c cp2615_i2c_send(struct usb_interface *usbif, struct cp2615_i2c_transfer *i2c_w) · line 142

Quoted 5 lines
drivers/i2c/busses/i2c-cp2615.c +0 -2
@@ -138,17 +138,23 @@ cp2615_i2c_send(struct usb_interface *usbif, struct cp2615_i2c_transfer *i2c_w)
 static int
 cp2615_i2c_recv(struct usb_interface *usbif, unsigned char tag, void *buf)
 {
-       struct cp2615_iop_msg *msg = kzalloc(sizeof(*msg), GFP_KERNEL);
-       struct cp2615_i2c_transfer_result *i2c_r = (struct cp2615_i2c_transfer_result *)&msg->data;

These two lines could stay as-is, since the invalid i2c_r will never be used.

drivers/i2c/busses/i2c-cp2615.c cp2615_i2c_send(struct usb_interface *usbif, struct cp2615_i2c_transfer *i2c_w) · line 148

Quoted 10 lines
drivers/i2c/busses/i2c-cp2615.c +7 -2
        struct usb_device *usbdev = interface_to_usbdev(usbif);
-       int res = usb_bulk_msg(usbdev, usb_rcvbulkpipe(usbdev, IOP_EP_IN),
-                              msg, sizeof(struct cp2615_iop_msg), NULL, 0);
+       struct cp2615_iop_msg *msg;
+       struct cp2615_i2c_transfer_result *i2c_r;
+       int res;
+
+       msg = kzalloc(sizeof(*msg), GFP_KERNEL);
+       if (!msg)
+               return -ENOMEM;

You will want to also make cp2615_init_iop_msg() return -ENOMEM instead of -EINVAL, for consistency's sake.

Quoted 13 lines
drivers/i2c/busses/i2c-cp2615.c +3 -1
+       res = usb_bulk_msg(usbdev, usb_rcvbulkpipe(usbdev, IOP_EP_IN), msg,
+                          sizeof(struct cp2615_iop_msg), NULL, 0);
        if (res < 0) {
                kfree(msg);
                return res;
        }
 
+       i2c_r = (struct cp2615_i2c_transfer_result *)&msg->data;
        if (msg->msg != htons(iop_I2cTransferResult) || i2c_r->tag != tag) {
                kfree(msg);
                return -EIO;
--
Quoted 2 lines

2.30.2

Otherwise, the patch looks good, I shall sign off on the next version

author
#2 lore raw
Quoted 14 lines · Bence Csókás

Dan Carpenter <[email protected]> ezt írta (időpont: 2021. máj. 12., Sze, 12:07):

Quoted 2 lines

We need to add a check for if the kzalloc() fails.

That is correct, I missed that :/

Quoted 7 lines
Fixes 4a7695429ead ("i2c: cp2615: add i2c driver for Silicon Labs' CP2615 Digital Audio Bridge")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/i2c/busses/i2c-cp2615.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
Quoted 13 lines

drivers/i2c/busses/i2c-cp2615.c cp2615_i2c_send(struct usb_interface *usbif, struct cp2615_i2c_transfer *i2c_w) · line 142

Quoted 10 lines
drivers/i2c/busses/i2c-cp2615.c +0 -2
index 78cfecd1ea76..3ded28632e4c 100644
@@ -138,17 +138,23 @@ cp2615_i2c_send(struct usb_interface *usbif, struct cp2615_i2c_transfer *i2c_w)
 static int
 cp2615_i2c_recv(struct usb_interface *usbif, unsigned char tag, void *buf)
 {
-       struct cp2615_iop_msg *msg = kzalloc(sizeof(*msg), GFP_KERNEL);
-       struct cp2615_i2c_transfer_result *i2c_r = (struct cp2615_i2c_transfer_result *)&msg->data;

These two lines could stay as-is, since the invalid i2c_r will never be used.

Yeah, I know but people shouldn't put functions which can fail inside the declaration block. And I also know that doing:

	struct cp2615_i2c_transfer_result *i2c_r = (struct cp2615_i2c_transfer_result *)&msg->data;

is fine even when "msg" is NULL. But don't do that, because it is ugly.

Quoted 11 lines

drivers/i2c/busses/i2c-cp2615.c cp2615_i2c_send(struct usb_interface *usbif, struct cp2615_i2c_transfer *i2c_w) · line 148

Quoted 10 lines
drivers/i2c/busses/i2c-cp2615.c +7 -2
        struct usb_device *usbdev = interface_to_usbdev(usbif);
-       int res = usb_bulk_msg(usbdev, usb_rcvbulkpipe(usbdev, IOP_EP_IN),
-                              msg, sizeof(struct cp2615_iop_msg), NULL, 0);
+       struct cp2615_iop_msg *msg;
+       struct cp2615_i2c_transfer_result *i2c_r;
+       int res;
+
+       msg = kzalloc(sizeof(*msg), GFP_KERNEL);
+       if (!msg)
+               return -ENOMEM;
Quoted 3 lines

You will want to also make cp2615_init_iop_msg() return -ENOMEM instead of -EINVAL, for consistency's sake.

I view that as unrelated and I'm sorry but this error handling code is not really best practices... I don't have time to fix it up. :/

drivers/i2c/busses/i2c-cp2615.c

   124  static int
   125  cp2615_i2c_send(struct usb_interface *usbif, struct cp2615_i2c_transfer *i2c_w)
   126  {

127 struct cp2615_iop_msg *msg = kzalloc(sizeof(*msg), GFP_KERNEL);

Don't put functions which can fail inside the declaration block. Although, it is relatively uncommon to put allocations inside the declaration block these sorts of allocations are over represented when it comes to memory leaks and other static analysis bugs.

   128          struct usb_device *usbdev = interface_to_usbdev(usbif);
   129          int res = cp2615_init_i2c_msg(msg, i2c_w);

Handle failures immediately. Try not to mix the error paths and the success paths. After a function call, then you're going to have to deal with both failure and success path, but the failure path is just "clean up and return an error code" and the success path is hopefully going to continue for days or months. So get the shorter failure path out of the way first, then continue with the success path.

130
131 if (!res)

Always do error handling, don't do success handling.

132 res = usb_bulk_msg(usbdev, usb_sndbulkpipe(usbdev, IOP_EP_OUT),
133 msg, ntohs(msg->length), NULL, 0);

The success path should be indented one tab and the error handling should be indented two tabs.

   134          kfree(msg);
   135          return res;
   136  }

The function should look like this.

static int
cp2615_i2c_send(struct usb_interface *usbif, struct cp2615_i2c_transfer *i2c_w)

{
	struct usb_device *usbdev = interface_to_usbdev(usbif);
	struct cp2615_iop_msg *msg;
	int res;

	msg = kzalloc(sizeof(*msg), GFP_KERNEL);
	if (!msg)
		return -ENOMEM;

	res = cp2615_init_i2c_msg(msg, i2c_w);
	if (res)
		goto free;

	res = usb_bulk_msg(usbdev, usb_sndbulkpipe(usbdev, IOP_EP_OUT),
			   msg, ntohs(msg->length), NULL, 0);
free:
        kfree(msg);
        return res;
}

regards,
dan carpenter

maintainer
#3 lore raw

All right, if you want to do that, fine. I did it this way, because it took 1 less gotos... But your approach is equally valid.

But, to keep it consistent, if you change it one place, change it everywhere else.

Regards,
Bence

Dan Carpenter <[email protected]> ezt írta (időpont: 2021. máj. 12., Sze, 15:11):

Quoted 118 lines · Bence Csókás
Quoted 14 lines

Dan Carpenter <[email protected]> ezt írta (időpont: 2021. máj. 12., Sze, 12:07):

Quoted 2 lines

We need to add a check for if the kzalloc() fails.

That is correct, I missed that :/

Quoted 7 lines
Fixes 4a7695429ead ("i2c: cp2615: add i2c driver for Silicon Labs' CP2615 Digital Audio Bridge")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/i2c/busses/i2c-cp2615.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
Quoted 13 lines

drivers/i2c/busses/i2c-cp2615.c cp2615_i2c_send(struct usb_interface *usbif, struct cp2615_i2c_transfer *i2c_w) · line 142

Quoted 10 lines
drivers/i2c/busses/i2c-cp2615.c +0 -2
index 78cfecd1ea76..3ded28632e4c 100644
@@ -138,17 +138,23 @@ cp2615_i2c_send(struct usb_interface *usbif, struct cp2615_i2c_transfer *i2c_w)
 static int
 cp2615_i2c_recv(struct usb_interface *usbif, unsigned char tag, void *buf)
 {
-       struct cp2615_iop_msg *msg = kzalloc(sizeof(*msg), GFP_KERNEL);
-       struct cp2615_i2c_transfer_result *i2c_r = (struct cp2615_i2c_transfer_result *)&msg->data;

These two lines could stay as-is, since the invalid i2c_r will never be used.

Yeah, I know but people shouldn't put functions which can fail inside the declaration block. And I also know that doing:

        struct cp2615_i2c_transfer_result *i2c_r = (struct cp2615_i2c_transfer_result *)&msg->data;

is fine even when "msg" is NULL. But don't do that, because it is ugly.

Quoted 11 lines

drivers/i2c/busses/i2c-cp2615.c cp2615_i2c_send(struct usb_interface *usbif, struct cp2615_i2c_transfer *i2c_w) · line 148

Quoted 10 lines
drivers/i2c/busses/i2c-cp2615.c +7 -2
        struct usb_device *usbdev = interface_to_usbdev(usbif);
-       int res = usb_bulk_msg(usbdev, usb_rcvbulkpipe(usbdev, IOP_EP_IN),
-                              msg, sizeof(struct cp2615_iop_msg), NULL, 0);
+       struct cp2615_iop_msg *msg;
+       struct cp2615_i2c_transfer_result *i2c_r;
+       int res;
+
+       msg = kzalloc(sizeof(*msg), GFP_KERNEL);
+       if (!msg)
+               return -ENOMEM;
Quoted 3 lines

You will want to also make cp2615_init_iop_msg() return -ENOMEM instead of -EINVAL, for consistency's sake.

I view that as unrelated and I'm sorry but this error handling code is not really best practices... I don't have time to fix it up. :/

drivers/i2c/busses/i2c-cp2615.c

   124  static int
   125  cp2615_i2c_send(struct usb_interface *usbif, struct cp2615_i2c_transfer *i2c_w)
   126  {

127 struct cp2615_iop_msg *msg = kzalloc(sizeof(*msg), GFP_KERNEL);

Don't put functions which can fail inside the declaration block. Although, it is relatively uncommon to put allocations inside the declaration block these sorts of allocations are over represented when it comes to memory leaks and other static analysis bugs.

   128          struct usb_device *usbdev = interface_to_usbdev(usbif);
   129          int res = cp2615_init_i2c_msg(msg, i2c_w);

Handle failures immediately. Try not to mix the error paths and the success paths. After a function call, then you're going to have to deal with both failure and success path, but the failure path is just "clean up and return an error code" and the success path is hopefully going to continue for days or months. So get the shorter failure path out of the way first, then continue with the success path.

   130
   131          if (!res)

Always do error handling, don't do success handling.

132 res = usb_bulk_msg(usbdev, usb_sndbulkpipe(usbdev, IOP_EP_OUT),
133 msg, ntohs(msg->length), NULL, 0);

The success path should be indented one tab and the error handling should be indented two tabs.

   134          kfree(msg);
   135          return res;
   136  }

The function should look like this.

static int
cp2615_i2c_send(struct usb_interface *usbif, struct cp2615_i2c_transfer *i2c_w)

{
        struct usb_device *usbdev = interface_to_usbdev(usbif);
        struct cp2615_iop_msg *msg;
        int res;

        msg = kzalloc(sizeof(*msg), GFP_KERNEL);
        if (!msg)
                return -ENOMEM;

        res = cp2615_init_i2c_msg(msg, i2c_w);
        if (res)
                goto free;

        res = usb_bulk_msg(usbdev, usb_sndbulkpipe(usbdev, IOP_EP_OUT),
                           msg, ntohs(msg->length), NULL, 0);
free:
        kfree(msg);
        return res;
}

regards,
dan carpenter

maintainer
#5 lore raw

Sure, though I had hoped Dan would update his patch so that all allocation failures return -ENOMEM... Oh well.

Signed-off-by Bence Csókás<[email protected]>

Wolfram Sang <[email protected]> ezt írta (időpont: 2021. jún. 4., P 22:45):

Quoted 9 lines · Dan Carpenter
Quoted 4 lines

We need to add a check for if the kzalloc() fails.

Fixes 4a7695429ead ("i2c: cp2615: add i2c driver for Silicon Labs' CP2615 Digital Audio Bridge")
Signed-off-by Dan Carpenter <[email protected]>

Bence, are you okay with applying this patch as it fixes a bug?