xorl %eax, %eax

Linux kernel newport Driver Read Out of Bounds

leave a comment »

This is really nice bug I just saw in LKML by Roel Kluin. The buggy kernel module is located at drivers/video/console/newport_con.c and here is the equivalent function as seen in 2.6.30 release of the Linux kernel.

/*
 * calculate the actual screen size by reading
 * the video timing out of the VC2
 */
static void newport_get_screensize(void)
{
        int i, cols;
        unsigned short ventry, treg;
        unsigned short linetable[128];  /* should be enough */

        ventry = newport_vc2_get(npregs, VC2_IREG_VENTRY);
        newport_vc2_set(npregs, VC2_IREG_RADDR, ventry);
        npregs->set.dcbmode = (NPORT_DMODE_AVC2 | VC2_REGADDR_RAM |
                               NPORT_DMODE_W2 | VC2_PROTOCOL);
        for (i = 0; i < 128; i++) {
                newport_bfwait(npregs);
                linetable&#91;i&#93; = npregs->set.dcbdata0.byshort.s1;
        }

        newport_xsize = newport_ysize = 0;
        for (i = 0; linetable[i + 1] && (i < sizeof(linetable)); i += 2) {
                cols = 0;
                newport_vc2_set(npregs, VC2_IREG_RADDR, linetable&#91;i&#93;);
                npregs->set.dcbmode = (NPORT_DMODE_AVC2 | VC2_REGADDR_RAM |
                                       NPORT_DMODE_W2 | VC2_PROTOCOL);
                do {
                        newport_bfwait(npregs);
                        treg = npregs->set.dcbdata0.byshort.s1;
                        if ((treg & 1) == 0)
                                cols += (treg >> 7) & 0xfe;
                        if ((treg & 0x80) == 0) {
                                newport_bfwait(npregs);
                                treg = npregs->set.dcbdata0.byshort.s1;
                        }
                } while ((treg & 0x8000) == 0);
                if (cols) {
                        if (cols > newport_xsize)
                                newport_xsize = cols;
                        newport_ysize += linetable[i + 1];
                }
        }
        printk("NG1: Screensize %dx%d\n", newport_xsize, newport_ysize);
}

As you can see, they use a stack allocated buffer (linetable[]) of short integers which is initialized in the for loop with the contents of npregs->set.dcbdata0.byshort.s1 which is updated in each iteration using newport_bfwait(). Then, X and Y size are both initialized to 0 and another for loop takes place. This will iterate as long as linetable[i+1] is not NULL and ‘i’ is less than the sizeof linetable[] array. However, sizeof operator returns the size of bytes, not the number of slots of this array. Consequently, since this is short (meaning 16-bit / 2 Byte long), sizeof will return 256 instead of the expected 128. This will lead to read out of buffer as long as linetable[i+1] is not NULL for maximum of 128 additional iterations outside buffer’s boundaries. The subsequent calls to a few routines using those unitialized/random data seems interesting from a security point of view, of course, as long as the user can trigger this at will. I have not tested any of my assumptions. Nevertheless, it is an interesting bug.
The fix was to use ARRAY_SIZE() macro from include/linux/kernel.h which is a simple array caclulation macro.

#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))

And the patch is this:

newport_xsize = newport_ysize = 0;
– for (i = 0; linetable[i + 1] && (i < sizeof(linetable)); i += 2) { + for (i = 0; i < ARRAY_SIZE(linetable) - 1 && linetable[i + 1]; i += 2) { cols = 0; [/sourcecode] Even if this turns out to be a non-security related bug, it is definitely a nice bug.

Written by xorl

July 29, 2009 at 22:27

Posted in bugs, linux

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s