* [PATCH] i2c: cp2615: check for allocation failure in cp2615_i2c_recv() @ 2021-05-12 10:06 Dan Carpenter 2021-05-12 10:54 ` Bence Csókás ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Dan Carpenter @ 2021-05-12 10:06 UTC (permalink / raw) To: Bence Csókás; +Cc: Wolfram Sang, linux-i2c, kernel-janitors 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 <dan.carpenter@oracle.com> --- drivers/i2c/busses/i2c-cp2615.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/drivers/i2c/busses/i2c-cp2615.c b/drivers/i2c/busses/i2c-cp2615.c index 78cfecd1ea76..3ded28632e4c 100644 --- a/drivers/i2c/busses/i2c-cp2615.c +++ b/drivers/i2c/busses/i2c-cp2615.c @@ -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; -- 2.30.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c: cp2615: check for allocation failure in cp2615_i2c_recv() 2021-05-12 10:06 [PATCH] i2c: cp2615: check for allocation failure in cp2615_i2c_recv() Dan Carpenter @ 2021-05-12 10:54 ` Bence Csókás 2021-05-12 13:10 ` Dan Carpenter 2021-06-04 20:44 ` Wolfram Sang 2021-06-20 21:12 ` Wolfram Sang 2 siblings, 1 reply; 7+ messages in thread From: Bence Csókás @ 2021-05-12 10:54 UTC (permalink / raw) To: Dan Carpenter; +Cc: Wolfram Sang, linux-i2c, kernel-janitors Dan Carpenter <dan.carpenter@oracle.com> ezt írta (időpont: 2021. máj. 12., Sze, 12:07): > > We need to add a check for if the kzalloc() fails. That is correct, I missed that :/ > > Fixes: 4a7695429ead ("i2c: cp2615: add i2c driver for Silicon Labs' CP2615 Digital Audio Bridge") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > drivers/i2c/busses/i2c-cp2615.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-cp2615.c b/drivers/i2c/busses/i2c-cp2615.c > index 78cfecd1ea76..3ded28632e4c 100644 > --- a/drivers/i2c/busses/i2c-cp2615.c > +++ b/drivers/i2c/busses/i2c-cp2615.c > @@ -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. > 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. > > + 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; > -- > 2.30.2 > Otherwise, the patch looks good, I shall sign off on the next version ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c: cp2615: check for allocation failure in cp2615_i2c_recv() 2021-05-12 10:54 ` Bence Csókás @ 2021-05-12 13:10 ` Dan Carpenter 2021-05-12 13:52 ` Bence Csókás 0 siblings, 1 reply; 7+ messages in thread From: Dan Carpenter @ 2021-05-12 13:10 UTC (permalink / raw) To: Bence Csókás; +Cc: Wolfram Sang, linux-i2c, kernel-janitors On Wed, May 12, 2021 at 12:54:47PM +0200, Bence Csókás wrote: > Dan Carpenter <dan.carpenter@oracle.com> ezt írta (időpont: 2021. máj. > 12., Sze, 12:07): > > > > We need to add a check for if the kzalloc() fails. > > That is correct, I missed that :/ > > > > > Fixes: 4a7695429ead ("i2c: cp2615: add i2c driver for Silicon Labs' CP2615 Digital Audio Bridge") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > drivers/i2c/busses/i2c-cp2615.c | 14 ++++++++++---- > > 1 file changed, 10 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-cp2615.c b/drivers/i2c/busses/i2c-cp2615.c > > index 78cfecd1ea76..3ded28632e4c 100644 > > --- a/drivers/i2c/busses/i2c-cp2615.c > > +++ b/drivers/i2c/busses/i2c-cp2615.c > > @@ -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. > > 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. > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c: cp2615: check for allocation failure in cp2615_i2c_recv() 2021-05-12 13:10 ` Dan Carpenter @ 2021-05-12 13:52 ` Bence Csókás 0 siblings, 0 replies; 7+ messages in thread From: Bence Csókás @ 2021-05-12 13:52 UTC (permalink / raw) To: Dan Carpenter; +Cc: Wolfram Sang, linux-i2c, kernel-janitors All right, if you want to do that, fine. I did it this way, because it took 1 less `goto`s... 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 <dan.carpenter@oracle.com> ezt írta (időpont: 2021. máj. 12., Sze, 15:11): > > On Wed, May 12, 2021 at 12:54:47PM +0200, Bence Csókás wrote: > > Dan Carpenter <dan.carpenter@oracle.com> ezt írta (időpont: 2021. máj. > > 12., Sze, 12:07): > > > > > > We need to add a check for if the kzalloc() fails. > > > > That is correct, I missed that :/ > > > > > > > > Fixes: 4a7695429ead ("i2c: cp2615: add i2c driver for Silicon Labs' CP2615 Digital Audio Bridge") > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > --- > > > drivers/i2c/busses/i2c-cp2615.c | 14 ++++++++++---- > > > 1 file changed, 10 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/i2c/busses/i2c-cp2615.c b/drivers/i2c/busses/i2c-cp2615.c > > > index 78cfecd1ea76..3ded28632e4c 100644 > > > --- a/drivers/i2c/busses/i2c-cp2615.c > > > +++ b/drivers/i2c/busses/i2c-cp2615.c > > > @@ -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. > > > > 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. > > > > 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c: cp2615: check for allocation failure in cp2615_i2c_recv() 2021-05-12 10:06 [PATCH] i2c: cp2615: check for allocation failure in cp2615_i2c_recv() Dan Carpenter 2021-05-12 10:54 ` Bence Csókás @ 2021-06-04 20:44 ` Wolfram Sang 2021-06-05 5:17 ` Bence Csókás 2021-06-20 21:12 ` Wolfram Sang 2 siblings, 1 reply; 7+ messages in thread From: Wolfram Sang @ 2021-06-04 20:44 UTC (permalink / raw) To: Dan Carpenter; +Cc: Bence Csókás, linux-i2c, kernel-janitors [-- Attachment #1: Type: text/plain, Size: 351 bytes --] On Wed, May 12, 2021 at 01:06:41PM +0300, Dan Carpenter wrote: > 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 <dan.carpenter@oracle.com> Bence, are you okay with applying this patch as it fixes a bug? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c: cp2615: check for allocation failure in cp2615_i2c_recv() 2021-06-04 20:44 ` Wolfram Sang @ 2021-06-05 5:17 ` Bence Csókás 0 siblings, 0 replies; 7+ messages in thread From: Bence Csókás @ 2021-06-05 5:17 UTC (permalink / raw) To: Wolfram Sang, Dan Carpenter, Bence Csókás, linux-i2c, kernel-janitors 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<bence98@sch.bme.hu> Wolfram Sang <wsa@kernel.org> ezt írta (időpont: 2021. jún. 4., P 22:45): > > On Wed, May 12, 2021 at 01:06:41PM +0300, Dan Carpenter wrote: > > 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 <dan.carpenter@oracle.com> > > Bence, are you okay with applying this patch as it fixes a bug? > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c: cp2615: check for allocation failure in cp2615_i2c_recv() 2021-05-12 10:06 [PATCH] i2c: cp2615: check for allocation failure in cp2615_i2c_recv() Dan Carpenter 2021-05-12 10:54 ` Bence Csókás 2021-06-04 20:44 ` Wolfram Sang @ 2021-06-20 21:12 ` Wolfram Sang 2 siblings, 0 replies; 7+ messages in thread From: Wolfram Sang @ 2021-06-20 21:12 UTC (permalink / raw) To: Dan Carpenter; +Cc: Bence Csókás, linux-i2c, kernel-janitors [-- Attachment #1: Type: text/plain, Size: 319 bytes --] On Wed, May 12, 2021 at 01:06:41PM +0300, Dan Carpenter wrote: > 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 <dan.carpenter@oracle.com> Applied to for-current, thanks! [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-06-20 21:12 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-12 10:06 [PATCH] i2c: cp2615: check for allocation failure in cp2615_i2c_recv() Dan Carpenter 2021-05-12 10:54 ` Bence Csókás 2021-05-12 13:10 ` Dan Carpenter 2021-05-12 13:52 ` Bence Csókás 2021-06-04 20:44 ` Wolfram Sang 2021-06-05 5:17 ` Bence Csókás 2021-06-20 21:12 ` Wolfram Sang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).