Tone and correctness [LWN.net]

Tone and correctness

Posted Jan 8, 2005 0:24 UTC (Sat) by BruceRamsay (guest, #2068)
Parent article: grsecurity 2.1.0 and kernel vulnerabilities

I'm sure there are valuable fixes in these patches. I look forward to their inclusion in a future kernel. However, it is good to keep things in perspective. The world did not collapse without these patches.

After a quick look at the bugs listed I have a few questions about some of the analysis. For example:

>> if(len > sizeof(moxaBuff))
> ^ signed int has only upper-bound checked
>> return -EINVAL;

On all systems I know of, sizeof() produces an unsigned number. In C, comparisons between unsigned numbers and signed numbers are done as unsigned comparisons. In fact, -1 > sizeof(moxaBuff) is true. Therefore the comment "signed int has only upper-bound checked" is incorrect. After the test we are guaranteed that 0 <= len <= sizeof(moxaBuff). (I am speaking about real world C implementations and not theoretically possible C compilers.)

A quick look at Linux source code shows me that, at least on some architectures, PAGE_SIZE is an unsigned number. So tests like "len < PAGE_SIZE" also check for negative values of len.

It is hard to put a high priority on something which also includes incorrect analysis.

Still, I applaud the use automatic code analysis for producing clean and correct code. The more bugs removed the better.

Bruce

(Log in to post comments)

Tone and correctness

Posted Jan 8, 2005 17:08 UTC (Sat) by kleptog (subscriber, #1183) [Link]

Fascinating, I'd never noticed this part of the standard before but some tests on my machine here reveal you are correct, at least on the version I'm testing (gcc 3.3.2). Any comparisons with unsigned turns both sides unsigned and sizeof() returns unsigned.

While I sympathise with the guy, there are a number of things he could have done better. For example, including "[SECURITY]" in the subject of the emails might have been a start. The subject as given means nothing to me. I would not be surprised if the emails simply vanished in the bit bucket. Sending to Alan or Linus directly if they don't know you is a dead end.

Sending it to any of the other security teams, security focus, redhat, debian, whoever would probably have got the message somewhere where people know the right channels to use.

usual arithmetic promotions

Posted Jan 10, 2005 12:59 UTC (Mon) by jejones3141 (guest, #27116) [Link]

>After a quick look at the bugs listed I have a few questions about some of
>the analysis. For example:

>>> if(len > sizeof(moxaBuff))
>> ^ signed int has only upper-bound checked
>>> return -EINVAL;

>On all systems I know of, sizeof() produces an unsigned number. In C,
>comparisons between unsigned numbers and signed numbers are done as
>unsigned comparisons. In fact, -1 > sizeof(moxaBuff) is true. Therefore
>the comment "signed int has only upper-bound checked" is incorrect. After
>the test we are guaranteed that 0 <= len <= sizeof(moxaBuff). (I am
>speaking about real world C implementations and not theoretically possible
>C compilers.)

Some pre-ANSI/ISO C compilers did "unsigned preserving" promotions,
and for them your analysis is correct. ANSI/ISO, however, has "value
preserving" promotions, i.e. the compiler tries to find a type that
will represent all possible values of the types of both operands of >
to widen the operands to. Pre-C9X, since 64-bit arithmetic isn't
mandated, given that size_t (the type that sizeof() yields) is an
unsigned 32-bit type, the compiler will fail to find such a type.
I _think_ it ends up using an unsigned 32-bit integer. If C9X, which
requires 64-bit integer types, follows through with value preserving
rules (and size_t is still 32-bit), it may well do the comparison
in 64-bit signed integers, in which case -1 will not be greater than
sizeof(moxaBuff).

usual arithmetic promotions

Posted Jan 10, 2005 14:38 UTC (Mon) by khim (subscriber, #9252) [Link]

Have you actually tried gcc ?

The only compiler used by kernel developers will use "unsigned int" for calculation - thus all standards are irrelevant.

Yes, it's still nice thing to fix (who know what'll happen in future version of gcc?) but it's not "security bug" anymore. That's the point.

usual arithmetic promotions

Posted Jan 10, 2005 15:37 UTC (Mon) by velco (guest, #27121) [Link]

> ANSI/ISO, however, has "value preserving" promotions, i.e. the compiler
> tries to find a type that will represent all possible values of the types
> of both operands of to widen the operands to.

Incorrect. If operand types (after integer promotions) differ, one of
the operands can be at most widened to the unsigned version of the type of
the other operand. IOW, the original operand types set an upper limit on
the width of the promoted operands. The relevant section of Teh Standard
follows:

| If both operands have the same type, then no
| further conversion is needed.
|
| Otherwise, if both operands have signed integer
| types or both have unsigned integer types, the
| operand with the type of lesser integer conversion
| rank is converted to the type of the operand with
| greater rank.
|
| Otherwise, if the operand that has unsigned
| integer type has rank greater or equal to the rank
| of the type of the other operand, then the operand
| with signed integer type is converted to the type
| of the operand with unsigned integer type.
|
| Otherwise, if the type of the operand with signed
| integer type can represent all of the values of
| the type of the operand with unsigned integer
| type, then the operand with unsigned integer type
| is converted to the type of the operand with
| signed integer type.
|
| Otherwise, both operands are converted to the
| unsigned integer type corresponding to the type of
| the operand with signed integer type.

In the concrete case of comparing int and size_t, for all the cases of
practical interest (i.e., size_t having conversion rank greater than or
equal to the conversion rank of int), the int operand will be converted to
size_t and negative value will result in an error return.

~velco