* [PATCH] media: dvd_usb: memory leak in cinergyt2_fe_attach
@ 2021-05-25 5:33 Dongliang Mu
2021-05-25 6:19 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 3+ messages in thread
From: Dongliang Mu @ 2021-05-25 5:33 UTC (permalink / raw)
To: mchehab
Cc: linux-media, linux-kernel, Dongliang Mu, syzbot+e1de8986786b3722050e
When cinergyt2_frontend_attach returns a negative value, the allocation
is already successful, but in the error handling, there is no any clean
corresponding operation, which leads to memory leak.
Fix it by freeing struct cinergyt2_fe_state when the return value is
nonzero.
backtrace:
[<0000000056e17b1a>] kmalloc include/linux/slab.h:552 [inline]
[<0000000056e17b1a>] kzalloc include/linux/slab.h:682 [inline]
[<0000000056e17b1a>] cinergyt2_fe_attach+0x21/0x80 drivers/media/usb/dvb-usb/cinergyT2-fe.c:271
[<00000000ae0b1711>] cinergyt2_frontend_attach+0x21/0x70 drivers/media/usb/dvb-usb/cinergyT2-core.c:74
[<00000000d0254861>] dvb_usb_adapter_frontend_init+0x11b/0x1b0 drivers/media/usb/dvb-usb/dvb-usb-dvb.c:290
[<0000000002e08ac6>] dvb_usb_adapter_init drivers/media/usb/dvb-usb/dvb-usb-init.c:84 [inline]
[<0000000002e08ac6>] dvb_usb_init drivers/media/usb/dvb-usb/dvb-usb-init.c:173 [inline]
[<0000000002e08ac6>] dvb_usb_device_init.cold+0x4d0/0x6ae drivers/media/usb/dvb-usb/dvb-usb-init.c:287
Reported-by: syzbot+e1de8986786b3722050e@syzkaller.appspotmail.com
Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
---
drivers/media/usb/dvb-usb/dvb-usb-dvb.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/media/usb/dvb-usb/dvb-usb-dvb.c b/drivers/media/usb/dvb-usb/dvb-usb-dvb.c
index 0a7f8ba90992..f9f004fb0a92 100644
--- a/drivers/media/usb/dvb-usb/dvb-usb-dvb.c
+++ b/drivers/media/usb/dvb-usb/dvb-usb-dvb.c
@@ -288,7 +288,7 @@ int dvb_usb_adapter_frontend_init(struct dvb_usb_adapter *adap)
}
ret = adap->props.fe[i].frontend_attach(adap);
- if (ret || adap->fe_adap[i].fe == NULL) {
+ if (adap->fe_adap[i].fe == NULL) {
/* only print error when there is no FE at all */
if (i == 0)
err("no frontend was attached by '%s'",
@@ -297,6 +297,12 @@ int dvb_usb_adapter_frontend_init(struct dvb_usb_adapter *adap)
return 0;
}
+ if (ret) {
+ struct dvb_frontend *fe = adap->fe_adap[i].fe;
+
+ fe->ops.release(fe);
+ return 0;
+ }
+
adap->fe_adap[i].fe->id = i;
/* re-assign sleep and wakeup functions */
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] media: dvd_usb: memory leak in cinergyt2_fe_attach
2021-05-25 5:33 [PATCH] media: dvd_usb: memory leak in cinergyt2_fe_attach Dongliang Mu
@ 2021-05-25 6:19 ` Mauro Carvalho Chehab
2021-05-25 6:46 ` 慕冬亮
0 siblings, 1 reply; 3+ messages in thread
From: Mauro Carvalho Chehab @ 2021-05-25 6:19 UTC (permalink / raw)
To: Dongliang Mu; +Cc: linux-media, linux-kernel, syzbot+e1de8986786b3722050e
Em Tue, 25 May 2021 13:33:59 +0800
Dongliang Mu <mudongliangabcd@gmail.com> escreveu:
> When cinergyt2_frontend_attach returns a negative value, the allocation
> is already successful, but in the error handling, there is no any clean
> corresponding operation, which leads to memory leak.
>
> Fix it by freeing struct cinergyt2_fe_state when the return value is
> nonzero.
>
> backtrace:
> [<0000000056e17b1a>] kmalloc include/linux/slab.h:552 [inline]
> [<0000000056e17b1a>] kzalloc include/linux/slab.h:682 [inline]
> [<0000000056e17b1a>] cinergyt2_fe_attach+0x21/0x80 drivers/media/usb/dvb-usb/cinergyT2-fe.c:271
> [<00000000ae0b1711>] cinergyt2_frontend_attach+0x21/0x70 drivers/media/usb/dvb-usb/cinergyT2-core.c:74
> [<00000000d0254861>] dvb_usb_adapter_frontend_init+0x11b/0x1b0 drivers/media/usb/dvb-usb/dvb-usb-dvb.c:290
> [<0000000002e08ac6>] dvb_usb_adapter_init drivers/media/usb/dvb-usb/dvb-usb-init.c:84 [inline]
> [<0000000002e08ac6>] dvb_usb_init drivers/media/usb/dvb-usb/dvb-usb-init.c:173 [inline]
> [<0000000002e08ac6>] dvb_usb_device_init.cold+0x4d0/0x6ae drivers/media/usb/dvb-usb/dvb-usb-init.c:287
>
> Reported-by: syzbot+e1de8986786b3722050e@syzkaller.appspotmail.com
> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> ---
> drivers/media/usb/dvb-usb/dvb-usb-dvb.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/usb/dvb-usb/dvb-usb-dvb.c b/drivers/media/usb/dvb-usb/dvb-usb-dvb.c
> index 0a7f8ba90992..f9f004fb0a92 100644
> --- a/drivers/media/usb/dvb-usb/dvb-usb-dvb.c
> +++ b/drivers/media/usb/dvb-usb/dvb-usb-dvb.c
> @@ -288,7 +288,7 @@ int dvb_usb_adapter_frontend_init(struct dvb_usb_adapter *adap)
> }
>
> ret = adap->props.fe[i].frontend_attach(adap);
> - if (ret || adap->fe_adap[i].fe == NULL) {
> + if (adap->fe_adap[i].fe == NULL) {
> /* only print error when there is no FE at all */
> if (i == 0)
> err("no frontend was attached by '%s'",
> @@ -297,6 +297,12 @@ int dvb_usb_adapter_frontend_init(struct dvb_usb_adapter *adap)
> return 0;
> }
>
> + if (ret) {
> + struct dvb_frontend *fe = adap->fe_adap[i].fe;
> +
> + fe->ops.release(fe);
> + return 0;
> + }
> +
Touching dvb-usb core doesn't seem the right fix here, as it will
affect all other drivers that depend on it.
Basically, when a driver returns an error, it has to cleanup
whatever it did, as the core has no way to know where the
error happened inside the driver logic.
The problem seems to be at cinergyt2_frontend_attach() instead:
adap->fe_adap[0].fe = cinergyt2_fe_attach(adap->dev);
mutex_lock(&d->data_mutex);
st->data[0] = CINERGYT2_EP1_GET_FIRMWARE_VERSION;
ret = dvb_usb_generic_rw(d, st->data, 1, st->data, 3, 0);
if (ret < 0) {
deb_rc("cinergyt2_power_ctrl() Failed to retrieve sleep state info\n");
}
mutex_unlock(&d->data_mutex);
/* Copy this pointer as we are gonna need it in the release phase */
cinergyt2_usb_device = adap->dev;
return ret;
See, this driver returns an error if it fails to talk with the hardware
when it calls dvb_usb_generic_rw(). Yet, it doesn't cleanup its own mess,
as it keeps the frontend attached. The right fix would be to call
cinergyt2_fe_release() if ret < 0.
E. g., the above code should be, instead:
if (ret < 0) {
fe->ops.release(adap->fe_adap[0].fe);
deb_rc("cinergyt2_power_ctrl() Failed to retrieve sleep state info\n");
}
Thanks,
Mauro
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] media: dvd_usb: memory leak in cinergyt2_fe_attach
2021-05-25 6:19 ` Mauro Carvalho Chehab
@ 2021-05-25 6:46 ` 慕冬亮
0 siblings, 0 replies; 3+ messages in thread
From: 慕冬亮 @ 2021-05-25 6:46 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: linux-media, linux-kernel, syzbot
On Tue, May 25, 2021 at 2:20 PM Mauro Carvalho Chehab
<mchehab@kernel.org> wrote:
>
> Em Tue, 25 May 2021 13:33:59 +0800
> Dongliang Mu <mudongliangabcd@gmail.com> escreveu:
>
> > When cinergyt2_frontend_attach returns a negative value, the allocation
> > is already successful, but in the error handling, there is no any clean
> > corresponding operation, which leads to memory leak.
> >
> > Fix it by freeing struct cinergyt2_fe_state when the return value is
> > nonzero.
> >
> > backtrace:
> > [<0000000056e17b1a>] kmalloc include/linux/slab.h:552 [inline]
> > [<0000000056e17b1a>] kzalloc include/linux/slab.h:682 [inline]
> > [<0000000056e17b1a>] cinergyt2_fe_attach+0x21/0x80 drivers/media/usb/dvb-usb/cinergyT2-fe.c:271
> > [<00000000ae0b1711>] cinergyt2_frontend_attach+0x21/0x70 drivers/media/usb/dvb-usb/cinergyT2-core.c:74
> > [<00000000d0254861>] dvb_usb_adapter_frontend_init+0x11b/0x1b0 drivers/media/usb/dvb-usb/dvb-usb-dvb.c:290
> > [<0000000002e08ac6>] dvb_usb_adapter_init drivers/media/usb/dvb-usb/dvb-usb-init.c:84 [inline]
> > [<0000000002e08ac6>] dvb_usb_init drivers/media/usb/dvb-usb/dvb-usb-init.c:173 [inline]
> > [<0000000002e08ac6>] dvb_usb_device_init.cold+0x4d0/0x6ae drivers/media/usb/dvb-usb/dvb-usb-init.c:287
> >
> > Reported-by: syzbot+e1de8986786b3722050e@syzkaller.appspotmail.com
> > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > ---
> > drivers/media/usb/dvb-usb/dvb-usb-dvb.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/usb/dvb-usb/dvb-usb-dvb.c b/drivers/media/usb/dvb-usb/dvb-usb-dvb.c
> > index 0a7f8ba90992..f9f004fb0a92 100644
> > --- a/drivers/media/usb/dvb-usb/dvb-usb-dvb.c
> > +++ b/drivers/media/usb/dvb-usb/dvb-usb-dvb.c
> > @@ -288,7 +288,7 @@ int dvb_usb_adapter_frontend_init(struct dvb_usb_adapter *adap)
> > }
> >
> > ret = adap->props.fe[i].frontend_attach(adap);
> > - if (ret || adap->fe_adap[i].fe == NULL) {
> > + if (adap->fe_adap[i].fe == NULL) {
> > /* only print error when there is no FE at all */
> > if (i == 0)
> > err("no frontend was attached by '%s'",
> > @@ -297,6 +297,12 @@ int dvb_usb_adapter_frontend_init(struct dvb_usb_adapter *adap)
> > return 0;
> > }
> >
> > + if (ret) {
> > + struct dvb_frontend *fe = adap->fe_adap[i].fe;
> > +
> > + fe->ops.release(fe);
> > + return 0;
> > + }
> > +
>
> Touching dvb-usb core doesn't seem the right fix here, as it will
> affect all other drivers that depend on it.
>
> Basically, when a driver returns an error, it has to cleanup
> whatever it did, as the core has no way to know where the
> error happened inside the driver logic.
>
> The problem seems to be at cinergyt2_frontend_attach() instead:
>
> adap->fe_adap[0].fe = cinergyt2_fe_attach(adap->dev);
>
> mutex_lock(&d->data_mutex);
> st->data[0] = CINERGYT2_EP1_GET_FIRMWARE_VERSION;
>
> ret = dvb_usb_generic_rw(d, st->data, 1, st->data, 3, 0);
> if (ret < 0) {
> deb_rc("cinergyt2_power_ctrl() Failed to retrieve sleep state info\n");
> }
> mutex_unlock(&d->data_mutex);
>
> /* Copy this pointer as we are gonna need it in the release phase */
> cinergyt2_usb_device = adap->dev;
>
> return ret;
>
> See, this driver returns an error if it fails to talk with the hardware
> when it calls dvb_usb_generic_rw(). Yet, it doesn't cleanup its own mess,
> as it keeps the frontend attached. The right fix would be to call
> cinergyt2_fe_release() if ret < 0.
>
> E. g., the above code should be, instead:
>
> if (ret < 0) {
> fe->ops.release(adap->fe_adap[0].fe);
> deb_rc("cinergyt2_power_ctrl() Failed to retrieve sleep state info\n");
> }
You're right. This is a good idea to handle the error inside the logic
of device driver.
I will test this proposed patch and send patch v2.
BTW, Mauro, did you see another mail thread [1] I sent? I doubt there
is an error between dvb_usb_adapter_frontend_init and
cinergyt2_frontend_attach
[1] https://www.spinics.net/lists/linux-media/msg193227.html
>
> Thanks,
> Mauro
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-05-25 6:46 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-25 5:33 [PATCH] media: dvd_usb: memory leak in cinergyt2_fe_attach Dongliang Mu
2021-05-25 6:19 ` Mauro Carvalho Chehab
2021-05-25 6:46 ` 慕冬亮
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).