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"

[Sean Young <sean@mess.org>]

On Mon, May 03, 2021 at 07:24:35PM +0800, 慕冬亮 wrote:
> On Mon, May 3, 2021 at 5:28 PM Sean Young <sean@mess.org> wrote:
> >
> > HI,
> >
> > On Sun, May 02, 2021 at 10:29:25PM +0800, 慕冬亮 wrote:
> > > Hi kernel developers,
> > >
> > > I found one interesting follow-up for these two crash reports. In the
> > > syzbot dashboard, they are fixed with different patches. Each patch
> > > fixes at the failure point - mceusb_handle_command  and
> > > mceusb_dev_printdata. For patch details, please have a look at the
> > > crash reports [1] and [2].
> > >
> > > Recall the vulnerability below, and kernel crashes both at the case
> > > SUBCMD with incorrect value in ir_buf_in[i+2]. I still think they
> > > share the same root cause and fixing this bug needs two patches at the
> > > same time.
> > >
> > > --------------------------------------------------------------------------------------------------------------------------
> > > for (; i < buf_len; i++) {
> > >      switch (ir->parser_state) {
> > >      case SUBCMD:
> > >              ir->rem = 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]);
> > > --------------------------------------------------------------------------------------------------------------------------
> > >
> > > I wonder if developers can see two crash reports in the very
> > > beginning, they may craft different patches which fix this bug in the
> > > root cause. Meanwhile, if developers can see those crash reports in
> > > advance, this may save some time for developers since only one takes
> > > time to analyze this bug. If you have any issues about this statement,
> > > please let me know.
> >
> > I am sorry, I have a hard time following. As far as I am aware, the issue
> > with mceusb_dev_printdata() have been resolved. If you think there is still
> > is an issue, please do send a patch and then we can discuss it. As far as I
> > know there is noone else working on this.
> 
> Hi Sean,
> 
> Sorry for the bad logic. Let me organize my logic about these two
> crashes and the underlying bug.
> 
> First, let's sync on the same page. In this thread, I would like to
> prove to you guys these two crash reports share the same root cause -
> they both miss the sanity check of the same field from user space.

So you mean:
[1] UBSAN: shift-out-of-bounds in mceusb_dev_printdata
https://syzkaller.appspot.com/bug?id=df1efbbf75149f5853ecff1938ffd3134f269119
[2] UBSAN: shift-out-of-bounds in mceusb_dev_recv
https://syzkaller.appspot.com/bug?id=50d4123e6132c9563297ecad0479eaad7480c172

1) So these bugs are not crashes -- shift out of bounds is the error.
2) The "bug" is that garbage will be printed to the kernel log when
   garbage data is received. I'm not sure it is a bug.
2) The data comes from the usb device, not user space
3) They are both fixed
4) They are in different parts of the code

> Second, if you agree with the first point, let's move on. If we can
> know the duplication information before, you and James Reynolds, who
> fixes another crash at mceusb_handle_command do not need to analyze it
> twice. And I think either your patch or the patch developed by James
> Reynolds only fixes the crash reports at the failure point, without
> completely fixing the underlying bug.

Please send a patch which shows this is the case.

> Please let me know if you have any questions about the above text.
> Thanks in advance.

Thanks,

Sean