Re: Need help in debugging "memory leak in em28xx_init_dev"

Re: Need help in debugging "memory leak in em28xx_init_dev"

[Pavel Skripkin]

[-- Attachment #1: Type: text/plain, Size: 1311 bytes --]

On 11/1/21 06:02, Dongliang Mu wrote:
> Hi all,
> 
> My local syzkaller instance found one bug named "memory leak in
> em28xx_init_dev" in 5.14-rc5. Kernel configuration and PoC file are
> attached(I don't check if the latest kernel is vulnerable, but it
> should be). The trace from memleak is as follows:
> 
> backtrace:
>      [<ffffffff842cc66d>] kmalloc include/linux/slab.h:591 [inline]
>      [<ffffffff842cc66d>] kzalloc include/linux/slab.h:721 [inline]
>      [<ffffffff842cc66d>] em28xx_media_device_init
> drivers/media/usb/em28xx/em28xx-cards.c:3444 [inline]
>      [<ffffffff842cc66d>] em28xx_init_dev.isra.0+0x366/0x9bf
> drivers/media/usb/em28xx/em28xx-cards.c:3624
>      [<ffffffff842cd1bd>] em28xx_usb_probe.cold+0x4f7/0xf95
> drivers/media/usb/em28xx/em28xx-cards.c:3979
>      [<ffffffff82bf0815>] usb_probe_interface+0x185/0x350
> drivers/usb/core/driver.c:396
> 


Looks like missing clean up on error handling path.

->probe()
     em28xx_init_dev()
       em28xx_media_device_init() <- dev->media_dev allocated
       *error somewhere in em28xx_init_dev()*


And then nothing unwinds em28xx_media_device_init() call, since 
disconnect won't be called in case of failure in ->probe()


Just build tested, but, I guess, something like this should work.



With regards,
Pavel Skripkin




[-- Attachment #2: ph --]
[-- Type: text/plain, Size: 1219 bytes --]

diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
index c1e0dccb7408..f22e5ca2d1b3 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -3626,7 +3626,7 @@ 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;
+			goto deinit_media;
 		em28xx_init_extension(dev);
 
 		return 0;
@@ -3645,7 +3645,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 deinit_media;
 	}
 
 	/* register i2c bus 1 */
@@ -3663,7 +3663,7 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
 
 			em28xx_i2c_unregister(dev, 0);
 
-			return retval;
+			goto deinit_media;
 		}
 	}
 
@@ -3671,6 +3671,10 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
 	em28xx_card_setup(dev);
 
 	return 0;
+
+deinit_media:
+	em28xx_unregister_media_device(dev);
+	return retval;
 }
 
 static int em28xx_duplicate_dev(struct em28xx *dev)

Re: Need help in debugging "memory leak in em28xx_init_dev"

[Dongliang Mu]

On Mon, Nov 1, 2021 at 3:50 PM Pavel Skripkin <paskripkin@gmail.com> wrote:
>
> On 11/1/21 06:02, Dongliang Mu wrote:
> > Hi all,
> >
> > My local syzkaller instance found one bug named "memory leak in
> > em28xx_init_dev" in 5.14-rc5. Kernel configuration and PoC file are
> > attached(I don't check if the latest kernel is vulnerable, but it
> > should be). The trace from memleak is as follows:
> >
> > backtrace:
> >      [<ffffffff842cc66d>] kmalloc include/linux/slab.h:591 [inline]
> >      [<ffffffff842cc66d>] kzalloc include/linux/slab.h:721 [inline]
> >      [<ffffffff842cc66d>] em28xx_media_device_init
> > drivers/media/usb/em28xx/em28xx-cards.c:3444 [inline]
> >      [<ffffffff842cc66d>] em28xx_init_dev.isra.0+0x366/0x9bf
> > drivers/media/usb/em28xx/em28xx-cards.c:3624
> >      [<ffffffff842cd1bd>] em28xx_usb_probe.cold+0x4f7/0xf95
> > drivers/media/usb/em28xx/em28xx-cards.c:3979
> >      [<ffffffff82bf0815>] usb_probe_interface+0x185/0x350
> > drivers/usb/core/driver.c:396
> >
>
>
> Looks like missing clean up on error handling path.
>
> ->probe()
>      em28xx_init_dev()
>        em28xx_media_device_init() <- dev->media_dev allocated
>        *error somewhere in em28xx_init_dev()*
>

Hi Pavel,

you're right. In some error handling code (em28xx_audio_setup fails),
em28xx_init_dev fails to deallocated the media_dev field.

>
> And then nothing unwinds em28xx_media_device_init() call, since
> disconnect won't be called in case of failure in ->probe()
>
>
> Just build tested, but, I guess, something like this should work.
>
>
>
> With regards,
> Pavel Skripkin
>
>
>

Re: Need help in debugging "memory leak in em28xx_init_dev"

[Pavel Skripkin]

On 11/1/21 11:55, Dongliang Mu wrote:

>>
>>
>> Looks like missing clean up on error handling path.
>>
>> ->probe()
>>      em28xx_init_dev()
>>        em28xx_media_device_init() <- dev->media_dev allocated
>>        *error somewhere in em28xx_init_dev()*
>>
> 
> Hi Pavel,
> 
> you're right. In some error handling code (em28xx_audio_setup fails),
> em28xx_init_dev fails to deallocated the media_dev field.
> 

Hi, Dongliang,

Did patch attached to my previous email pass syzbot's reproducer test? 
Unfortunately, I am not able to test rn :(




With regards,
Pavel Skripkin

Re: Need help in debugging "memory leak in em28xx_init_dev"

[Dongliang Mu]

On Mon, Nov 1, 2021 at 5:19 PM Pavel Skripkin <paskripkin@gmail.com> wrote:
>
> On 11/1/21 11:55, Dongliang Mu wrote:
>
> >>
> >>
> >> Looks like missing clean up on error handling path.
> >>
> >> ->probe()
> >>      em28xx_init_dev()
> >>        em28xx_media_device_init() <- dev->media_dev allocated
> >>        *error somewhere in em28xx_init_dev()*
> >>
> >
> > Hi Pavel,
> >
> > you're right. In some error handling code (em28xx_audio_setup fails),
> > em28xx_init_dev fails to deallocated the media_dev field.
> >
>
> Hi, Dongliang,
>
> Did patch attached to my previous email pass syzbot's reproducer test?
> Unfortunately, I am not able to test rn :(

Yes, it works. The memory leak does not occur anymore.

But I am crafting another patch based on yours as there is a small
issue in the retval and I would like to make the error handling code
uniform.

>
>
>
>
> With regards,
> Pavel Skripkin

Re: Need help in debugging "memory leak in em28xx_init_dev"

[Pavel Skripkin]

On 11/1/21 12:41, Dongliang Mu wrote:
>> Hi, Dongliang,
>>
>> Did patch attached to my previous email pass syzbot's reproducer test?
>> Unfortunately, I am not able to test rn :(
> 
> Yes, it works. The memory leak does not occur anymore.
> 
> But I am crafting another patch based on yours as there is a small
> issue in the retval and I would like to make the error handling code
> uniform.
> 

Cool! Thank you for confirmation.


With regards,
Pavel Skripkin

Re: Need help in debugging "memory leak in em28xx_init_dev"

[Dongliang Mu]

On Mon, Nov 1, 2021 at 5:43 PM Pavel Skripkin <paskripkin@gmail.com> wrote:
>
> On 11/1/21 12:41, Dongliang Mu wrote:
> >> Hi, Dongliang,
> >>
> >> Did patch attached to my previous email pass syzbot's reproducer test?
> >> Unfortunately, I am not able to test rn :(
> >
> > Yes, it works. The memory leak does not occur anymore.
> >
> > But I am crafting another patch based on yours as there is a small
> > issue in the retval and I would like to make the error handling code
> > uniform.
> >
>
> Cool! Thank you for confirmation.

Hi Pavel,

Thanks for your advice. I have sent the patch and you are on the CC
list. Can you please take a look at and review my patch?

It should cover your patch. But I am not sure if I introduce any new
issue in the patch.

>
>
> With regards,
> Pavel Skripkin

Re: Need help in debugging "memory leak in em28xx_init_dev"

[Pavel Skripkin]

On 11/1/21 12:58, Dongliang Mu wrote:
> On Mon, Nov 1, 2021 at 5:43 PM Pavel Skripkin <paskripkin@gmail.com> wrote:
>>
>> On 11/1/21 12:41, Dongliang Mu wrote:
>> >> Hi, Dongliang,
>> >>
>> >> Did patch attached to my previous email pass syzbot's reproducer test?
>> >> Unfortunately, I am not able to test rn :(
>> >
>> > Yes, it works. The memory leak does not occur anymore.
>> >
>> > But I am crafting another patch based on yours as there is a small
>> > issue in the retval and I would like to make the error handling code
>> > uniform.
>> >
>>
>> Cool! Thank you for confirmation.
> 
> Hi Pavel,
> 
> Thanks for your advice. I have sent the patch and you are on the CC
> list. Can you please take a look at and review my patch?
> 
> It should cover your patch. But I am not sure if I introduce any new
> issue in the patch.
> 

The patch LGTM, but I can't drop R-b tag, since I am not an expert in 
this driver. Anyway, there is 100% missing clean up, so, I believe, you 
don't introduce new bugs



With regards,
Pavel Skripkin

Re: Need help in debugging "memory leak in em28xx_init_dev"

[Dongliang Mu]

On Mon, Nov 1, 2021 at 8:17 PM Pavel Skripkin <paskripkin@gmail.com> wrote:
>
> On 11/1/21 12:58, Dongliang Mu wrote:
> > On Mon, Nov 1, 2021 at 5:43 PM Pavel Skripkin <paskripkin@gmail.com> wrote:
> >>
> >> On 11/1/21 12:41, Dongliang Mu wrote:
> >> >> Hi, Dongliang,
> >> >>
> >> >> Did patch attached to my previous email pass syzbot's reproducer test?
> >> >> Unfortunately, I am not able to test rn :(
> >> >
> >> > Yes, it works. The memory leak does not occur anymore.
> >> >
> >> > But I am crafting another patch based on yours as there is a small
> >> > issue in the retval and I would like to make the error handling code
> >> > uniform.
> >> >
> >>
> >> Cool! Thank you for confirmation.
> >
> > Hi Pavel,
> >
> > Thanks for your advice. I have sent the patch and you are on the CC
> > list. Can you please take a look at and review my patch?
> >
> > It should cover your patch. But I am not sure if I introduce any new
> > issue in the patch.
> >
>
> The patch LGTM, but I can't drop R-b tag, since I am not an expert in
> this driver. Anyway, there is 100% missing clean up, so, I believe, you
> don't introduce new bugs

drop? I do see some patches from local syzkaller will attach this tag
to assign credits to syzkaller/syzbot.

I think this form is good. Thus I copy this tag from them.

>
>
>
> With regards,
> Pavel Skripkin

Re: Need help in debugging "memory leak in em28xx_init_dev"

[Pavel Skripkin]

On 11/1/21 15:23, Dongliang Mu wrote:
> drop? I do see some patches from local syzkaller will attach this tag
> to assign credits to syzkaller/syzbot.
> 
> I think this form is good. Thus I copy this tag from them.
> 

Sorry for not being clear :(

I mean, I can't send my "Reviewed-by" tag to the patch. Missed that 
"R-b" can be also decoded as Reported-by


With regards,
Pavel Skripkin

Re: Need help in debugging "memory leak in em28xx_init_dev"

[Dongliang Mu]

On Mon, Nov 1, 2021 at 8:26 PM Pavel Skripkin <paskripkin@gmail.com> wrote:
>
> On 11/1/21 15:23, Dongliang Mu wrote:
> > drop? I do see some patches from local syzkaller will attach this tag
> > to assign credits to syzkaller/syzbot.
> >
> > I think this form is good. Thus I copy this tag from them.
> >
>
> Sorry for not being clear :(
>
> I mean, I can't send my "Reviewed-by" tag to the patch. Missed that
> "R-b" can be also decoded as Reported-by

OK. It's fine. Just want to make sure we don't introduce any new
issues in the patch.

>
>
> With regards,
> Pavel Skripkin

Re: Need help in debugging "memory leak in em28xx_init_dev"

[Dan Carpenter]

On Mon, Nov 01, 2021 at 05:58:56PM +0800, Dongliang Mu wrote:
> On Mon, Nov 1, 2021 at 5:43 PM Pavel Skripkin <paskripkin@gmail.com> wrote:
> >
> > On 11/1/21 12:41, Dongliang Mu wrote:
> > >> Hi, Dongliang,
> > >>
> > >> Did patch attached to my previous email pass syzbot's reproducer test?
> > >> Unfortunately, I am not able to test rn :(
> > >
> > > Yes, it works. The memory leak does not occur anymore.
> > >
> > > But I am crafting another patch based on yours as there is a small
> > > issue in the retval and I would like to make the error handling code
> > > uniform.
> > >
> >
> > Cool! Thank you for confirmation.
> 
> Hi Pavel,
> 
> Thanks for your advice. I have sent the patch and you are on the CC
> list. Can you please take a look at and review my patch?
> 

What's the Message-ID of your patch so I can b4 it.

This whole thread has no patches.  I don't know why I'm CC'd when the
only part I'm interested in is not included...  :/  Hashtag Grumble.

regards,
dan carpenter

Re: Need help in debugging "memory leak in em28xx_init_dev"

[Dongliang Mu]

On Mon, Nov 1, 2021 at 10:30 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Mon, Nov 01, 2021 at 05:58:56PM +0800, Dongliang Mu wrote:
> > On Mon, Nov 1, 2021 at 5:43 PM Pavel Skripkin <paskripkin@gmail.com> wrote:
> > >
> > > On 11/1/21 12:41, Dongliang Mu wrote:
> > > >> Hi, Dongliang,
> > > >>
> > > >> Did patch attached to my previous email pass syzbot's reproducer test?
> > > >> Unfortunately, I am not able to test rn :(
> > > >
> > > > Yes, it works. The memory leak does not occur anymore.
> > > >
> > > > But I am crafting another patch based on yours as there is a small
> > > > issue in the retval and I would like to make the error handling code
> > > > uniform.
> > > >
> > >
> > > Cool! Thank you for confirmation.
> >
> > Hi Pavel,
> >
> > Thanks for your advice. I have sent the patch and you are on the CC
> > list. Can you please take a look at and review my patch?
> >
>
> What's the Message-ID of your patch so I can b4 it.
>
> This whole thread has no patches.  I don't know why I'm CC'd when the
> only part I'm interested in is not included...  :/  Hashtag Grumble.
>

You are back to work already?

I forget to CC you in my `git send-email'. This thread asks for help
as I cannot figure out the root cause of this bug.

> regards,
> dan carpenter
>

Re: Need help in debugging "memory leak in em28xx_init_dev"

[Randy Dunlap]

On 11/1/21 7:30 AM, Dan Carpenter wrote:
> On Mon, Nov 01, 2021 at 05:58:56PM +0800, Dongliang Mu wrote:
>> On Mon, Nov 1, 2021 at 5:43 PM Pavel Skripkin <paskripkin@gmail.com> wrote:
>>>
>>> On 11/1/21 12:41, Dongliang Mu wrote:
>>>>> Hi, Dongliang,
>>>>>
>>>>> Did patch attached to my previous email pass syzbot's reproducer test?
>>>>> Unfortunately, I am not able to test rn :(
>>>>
>>>> Yes, it works. The memory leak does not occur anymore.
>>>>
>>>> But I am crafting another patch based on yours as there is a small
>>>> issue in the retval and I would like to make the error handling code
>>>> uniform.
>>>>
>>>
>>> Cool! Thank you for confirmation.
>>
>> Hi Pavel,
>>
>> Thanks for your advice. I have sent the patch and you are on the CC
>> list. Can you please take a look at and review my patch?
>>
> 
> What's the Message-ID of your patch so I can b4 it.

<20211101095539.423246-1-mudongliangabcd@gmail.com>

> This whole thread has no patches.  I don't know why I'm CC'd when the
> only part I'm interested in is not included...  :/  Hashtag Grumble.
> 
> regards,
> dan carpenter
> 


-- 
~Randy

Re: Need help in debugging "memory leak in em28xx_init_dev"

[Dan Carpenter]

On Mon, Nov 01, 2021 at 08:05:47AM -0700, Randy Dunlap wrote:
> On 11/1/21 7:30 AM, Dan Carpenter wrote:
> > On Mon, Nov 01, 2021 at 05:58:56PM +0800, Dongliang Mu wrote:
> > > On Mon, Nov 1, 2021 at 5:43 PM Pavel Skripkin <paskripkin@gmail.com> wrote:
> > > > 
> > > > On 11/1/21 12:41, Dongliang Mu wrote:
> > > > > > Hi, Dongliang,
> > > > > > 
> > > > > > Did patch attached to my previous email pass syzbot's reproducer test?
> > > > > > Unfortunately, I am not able to test rn :(
> > > > > 
> > > > > Yes, it works. The memory leak does not occur anymore.
> > > > > 
> > > > > But I am crafting another patch based on yours as there is a small
> > > > > issue in the retval and I would like to make the error handling code
> > > > > uniform.
> > > > > 
> > > > 
> > > > Cool! Thank you for confirmation.
> > > 
> > > Hi Pavel,
> > > 
> > > Thanks for your advice. I have sent the patch and you are on the CC
> > > list. Can you please take a look at and review my patch?
> > > 
> > 
> > What's the Message-ID of your patch so I can b4 it.
> 
> <20211101095539.423246-1-mudongliangabcd@gmail.com>
> 

Thanks, Randy!

regards,
dan carpenter