Undefined behaviour [LWN.net]

Undefined behaviour

Posted Oct 8, 2024 9:47 UTC (Tue) by mgerstner (guest, #164735)
In reply to: Is the co-pilot itself performance-sensitive? by rweikusat2
Parent article: pcp: pmcd network daemon review (SUSE Security Team Blog)

> You'll have to appreciate the irony that the SUSE fix for this issue has undefined behaviour.
>
> - if (vindex < 0 || vindex > pdulen) {
> + if (vindex < 0 || (char *)&pdubuf[vindex] >= pduend) {

I agree that this is formally undefined behaviour. Note that this is not specifically a SUSE authored patch, though, it has been authored by PCP upstream. There are a lot more such constructs found in the PCP codebase, and I actually recommended to upstream to refactor the protocol processing on a larger scale, because it is hard to read, partly confusing and error prone.

to post comments

Undefined behaviour

Posted Oct 9, 2024 0:25 UTC (Wed) by viro (subscriber, #7872) [Link]

It's more than formally undefined behaviour; it's a very real bug on any architecture where you might get pdubuf close to the top of address space. If (uintptr_t)pdubuf > (uintptr_t)( -sizeof(pdubuf[0]) * vindex), you are likely to end up with code that will cheerfully calculate an address well below pdubuf itself, let alone pduend.

I hadn't seen the function in question, but if pdubuf elements are more than single-byte (as they appear to be, from the context) and vindex is signed 32bit, _any_ address on 32bit architecture is vulnerable to that.

You might get away with that in case if vindex is 16bit and you know that e.g. userland addresses are guaranteed to be under 4G-32K*sizeof, but such warranties are brittle - while for i386 Linux userland this is normally promised (the upper 1G is used by the kernel), just run that 32bit program on amd64 box with 32bit emulation enabled (default for a lot of amd64 configs) and you suddenly can see addresses very close to the top of 32bit space.

There's a good reason why you are not allowed to take pointers to array elements beyond just after the object, and it's not anything exotic as it is in case of quite a few UB. This one really bites on bog-standard architectures, without insane optimizers, etc.
If their codebase really has a lot of such examples, well... welcome to Dave Bowman code review moment. As in, "it goes on forever - oh my God - it's full of s<garbled><transmission terminated>"...