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 */
};
if (pins[syscalls[i].sysno])
pins = mallocarray(npins, sizeof(int), M_PINSYSCALL, M_WAITOK|M_ZERO);
for (i = 0; i < nsyscalls; i++) {
pins[syscalls[i].sysno] = syscalls[i].offset;
}
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
struct pinsyscall entries[] = {
{ .sysno = 1, .offset = 0x1234 },
{ .sysno = 2, .offset = 0x5678 },
{ .sysno = 1, .offset = 0x9abc }
};
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`
#define MAX(a, b) ((a) > (b) ? (a) : (b))
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 = MAX((u_int)npins, syscalls[i].sysno);
...
Once we're out of the loop, `MAX` is used again here:
...
npins = MAX(npins, SYS_kbind);
...
...
#define SYS_kbind 86
...
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.
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.
At [4], we want to figure out how big to make our pin array, so we loop over all of the syscall entries and record the largest we've seen so far [5]. (Note: the use of `MAX` here is fine since `sysno` is unsigned -- see near the top of the function).
With the maximum `sysno` found, we then crucially go on to clamp the value to `SYS_kbind` [6] and +1 at [7].
This clamped maximum value is used for the array allocation at [8].
We now loop through the syscall list again, but now take the unclamped `sysno` as the index into the array to read at [9] and write at [10] and [11]. This is essentially the vulnerability right here.
Through heap grooming, there's a good chance you could arrange for a useful structure to be placed within range of the write at [11] -- and `offset` is essentially an arbitrary value you can write. So it looks like it would be relatively easy to exploit.