Is the co-pilot itself performance-sensitive? [LWN.net]

Is the co-pilot itself performance-sensitive?

Posted Sep 23, 2024 0:24 UTC (Mon) by ms-tg (subscriber, #89231)
In reply to: Is the co-pilot itself performance-sensitive? by rweikusat2
Parent article: pcp: pmcd network daemon review (SUSE Security Team Blog)

Thanks so much for this:

> 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) {
>
> The first (original line) is the wrong comparison. > It's wrong because vindex is used to index an array of __mPDU (32 bit with current definition) and not bytes. Correct code would be
>
> if (vindex < 0 || vindex > pdulen / sizeof(__pmPDU)) {

Question: is it worth someone making a tiny PR with this change and the explanation above?

to post comments

Is the co-pilot itself performance-sensitive?

Posted Sep 23, 2024 2:03 UTC (Mon) by viro (subscriber, #7872) [Link] (2 responses)

>> if (vindex < 0 || vindex > pdulen / sizeof(__pmPDU)) {

> Question: is it worth someone making a tiny PR with this change and the explanation above?

Wouldn't vindex equal to pdulen / sizeof(....) end up with off-by-one?

Is the co-pilot itself performance-sensitive?

Posted Sep 23, 2024 9:12 UTC (Mon) by rweikusat2 (subscriber, #117920) [Link] (1 responses)

It would. The best way to express this without errors is probably

if (vindex < 0 || vindex >= pdulen / sizeof(*pdubuf)) {

as this uses the actual type of the array element, whatever it is.

Is the co-pilot itself performance-sensitive?

Posted Sep 23, 2024 9:25 UTC (Mon) by rweikusat2 (subscriber, #117920) [Link]

Additional remark: Assuming that pdubuf if either an array of __pmPDU or a pointer to __pmPDU, the expressions

pdubuf[0]

and

*pdubuf

are semantically identical.

Is the co-pilot itself performance-sensitive?

Posted Sep 23, 2024 11:07 UTC (Mon) by rweikusat2 (subscriber, #117920) [Link]

I don't use this program. Because of this, I can't test any changes to it and because of that, I won't make any as code which hasn't been tested likely doesn't work, anyway. :-)

In theory, ie, it will likely be impossible to hit this in practice, the

&pdubuf[vindex]

could also trigger a pointer wraparound at the end of the address space and the >= test could thus fail because the code is really equivalent to

(char *)pdubuf + vindex * sizeof(*pdubuf)

The multiplication which opens up the path to a possible overflow isn't visible when using index-notation but the actual address calculation will have to employ it nevertheless.