2022-07-23 04:52:37

by williamsukatube

Subject: [PATCH] i2c: cp2615: check the return value of kzalloc()

From: William Dean <[email protected]>

kzalloc() is a memory allocation function which can return NULL when
some internal memory errors happen. So it is better to check the
return value of it to prevent potential wrong memory access or
memory leak.

Fixes: 4a7695429eade ("i2c: cp2615: add i2c driver for Silicon Labs' CP2615 Digital Audio Bridge")
Reported-by: Hacash Robot <[email protected]>
Signed-off-by: William Dean <[email protected]>
---
drivers/i2c/busses/i2c-cp2615.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cp2615.c b/drivers/i2c/busses/i2c-cp2615.c
index 3ded28632e4c..7c9403346615 100644
--- a/drivers/i2c/busses/i2c-cp2615.c
+++ b/drivers/i2c/busses/i2c-cp2615.c
@@ -171,11 +171,17 @@ cp2615_i2c_recv(struct usb_interface *usbif, unsigned char tag, void *buf)
/* Checks if the IOP is functional by querying the part's ID */
static int cp2615_check_iop(struct usb_interface *usbif)
{
- struct cp2615_iop_msg *msg = kzalloc(sizeof(*msg), GFP_KERNEL);
- struct cp2615_iop_accessory_info *info = (struct cp2615_iop_accessory_info *)&msg->data;
+ struct cp2615_iop_msg *msg;
+ struct cp2615_iop_accessory_info *info;
struct usb_device *usbdev = interface_to_usbdev(usbif);
- int res = cp2615_init_iop_msg(msg, iop_GetAccessoryInfo, NULL, 0);
+ int res;
+
+ msg = kzalloc(sizeof(*msg), GFP_KERNEL);
+ if (!msg)
+ return -ENOMEM;

+ info = (struct cp2615_iop_accessory_info *)&msg->data;
+ res = cp2615_init_iop_msg(msg, iop_GetAccessoryInfo, NULL, 0);
if (res)
goto out;

--
2.25.1

2022-07-23 21:04:00

by Bence Csókás

Subject: Re: [PATCH] i2c: cp2615: check the return value of kzalloc()

Dear William,
thank you for your insight. However, you will find that the first
access of the variable `msg` is a call to `cp2615_init_iop_msg()`
which, on line 85, checks for the variable being NULL, and if so,
returns -EINVAL, which in turn causes `cp2615_check_iop()` to return
too. In fact, the same mechanism allows for `cp2615_i2c_send()` to
also not have to worry about `kzalloc()` returning NULL.
Though you could argue that in these cases, `cp2615_init_iop_msg()`
should return -ENOMEM instead of -EINVAL, and you would be correct.
Cheers,
Bence


<[email protected]> ezt írta (időpont: 2022. júl. 23., Szo, 6:33):
>
> From: William Dean <[email protected]>
>
> kzalloc() is a memory allocation function which can return NULL when
> some internal memory errors happen. So it is better to check the
> return value of it to prevent potential wrong memory access or
> memory leak.
>
> Fixes: 4a7695429eade ("i2c: cp2615: add i2c driver for Silicon Labs' CP2615 Digital Audio Bridge")
> Reported-by: Hacash Robot <[email protected]>
> Signed-off-by: William Dean <[email protected]>
> ---
> drivers/i2c/busses/i2c-cp2615.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-cp2615.c b/drivers/i2c/busses/i2c-cp2615.c
> index 3ded28632e4c..7c9403346615 100644
> --- a/drivers/i2c/busses/i2c-cp2615.c
> +++ b/drivers/i2c/busses/i2c-cp2615.c
> @@ -171,11 +171,17 @@ cp2615_i2c_recv(struct usb_interface *usbif, unsigned char tag, void *buf)
> /* Checks if the IOP is functional by querying the part's ID */
> static int cp2615_check_iop(struct usb_interface *usbif)
> {
> - struct cp2615_iop_msg *msg = kzalloc(sizeof(*msg), GFP_KERNEL);
> - struct cp2615_iop_accessory_info *info = (struct cp2615_iop_accessory_info *)&msg->data;
> + struct cp2615_iop_msg *msg;
> + struct cp2615_iop_accessory_info *info;
> struct usb_device *usbdev = interface_to_usbdev(usbif);
> - int res = cp2615_init_iop_msg(msg, iop_GetAccessoryInfo, NULL, 0);
> + int res;
> +
> + msg = kzalloc(sizeof(*msg), GFP_KERNEL);
> + if (!msg)
> + return -ENOMEM;
>
> + info = (struct cp2615_iop_accessory_info *)&msg->data;
> + res = cp2615_init_iop_msg(msg, iop_GetAccessoryInfo, NULL, 0);
> if (res)
> goto out;
>
> --
> 2.25.1
>
>