Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230714074011.20989-1-qiang.zhang1211@gmail.com> <CA+fCnZfxLLLVJdPZmVKg5w4YY+i09ZRz-wu+TAXBaroRgGsAsQ@mail.gmail.com>
In-Reply-To: <CA+fCnZfxLLLVJdPZmVKg5w4YY+i09ZRz-wu+TAXBaroRgGsAsQ@mail.gmail.com>
From:   Z qiang <qiang.zhang1211@gmail.com>
Date:   Tue, 18 Jul 2023 10:17:28 +0800
Message-ID: <CALm+0cUUm6g+TWCso_UOFWJYGBZDHaDY29r1eMN44HBAuKUFVQ@mail.gmail.com>
Subject: Re: [PATCH] USB: gadget: Fix the memory leak in raw_gadget driver
To:     Andrey Konovalov <andreyknvl@gmail.com>
Cc:     gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

>
> On Fri, Jul 14, 2023 at 9:40=E2=80=AFAM Zqiang <qiang.zhang1211@gmail.com=
> wrote:
> >
> > Currently, increasing raw_dev->count happens before invoke the
> > raw_queue_event(), if the raw_queue_event() return error, invoke
> > raw_release() will not trigger the dev_free() to be called.
> >
> > [  268.905865][ T5067] raw-gadget.0 gadget.0: failed to queue event
> > [  268.912053][ T5067] udc dummy_udc.0: failed to start USB Raw Gadget:=
 -12
> > [  268.918885][ T5067] raw-gadget.0: probe of gadget.0 failed with erro=
r -12
> > [  268.925956][ T5067] UDC core: USB Raw Gadget: couldn't find an avail=
able UDC or it's busy
> > [  268.934657][ T5067] misc raw-gadget: fail, usb_gadget_register_drive=
r returned -16
> >
> > BUG: memory leak
> >
> > [<ffffffff8154bf94>] kmalloc_trace+0x24/0x90 mm/slab_common.c:1076
> > [<ffffffff8347eb55>] kmalloc include/linux/slab.h:582 [inline]
> > [<ffffffff8347eb55>] kzalloc include/linux/slab.h:703 [inline]
> > [<ffffffff8347eb55>] dev_new drivers/usb/gadget/legacy/raw_gadget.c:191=
 [inline]
> > [<ffffffff8347eb55>] raw_open+0x45/0x110 drivers/usb/gadget/legacy/raw_=
gadget.c:385
> > [<ffffffff827d1d09>] misc_open+0x1a9/0x1f0 drivers/char/misc.c:165
> >
> > [<ffffffff8154bf94>] kmalloc_trace+0x24/0x90 mm/slab_common.c:1076
> > [<ffffffff8347cd2f>] kmalloc include/linux/slab.h:582 [inline]
> > [<ffffffff8347cd2f>] raw_ioctl_init+0xdf/0x410 drivers/usb/gadget/legac=
y/raw_gadget.c:460
> > [<ffffffff8347dfe9>] raw_ioctl+0x5f9/0x1120 drivers/usb/gadget/legacy/r=
aw_gadget.c:1250
> > [<ffffffff81685173>] vfs_ioctl fs/ioctl.c:51 [inline]
> >
> > [<ffffffff8154bf94>] kmalloc_trace+0x24/0x90 mm/slab_common.c:1076
> > [<ffffffff833ecc6a>] kmalloc include/linux/slab.h:582 [inline]
> > [<ffffffff833ecc6a>] kzalloc include/linux/slab.h:703 [inline]
> > [<ffffffff833ecc6a>] dummy_alloc_request+0x5a/0xe0 drivers/usb/gadget/u=
dc/dummy_hcd.c:665
> > [<ffffffff833e9132>] usb_ep_alloc_request+0x22/0xd0 drivers/usb/gadget/=
udc/core.c:196
> > [<ffffffff8347f13d>] gadget_bind+0x6d/0x370 drivers/usb/gadget/legacy/r=
aw_gadget.c:292
> >
> > This commit therefore invoke kref_get() under the condition that
> > raw_queue_event() return success.
> >
> > Reported-by: syzbot+feb045d335c1fdde5bf7@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=3Dfeb045d335c1fdde5bf7
> > Signed-off-by: Zqiang <qiang.zhang1211@gmail.com>
> > ---
> >  drivers/usb/gadget/legacy/raw_gadget.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/legacy/raw_gadget.c b/drivers/usb/gadge=
t/legacy/raw_gadget.c
> > index 2acece16b890..e549022642e5 100644
> > --- a/drivers/usb/gadget/legacy/raw_gadget.c
> > +++ b/drivers/usb/gadget/legacy/raw_gadget.c
> > @@ -310,13 +310,15 @@ static int gadget_bind(struct usb_gadget *gadget,
> >         dev->eps_num =3D i;
> >         spin_unlock_irqrestore(&dev->lock, flags);
> >
> > -       /* Matches kref_put() in gadget_unbind(). */
> > -       kref_get(&dev->count);
> > -
> >         ret =3D raw_queue_event(dev, USB_RAW_EVENT_CONNECT, 0, NULL);
> > -       if (ret < 0)
> > +       if (ret < 0) {
> >                 dev_err(&gadget->dev, "failed to queue event\n");
> > +               set_gadget_data(gadget, NULL);
> > +               return ret;
> > +       }
> >
> > +       /* Matches kref_put() in gadget_unbind(). */
> > +       kref_get(&dev->count);
> >         return ret;
> >  }
> >
> > --
> > 2.17.1
>
> Indeed, if gadget_bind fails due to a raw_queue_event failure, the
> core gadget code will never call gadget_unbind.
>
>
> Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com>
> Tested-by: Andrey Konovalov <andreyknvl@gmail.com>

Thanks for review and test !


>
> Thanks!