Changeset 07d4271 in mainline


Ignore:
Timestamp:
2024-01-25T16:22:55Z (3 months ago)
Author:
Jiří Zárevúcky <zarevucky.jiri@…>
Branches:
master
Children:
f8b69a1e
Parents:
1a1e124
git-author:
Jiří Zárevúcky <zarevucky.jiri@…> (2024-01-25 15:56:31)
git-committer:
Jiří Zárevúcky <zarevucky.jiri@…> (2024-01-25 16:22:55)
Message:

Fix some unsound task reference manipulation and locking

In some operations that take task ID as an argument,
there's a possibility of the task being destroyed mid-operation
and a subsequent use-after-free situation.
As a general solution, task_find_by_id() is reimplemented to
check for this situation and always return a valid strong reference.
The callers then only need to handle the reference itself, and
don't need to concern themselves with tasks_lock.

Location:
kernel/generic
Files:
8 edited

Legend:

Unmodified
Added
Removed

  • kernel/generic/include/proc/task.h

    r1a1e124 r07d4271  
    8888
    8989        /** Number of references (i.e. threads). */
    90         atomic_size_t refcount;
     90        atomic_refcount_t refcount;
    9191        /** Number of threads that haven't exited yet. */
    9292        // TODO: remove
    … …  
    144144extern void task_done(void);
    145145extern task_t *task_create(as_t *, const char *);
    146 extern void task_destroy(task_t *);
    147146extern void task_hold(task_t *);
    148147extern void task_release(task_t *);

  • kernel/generic/src/ddi/ddi.c

    r1a1e124 r07d4271  
    336336                return EPERM;
    337337
    338         irq_spinlock_lock(&tasks_lock, true);
    339 
    340338        task_t *task = task_find_by_id(id);
    341339
    342         if ((!task) || (!container_check(CONTAINER, task->container))) {
    343                 /*
    344                  * There is no task with the specified ID
    345                  * or the task belongs to a different security
    346                  * context.
    347                  */
    348                 irq_spinlock_unlock(&tasks_lock, true);
     340        if (!task)
    349341                return ENOENT;
    350         }
    351 
    352         /* Lock the task and release the lock protecting tasks dictionary. */
    353         irq_spinlock_exchange(&tasks_lock, &task->lock);
    354         errno_t rc = ddi_iospace_enable_arch(task, ioaddr, size);
     342
     343        errno_t rc = ENOENT;
     344
     345        irq_spinlock_lock(&task->lock, true);
     346
     347        /* Check that the task belongs to the correct security context. */
     348        if (container_check(CONTAINER, task->container))
     349                rc = ddi_iospace_enable_arch(task, ioaddr, size);
     350
    355351        irq_spinlock_unlock(&task->lock, true);
    356 
     352        task_release(task);
    357353        return rc;
    358354}
    … …  
    377373                return EPERM;
    378374
    379         irq_spinlock_lock(&tasks_lock, true);
    380 
    381375        task_t *task = task_find_by_id(id);
    382376
    383         if ((!task) || (!container_check(CONTAINER, task->container))) {
    384                 /*
    385                  * There is no task with the specified ID
    386                  * or the task belongs to a different security
    387                  * context.
    388                  */
    389                 irq_spinlock_unlock(&tasks_lock, true);
     377        if (!task)
    390378                return ENOENT;
    391         }
    392 
    393         /* Lock the task and release the lock protecting tasks dictionary. */
    394         irq_spinlock_exchange(&tasks_lock, &task->lock);
    395         errno_t rc = ddi_iospace_disable_arch(task, ioaddr, size);
     379
     380        errno_t rc = ENOENT;
     381
     382        irq_spinlock_lock(&task->lock, true);
     383
     384        /* Check that the task belongs to the correct security context. */
     385        if (container_check(CONTAINER, task->container))
     386                rc = ddi_iospace_disable_arch(task, ioaddr, size);
     387
    396388        irq_spinlock_unlock(&task->lock, true);
    397 
     389        task_release(task);
    398390        return rc;
    399391}

  • kernel/generic/src/ipc/ipc.c

    r1a1e124 r07d4271  
    967967void ipc_print_task(task_id_t taskid)
    968968{
    969         irq_spinlock_lock(&tasks_lock, true);
    970969        task_t *task = task_find_by_id(taskid);
    971         if (!task) {
    972                 irq_spinlock_unlock(&tasks_lock, true);
     970        if (!task)
    973971                return;
    974         }
    975         task_hold(task);
    976         irq_spinlock_unlock(&tasks_lock, true);
    977972
    978973        printf("[phone cap] [calls] [state\n");

  • kernel/generic/src/ipc/kbox.c

    r1a1e124 r07d4271  
    200200/** Connect phone to a task kernel-box specified by id.
    201201 *
    202  * Note that this is not completely atomic. For optimisation reasons, the task
    203  * might start cleaning up kbox after the phone has been connected and before
    204  * a kbox thread has been created. This must be taken into account in the
    205  * cleanup code.
    206  *
    207202 * @param[out] out_phone  Phone capability handle on success.
    208203 * @return Error code.
    … …  
    211206errno_t ipc_connect_kbox(task_id_t taskid, cap_phone_handle_t *out_phone)
    212207{
    213         irq_spinlock_lock(&tasks_lock, true);
    214 
    215208        task_t *task = task_find_by_id(taskid);
    216         if (task == NULL) {
    217                 irq_spinlock_unlock(&tasks_lock, true);
     209        if (!task)
    218210                return ENOENT;
    219         }
    220 
    221         atomic_inc(&task->refcount);
    222 
    223         irq_spinlock_unlock(&tasks_lock, true);
    224211
    225212        mutex_lock(&task->kb.cleanup_lock);
    226 
    227         if (atomic_predec(&task->refcount) == 0) {
    228                 mutex_unlock(&task->kb.cleanup_lock);
    229                 task_destroy(task);
    230                 return ENOENT;
    231         }
    232213
    233214        if (task->kb.finished) {
    234215                mutex_unlock(&task->kb.cleanup_lock);
     216                task_release(task);
    235217                return EINVAL;
    236218        }
    … …  
    243225                if (!kb_thread) {
    244226                        mutex_unlock(&task->kb.cleanup_lock);
     227                        task_release(task);
    245228                        return ENOMEM;
    246229                }
    … …  
    255238        if (rc != EOK) {
    256239                mutex_unlock(&task->kb.cleanup_lock);
     240                task_release(task);
    257241                return rc;
    258242        }
    … …  
    265249
    266250        mutex_unlock(&task->kb.cleanup_lock);
     251        task_release(task);
    267252        *out_phone = phone_handle;
    268253        return EOK;

  • kernel/generic/src/proc/program.c

    r1a1e124 r07d4271  
    9999        if (!area) {
    100100                free(kernel_uarg);
    101                 task_destroy(prg->task);
     101                task_release(prg->task);
    102102                prg->task = NULL;
    103103                return ENOMEM;
    … …  
    119119                free(kernel_uarg);
    120120                as_area_destroy(as, virt);
    121                 task_destroy(prg->task);
     121                task_release(prg->task);
    122122                prg->task = NULL;
    123123                return ELIMIT;

  • kernel/generic/src/proc/task.c

    r1a1e124 r07d4271  
    158158                return rc;
    159159
    160         atomic_store(&task->refcount, 0);
    161160        atomic_store(&task->lifecount, 0);
    162161
    … …  
    201200        if (!task)
    202201                return NULL;
     202
     203        refcount_init(&task->refcount);
    203204
    204205        task_create_arch(task);
    … …  
    268269 *
    269270 */
    270 void task_destroy(task_t *task)
     271static void task_destroy(task_t *task)
    271272{
    272273        /*
    … …  
    299300void task_hold(task_t *task)
    300301{
    301         atomic_inc(&task->refcount);
     302        refcount_up(&task->refcount);
    302303}
    303304
    … …  
    311312void task_release(task_t *task)
    312313{
    313         if ((atomic_predec(&task->refcount)) == 0)
     314        if (refcount_down(&task->refcount))
    314315                task_destroy(task);
    315316}
    … …  
    416417/** Find task structure corresponding to task ID.
    417418 *
    418  * The tasks_lock must be already held by the caller of this function and
    419  * interrupts must be disabled.
    420  *
    421419 * @param id Task ID.
    422420 *
    423  * @return Task structure address or NULL if there is no such task ID.
     421 * @return Task reference or NULL if there is no such task ID.
    424422 *
    425423 */
    426424task_t *task_find_by_id(task_id_t id)
    427425{
    428         assert(interrupts_disabled());
    429         assert(irq_spinlock_locked(&tasks_lock));
     426        task_t *task = NULL;
     427
     428        irq_spinlock_lock(&tasks_lock, true);
    430429
    431430        odlink_t *odlink = odict_find_eq(&tasks, &id, NULL);
    432         if (odlink != NULL)
    433                 return odict_get_instance(odlink, task_t, ltasks);
    434 
    435         return NULL;
     431        if (odlink != NULL) {
     432                task = odict_get_instance(odlink, task_t, ltasks);
     433
     434                /*
     435                 * The directory of tasks can't hold a reference, since that would
     436                 * prevent task from ever being destroyed. That means we have to
     437                 * check for the case where the task is already being destroyed, but
     438                 * not yet removed from the directory.
     439                 */
     440                if (!refcount_try_up(&task->refcount))
     441                        task = NULL;
     442        }
     443
     444        irq_spinlock_unlock(&tasks_lock, true);
     445
     446        return task;
    436447}
    437448
    … …  
    524535static void task_kill_internal(task_t *task)
    525536{
    526         irq_spinlock_lock(&task->lock, false);
     537        irq_spinlock_lock(&task->lock, true);
    527538
    528539        /*
    … …  
    534545        }
    535546
    536         irq_spinlock_unlock(&task->lock, false);
     547        irq_spinlock_unlock(&task->lock, true);
    537548}
    538549
    … …  
    552563                return EPERM;
    553564
    554         irq_spinlock_lock(&tasks_lock, true);
    555 
    556565        task_t *task = task_find_by_id(id);
    557         if (!task) {
    558                 irq_spinlock_unlock(&tasks_lock, true);
     566        if (!task)
    559567                return ENOENT;
    560         }
    561568
    562569        task_kill_internal(task);
    563         irq_spinlock_unlock(&tasks_lock, true);
    564 
     570        task_release(task);
    565571        return EOK;
    566572}
    … …  
    592598        }
    593599
    594         irq_spinlock_lock(&tasks_lock, true);
    595600        task_kill_internal(TASK);
    596         irq_spinlock_unlock(&tasks_lock, true);
    597 
    598601        thread_exit();
    599602}
    … …  
    624627        if (additional)
    625628                printf("%-8" PRIu64 " %9zu", task->taskid,
    626                     atomic_load(&task->refcount));
     629                    atomic_load(&task->lifecount));
    627630        else
    628631                printf("%-8" PRIu64 " %-14s %-5" PRIu32 " %10p %10p"
    … …  
    636639                printf("%-8" PRIu64 " %9" PRIu64 "%c %9" PRIu64 "%c "
    637640                    "%9zu\n", task->taskid, ucycles, usuffix, kcycles,
    638                     ksuffix, atomic_load(&task->refcount));
     641                    ksuffix, atomic_load(&task->lifecount));
    639642        else
    640643                printf("%-8" PRIu64 " %-14s %-5" PRIu32 " %18p %18p\n",

  • kernel/generic/src/security/perm.c

    r1a1e124 r07d4271  
    8989                return EPERM;
    9090
    91         irq_spinlock_lock(&tasks_lock, true);
    9291        task_t *task = task_find_by_id(taskid);
    93 
    94         if ((!task) || (!container_check(CONTAINER, task->container))) {
    95                 irq_spinlock_unlock(&tasks_lock, true);
     92        if (!task)
    9693                return ENOENT;
     94
     95        errno_t rc = ENOENT;
     96
     97        irq_spinlock_lock(&task->lock, true);
     98        if (container_check(CONTAINER, task->container)) {
     99                task->perms |= perms;
     100                rc = EOK;
    97101        }
    98 
    99         irq_spinlock_lock(&task->lock, false);
    100         task->perms |= perms;
    101         irq_spinlock_unlock(&task->lock, false);
    102 
    103         irq_spinlock_unlock(&tasks_lock, true);
    104         return EOK;
     102        irq_spinlock_unlock(&task->lock, true);
     103
     104        task_release(task);
     105        return rc;
    105106}
    106107
    … …  
    118119static errno_t perm_revoke(task_id_t taskid, perm_t perms)
    119120{
    120         irq_spinlock_lock(&tasks_lock, true);
    121 
    122121        task_t *task = task_find_by_id(taskid);
    123         if ((!task) || (!container_check(CONTAINER, task->container))) {
    124                 irq_spinlock_unlock(&tasks_lock, true);
     122        if (!task)
    125123                return ENOENT;
    126         }
    127124
    128125        /*
    … …  
    131128         * doesn't have PERM_PERM.
    132129         */
    133         irq_spinlock_lock(&TASK->lock, false);
    134 
    135         if ((!(TASK->perms & PERM_PERM)) || (task != TASK)) {
    136                 irq_spinlock_unlock(&TASK->lock, false);
    137                 irq_spinlock_unlock(&tasks_lock, true);
     130        if (task != TASK && !(perm_get(TASK) & PERM_PERM)) {
     131                task_release(task);
    138132                return EPERM;
    139133        }
    140134
    141         task->perms &= ~perms;
    142         irq_spinlock_unlock(&TASK->lock, false);
    143 
    144         irq_spinlock_unlock(&tasks_lock, true);
    145         return EOK;
     135        errno_t rc = ENOENT;
     136
     137        irq_spinlock_lock(&task->lock, true);
     138        if (container_check(CONTAINER, task->container)) {
     139                task->perms &= ~perms;
     140                rc = EOK;
     141        }
     142        irq_spinlock_unlock(&task->lock, true);
     143
     144        task_release(task);
     145        return rc;
    146146}
    147147

  • kernel/generic/src/sysinfo/stats.c

    r1a1e124 r07d4271  
    221221        stats_task->virtmem = get_task_virtmem(task->as);
    222222        stats_task->resmem = get_task_resmem(task->as);
    223         stats_task->threads = atomic_load(&task->refcount);
     223        stats_task->threads = atomic_load(&task->lifecount);
    224224        task_get_accounting(task, &(stats_task->ucycles),
    225225            &(stats_task->kcycles));
    … …  
    511511{
    512512        /* Initially no return value */
    513         sysinfo_return_t ret;
    514         ret.tag = SYSINFO_VAL_UNDEFINED;
     513        sysinfo_return_t ret = {
     514                .tag = SYSINFO_VAL_UNDEFINED,
     515        };
    515516
    516517        /* Parse the task ID */
    … …  
    519520                return ret;
    520521
    521         /* Messing with task structures, avoid deadlock */
    522         irq_spinlock_lock(&tasks_lock, true);
    523 
    524522        task_t *task = task_find_by_id(task_id);
    525         if (task == NULL) {
    526                 /* No task with this ID */
    527                 irq_spinlock_unlock(&tasks_lock, true);
     523        if (!task)
    528524                return ret;
    529         }
    530525
    531526        if (dry_run) {
    … …  
    533528                ret.data.data = NULL;
    534529                ret.data.size = sizeof(stats_task_t);
    535 
    536                 irq_spinlock_unlock(&tasks_lock, true);
    537530        } else {
    538531                /* Allocate stats_task_t structure */
    539                 stats_task_t *stats_task =
    540                     (stats_task_t *) malloc(sizeof(stats_task_t));
    541                 if (stats_task == NULL) {
    542                         irq_spinlock_unlock(&tasks_lock, true);
    543                         return ret;
     532                stats_task_t *stats_task = malloc(sizeof(stats_task_t));
     533
     534                if (stats_task != NULL) {
     535                        /* Correct return value */
     536                        ret.tag = SYSINFO_VAL_FUNCTION_DATA;
     537                        ret.data.data = stats_task;
     538                        ret.data.size = sizeof(stats_task_t);
     539
     540                        irq_spinlock_lock(&task->lock, true);
     541                        produce_stats_task(task, stats_task);
     542                        irq_spinlock_unlock(&task->lock, true);
    544543                }
    545 
    546                 /* Correct return value */
    547                 ret.tag = SYSINFO_VAL_FUNCTION_DATA;
    548                 ret.data.data = (void *) stats_task;
    549                 ret.data.size = sizeof(stats_task_t);
    550 
    551                 /* Hand-over-hand locking */
    552                 irq_spinlock_exchange(&tasks_lock, &task->lock);
    553 
    554                 produce_stats_task(task, stats_task);
    555 
    556                 irq_spinlock_unlock(&task->lock, true);
    557         }
    558 
     544        }
     545
     546        task_release(task);
    559547        return ret;
    560548}
Note: See TracChangeset for help on using the changeset viewer.