xorl %eax, %eax

CVE-2010-4527: Linux kernel OSS Sound Card Driver Buffer Overflow

leave a comment »

This issue was reported by Dan Rosenberg and it affects systems using the OSS sound card driver for Linux. The vulnerable code resides in load_mixer_volumes() routine which is part of the sound/oss/soundcard.c file. Specifically, here is the susceptible code…

/*
 * Table for configurable mixer volume handling
 */
static mixer_vol_table mixer_vols[MAX_MIXER_DEV];
static int num_mixer_volumes;

int *load_mixer_volumes(char *name, int *levels, int present)
{
        int             i, n;

        for (i = 0; i < num_mixer_volumes; i++) {
                if (strcmp(name, mixer_vols[i].name) == 0) {
     ...
        strcpy(mixer_vols[n].name, name);
     ...
}
EXPORT_SYMBOL(load_mixer_volumes);

The initial ‘for’ loop iterates through the ‘mixer_vols[]’ array and uses strcmp(3) to find the requested name. However, there is no check that the provided name is 32 or less Bytes long. This could result in a read out of bounds condition since the name member is statically allocated as we can see at include/linux/soundcard.h header file where that structure is defined.

typedef struct mixer_vol_table {
  int num;      /* Index to volume table */
  char name[32];
  int levels[32];
} mixer_vol_table;

Of course, the subsequent call to strcpy(3) lacks of the exact same length checks leading to a possible kernel stack buffer overflow if a user provides a name larger than 32 Bytes long. The fix for the first issue was to use the strncmp(3) routine that performs the same task having a maximum acceptable size.

 	for (i = 0; i < num_mixer_volumes; i++) {
-		if (strcmp(name, mixer_vols[i].name) == 0) {
+		if (strncmp(name, mixer_vols[i].name, 32) == 0) {
 			if (present)

And similarly, strcpy(3) was replaced with the strncpy(3) one…

 	n = num_mixer_volumes++;
 
-	strcpy(mixer_vols[n].name, name);
+	strncpy(mixer_vols[n].name, name, 32);
 
 	if (present)

As Dan Rosenberg pointed out, the vulnerable routine is reachable through the following IOCTL command…

static long sound_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
        int len = 0, dtype;
     ...
        switch (dev & 0x0f) {
     ...
                else if (cmd == SOUND_MIXER_SETLEVELS)
                        ret = set_mixer_levels(p);
     ...
        unlock_kernel();
        return ret;
}

Where set_mixer_levels() is nothing more than a wrapper around load_mixer_volumes() as we can see here:

static int set_mixer_levels(void __user * arg)
{
        /* mixer_vol_table is 174 bytes, so IMHO no reason to not allocate it on the stack */
        mixer_vol_table buf;   

        if (__copy_from_user(&buf, arg, sizeof(buf)))
                return -EFAULT;
        load_mixer_volumes(buf.name, buf.levels, 0);
        if (__copy_to_user(arg, &buf, sizeof(buf)))
                return -EFAULT;
        return 0;
}
About these ads

Written by xorl

January 9, 2011 at 21:19

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

Follow

Get every new post delivered to your Inbox.

Join 60 other followers