Message ID | 20211101095539.423246-1-mudongliangabcd@gmail.com (mailing list archive) |
---|---|
State | Accepted, archived |
Delegated to: | Hans Verkuil |
Headers |
Received: from vger.kernel.org ([23.128.96.18]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1mhUGw-00AZwf-79; Mon, 01 Nov 2021 10:10:34 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231777AbhKAKNE (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Mon, 1 Nov 2021 06:13:04 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54392 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232161AbhKAKM4 (ORCPT <rfc822;linux-media@vger.kernel.org>); Mon, 1 Nov 2021 06:12:56 -0400 Received: from mail-pf1-x429.google.com (mail-pf1-x429.google.com [IPv6:2607:f8b0:4864:20::429]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BB976C061767; Mon, 1 Nov 2021 02:55:52 -0700 (PDT) Received: by mail-pf1-x429.google.com with SMTP id m26so15873978pff.3; Mon, 01 Nov 2021 02:55:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=UVw7JpRuL3Zpasl1yKpDjtfcQv9s3AR150vJWnlV/mo=; b=VF7DsrQ3BN98ddp3+F0aoZvy4BRNhLGMryo/aRHLnVAqnoSq1vrpYIbem0ev3R6De+ x0rEe7MPkW96oOhVrK/r23PyXE6tXqmlUuJspm63C2eEn3pFpiELfg4Dxo3UhZVx7iVK 4pU7zrvgQ+zNYERog0QnsoC0iu7ZTYwZxsvbaUAcyFGs1xYraZvStZAwaqX0plP0tAIU 5z3PS+NfJtpphVdWd3+HbFa6NPOjBTef7oB0mLT9iIFbtnxgThJtxX7+0a/HJSZg/cSv ruNExtDPhFKub0Iqv7cEDcjvHq+epc4EaHE6KORCLkoJJxGa8NcDoaLZxlDFnnIjAo7j 55AQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=UVw7JpRuL3Zpasl1yKpDjtfcQv9s3AR150vJWnlV/mo=; b=rKaICKNyI+H6MDJLGnls/NBhfWKOId6vQeVM1iW+/PWJo9fHypg2XixdDCOMTjSBOo VOCfr8afDrqJiS1GBUhBwKu+nXWpUt80XT6eQLjE/D6WN3qEj7It+/7UmMUkSjx5PTr9 fMS+Jo2EhSN5Zxuj3U76lu+rL6EjsYas4bPZRCIx605pXUyIGFcS9Ib3x1uu4ob+FuIh DVMrESqyXDwBMj/OCnEHU1rOt1kQIlzmo3NLv2qQ4F9wb92qgQnL0hUHU+ihhBb3PC42 CdH3WARrN/5Q4ROzpkflH0jwcuZrD+i4yuvnaGIaNY6HSxmMCOFVBqKt0urCwGFPMgSL 1RQw== X-Gm-Message-State: AOAM530i7okk5bw5wJ+dhFZECULWFcA+Uu8V6cOPnZD9GPvl7iAYjVa/ DL0HN7eig3g64wBFb4qphv8= X-Google-Smtp-Source: ABdhPJwcPzEvKL6lcLx0z54E2uefZwIWW/fS4DODk3QnRyS4ZehmSIYnNsyH6uHTZl1tTAVariYdAQ== X-Received: by 2002:a63:8842:: with SMTP id l63mr10247192pgd.280.1635760552289; Mon, 01 Nov 2021 02:55:52 -0700 (PDT) Received: from localhost.localdomain ([94.177.118.117]) by smtp.gmail.com with ESMTPSA id u11sm14727719pfk.151.2021.11.01.02.55.49 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 01 Nov 2021 02:55:51 -0700 (PDT) From: Dongliang Mu <mudongliangabcd@gmail.com> To: Mauro Carvalho Chehab <mchehab@kernel.org> Cc: Dongliang Mu <mudongliangabcd@gmail.com>, Pavel Skripkin <paskripkin@gmail.com>, syzbot <syzkaller@googlegroups.com>, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH] media: em28xx: fix memory leak in em28xx_init_dev Date: Mon, 1 Nov 2021 17:55:39 +0800 Message-Id: <20211101095539.423246-1-mudongliangabcd@gmail.com> X-Mailer: git-send-email 2.25.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -2.5 (--) X-LSpam-Report: No, score=-2.5 required=5.0 tests=BAYES_00=-1.9,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,FREEMAIL_FORGED_FROMDOMAIN=0.001,FREEMAIL_FROM=0.001,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1,RCVD_IN_DNSWL_NONE=-0.0001 autolearn=ham autolearn_force=no |
Series |
media: em28xx: fix memory leak in em28xx_init_dev
|
|
Commit Message
Dongliang Mu
Nov. 1, 2021, 9:55 a.m. UTC
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(-)
Comments
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)