Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210525053359.1147899-1-mudongliangabcd@gmail.com> <20210525081958.22f1e2b6@coco.lan>
In-Reply-To: <20210525081958.22f1e2b6@coco.lan>
From:   =?UTF-8?B?5oWV5Yas5Lqu?= <mudongliangabcd@gmail.com>
Date:   Tue, 25 May 2021 14:46:00 +0800
Message-ID: <CAD-N9QU+f1+CegprF-YOC85jrsOCTm1+W9c3cgebrG3J2psibg@mail.gmail.com>
Subject: Re: [PATCH] media: dvd_usb: memory leak in cinergyt2_fe_attach
To:     Mauro Carvalho Chehab <mchehab@kernel.org>
Cc:     linux-media@vger.kernel.org,
        linux-kernel <linux-kernel@vger.kernel.org>,
        syzbot <syzbot+e1de8986786b3722050e@syzkaller.appspotmail.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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