Skip to contentSkip to conversation

vsprintf: Remove accidental VLA usage

PATCH In review Kees Cook 3 participants 7 messages Mar 7, 2018 – Mar 8, 2018

Subsystems (2)
Library code Vsprintf
lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH 1/1 vsprintf: Remove accidental VLA usage

The "sym" calculation is actually a fixed size, but since the max() macro uses some extensive tricks for safety, it ends up looking like a variable size. This replaces max() with a simple max macro which is sufficient for the calculation of the array size.

Seen with -Wvla. Fixed as part of the directive to remove all VLAs from the kernel: https://lkml.org/lkml/2018/3/7/621

Signed-off-by Kees Cook <[email protected]>
---
 lib/vsprintf.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
lib/vsprintf.c +3 -2
index d7a708f82559..f420ab1477cb 100644
@@ -744,8 +744,9 @@ char *resource_string(char *buf, char *end, struct resource *res,
 #define FLAG_BUF_SIZE		(2 * sizeof(res->flags))
 #define DECODED_BUF_SIZE	sizeof("[mem - 64bit pref window disabled]")
 #define RAW_BUF_SIZE		sizeof("[mem - flags 0x]")
-	char sym[max(2*RSRC_BUF_SIZE + DECODED_BUF_SIZE,
-		     2*RSRC_BUF_SIZE + FLAG_BUF_SIZE + RAW_BUF_SIZE)];
+#define SIMPLE_MAX(x, y)	((x) > (y) ? (x) : (y))
+	char sym[SIMPLE_MAX(2*RSRC_BUF_SIZE + DECODED_BUF_SIZE,
+			    2*RSRC_BUF_SIZE + FLAG_BUF_SIZE + RAW_BUF_SIZE)];
 
 	char *p = sym, *pend = sym + sizeof(sym);
 	int decode = (fmt[0] == 'R') ? 1 : 0;
Signature · 6 lines
2.7.4


-- 
Kees Cook
Pixel Security
maintainer
#1 lore raw
Quoted 14 lines · On Wed, 7 Mar 2018 15:07:14 -0800 Kees Cook <[email protected]> wrote:

The "sym" calculation is actually a fixed size, but since the max() macro uses some extensive tricks for safety, it ends up looking like a variable size. This replaces max() with a simple max macro which is sufficient for the calculation of the array size.

Seen with -Wvla. Fixed as part of the directive to remove all VLAs from the kernel: https://lkml.org/lkml/2018/3/7/621

...

lib/vsprintf.c +0 -0
@@ -744,8 +744,9 @@ char *resource_string(char *buf, char *end, struct resource *res,
 #define FLAG_BUF_SIZE		(2 * sizeof(res->flags))

lib/vsprintf.c char *resource_string(char *buf, char *end, struct resource *res, · line 752

Quoted 10 lines
lib/vsprintf.c +3 -2
 #define DECODED_BUF_SIZE	sizeof("[mem - 64bit pref window disabled]")
 #define RAW_BUF_SIZE		sizeof("[mem - flags 0x]")
-	char sym[max(2*RSRC_BUF_SIZE + DECODED_BUF_SIZE,
-		     2*RSRC_BUF_SIZE + FLAG_BUF_SIZE + RAW_BUF_SIZE)];
+#define SIMPLE_MAX(x, y)	((x) > (y) ? (x) : (y))
+	char sym[SIMPLE_MAX(2*RSRC_BUF_SIZE + DECODED_BUF_SIZE,
+			    2*RSRC_BUF_SIZE + FLAG_BUF_SIZE + RAW_BUF_SIZE)];
 
 	char *p = sym, *pend = sym + sizeof(sym);
 	int decode = (fmt[0] == 'R') ? 1 : 0;

A year from now I'll be receiving an email titled

[patch] lib/vsprintf.c: use standard max() macro

won't I?

lib/vsprintf.c renamed from lib/vsprintf.c~vsprintf-remove-accidental-vla-usage-fix +1 -0
@@ -754,6 +754,7 @@ char *resource_string(char *buf, char *e
 #define FLAG_BUF_SIZE		(2 * sizeof(res->flags))
 #define DECODED_BUF_SIZE	sizeof("[mem - 64bit pref window disabled]")
 #define RAW_BUF_SIZE		sizeof("[mem - flags 0x]")
+/* regular max() tricks gcc into creating a variable length array */
 #define SIMPLE_MAX(x, y)	((x) > (y) ? (x) : (y))
 	char sym[SIMPLE_MAX(2*RSRC_BUF_SIZE + DECODED_BUF_SIZE,
 			    2*RSRC_BUF_SIZE + FLAG_BUF_SIZE + RAW_BUF_SIZE)];

_

#3 lore raw
In reply to Kees Cook

Hi Kees,

Quoted 18 lines · Kees Cook

The "sym" calculation is actually a fixed size, but since the max() macro uses some extensive tricks for safety, it ends up looking like a variable size. This replaces max() with a simple max macro which is sufficient for the calculation of the array size.

Seen with -Wvla. Fixed as part of the directive to remove all VLAs from the kernel: https://lkml.org/lkml/2018/3/7/621

Signed-off-by Kees Cook <[email protected]>
---
 lib/vsprintf.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
lib/vsprintf.c +0 -0
index d7a708f82559..f420ab1477cb 100644

lib/vsprintf.c char *resource_string(char *buf, char *end, struct resource *res, · line 747

Quoted 6 lines
lib/vsprintf.c +1 -2
@@ -744,8 +744,9 @@ char *resource_string(char *buf, char *end, struct resource *res,
 #define FLAG_BUF_SIZE		(2 * sizeof(res->flags))
 #define DECODED_BUF_SIZE	sizeof("[mem - 64bit pref window disabled]")
 #define RAW_BUF_SIZE		sizeof("[mem - flags 0x]")
-	char sym[max(2*RSRC_BUF_SIZE + DECODED_BUF_SIZE,
-		     2*RSRC_BUF_SIZE + FLAG_BUF_SIZE + RAW_BUF_SIZE)];
+#define SIMPLE_MAX(x, y)	((x) > (y) ? (x) : (y))

It's probably worth hoisting this out into some other header. When I was looking at this a while ago, this problem happens in a few places, see e.g. net/ipv4/proc.c:TCPUDP_MIB_MAX.

Cheers,

Tycho

author
#2 lore raw
In reply to Andrew Morton
Quoted 24 lines · Andrew Morton
Quoted 22 lines · On Wed, 7 Mar 2018 15:07:14 -0800 Kees Cook <[email protected]> wrote:

The "sym" calculation is actually a fixed size, but since the max() macro uses some extensive tricks for safety, it ends up looking like a variable size. This replaces max() with a simple max macro which is sufficient for the calculation of the array size.

Seen with -Wvla. Fixed as part of the directive to remove all VLAs from the kernel: https://lkml.org/lkml/2018/3/7/621

...

lib/vsprintf.c +3 -2
@@ -744,8 +744,9 @@ char *resource_string(char *buf, char *end, struct resource *res,
 #define FLAG_BUF_SIZE                (2 * sizeof(res->flags))
 #define DECODED_BUF_SIZE     sizeof("[mem - 64bit pref window disabled]")
 #define RAW_BUF_SIZE         sizeof("[mem - flags 0x]")
-     char sym[max(2*RSRC_BUF_SIZE + DECODED_BUF_SIZE,
-                  2*RSRC_BUF_SIZE + FLAG_BUF_SIZE + RAW_BUF_SIZE)];
+#define SIMPLE_MAX(x, y)     ((x) > (y) ? (x) : (y))
+     char sym[SIMPLE_MAX(2*RSRC_BUF_SIZE + DECODED_BUF_SIZE,
+                         2*RSRC_BUF_SIZE + FLAG_BUF_SIZE + RAW_BUF_SIZE)];
Quoted 8 lines

lib/vsprintf.c char *resource_string(char *buf, char *end, struct resource *res, · line 752

Quoted 2 lines
lib/vsprintf.c +0 -0
      char *p = sym, *pend = sym + sizeof(sym);
      int decode = (fmt[0] == 'R') ? 1 : 0;

A year from now I'll be receiving an email titled

[patch] lib/vsprintf.c: use standard max() macro

won't I?

Hopefully a year from now we'll have added -Wvla to the global build flags and no one will be able to try that. :)

-Kees

Signature · 2 lines
Kees Cook
Pixel Security
author
#4 lore raw
In reply to Tycho Andersen
Quoted 16 lines · Tycho Andersen

Hi Kees,

Quoted 13 lines · Kees Cook

The "sym" calculation is actually a fixed size, but since the max() macro uses some extensive tricks for safety, it ends up looking like a variable size. This replaces max() with a simple max macro which is sufficient for the calculation of the array size.

Seen with -Wvla. Fixed as part of the directive to remove all VLAs from the kernel: https://lkml.org/lkml/2018/3/7/621

Signed-off-by Kees Cook <[email protected]>
---
 lib/vsprintf.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Quoted 15 lines

lib/vsprintf.c char *resource_string(char *buf, char *end, struct resource *res, · line 747

Quoted 11 lines
lib/vsprintf.c +1 -2
index d7a708f82559..f420ab1477cb 100644
@@ -744,8 +744,9 @@ char *resource_string(char *buf, char *end, struct resource *res,
 #define FLAG_BUF_SIZE                (2 * sizeof(res->flags))
 #define DECODED_BUF_SIZE     sizeof("[mem - 64bit pref window disabled]")
 #define RAW_BUF_SIZE         sizeof("[mem - flags 0x]")
-     char sym[max(2*RSRC_BUF_SIZE + DECODED_BUF_SIZE,
-                  2*RSRC_BUF_SIZE + FLAG_BUF_SIZE + RAW_BUF_SIZE)];
+#define SIMPLE_MAX(x, y)     ((x) > (y) ? (x) : (y))

It's probably worth hoisting this out into some other header. When I was looking at this a while ago, this problem happens in a few places, see e.g. net/ipv4/proc.c:TCPUDP_MIB_MAX.

Hmm, good point. All of those suffer from the same "max*() is too fancy".

I didn't want to encourage a global macro that _lacked_ the safety built into the max*() family, though... thoughts for a reasonable approach?

-Kees

Signature · 2 lines
Kees Cook
Pixel Security
maintainer
#5 lore raw
Quoted 3 lines · On Wed, 7 Mar 2018 15:59:27 -0800 Kees Cook <[email protected]> wrote:

I didn't want to encourage a global macro that _lacked_ the safety built into the max*() family, though... thoughts for a reasonable approach?

I think SIMPLE_MAX() is OK. Along with one of /* these */ things ;)

author
#6 lore raw
Quoted 7 lines · Andrew Morton
Quoted 3 lines · On Wed, 7 Mar 2018 15:59:27 -0800 Kees Cook <[email protected]> wrote:

I didn't want to encourage a global macro that _lacked_ the safety built into the max*() family, though... thoughts for a reasonable approach?

I think SIMPLE_MAX() is OK. Along with one of /* these */ things ;)

Sounds good. I will resend with net/ipv[46]/proc.c and one other fixed as well...

-Kees

Signature · 2 lines
Kees Cook
Pixel Security