"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.th...@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))
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

[Greg KH]

Why not create a patch yourself and submit it? That way you get the
correct credit for solving the problem.

thanks,

greg k-h

[Dan Carpenter]

On Wed, Jan 13, 2021 at 01:04:44PM +0800, 慕冬亮 wrote:

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

[慕冬亮]

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

[慕冬亮]

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.


[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

[Sean Young]

HI,

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.

This mceusb_dev_printdata() function has been very troublesome, maybe it
could be written in a different way.

Thanks,

Sean

[慕冬亮]

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.

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 let me know if you have any questions about the above text.
Thanks in advance.

[Sean Young]

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