Messages in this thread | | | Subject | Re: [PATCH] media: em28xx: Fix possible memory leak of em28xx struct | From | Igor Torrente <> | Date | Tue, 4 May 2021 16:07:27 -0300 |
| |
On 5/3/21 5:06 PM, Shuah Khan wrote: > Hi Igor, > > On 5/3/21 11:37 AM, Igor Matheus Andrade Torrente wrote: >> The em28xx struct kref isn't being decreased after an error in the >> em28xx_ir_init, leading to a possible memory leak. >> >> A kref_put is added to the error handler code. >> >> Signed-off-by: Igor Matheus Andrade Torrente <igormtorrente@gmail.com> >> --- >> drivers/media/usb/em28xx/em28xx-input.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/usb/em28xx/em28xx-input.c >> b/drivers/media/usb/em28xx/em28xx-input.c >> index 5aa15a7a49de..b89527014cad 100644 >> --- a/drivers/media/usb/em28xx/em28xx-input.c >> +++ b/drivers/media/usb/em28xx/em28xx-input.c >> @@ -720,7 +720,8 @@ static int em28xx_ir_init(struct em28xx *dev) >> dev->board.has_ir_i2c = 0; >> dev_warn(&dev->intf->dev, >> "No i2c IR remote control device found.\n"); >> - return -ENODEV; >> + err = -ENODEV; >> + goto ref_put; > > This doesn't look right. em28xx_init_buttons() is already happened and > em28xx_shutdown_buttons() needs to be done from fini. fini needs to run > with this ref. If ref is released here, device might be released before > em28xx_shutdown_buttons() can run leading to potential use-after-free >
Thanks for the feedback.
I sent a patch V2 that I think fix the problem pointed out.
>> } >> } >> @@ -735,7 +736,7 @@ static int em28xx_ir_init(struct em28xx *dev) >> ir = kzalloc(sizeof(*ir), GFP_KERNEL); >> if (!ir) >> - return -ENOMEM; >> + goto ref_put; > > This doesn't look right. Same comment as above. fini accounts for null > ir. > > em28xx_shutdown_buttons(dev); > > /* skip detach on non attached boards */ > if (!ir) > goto ref_put; > > >> rc = rc_allocate_device(RC_DRIVER_SCANCODE); >> if (!rc) >> goto error; >> @@ -839,6 +840,8 @@ static int em28xx_ir_init(struct em28xx *dev) >> dev->ir = NULL; >> rc_free_device(rc); >> kfree(ir); >> +ref_put: >> + kref_put(&dev->ref, em28xx_free_device); >> return err; >> } >> > > thanks, > -- Shuah
Best regard, --- Igor M. A. Torrente
| |