xorl %eax, %eax

CVE-2009-1763: Solaris Secure Digital slot driver Invalid Memory Access

with 4 comments

This vulnerability affects Solaris 8, 9 and 10 as well as OpenSolaris builds snv_105 through snv_108 according to Sun’s official advisory. The vulnerability was disclosed in 21 May 2009 and it is located at the Secure Digital Slot Driver Here is the vulnerable code:

int
sdhost_attach(dev_info_t *dip, ddi_attach_cmd_t cmd)
{
    sdhost_t        *shp;
    ddi_acc_handle_t    pcih;
    uint8_t            slotinfo;
    uint8_t            bar;
    int            i;
    int            rv;

    switch (cmd) {
    case DDI_ATTACH:
        break;
     ...
    }

    /*
     * Soft state allocation.
     */
    shp = kmem_zalloc(sizeof (*shp), KM_SLEEP);
    ddi_set_driver_private(dip, shp);
     ...
    /*
     * Tear down register mappings, etc.
     */
    for (i = 0; i < shp->sh_numslots; i++)
        sdhost_uninit_slot(shp, i);
    sda_host_free(shp->sh_host);
    kmem_free(shp, sizeof (*shp));

    return (DDI_SUCCESS);
}

Among other things, sdhost_uninit_slot() will attempt to iterate through every slot pointed by i. For clarity, here is the required information about sdhost_t structure. shp is a pointer to that structure:

typedef    struct sdhost    sdhost_t;

And sdhost is this structure:

/*
 * Per controller state.
 */
struct sdhost {
    int            sh_numslots;
    ddi_dma_attr_t        sh_dmaattr;
    sdslot_t        sh_slots[SDHOST_MAXSLOTS];
    sda_host_t        *sh_host;

    /*
     * Interrupt related information.
     */
    ddi_intr_handle_t    sh_ihandle;
    int            sh_icap;
    uint_t            sh_ipri;
};

Its member, sh_slots is an array of sdslot_t which is:

typedef    struct sdslot    sdslot_t;

And sdslot contains:

/*
 * Per slot state.
 */
struct sdslot {
    sda_host_t        *ss_host;
    int            ss_num;
    ddi_acc_handle_t    ss_acch;
      ...
};

sdhost allocates space for that structure using kmem_zalloc(), then it invokes ddi_set_driver_private() which simply sets shp to the dip’s device driver’s data. If the code reaches the for loop at the end of the function, it will attempt to iterate through each of the shp->sh_slots through sdhost_uninit_slot() which does this:

void
sdhost_uninit_slot(sdhost_t *shp, int num)
{
    sdslot_t    *ss;

    ss = &shp->sh_slots[num];

    if (ss->ss_acch != NULL)
        (void) sdhost_soft_reset(ss, SOFT_RESET_ALL);

    if (ss->ss_bufdmac.dmac_address)
        (void) ddi_dma_unbind_handle(ss->ss_bufdmah);

    if (ss->ss_bufacch != NULL)
        ddi_dma_mem_free(&ss->ss_bufacch);

    if (ss->ss_bufdmah != NULL)
        ddi_dma_free_handle(&ss->ss_bufdmah);

    if (ss->ss_ksp != NULL)
        kstat_delete(ss->ss_ksp);

    if (ss->ss_acch != NULL)
        ddi_regs_map_free(&ss->ss_acch);

    if (ss->ss_num != -1)
        mutex_destroy(&ss->ss_lock);
}

However, all these slots might contain uninitialized or incorrect values and these operations will lead to invalid memory access. To fix this behavior, they patched sdhost_attach() by adding this loop:

    ddi_set_driver_private(dip, shp);

+    /*
+     * Reset the "slot number", so uninit slot works properly.
+     */
+    for (i = 0; i < SDHOST_MAXSLOTS; i++) {
+        shp->sh_slots[i].ss_num = -1;
+    }

This patch simply initializes the ss_num of each slot to -1.

Written by xorl

June 4, 2009 at 16:05

Posted in bugs, solaris

4 Responses

Subscribe to comments with RSS.

  1. Why in most of big appz people write functions like this:

    void
    foo(a,b,c);

    rather than “normal” way:

    void foo(a,b,c);

    Tell me xorl, is it really more readable like that?

    ad

    June 4, 2009 at 22:31

  2. This is kind of functions’ definitions is used mostly in BSD and OpenSolaris (possibly in Solaris as well) operating systems. It is not being used by Linux however. I personally like this kind of definition because functions are more readable, you have:

    return value
    function

    And it is also much more easier to locate functions using regex. Silvio wrote a post regarding this subject on 2007:
    http://silviocesare.wordpress.com/2007/10/22/function-definitions-begin-on-a-new-line/

    xorl

    June 5, 2009 at 11:08

  3. I don’t quite understand the solution to this problem. The unsafe function checks a number of flags in the sdslot (ss_acch, ss_bufdmac, etc) until finally checking the ss_num. You note that “all these slots might contain uninitialized or incorrect values.” How is this then solved by only setting ss_num to -1, especially when ss_num is the last flag checked? What about invalid memory reads for any of the other flags?

    ankrgyl

    June 26, 2009 at 13:41

  4. Sorry for the delayed reply.
    Indeed you are right, this patch was fixing mostly some memory leaks in src/uts/common/io/sdcard/adapters/sdhost/sdhost.c (which I didn’t mention here) and since the allocated buffer is initilized using kmem_zalloc() which zeroes out (notice the z in zalloc) the requested space there is no uninitialized data.
    Thanks for pointing this out and I promise to be more careful in my future posts.

    Thanks a lot.

    xorl

    July 7, 2009 at 01:52


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