CVE-2009-3080: Linux kernel GDT ISA/EISA/PCI Disk Array Controllers Signedness Issue

This bug was discovered and reported by Dave Jones of Red Hat. The susceptible code resides in drivers/scsi/gdth.c which has the device driver code for ICP vortex GmbH. GDT ISA/EISA/PCI disk array controllers and Intel storage RAID controllers. Here is the buggy routine as seen in 2.6.31 release of the Linux kernel.

static int gdth_read_event(gdth_ha_str *ha, int handle, gdth_evt_str *estr)
{
    gdth_evt_str *e;
    int eindex;
    ulong flags;

    TRACE2(("gdth_read_event() handle %d\n", handle));
    spin_lock_irqsave(&ha->smp_lock, flags);
    if (handle == -1)
        eindex = eoldidx;
    else
        eindex = handle;
    estr->event_source = 0;

    if (eindex >= MAX_EVENTS) {
        spin_unlock_irqrestore(&ha->smp_lock, flags);
        return eindex;
    }
    e = &ebuffer[eindex];
    if (e->event_source != 0) {
        if (eindex != elastidx) {
            if (++eindex == MAX_EVENTS)
                eindex = 0;
        } else {
            eindex = -1;
        }
        memcpy(estr, e, sizeof(gdth_evt_str));
    }
    spin_unlock_irqrestore(&ha->smp_lock, flags);
    return eindex;
}

As it is implied by its name, this function is used to read the event passed through its third argument that is a pointer to a ‘gdth_evt_str’ which as you can read in drivers/scsi/gdth_ioctl.h is a type definition for a structure that describes the event.

typedef struct {
    ulong32         first_stamp;
    ulong32         last_stamp;
    ushort          same_count;
    ushort          event_source;
    ushort          event_idx;
    unchar          application;
    unchar          reserved;
    gdth_evt_data   event_data;
} PACKED gdth_evt_str;

Back to gdth_read_event() we can read that ‘eindex’ is initialized with the old index value if the handle passed to that routine by its first argument is -1, otherwise it will be initialized with handle’s value. The next check against ‘MAX_EVENTS’, which is the maximum buffer events and it is declared as 100 in drivers/scsi/gdth.h, can be bypassed since ‘eindex’ is a singed integer and it is perfectably legal to contain negative values which are obviously less than 100 and can also be something else instead of -1.
The subsequent operations that use that value as an index to events’ buffer ‘ebuffer[]’ as well as the final memcpy() from the possibly invalid ‘e’ pointer can result in a memory corruption situation.
This vulnerability was fixed using the patch below.

     estr->event_source = 0;
 
-    if (eindex >= MAX_EVENTS) {
+    if (eindex < 0 || eindex >= MAX_EVENTS) {
         spin_unlock_irqrestore(&ha->smp_lock, flags);
         return eindex;

In order to check for negative values too.
So, before I end this post have a look at the following function which you’ll probably find useful…

static int ioc_event(void __user *arg)
{
    gdth_ioctl_event evt;
    gdth_ha_str *ha;
    ulong flags;

    if (copy_from_user(&evt, arg, sizeof(gdth_ioctl_event)))
  ...
    if (evt.erase == 0xff) {
  ...
    } else if (evt.erase == 0) {
        evt.handle = gdth_read_event(ha, evt.handle, &evt.event);
  ...
    if (copy_to_user(arg, &evt, sizeof(gdth_ioctl_event)))
        return -EFAULT;
    return 0;
}