Project

General

Profile

Bug #4545

_t_create(): Use after free in error code paths

Added by Marcel Telka over 6 years ago. Updated over 5 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
lib - userland libraries
Start date:
2014-01-29
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
needs-triage
Gerrit CR:

Description

There is this code in the _t_create() function:

961    ntiptr = add_tilink(fd);
962    if (ntiptr == NULL) {
963        t_errno = TSYSERR;
964        errno = ENOMEM;
965        return (NULL);
966    }
967    sig_mutex_lock(&ntiptr->ti_lock);
968
969    /*
970     * Allocate buffers for the new descriptor
971     */
972    if (_t_alloc_bufs(fd, ntiptr, tiap) < 0) {
973        sv_errno = errno;
974        (void) _t_delete_tilink(fd);
975        t_errno = TSYSERR;
976        sig_mutex_unlock(&ntiptr->ti_lock);
977        errno = sv_errno;
978        return (NULL);
979    }

The ntiptr is allocated at line 961 in add_tilink() and freed at line 974 via _t_delete_tilink(). However, at line 976 the freed memory is referenced.

Similar error handling code is in the _t_create() used several times.


Files

test.c (172 Bytes) test.c Marcel Telka, 2015-02-10 11:45 PM
test.d (709 Bytes) test.d Marcel Telka, 2015-02-10 11:45 PM

Related issues

Related to illumos gate - Bug #4871: t_sync(3nsl) is not thread safeIn Progress2014-05-20

Actions

History

#1

Updated by Marcel Telka over 6 years ago

  • Assignee set to Marcel Telka
#2

Updated by Marcel Telka about 6 years ago

The newly allocated ntiptr (via the add_tilink() function) is added into the hash_bucket array in the add_tilink(). All operations with the hash_bucket array are protected by _ti_userlock mutex. This also includes add_tilink() and find_tilink() functions. The find_tilink() function is used to get a tiptr from the hash_bucket array for the particular fd. Since we hold the _ti_userlock during the _t_create() execution, there is no way how any other thread could obtain a pointer to our newly allocated ntiptr. If such attempt is made, the thread will be blocked on the _ti_userlock mutex lock; even before the find_tilink() is called.

Based on the above, our newly allocated ntiptr is sufficiently protected by the main _ti_userlock mutex, and we do no need to hold ntiptr->ti_lock when we are working with ntiptr in _t_create().

#3

Updated by Marcel Telka about 6 years ago

  • Status changed from New to In Progress
#4

Updated by Marcel Telka about 6 years ago

In addition, none of the code executed after the add_tilink() call in _t_create() requires to hold the ntiptr->ti_lock. This means that we could safely remove the locking/unlocking of the ntiptr->ti_lock from _t_create(). The removal will also fix the problem with use after free.

#5

Updated by Marcel Telka over 5 years ago

To reproduce the bug use the attached test.c and test.d files and do the following:

# gcc -lnsl -lumem test.c -o test
# export UMEM_DEBUG=default
# dtrace -Cs test.d -c ./test
dtrace: script 'test.d' matched 6 probes
dtrace: allowing destructive actions
t_open failed
CPU     ID                    FUNCTION:NAME
  0  64690           _t_delete_tilink:entry 
  0  64693           sig_mutex_unlock:entry mutex_t {
    struct flags = {
        uint16_t flag1 = 0xbeef
        uint8_t flag2 = 0xad
        uint8_t ceiling = 0xde
        union mbcp_type_un = {
            uint16_t bcptype = 0xbeef
            struct mtype_rcount = {
                uint8_t count_type1 = 0xef
                uint8_t count_type2 = 0xbe
            }
        }
        uint16_t magic = 0xdead
    }
    union lock = {
        struct lock64 = {
            uint8_t [8] pad = [ 0xef, 0xbe, 0xad, 0xde, 0xef, 0xbe, 0xad, 0xde ]
        }
        struct lock32 = {
            uint32_t ownerpid = 0xdeadbeef
            uint32_t lockword = 0xdeadbeef
        }
        upad64_t owner64 = 0xdeadbeefdeadbeef
    }
    upad64_t data = 0xdeadbeefdeadbeef
}
dtrace: pid 2252 has exited

#

We see the mutex passed to sig_mutex_unlock() is already freed (0xdeadbeef).

#6

Updated by Marcel Telka over 5 years ago

  • Status changed from In Progress to Pending RTI
#7

Updated by Electric Monk over 5 years ago

  • Status changed from Pending RTI to Closed
  • % Done changed from 0 to 100

git commit d0fcb88af333aa48dd2b958f3681f1b597b924cc

commit  d0fcb88af333aa48dd2b958f3681f1b597b924cc
Author: Marcel Telka <marcel.telka@nexenta.com>
Date:   2015-02-11T16:30:40.000Z

    4545 _t_create(): Use after free in error code paths
    Reviewed by: Garrett D'Amore <garrett@damore.org>
    Reviewed by: Dan McDonald <danmcd@omniti.com>
    Approved by: Richard Lowe <richlowe@richlowe.net>

Also available in: Atom PDF