Re: VLA removal (was Re: [RFC 2/2] lustre: use VLA_SAFE)

[Daniel Micay]

On 7 March 2018 at 13:09, Linus Torvalds  wrote:
> On Wed, Mar 7, 2018 at 9:37 AM, Kees Cook  wrote:
>>
>> Building with -Wvla, I see 209 unique locations reported in 60 directories:
>> http://paste.ubuntu.com/p/srQxwPQS9s/
>
> Ok, that's not so bad. Maybe Greg could even add it to one of those
> things he encourages new people to do?
>
> Because at least *some* of them are pretty trivial. For example,
> looking at the core code, I was surprised to see something in
> lib/btree.c

Some are probably just the issue of technically having a VLA that's
not really a VLA:

static const int size = 5;

void foo(void) {
  int x[size];
}

% gcc -c -Wvla foo.c
foo.c: In function ‘foo’:
foo.c:4:3: warning: ISO C90 forbids variable length array ‘x’ [-Wvla]
   int x[size];
   ^~~

I don't really understand why the C standard didn't make `static
const` declarations usable as constant expressions like C++. They made
the pointer conversions more painful too.

It would be nice to get rid of those cases to use -Werror=vla though.

Re: VLA removal (was Re: [RFC 2/2] lustre: use VLA_SAFE)

[Daniel Micay]

On 7 March 2018 at 13:09, Linus Torvalds  wrote:
> On Wed, Mar 7, 2018 at 9:37 AM, Kees Cook  wrote:
>>
>> Building with -Wvla, I see 209 unique locations reported in 60 directories:
>> http://paste.ubuntu.com/p/srQxwPQS9s/
>
> Ok, that's not so bad. Maybe Greg could even add it to one of those
> things he encourages new people to do?
>
> Because at least *some* of them are pretty trivial. For example,
> looking at the core code, I was surprised to see something in
> lib/btree.c

Some are probably just the issue of technically having a VLA that's
not really a VLA:

static const int size = 5;

void foo(void) {
  int x[size];
}

% gcc -c -Wvla foo.c
foo.c: In function ‘foo’:
foo.c:4:3: warning: ISO C90 forbids variable length array ‘x’ [-Wvla]
   int x[size];
   ^~~

I don't really understand why the C standard didn't make `static
const` declarations usable as constant expressions like C++. They made
the pointer conversions more painful too.

It would be nice to get rid of those cases to use -Werror=vla though.

Re: VLA removal (was Re: [RFC 2/2] lustre: use VLA_SAFE)

[Linus Torvalds]

On Wed, Mar 7, 2018 at 9:37 AM, Kees Cook  wrote:
>
> Building with -Wvla, I see 209 unique locations reported in 60 directories:
> http://paste.ubuntu.com/p/srQxwPQS9s/

Ok, that's not so bad. Maybe Greg could even add it to one of those
things he encourages new people to do?

Because at least *some* of them are pretty trivial. For example,
looking at the core code, I was surprised to see something in
lib/btree.c

And that is just garbage: it uses

unsigned long key[geo->keylen];

which looks really dangerous, but that "struct btree_geo" is internal
to that file, and there are exactly three instances of it, with 32, 64
and 128 bit keys respectively. Note that "keylen" isn't actually
number of hits, but how many long-words you need.

So in actual fact, that array is limited to that 128 bits - just 16
bytes. So keylen is at most 4 (on 32-bit architectures) or 2 (on
64-bit ones).

Using

   #define MAXKEYLEN BITS_TO_LONGS(128)

or something like that would be trivial.

AND USING VLA'S IS ACTIVELY STUPID! It generates much more code, and
much _slower_ code (and more fragile code), than just using a fixed
key size would have done.

Ok, so lib/btree.c looks more core (by being in lib/) than it actually
is - I don't see the 128-bit btree being used *anywhere*, and the
others are only used by two drivers: the qla2xxx scsi driver and the
bcm2835-camera driver in staging.

Anyway, some of these are definitely easy to just fix, and using VLA's
is actively bad not just for security worries, but simply because
VLA's are a really horribly bad idea in general in the kernel.

Added Jörn Engel to the cc, since I looked at that lib/btree.c thing.

But that is just three of the 209 instances. Some of the others might
be slightly more painful to fix.

  Linus

Re: VLA removal (was Re: [RFC 2/2] lustre: use VLA_SAFE)

[Linus Torvalds]

On Wed, Mar 7, 2018 at 9:37 AM, Kees Cook  wrote:
>
> Building with -Wvla, I see 209 unique locations reported in 60 directories:
> http://paste.ubuntu.com/p/srQxwPQS9s/

Ok, that's not so bad. Maybe Greg could even add it to one of those
things he encourages new people to do?

Because at least *some* of them are pretty trivial. For example,
looking at the core code, I was surprised to see something in
lib/btree.c

And that is just garbage: it uses

unsigned long key[geo->keylen];

which looks really dangerous, but that "struct btree_geo" is internal
to that file, and there are exactly three instances of it, with 32, 64
and 128 bit keys respectively. Note that "keylen" isn't actually
number of hits, but how many long-words you need.

So in actual fact, that array is limited to that 128 bits - just 16
bytes. So keylen is at most 4 (on 32-bit architectures) or 2 (on
64-bit ones).

Using

   #define MAXKEYLEN BITS_TO_LONGS(128)

or something like that would be trivial.

AND USING VLA'S IS ACTIVELY STUPID! It generates much more code, and
much _slower_ code (and more fragile code), than just using a fixed
key size would have done.

Ok, so lib/btree.c looks more core (by being in lib/) than it actually
is - I don't see the 128-bit btree being used *anywhere*, and the
others are only used by two drivers: the qla2xxx scsi driver and the
bcm2835-camera driver in staging.

Anyway, some of these are definitely easy to just fix, and using VLA's
is actively bad not just for security worries, but simply because
VLA's are a really horribly bad idea in general in the kernel.

Added Jörn Engel to the cc, since I looked at that lib/btree.c thing.

But that is just three of the 209 instances. Some of the others might
be slightly more painful to fix.

  Linus