Comment by actionfromafar

2 years ago

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

4 comments

actionfromafar

accessvector 2 years ago

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 2 years ago
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 2 years ago
  
  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.
  
  1 reply →