lkml.org 
[lkml]   [2021]   [Jan]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: "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"
    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))
    > 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.

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

    thanks,

    greg k-h

    \
     
     \ /
      Last update: 2021-01-13 08:53    [W:2.575 / U:0.460 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site