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

It really does need to be a case-by-case basis. It'll be a balance of speed, safety, and sanity. :) In the lustre case, that's both a bug fix (buffer over-run) and an unbounded VLA removal. Putting a string of unknown length on the stack tends not to be sensible, so the kmalloc/kfree is reasonable, IMO.

Building with -Wvla, I see 209 unique locations reported in 60 directories:
http://paste.ubuntu.com/p/srQxwPQS9s/

In the case of the crypto, my past thoughts have included either adding a buffer to some already-allocated context, or using an upper bound on the VLAs, since there's a fixed number of implementations built in at any given time. Though, I suspect neither will work without more examination. Usually, if it were easy, it'd be done already. ;)

To try to keep from adding new VLAs, maybe we could add -Wvla to the W=n level in scripts/Makefile.extrawarn. Likely W=2:

# W=1 - warnings that may be relevant and does not occur too often # W=2 - warnings that occur quite often but may still be relevant # W=3 - the more obscure warnings, can most likely be ignored

And frankly, maybe -Wformat-security -- and perhaps format-truncation and format-overflow -- should get added to W=2 too... they've gotten it much less noisy over time, though still very noisy. ;)

Or, as mentioned in another thread, disable -Wvla in certain directories but enable it at the top-level. I'm less of a fan of that, though, since it tends to lead to the problem just getting forgotten.

-Kees

Kees Cook
Pixel Security

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

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.

I agree that the code is garbage. In my defense, creating generic iterator-type functions for multiple data types appears to be one of the hardest problems in CS with many bad examples of what not to do.

Patch below should fix it. We have tcm_qla2xxx systems, so I will stick it into our test system as well.

Jörn

--
It is a cliché that most clichés are true, but then, like most clichés, that cliché is untrue.
-- Stephen Fry

From: Joern Engel <[email protected]>
Date: Tue, 13 Mar 2018 11:36:49 -0700
Subject: [PATCH] btree: avoid variable-length allocations

geo->keylen cannot be larger than 4. So we might as well make fixed-size allocations.

Given the one remaining user, geo->keylen cannot even be larger than 1. Logfs used to have 64bit and 128bit keys, tcm_qla2xxx only has 32bit keys. But let's not break the code if we don't have to.

---
 lib/btree.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

index f93a945274af..590facba2c50 100644
@@ -3,7 +3,7 @@
  *
  * As should be obvious for Linux kernel code, license is GPLv2
  *
- * Copyright (c) 2007-2008 Joern Engel <[email protected]>
+ * Copyright (c) 2007-2008 Joern Engel <[email protected]>
  * Bits and pieces stolen from Peter Zijlstra's code, which is
  * Copyright 2007, Red Hat Inc. Peter Zijlstra
  * GPLv2

@@ -76,6 +76,8 @@ struct btree_geo btree_geo128 = {
 };
 EXPORT_SYMBOL_GPL(btree_geo128);
 
+#define MAX_KEYLEN	(2 * LONG_PER_U64)
+
 static struct kmem_cache *btree_cachep;
 
 void *btree_alloc(gfp_t gfp_mask, void *pool_data)

@@ -313,7 +315,7 @@ void *btree_get_prev(struct btree_head *head, struct btree_geo *geo,
 {
 	int i, height;
 	unsigned long *node, *oldnode;
-	unsigned long *retry_key = NULL, key[geo->keylen];
+	unsigned long *retry_key = NULL, key[MAX_KEYLEN];
 
 	if (keyzero(geo, __key))
 		return NULL;

@@ -639,8 +641,8 @@ EXPORT_SYMBOL_GPL(btree_remove);
 int btree_merge(struct btree_head *target, struct btree_head *victim,
 		struct btree_geo *geo, gfp_t gfp)
 {
-	unsigned long key[geo->keylen];
-	unsigned long dup[geo->keylen];
+	unsigned long key[MAX_KEYLEN];
+	unsigned long dup[MAX_KEYLEN];
 	void *val;
 	int err;
 

2.15.1