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: <CAD-N9QW-zm37f9PW-iF-NaAH5LLePWFba3aG5LkXD2a07YBZpg@mail.gmail.com>
 <X/6mhQfPRt0QoorO@kroah.com>
In-Reply-To: <X/6mhQfPRt0QoorO@kroah.com>
From:   =?UTF-8?B?5oWV5Yas5Lqu?= <mudongliangabcd@gmail.com>
Date:   Wed, 13 Jan 2021 19:20:38 +0800
Message-ID: <CAD-N9QUuvxa3CCqru08O2x9p-AJp54qo-e-9O49YGQwQEWKLdA@mail.gmail.com>
Subject: Re: "UBSAN: shift-out-of-bounds in mceusb_dev_recv" should share the
 same root cause with "UBSAN: shift-out-of-bounds in mceusb_dev_printdata"
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     linux-kernel <linux-kernel@vger.kernel.org>,
        linux-media@vger.kernel.org, mchehab@kernel.org, sean@mess.org,
        anant.thazhemadam@gmail.com,
        syzkaller-bugs <syzkaller-bugs@googlegroups.com>,
        Dmitry Vyukov <dvyukov@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Jan 13, 2021 at 3:51 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jan 13, 2021 at 01:04:44PM +0800, =E6=85=95=E5=86=AC=E4=BA=AE wro=
te:
> > Hi developers,
> >
> > I found that "UBSAN: shift-out-of-bounds in mceusb_dev_recv" and
> > "UBSAN: shift-out-of-bounds in mceusb_dev_printdata" should share the
> > same root cause.
> > The reason is that the PoCs after minimization has a high similarity
> > with the other. And their stack trace only diverges at the last
> > function call. The following is some analysis for this bug.
> >
> > The following code in the mceusb_process_ir_data is the vulnerable
> > -----------------------------------------------------------------------=
---------------------------------------------------
> > for (; i < buf_len; i++) {
> >      switch (ir->parser_state) {
> >      case SUBCMD:
> >              ir->rem =3D mceusb_cmd_datasize(ir->cmd, ir->buf_in[i]);
> >              mceusb_dev_printdata(ir, ir->buf_in, buf_len, i - 1,
> >                                                    ir->rem + 2, false);
> >              if (i + ir->rem < buf_len)
> >              mceusb_handle_command(ir, &ir->buf_in[i - 1]);
> > -----------------------------------------------------------------------=
---------------------------------------------------
> >
> > The first report crashes at a shift operation(1<<*hi) in mceusb_handle_=
command.
> > -----------------------------------------------------------------------=
---------------------------------------------------
> > static void mceusb_handle_command(struct mceusb_dev *ir, u8 *buf_in)
> > {
> > u8 *hi =3D &buf_in[2]; /* read only when required */
> > if (cmd =3D=3D MCE_CMD_PORT_SYS) {
> >       switch (subcmd) {
> >       case MCE_RSP_GETPORTSTATUS:
> >               if (buf_in[5] =3D=3D 0)
> >                      ir->txports_cabled |=3D 1 << *hi;
> > -----------------------------------------------------------------------=
---------------------------------------------------
> >
> > The second report crashes at another shift operation (1U << data[0])
> > in mceusb_dev_printdata.
> > -----------------------------------------------------------------------=
---------------------------------------------------
> > static void mceusb_dev_printdata(struct mceusb_dev *ir, u8 *buf, int bu=
f_len,
> > int offset, int len, bool out)
> > {
> > data   =3D &buf[offset] + 2;
> >
> >           period =3D DIV_ROUND_CLOSEST((1U << data[0] * 2) *
> >                         (data[1] + 1), 10);
> > -----------------------------------------------------------------------=
---------------------------------------------------
> >
> > >From the analysis, we can know the data[0] and *hi access the same
> > memory cell - ``ir->buf_in[i+1]``. So the root cause should be that it
> > misses the check of ir->buf_in[i+1].
> >
> > For the patch of this bug, there is one from anant.thazhemadam@gmail.co=
m:
> > -----------------------------------------------------------------------=
---------------------------------------------------
> > diff --git a/drivers/media/rc/mceusb.c b/drivers/media/rc/mceusb.c
> > index f1dbd059ed08..79de721b1c4a 100644
> > --- a/drivers/media/rc/mceusb.c
> > +++ b/drivers/media/rc/mceusb.c
> > @@ -1169,7 +1169,7 @@ static void mceusb_handle_command(struct
> > mceusb_dev *ir, u8 *buf_in)
> >   switch (subcmd) {
> >   /* the one and only 5-byte return value command */
> >   case MCE_RSP_GETPORTSTATUS:
> > - if (buf_in[5] =3D=3D 0)
> > + if ((buf_in[5] =3D=3D 0) && (*hi <=3D 32))
> >   ir->txports_cabled |=3D 1 << *hi;
> >   break;
> > -----------------------------------------------------------------------=
---------------------------------------------------
> > I tried this patch in the second crash report and found it does not
> > work. I think we should add another filter for the value in
> > ``ir->buf_in[i+1]``.
> >
> > With this grouping, I think developers can take into consideration the
> > issue in mceusb_dev_printdata and generate a complete patch for this
> > bug.
>
> Why not create a patch yourself and submit it?  That way you get the
> correct credit for solving the problem.
>

I have sent a simple but working patch to the corresponding
developers. We can take it as a base to discuss.

And this email is to provide some information about bug duplication
for developers as I am doing some research on crash deduplication. I
want to get some credits if our grouping information is useful for
some kernel developers.

> thanks,
>
> greg k-h