linux-kernel - "UBSAN: shift-out-of-bounds in mceusb_dev_recv" should share the same root cause with "UBSAN: shift-out-of-bounds in mceusb_dev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAD-N9QW-zm37f9PW-iF-NaAH5LLePWFba3aG5LkXD2a07YBZpg@mail.gmail.com>
Date:   Wed, 13 Jan 2021 13:04:44 +0800
From:   慕冬亮 <mudongliangabcd@...il.com>
To:     linux-kernel <linux-kernel@...r.kernel.org>,
        linux-media@...r.kernel.org, mchehab@...nel.org, sean@...s.org,
        anant.thazhemadam@...il.com,
        syzkaller-bugs <syzkaller-bugs@...glegroups.com>,
        Greg KH <greg@...ah.com>, Dmitry Vyukov <dvyukov@...gle.com>
Subject: "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"

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@...il.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))
  ir->txports_cabled |= 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.

If any of my understanding is incorrect or has issues, please let me
know. Thanks very much.

--
My best regards to you.

     No System Is Safe!
     Dongliang Mu