[actionfromafar]

I still don't get it. Shouldn't [9] always evaluate to false, and the code be equivalent to:

    pins = mallocarray(npins, sizeof(int), M_PINSYSCALL, M_WAITOK|M_ZERO);
    for (i = 0; i < nsyscalls; i++) {
        pins[syscalls[i].sysno] = syscalls[i].offset;
    }

Edit:

Hang on - npins is already checked in the loop before, and incremented with ++

syscalls[i].sysno can't be larger than what is allocated with:

pins = mallocarray(npins, sizeof(int), M_PINSYSCALL, M_WAITOK|M_ZERO);

So I still can't find the problem

[accessvector]

Consider this:

    struct pinsyscall entries[] = {
        { .sysno = 1, .offset = 0x1234 },
        { .sysno = 2, .offset = 0x5678 },
        { .sysno = 1, .offset = 0x9abc }
    };

Now `nsyscalls` will be 3 and `pin` will be an array of 3 ints, initialised to `{ 0, 0, 0 }`.

When we loop through, we'll set:

    1. `pin[syscalls[0].sysno] = 0x1234` => `pin[1] = 0x1234`
    2. `pin[syscalls[1].sysno] = 0x5678` => `pin[2] = 0x5678`

Now when we come to 3, we'll find `pin[syscalls[2].sysno] != 0` since `syscalls[2].sysno == syscalls[0].sysno` - so we set `pin[1] = -1` instead of `0x9abc`.

[actionfromafar]

Oh, thanks, now I understand why there is an if in the for loop! But I still can't see how pin[] could be accessed out of bounds, since the array is allocated to be large enough to hold the largest value of .sysno occurring in the entries[] array.

[accessvector]

Right, so this is the crux of the vulnerability. Firstly, note that the `MAX` macro is defined as:

    #define MAX(a, b) ((a) > (b) ? (a) : (b))

This is important because it doesn't cast either arg to any particular type. You can use it with `float`s, or `int`s, or `u_int`s... or a combination.

Referring back to the implementation, the first use of `MAX` (inside the `for` loop) is this:

    ...
    for (i = 0; i < nsyscalls; i++)
        npins = MAX(npins, syscalls[i].sysno);
    ...

`npins` is an `int`, but `sysno` is a `u_int`. C integer promotion rules means that we'll actually be implicitly casting `npins` to a `u_int` here; it's as if we did this:

    ...
     npins = MAX((u_int)npins, syscalls[i].sysno);
    ...

This means that `npins` can end up as any value we like -- even up to `0xffffffff`. But remember that `npins` is _actually_ a signed int, so once it comes out of `MAX`, it'll be signed again. Thus we can use this to make `npins` negative.

Once we're out of the loop, `MAX` is used again here:

    ...
    npins = MAX(npins, SYS_kbind);
    ...

Where `SYS_kbind` is just:

    ...
    #define SYS_kbind       86
    ...

Integer literals in C are signed, so now this use of `MAX` is actually dealing with two signed integers. If we used the loop to make `npins` negative (as described just before) then this line will now take 86 as the maximum of the two values.

With `npins = 86`, an array of 86+1 will be allocated, but the `syscalls[i].sysno` in the next loop could of course easily be greater than 86 -- thus leading to out-of-bounds array access.

[actionfromafar]

Thanks! Very clear explanation!

So then it depends on if whatever code loaded the elf section does any validation of the data it reads. I can't help but thinking the whole code could use some structs and/or "getter setters" to talk dirty objective oriented speak. It needn't be that though, it could be as low level as some macros which helps doing the right thing with signedness and such. But my main impression is that a lot of the data structure semantics is kept in the heads of programmers instead of being formalised in the code.

In C there is always the opportunity to run with scissors in the middle of the road, but you can do a lot to protect yourself too, without loosing much, if any, performance.