On Tue, May 25, 2021 at 2:20 PM Mauro Carvalho Chehab
<mchehab@xxxxxxxxxx> wrote:
>
> Em Tue, 25 May 2021 13:33:59 +0800
> Dongliang Mu <mudongliangabcd@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Signed-off-by: Dongliang Mu <mudongliangabcd@xxxxxxxxx>
> > ---
> > 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
