| Message ID | 20241107014827.3962940-1-ipylypiv@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: dev: Fix memory leak when underlying adapter does not support I2C | expand |
Hi Igor, that's a good catch, but... On Thu, Nov 07, 2024 at 01:48:27AM +0000, Igor Pylypiv wrote: > i2cdev_ioctl_rdwr() receives a buffer which is allocated by the caller. This needs to be a bit re-written. In the commit log you should describe what the patch does. You are telling where the buffer is allocated. > Fixes: 97ca843f6ad3 ("i2c: dev: Check for I2C_FUNC_I2C before calling i2c_transfer") > Signed-off-by: Igor Pylypiv <ipylypiv@google.com> > --- > drivers/i2c/i2c-dev.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c > index 61f7c4003d2f..5d15519ef737 100644 > --- a/drivers/i2c/i2c-dev.c > +++ b/drivers/i2c/i2c-dev.c > @@ -247,8 +247,10 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client, > int i, res; > > /* Adapter must support I2C transfers */ > - if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { > + kfree(msgs); > return -EOPNOTSUPP; Please, don't free it here, free it where it has been allocated, i.e. in compat_i2cdev_ioctl(). Andi > + } > > data_ptrs = kmalloc_array(nmsgs, sizeof(u8 __user *), GFP_KERNEL); > if (data_ptrs == NULL) { > -- > 2.47.0.277.g8800431eea-goog >
On Thu, Nov 07, 2024 at 06:14:29PM +0100, Andi Shyti wrote: > Hi Igor, > > that's a good catch, but... > > On Thu, Nov 07, 2024 at 01:48:27AM +0000, Igor Pylypiv wrote: > > i2cdev_ioctl_rdwr() receives a buffer which is allocated by the caller. > > This needs to be a bit re-written. In the commit log you should > describe what the patch does. You are telling where the buffer is > allocated. I thought subject line covered what the patch does. Ack. I'll update the commit message in v2. > > > Fixes: 97ca843f6ad3 ("i2c: dev: Check for I2C_FUNC_I2C before calling i2c_transfer") > > Signed-off-by: Igor Pylypiv <ipylypiv@google.com> > > --- > > drivers/i2c/i2c-dev.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c > > index 61f7c4003d2f..5d15519ef737 100644 > > --- a/drivers/i2c/i2c-dev.c > > +++ b/drivers/i2c/i2c-dev.c > > @@ -247,8 +247,10 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client, > > int i, res; > > > > /* Adapter must support I2C transfers */ > > - if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) > > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { > > + kfree(msgs); > > return -EOPNOTSUPP; > > Please, don't free it here, free it where it has been allocated, > i.e. in compat_i2cdev_ioctl(). > Sounds good. I'll move kfree() in v2. Thanks, Igor > Andi > > > + } > > > > data_ptrs = kmalloc_array(nmsgs, sizeof(u8 __user *), GFP_KERNEL); > > if (data_ptrs == NULL) { > > -- > > 2.47.0.277.g8800431eea-goog > >
diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c index 61f7c4003d2f..5d15519ef737 100644 --- a/drivers/i2c/i2c-dev.c +++ b/drivers/i2c/i2c-dev.c @@ -247,8 +247,10 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client *client, int i, res; /* Adapter must support I2C transfers */ - if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { + kfree(msgs); return -EOPNOTSUPP; + } data_ptrs = kmalloc_array(nmsgs, sizeof(u8 __user *), GFP_KERNEL); if (data_ptrs == NULL) {
i2cdev_ioctl_rdwr() receives a buffer which is allocated by the caller. Fixes: 97ca843f6ad3 ("i2c: dev: Check for I2C_FUNC_I2C before calling i2c_transfer") Signed-off-by: Igor Pylypiv <ipylypiv@google.com> --- drivers/i2c/i2c-dev.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)