← Back to context

Comment by accessvector

2 years ago

Re-reading this, my analysis is slightly incorrect: the `MAX` at [5] with an unsigned arg means we can make `npins` an arbitrary `int` using the loop at [4].

Choosing to make `npins` negative using that loop means we'll end up allocating an array of 87 (`SYS_kbind + 1`) `int`s at [8] and continue with the OOB accesses described.

You'd set up your `pinsyscall` entries like this:

    struct pinsyscall entries[] = {
        { .sysno = 0x1111, .offset = 0xdeadbeef }, /* first oob write */
        { .sysno = 0x2222, .offset = 0xf000f000 }, /* second oob write */
        { .sysno = 0xffffffff } /* sets npins to 0xffffffff so we under-allocate */
    };

`npins` would be `0xffffffff` after the loop and then the `MAX` at [6] would then return `86`, since `MAX(-1, 86) == 86`.

7 comments

accessvector

Reply
actionfromafar  2 years ago

I must misunderstand something very basic about this code.

    if (pins[syscalls[i].sysno])

but pins is newly allocated and should just zero or "empty". Why dereference it right after allocation?

  • accessvector  2 years ago

    Just to handle the case where the same syscall number is specified twice by the ELF header: in that case, the entry is set to -1 (presumably meaning it’s invalid).

    • 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 replies →