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;
Date:   Wed, 13 Jan 2021 13:02:24 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     =?utf-8?B?5oWV5Yas5Lqu?= <mudongliangabcd@gmail.com>
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>,
        Greg KH <greg@kroah.com>, Dmitry Vyukov <dvyukov@google.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"
Message-ID: <20210113100224.GH5083@kadam>
References: <CAD-N9QW-zm37f9PW-iF-NaAH5LLePWFba3aG5LkXD2a07YBZpg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAD-N9QW-zm37f9PW-iF-NaAH5LLePWFba3aG5LkXD2a07YBZpg@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Wed, Jan 13, 2021 at 01:04:44PM +0800, 慕冬亮 wrote:
> 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 = 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 = &buf_in[2]; /* read only when required */
> if (cmd == MCE_CMD_PORT_SYS) {
>       switch (subcmd) {
>       case MCE_RSP_GETPORTSTATUS:
>               if (buf_in[5] == 0)
>                      ir->txports_cabled |= 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 buf_len,
> int offset, int len, bool out)
> {
> data   = &buf[offset] + 2;
> 
>           period = 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.com:
> --------------------------------------------------------------------------------------------------------------------------
> 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] == 0)
> + if ((buf_in[5] == 0) && (*hi <= 32))

This should be < instead of <=.  Shifting by 32 is undefined.  Also this
patch can't be applied at all so it's hard to review.  Read the two
paragraphs of Documentation/process/email-clients.rst

There are some other bugs:

	ir->num_txports = *hi;

If "ir->num_txports" is over 31 then it will lead to undefined behavior
in mceusb_set_tx_mask().  It not totally clear to me what the correct
limit is.  So search through the code a bit more I guess and try find
the remaining bugs and what the limits should be.

regards,
dan carpenter