Hacker News Reader

Comment by qzzi | Hacker News Reader

I don't know how they define `MAX`, but I'm guessing it's a typical "a>b?a:b". In function `elf_read_pintable` the `npins` is defined as signed int and `sysno` as unsigned int.

So this comparison will be unsigned and will allow to set `npins` to any value, even negative:

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

Then `SYS_kbind` seems to be a signed int. So this comparison will be signed and "fix" the negative `npins` to `SYS_kbind`:

  npins = MAX(npins, SYS_kbind)

And finally the `sysno` index might be out of bounds here:

  pins[syscalls[i].sysno] = syscalls[i].offset

But maybe I'm completely wrong, I'm not interested in researching it too much.

View on HN

> I don't know how they define `MAX`, but I'm guessing it's a typical "a>b?a:b"

Indeed: https://github.com/openbsd/src/blob/master/sys/sys/param.h#L...

> Then `SYS_kbind` seems to be a signed int.

It's an untyped #define: https://github.com/openbsd/src/blob/master/sys/sys/syscall.h...

I believe your whole analysis is correct, that running an elf file with an openbsd.syscalls entry with .sysno > INT_MAX will allow an out-of-bounds write.

> It's an untyped #define

Pure decimal integer literals (like 86) are typed as "int" in C, rather than being typeless and triggering type inference. This is a pain when you accidentally write something like this:

  uint64_t n = 1 << 32;

On modern desktop platforms, an int is 32 bits, so 1 << 32 is 0, not 2^32, even though a 64-bit integer is wide enough to support that.

Regardless, it's not relevant here, because if an integer and an unsigned integer of the same size are compared the integer is implicitly cast to unsigned integer, and 86 is fine for both signed and unsigned integers (so "MAX(npins, SYS_kbind)" is safe).

> Then `SYS_kbind` seems to be a signed int. So this comparison will be signed and "fix" the negative `npins` to `SYS_kbind`:

  npins = MAX(npins, SYS_kbind)

No, the comparison is unsigned here. They're integers of the same "conversion rank", so the unsigned type wins and the signed integer is interpreted as unsigned.

https://en.cppreference.com/w/c/language/conversion

I can never remember the integer conversion rules and end up looking up this link whenever necessary.

This isn't correct, as npins is a signed int and SYS_kbind is just a macro for the integer 86 (as defined in sys/syscall.h). So it will be a signed comparison between the value of npins and 86.

My bad, I thought npins was unsigned.

No.

That seems about right.

Reply on news.ycombinator.com