Message ID | 20211101095539.423246-1-mudongliangabcd@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | media: em28xx: fix memory leak in em28xx_init_dev | expand |
On Mon, Nov 01, 2021 at 05:55:39PM +0800, Dongliang Mu wrote: > In the em28xx_init_rev, if em28xx_audio_setup fails, this function fails > to deallocate the media_dev allocated in the em28xx_media_device_init. > > Fix this by adding em28xx_unregister_media_device to free media_dev. > > BTW, this patch is tested in my local syzkaller instance, and it can > prevent the memory leak from occurring again. > > CC: Pavel Skripkin <paskripkin@gmail.com> > Fixes: 37ecc7b1278f ("[media] em28xx: add media controller support") > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > Reported-by: syzbot <syzkaller@googlegroups.com> Is this really a syzbot warning? If so it should be in the format: Reported-by: syzbot+4c4ffd1e1094dae61035@syzkaller.appspotmail.com Syzbot is different from syzkaller. Syzkaller is the fuzzer and syzbot is the program which reports syzkaller bugs. > --- > drivers/media/usb/em28xx/em28xx-cards.c | 19 +++++++++++++------ > 1 file changed, 13 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c > index c1e0dccb7408..fca68939ca50 100644 > --- a/drivers/media/usb/em28xx/em28xx-cards.c > +++ b/drivers/media/usb/em28xx/em28xx-cards.c > @@ -3625,8 +3625,10 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev, > There is no check to see if the em28xx_media_device_init() fails. I don't love that we call unregister() to undo the init() but it seems like it should work... > if (dev->is_audio_only) { > retval = em28xx_audio_setup(dev); > - if (retval) > - return -ENODEV; > + if (retval) { > + retval = -ENODEV; This was in the original code, but probably we should preserve the error code. > + goto err_deinit_media; > + } regards, dan carpenter
On 11/1/21 21:32, Dan Carpenter wrote: > On Mon, Nov 01, 2021 at 05:55:39PM +0800, Dongliang Mu wrote: >> In the em28xx_init_rev, if em28xx_audio_setup fails, this function fails >> to deallocate the media_dev allocated in the em28xx_media_device_init. >> >> Fix this by adding em28xx_unregister_media_device to free media_dev. >> >> BTW, this patch is tested in my local syzkaller instance, and it can >> prevent the memory leak from occurring again. >> >> CC: Pavel Skripkin <paskripkin@gmail.com> >> Fixes: 37ecc7b1278f ("[media] em28xx: add media controller support") >> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> >> Reported-by: syzbot <syzkaller@googlegroups.com> > > Is this really a syzbot warning? If so it should be in the format: > > Reported-by: syzbot+4c4ffd1e1094dae61035@syzkaller.appspotmail.com > > Syzbot is different from syzkaller. Syzkaller is the fuzzer and syzbot > is the program which reports syzkaller bugs. > Bug report is from his local instance. He just wants to give credit to syzbot for finding it >> --- >> drivers/media/usb/em28xx/em28xx-cards.c | 19 +++++++++++++------ >> 1 file changed, 13 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c >> index c1e0dccb7408..fca68939ca50 100644 >> --- a/drivers/media/usb/em28xx/em28xx-cards.c >> +++ b/drivers/media/usb/em28xx/em28xx-cards.c >> @@ -3625,8 +3625,10 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev, >> > > There is no check to see if the em28xx_media_device_init() fails. I I guess, it should work, since there a lot of checks to see if this pointer is valid, i.e driver can work even without this pointer, AFAIK > don't love that we call unregister() to undo the init() but it seems > like it should work... Same here, but it is out of scope of this patch :) With regards, Pavel Skripkin
On Tue, Nov 2, 2021 at 3:28 AM Pavel Skripkin <paskripkin@gmail.com> wrote: > > On 11/1/21 21:32, Dan Carpenter wrote: > > On Mon, Nov 01, 2021 at 05:55:39PM +0800, Dongliang Mu wrote: > >> In the em28xx_init_rev, if em28xx_audio_setup fails, this function fails > >> to deallocate the media_dev allocated in the em28xx_media_device_init. > >> > >> Fix this by adding em28xx_unregister_media_device to free media_dev. > >> > >> BTW, this patch is tested in my local syzkaller instance, and it can > >> prevent the memory leak from occurring again. > >> > >> CC: Pavel Skripkin <paskripkin@gmail.com> > >> Fixes: 37ecc7b1278f ("[media] em28xx: add media controller support") > >> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > >> Reported-by: syzbot <syzkaller@googlegroups.com> > > > > Is this really a syzbot warning? If so it should be in the format: > > > > Reported-by: syzbot+4c4ffd1e1094dae61035@syzkaller.appspotmail.com > > > > Syzbot is different from syzkaller. Syzkaller is the fuzzer and syzbot > > is the program which reports syzkaller bugs. > > > > Bug report is from his local instance. He just wants to give credit to > syzbot for finding it Hi Dan, just as explained by Pavel, I leveraged the local syzkaller instance to find this bug. I can modify it to "Reported-by: syzkaller <syzkaller@googlegroups.com>", this one looks better. > > >> --- > >> drivers/media/usb/em28xx/em28xx-cards.c | 19 +++++++++++++------ > >> 1 file changed, 13 insertions(+), 6 deletions(-) > >> > >> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c > >> index c1e0dccb7408..fca68939ca50 100644 > >> --- a/drivers/media/usb/em28xx/em28xx-cards.c > >> +++ b/drivers/media/usb/em28xx/em28xx-cards.c > >> @@ -3625,8 +3625,10 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev, > >> > > > > There is no check to see if the em28xx_media_device_init() fails. I > > I guess, it should work, since there a lot of checks to see if this > pointer is valid, i.e driver can work even without this pointer, AFAIK > > > don't love that we call unregister() to undo the init() but it seems > > like it should work... > > Same here, but it is out of scope of this patch :) From the implementation, em28xx_media_device_init and em28xx_unregister_media_device should not be a pair of functions (do/undo). Maybe I can write em28xx_free_media_device to be paired with em28xx_media_device_init, like below. And then only call it from the error handling context. static void em28xx_free_media_device(struct em28xx *dev) { #ifdef CONFIG_MEDIA_CONTROLLER kfree(dev->media_dev); dev->media_dev = NULL; #endif } > > > > With regards, > Pavel Skripkin
On Tue, Nov 02, 2021 at 02:31:26PM +0800, Dongliang Mu wrote: > On Tue, Nov 2, 2021 at 3:28 AM Pavel Skripkin <paskripkin@gmail.com> wrote: > > > > On 11/1/21 21:32, Dan Carpenter wrote: > > > On Mon, Nov 01, 2021 at 05:55:39PM +0800, Dongliang Mu wrote: > > >> In the em28xx_init_rev, if em28xx_audio_setup fails, this function fails > > >> to deallocate the media_dev allocated in the em28xx_media_device_init. > > >> > > >> Fix this by adding em28xx_unregister_media_device to free media_dev. > > >> > > >> BTW, this patch is tested in my local syzkaller instance, and it can > > >> prevent the memory leak from occurring again. > > >> > > >> CC: Pavel Skripkin <paskripkin@gmail.com> > > >> Fixes: 37ecc7b1278f ("[media] em28xx: add media controller support") > > >> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > > >> Reported-by: syzbot <syzkaller@googlegroups.com> > > > > > > Is this really a syzbot warning? If so it should be in the format: > > > > > > Reported-by: syzbot+4c4ffd1e1094dae61035@syzkaller.appspotmail.com > > > > > > Syzbot is different from syzkaller. Syzkaller is the fuzzer and syzbot > > > is the program which reports syzkaller bugs. > > > > > > > Bug report is from his local instance. He just wants to give credit to > > syzbot for finding it > > Hi Dan, > > just as explained by Pavel, I leveraged the local syzkaller instance > to find this bug. > > I can modify it to "Reported-by: syzkaller > <syzkaller@googlegroups.com>", this one looks better. > No need for a Reported-by at all, but if you want to credit syzkaller that's an okay format. > > > > >> --- > > >> drivers/media/usb/em28xx/em28xx-cards.c | 19 +++++++++++++------ > > >> 1 file changed, 13 insertions(+), 6 deletions(-) > > >> > > >> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c > > >> index c1e0dccb7408..fca68939ca50 100644 > > >> --- a/drivers/media/usb/em28xx/em28xx-cards.c > > >> +++ b/drivers/media/usb/em28xx/em28xx-cards.c > > >> @@ -3625,8 +3625,10 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev, > > >> > > > > > > There is no check to see if the em28xx_media_device_init() fails. I > > > > I guess, it should work, since there a lot of checks to see if this > > pointer is valid, i.e driver can work even without this pointer, AFAIK > > > > > don't love that we call unregister() to undo the init() but it seems > > > like it should work... > > > > Same here, but it is out of scope of this patch :) > > >From the implementation, em28xx_media_device_init and > em28xx_unregister_media_device should not be a pair of functions > (do/undo). > That's how it is now, but it's not necessarily how it should be. Anyway, it's unrelated to you patch. Just forget I mentioned it. regards, dan carpenter
On Tue, Nov 2, 2021 at 9:51 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Tue, Nov 02, 2021 at 02:31:26PM +0800, Dongliang Mu wrote: > > On Tue, Nov 2, 2021 at 3:28 AM Pavel Skripkin <paskripkin@gmail.com> wrote: > > > > > > On 11/1/21 21:32, Dan Carpenter wrote: > > > > On Mon, Nov 01, 2021 at 05:55:39PM +0800, Dongliang Mu wrote: > > > >> In the em28xx_init_rev, if em28xx_audio_setup fails, this function fails > > > >> to deallocate the media_dev allocated in the em28xx_media_device_init. > > > >> > > > >> Fix this by adding em28xx_unregister_media_device to free media_dev. > > > >> > > > >> BTW, this patch is tested in my local syzkaller instance, and it can > > > >> prevent the memory leak from occurring again. > > > >> > > > >> CC: Pavel Skripkin <paskripkin@gmail.com> > > > >> Fixes: 37ecc7b1278f ("[media] em28xx: add media controller support") > > > >> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > > > >> Reported-by: syzbot <syzkaller@googlegroups.com> > > > > > > > > Is this really a syzbot warning? If so it should be in the format: > > > > > > > > Reported-by: syzbot+4c4ffd1e1094dae61035@syzkaller.appspotmail.com > > > > > > > > Syzbot is different from syzkaller. Syzkaller is the fuzzer and syzbot > > > > is the program which reports syzkaller bugs. > > > > > > > > > > Bug report is from his local instance. He just wants to give credit to > > > syzbot for finding it > > > > Hi Dan, > > > > just as explained by Pavel, I leveraged the local syzkaller instance > > to find this bug. > > > > I can modify it to "Reported-by: syzkaller > > <syzkaller@googlegroups.com>", this one looks better. > > > > No need for a Reported-by at all, but if you want to credit syzkaller > that's an okay format. > Hi all, do I need to send a v2 patch with a new Reported-by tag? > > > > > > >> --- > > > >> drivers/media/usb/em28xx/em28xx-cards.c | 19 +++++++++++++------ > > > >> 1 file changed, 13 insertions(+), 6 deletions(-) > > > >> > > > >> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c > > > >> index c1e0dccb7408..fca68939ca50 100644 > > > >> --- a/drivers/media/usb/em28xx/em28xx-cards.c > > > >> +++ b/drivers/media/usb/em28xx/em28xx-cards.c > > > >> @@ -3625,8 +3625,10 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev, > > > >> > > > > > > > > There is no check to see if the em28xx_media_device_init() fails. I > > > > > > I guess, it should work, since there a lot of checks to see if this > > > pointer is valid, i.e driver can work even without this pointer, AFAIK > > > > > > > don't love that we call unregister() to undo the init() but it seems > > > > like it should work... > > > > > > Same here, but it is out of scope of this patch :) > > > > >From the implementation, em28xx_media_device_init and > > em28xx_unregister_media_device should not be a pair of functions > > (do/undo). > > > > That's how it is now, but it's not necessarily how it should be. Anyway, > it's unrelated to you patch. Just forget I mentioned it. > > regards, > dan carpenter >
diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c index c1e0dccb7408..fca68939ca50 100644 --- a/drivers/media/usb/em28xx/em28xx-cards.c +++ b/drivers/media/usb/em28xx/em28xx-cards.c @@ -3625,8 +3625,10 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev, if (dev->is_audio_only) { retval = em28xx_audio_setup(dev); - if (retval) - return -ENODEV; + if (retval) { + retval = -ENODEV; + goto err_deinit_media; + } em28xx_init_extension(dev); return 0; @@ -3645,7 +3647,7 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev, dev_err(&dev->intf->dev, "%s: em28xx_i2c_register bus 0 - error [%d]!\n", __func__, retval); - return retval; + goto err_deinit_media; } /* register i2c bus 1 */ @@ -3661,9 +3663,7 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev, "%s: em28xx_i2c_register bus 1 - error [%d]!\n", __func__, retval); - em28xx_i2c_unregister(dev, 0); - - return retval; + goto err_unreg_i2c; } } @@ -3671,6 +3671,13 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev, em28xx_card_setup(dev); return 0; + + +err_unreg_i2c: + em28xx_i2c_unregister(dev, 0); +err_deinit_media: + em28xx_unregister_media_device(dev); + return retval; } static int em28xx_duplicate_dev(struct em28xx *dev)
In the em28xx_init_rev, if em28xx_audio_setup fails, this function fails to deallocate the media_dev allocated in the em28xx_media_device_init. Fix this by adding em28xx_unregister_media_device to free media_dev. BTW, this patch is tested in my local syzkaller instance, and it can prevent the memory leak from occurring again. CC: Pavel Skripkin <paskripkin@gmail.com> Fixes: 37ecc7b1278f ("[media] em28xx: add media controller support") Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> Reported-by: syzbot <syzkaller@googlegroups.com> --- drivers/media/usb/em28xx/em28xx-cards.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)