All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] Remove accidental VLA usage
@ 2018-03-08  3:30 ` Kees Cook
  0 siblings, 0 replies; 32+ messages in thread
From: Kees Cook @ 2018-03-08  3:30 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, linux-kernel, corbet, gustavo, rostedt, Chris Mason,
	Josef Bacik, David Sterba, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, Ingo Molnar, Peter Zijlstra, Thomas Gleixner,
	Masahiro Yamada, Borislav Petkov, Josh Poimboeuf, Randy Dunlap,
	Ian Abbott, Tobin C. Harding, Sergey Senozhatsky, Petr Mladek,
	Andy Shevchenko, Pantelis Antoniou, linux-btrfs, netdev,
	kernel-hardening

This series adds SIMPLE_MAX() to be used in places where a stack array
is actually fixed, but the compiler still warns about VLA usage due to
confusion caused by the safety checks in the max() macro.

I'm sending these via -mm since that's where I've introduced SIMPLE_MAX(),
and they should all have no operational differences.

-Kees


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 0/3] Remove accidental VLA usage
@ 2018-03-08  3:30 ` Kees Cook
  0 siblings, 0 replies; 32+ messages in thread
From: Kees Cook @ 2018-03-08  3:30 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, linux-kernel, corbet, gustavo, rostedt, Chris Mason,
	Josef Bacik, David Sterba, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, Ingo Molnar, Peter Zijlstra, Thomas Gleixner,
	Masahiro Yamada, Borislav Petkov, Josh Poimboeuf, Randy Dunlap,
	Ian Abbott, Tobin C. Harding, Sergey Senozhatsky, Petr Mladek

This series adds SIMPLE_MAX() to be used in places where a stack array
is actually fixed, but the compiler still warns about VLA usage due to
confusion caused by the safety checks in the max() macro.

I'm sending these via -mm since that's where I've introduced SIMPLE_MAX(),
and they should all have no operational differences.

-Kees


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v2 1/3] vsprintf: Remove accidental VLA usage
  2018-03-08  3:30 ` Kees Cook
@ 2018-03-08  3:30   ` Kees Cook
  -1 siblings, 0 replies; 32+ messages in thread
From: Kees Cook @ 2018-03-08  3:30 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, linux-kernel, corbet, gustavo, rostedt, Chris Mason,
	Josef Bacik, David Sterba, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, Ingo Molnar, Peter Zijlstra, Thomas Gleixner,
	Masahiro Yamada, Borislav Petkov, Josh Poimboeuf, Randy Dunlap,
	Ian Abbott, Tobin C. Harding, Sergey Senozhatsky, Petr Mladek,
	Andy Shevchenko, Pantelis Antoniou, linux-btrfs, netdev,
	kernel-hardening

In the quest to remove all stack VLAs from the kernel[1], this introduces
a new "simple max" macro, and changes the "sym" array size calculation to
use it. The value is actually a fixed size, but since the max() macro uses
some extensive tricks for safety, it ends up looking like a variable size
to the compiler.

[1] https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/kernel.h | 11 +++++++++++
 lib/vsprintf.c         |  4 ++--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 3fd291503576..1da554e9997f 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -820,6 +820,17 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
 	      x, y)
 
 /**
+ * SIMPLE_MAX - return maximum of two values without any type checking
+ * @x: first value
+ * @y: second value
+ *
+ * This should only be used in stack array sizes, since the type-checking
+ * from max() confuses the compiler into thinking a VLA is being used.
+ */
+#define SIMPLE_MAX(x, y)	((size_t)(x) > (size_t)(y) ? (size_t)(x) \
+							   : (size_t)(y))
+
+/**
  * min3 - return minimum of three values
  * @x: first value
  * @y: second value
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d7a708f82559..50cce36e1cdc 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -744,8 +744,8 @@ 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)];
+	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;
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v2 1/3] vsprintf: Remove accidental VLA usage
@ 2018-03-08  3:30   ` Kees Cook
  0 siblings, 0 replies; 32+ messages in thread
From: Kees Cook @ 2018-03-08  3:30 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, linux-kernel, corbet, gustavo, rostedt, Chris Mason,
	Josef Bacik, David Sterba, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, Ingo Molnar, Peter Zijlstra, Thomas Gleixner,
	Masahiro Yamada, Borislav Petkov, Josh Poimboeuf, Randy Dunlap,
	Ian Abbott, Tobin C. Harding, Sergey Senozhatsky, Petr Mladek,
	Andy Shevchenko, Pantelis Antoniou, linux-bt

In the quest to remove all stack VLAs from the kernel[1], this introduces
a new "simple max" macro, and changes the "sym" array size calculation to
use it. The value is actually a fixed size, but since the max() macro uses
some extensive tricks for safety, it ends up looking like a variable size
to the compiler.

[1] https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/kernel.h | 11 +++++++++++
 lib/vsprintf.c         |  4 ++--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 3fd291503576..1da554e9997f 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -820,6 +820,17 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
 	      x, y)
 
 /**
+ * SIMPLE_MAX - return maximum of two values without any type checking
+ * @x: first value
+ * @y: second value
+ *
+ * This should only be used in stack array sizes, since the type-checking
+ * from max() confuses the compiler into thinking a VLA is being used.
+ */
+#define SIMPLE_MAX(x, y)	((size_t)(x) > (size_t)(y) ? (size_t)(x) \
+							   : (size_t)(y))
+
+/**
  * min3 - return minimum of three values
  * @x: first value
  * @y: second value
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d7a708f82559..50cce36e1cdc 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -744,8 +744,8 @@ 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)];
+	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;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH 2/3] net: Remove accidental VLAs from proc buffers
  2018-03-08  3:30 ` Kees Cook
@ 2018-03-08  3:30   ` Kees Cook
  -1 siblings, 0 replies; 32+ messages in thread
From: Kees Cook @ 2018-03-08  3:30 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, linux-kernel, corbet, gustavo, rostedt, Chris Mason,
	Josef Bacik, David Sterba, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, Ingo Molnar, Peter Zijlstra, Thomas Gleixner,
	Masahiro Yamada, Borislav Petkov, Josh Poimboeuf, Randy Dunlap,
	Ian Abbott, Tobin C. Harding, Sergey Senozhatsky, Petr Mladek,
	Andy Shevchenko, Pantelis Antoniou, linux-btrfs, netdev,
	kernel-hardening

In the quest to remove all stack VLAs from the kernel[1], this refactors
the stack array size calculation to avoid using max(), which makes the
compiler think the size isn't fixed.

[1] https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 net/ipv4/proc.c | 10 ++++------
 net/ipv6/proc.c | 10 ++++------
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/net/ipv4/proc.c b/net/ipv4/proc.c
index dc5edc8f7564..c23c43803435 100644
--- a/net/ipv4/proc.c
+++ b/net/ipv4/proc.c
@@ -46,8 +46,6 @@
 #include <net/sock.h>
 #include <net/raw.h>
 
-#define TCPUDP_MIB_MAX max_t(u32, UDP_MIB_MAX, TCP_MIB_MAX)
-
 /*
  *	Report socket allocation statistics [mea@utu.fi]
  */
@@ -400,11 +398,11 @@ static int snmp_seq_show_ipstats(struct seq_file *seq, void *v)
 
 static int snmp_seq_show_tcp_udp(struct seq_file *seq, void *v)
 {
-	unsigned long buff[TCPUDP_MIB_MAX];
+	unsigned long buff[SIMPLE_MAX(UDP_MIB_MAX, TCP_MIB_MAX)];
 	struct net *net = seq->private;
 	int i;
 
-	memset(buff, 0, TCPUDP_MIB_MAX * sizeof(unsigned long));
+	memset(buff, 0, sizeof(buff));
 
 	seq_puts(seq, "\nTcp:");
 	for (i = 0; snmp4_tcp_list[i].name; i++)
@@ -421,7 +419,7 @@ static int snmp_seq_show_tcp_udp(struct seq_file *seq, void *v)
 			seq_printf(seq, " %lu", buff[i]);
 	}
 
-	memset(buff, 0, TCPUDP_MIB_MAX * sizeof(unsigned long));
+	memset(buff, 0, sizeof(buff));
 
 	snmp_get_cpu_field_batch(buff, snmp4_udp_list,
 				 net->mib.udp_statistics);
@@ -432,7 +430,7 @@ static int snmp_seq_show_tcp_udp(struct seq_file *seq, void *v)
 	for (i = 0; snmp4_udp_list[i].name; i++)
 		seq_printf(seq, " %lu", buff[i]);
 
-	memset(buff, 0, TCPUDP_MIB_MAX * sizeof(unsigned long));
+	memset(buff, 0, sizeof(buff));
 
 	/* the UDP and UDP-Lite MIBs are the same */
 	seq_puts(seq, "\nUdpLite:");
diff --git a/net/ipv6/proc.c b/net/ipv6/proc.c
index b67814242f78..5b0874c26802 100644
--- a/net/ipv6/proc.c
+++ b/net/ipv6/proc.c
@@ -30,10 +30,8 @@
 #include <net/transp_v6.h>
 #include <net/ipv6.h>
 
-#define MAX4(a, b, c, d) \
-	max_t(u32, max_t(u32, a, b), max_t(u32, c, d))
-#define SNMP_MIB_MAX MAX4(UDP_MIB_MAX, TCP_MIB_MAX, \
-			IPSTATS_MIB_MAX, ICMP_MIB_MAX)
+#define SNMP_MIB_MAX SIMPLE_MAX(SIMPLE_MAX(UDP_MIB_MAX, TCP_MIB_MAX), \
+				SIMPLE_MAX(IPSTATS_MIB_MAX, ICMP_MIB_MAX))
 
 static int sockstat6_seq_show(struct seq_file *seq, void *v)
 {
@@ -199,7 +197,7 @@ static void snmp6_seq_show_item(struct seq_file *seq, void __percpu *pcpumib,
 	int i;
 
 	if (pcpumib) {
-		memset(buff, 0, sizeof(unsigned long) * SNMP_MIB_MAX);
+		memset(buff, 0, sizeof(buff));
 
 		snmp_get_cpu_field_batch(buff, itemlist, pcpumib);
 		for (i = 0; itemlist[i].name; i++)
@@ -218,7 +216,7 @@ static void snmp6_seq_show_item64(struct seq_file *seq, void __percpu *mib,
 	u64 buff64[SNMP_MIB_MAX];
 	int i;
 
-	memset(buff64, 0, sizeof(u64) * SNMP_MIB_MAX);
+	memset(buff64, 0, sizeof(buff64));
 
 	snmp_get_cpu_field64_batch(buff64, itemlist, mib, syncpoff);
 	for (i = 0; itemlist[i].name; i++)
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH 2/3] net: Remove accidental VLAs from proc buffers
@ 2018-03-08  3:30   ` Kees Cook
  0 siblings, 0 replies; 32+ messages in thread
From: Kees Cook @ 2018-03-08  3:30 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, linux-kernel, corbet, gustavo, rostedt, Chris Mason,
	Josef Bacik, David Sterba, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, Ingo Molnar, Peter Zijlstra, Thomas Gleixner,
	Masahiro Yamada, Borislav Petkov, Josh Poimboeuf, Randy Dunlap,
	Ian Abbott, Tobin C. Harding, Sergey Senozhatsky, Petr Mladek

In the quest to remove all stack VLAs from the kernel[1], this refactors
the stack array size calculation to avoid using max(), which makes the
compiler think the size isn't fixed.

[1] https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 net/ipv4/proc.c | 10 ++++------
 net/ipv6/proc.c | 10 ++++------
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/net/ipv4/proc.c b/net/ipv4/proc.c
index dc5edc8f7564..c23c43803435 100644
--- a/net/ipv4/proc.c
+++ b/net/ipv4/proc.c
@@ -46,8 +46,6 @@
 #include <net/sock.h>
 #include <net/raw.h>
 
-#define TCPUDP_MIB_MAX max_t(u32, UDP_MIB_MAX, TCP_MIB_MAX)
-
 /*
  *	Report socket allocation statistics [mea@utu.fi]
  */
@@ -400,11 +398,11 @@ static int snmp_seq_show_ipstats(struct seq_file *seq, void *v)
 
 static int snmp_seq_show_tcp_udp(struct seq_file *seq, void *v)
 {
-	unsigned long buff[TCPUDP_MIB_MAX];
+	unsigned long buff[SIMPLE_MAX(UDP_MIB_MAX, TCP_MIB_MAX)];
 	struct net *net = seq->private;
 	int i;
 
-	memset(buff, 0, TCPUDP_MIB_MAX * sizeof(unsigned long));
+	memset(buff, 0, sizeof(buff));
 
 	seq_puts(seq, "\nTcp:");
 	for (i = 0; snmp4_tcp_list[i].name; i++)
@@ -421,7 +419,7 @@ static int snmp_seq_show_tcp_udp(struct seq_file *seq, void *v)
 			seq_printf(seq, " %lu", buff[i]);
 	}
 
-	memset(buff, 0, TCPUDP_MIB_MAX * sizeof(unsigned long));
+	memset(buff, 0, sizeof(buff));
 
 	snmp_get_cpu_field_batch(buff, snmp4_udp_list,
 				 net->mib.udp_statistics);
@@ -432,7 +430,7 @@ static int snmp_seq_show_tcp_udp(struct seq_file *seq, void *v)
 	for (i = 0; snmp4_udp_list[i].name; i++)
 		seq_printf(seq, " %lu", buff[i]);
 
-	memset(buff, 0, TCPUDP_MIB_MAX * sizeof(unsigned long));
+	memset(buff, 0, sizeof(buff));
 
 	/* the UDP and UDP-Lite MIBs are the same */
 	seq_puts(seq, "\nUdpLite:");
diff --git a/net/ipv6/proc.c b/net/ipv6/proc.c
index b67814242f78..5b0874c26802 100644
--- a/net/ipv6/proc.c
+++ b/net/ipv6/proc.c
@@ -30,10 +30,8 @@
 #include <net/transp_v6.h>
 #include <net/ipv6.h>
 
-#define MAX4(a, b, c, d) \
-	max_t(u32, max_t(u32, a, b), max_t(u32, c, d))
-#define SNMP_MIB_MAX MAX4(UDP_MIB_MAX, TCP_MIB_MAX, \
-			IPSTATS_MIB_MAX, ICMP_MIB_MAX)
+#define SNMP_MIB_MAX SIMPLE_MAX(SIMPLE_MAX(UDP_MIB_MAX, TCP_MIB_MAX), \
+				SIMPLE_MAX(IPSTATS_MIB_MAX, ICMP_MIB_MAX))
 
 static int sockstat6_seq_show(struct seq_file *seq, void *v)
 {
@@ -199,7 +197,7 @@ static void snmp6_seq_show_item(struct seq_file *seq, void __percpu *pcpumib,
 	int i;
 
 	if (pcpumib) {
-		memset(buff, 0, sizeof(unsigned long) * SNMP_MIB_MAX);
+		memset(buff, 0, sizeof(buff));
 
 		snmp_get_cpu_field_batch(buff, itemlist, pcpumib);
 		for (i = 0; itemlist[i].name; i++)
@@ -218,7 +216,7 @@ static void snmp6_seq_show_item64(struct seq_file *seq, void __percpu *mib,
 	u64 buff64[SNMP_MIB_MAX];
 	int i;
 
-	memset(buff64, 0, sizeof(u64) * SNMP_MIB_MAX);
+	memset(buff64, 0, sizeof(buff64));
 
 	snmp_get_cpu_field64_batch(buff64, itemlist, mib, syncpoff);
 	for (i = 0; itemlist[i].name; i++)
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH 3/3] btrfs: tree-checker: Avoid accidental stack VLA
  2018-03-08  3:30 ` Kees Cook
@ 2018-03-08  3:30   ` Kees Cook
  -1 siblings, 0 replies; 32+ messages in thread
From: Kees Cook @ 2018-03-08  3:30 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, linux-kernel, corbet, gustavo, rostedt, Chris Mason,
	Josef Bacik, David Sterba, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, Ingo Molnar, Peter Zijlstra, Thomas Gleixner,
	Masahiro Yamada, Borislav Petkov, Josh Poimboeuf, Randy Dunlap,
	Ian Abbott, Tobin C. Harding, Sergey Senozhatsky, Petr Mladek,
	Andy Shevchenko, Pantelis Antoniou, linux-btrfs, netdev,
	kernel-hardening

In the quest to remove all stack VLAs from the kernel[1], this refactors
the stack array size calculation to avoid using max(), which makes the
compiler think the size isn't fixed.

[1] https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/btrfs/tree-checker.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index c3c8d48f6618..59bd07694118 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -341,7 +341,8 @@ static int check_dir_item(struct btrfs_root *root,
 		 */
 		if (key->type == BTRFS_DIR_ITEM_KEY ||
 		    key->type == BTRFS_XATTR_ITEM_KEY) {
-			char namebuf[max(BTRFS_NAME_LEN, XATTR_NAME_MAX)];
+			char namebuf[SIMPLE_MAX(BTRFS_NAME_LEN,
+						XATTR_NAME_MAX)];
 
 			read_extent_buffer(leaf, namebuf,
 					(unsigned long)(di + 1), name_len);
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH 3/3] btrfs: tree-checker: Avoid accidental stack VLA
@ 2018-03-08  3:30   ` Kees Cook
  0 siblings, 0 replies; 32+ messages in thread
From: Kees Cook @ 2018-03-08  3:30 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, linux-kernel, corbet, gustavo, rostedt, Chris Mason,
	Josef Bacik, David Sterba, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, Ingo Molnar, Peter Zijlstra, Thomas Gleixner,
	Masahiro Yamada, Borislav Petkov, Josh Poimboeuf, Randy Dunlap,
	Ian Abbott, Tobin C. Harding, Sergey Senozhatsky, Petr Mladek,
	Andy Shevchenko, Pantelis Antoniou, linux-bt

In the quest to remove all stack VLAs from the kernel[1], this refactors
the stack array size calculation to avoid using max(), which makes the
compiler think the size isn't fixed.

[1] https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/btrfs/tree-checker.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index c3c8d48f6618..59bd07694118 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -341,7 +341,8 @@ static int check_dir_item(struct btrfs_root *root,
 		 */
 		if (key->type == BTRFS_DIR_ITEM_KEY ||
 		    key->type == BTRFS_XATTR_ITEM_KEY) {
-			char namebuf[max(BTRFS_NAME_LEN, XATTR_NAME_MAX)];
+			char namebuf[SIMPLE_MAX(BTRFS_NAME_LEN,
+						XATTR_NAME_MAX)];
 
 			read_extent_buffer(leaf, namebuf,
 					(unsigned long)(di + 1), name_len);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 1/3] vsprintf: Remove accidental VLA usage
  2018-03-08  3:30   ` Kees Cook
@ 2018-03-08  8:25     ` Rasmus Villemoes
  -1 siblings, 0 replies; 32+ messages in thread
From: Rasmus Villemoes @ 2018-03-08  8:25 UTC (permalink / raw)
  To: Kees Cook, Andrew Morton
  Cc: linux-kernel, corbet, gustavo, rostedt, Chris Mason, Josef Bacik,
	David Sterba, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, Ingo Molnar, Peter Zijlstra, Thomas Gleixner,
	Masahiro Yamada, Borislav Petkov, Josh Poimboeuf, Randy Dunlap,
	Ian Abbott, Tobin C. Harding, Sergey Senozhatsky, Petr Mladek,
	Andy Shevchenko, Pantelis Antoniou, linux-btrfs, netdev,
	kernel-hardening

On 2018-03-08 04:30, Kees Cook wrote:
> In the quest to remove all stack VLAs from the kernel[1], this introduces
> a new "simple max" macro, and changes the "sym" array size calculation to
> use it. The value is actually a fixed size, but since the max() macro uses
> some extensive tricks for safety, it ends up looking like a variable size
> to the compiler.
> 
> [1] https://lkml.org/lkml/2018/3/7/621
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  include/linux/kernel.h | 11 +++++++++++
>  lib/vsprintf.c         |  4 ++--
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 3fd291503576..1da554e9997f 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -820,6 +820,17 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
>  	      x, y)
>  
>  /**
> + * SIMPLE_MAX - return maximum of two values without any type checking
> + * @x: first value
> + * @y: second value
> + *
> + * This should only be used in stack array sizes, since the type-checking
> + * from max() confuses the compiler into thinking a VLA is being used.
> + */
> +#define SIMPLE_MAX(x, y)	((size_t)(x) > (size_t)(y) ? (size_t)(x) \
> +							   : (size_t)(y))

This will be abused at some point, leading to the usual double
evaluation etc. etc. problems. The name is also too long (and in general
we should avoid adjectives like "simple", "safe", people reading the
code won't know what is simple or safe about it). I think this should work

#define MAX(x, y) (__builtin_choose_expr((x) > (y), x, y))

That forces (x)>(y) to be a compile-time constant, so x and y must also
be; hence there can be no side effects. The MIN version of this could
replace the custom __const_min in fs/file.c, and probably other places
as well.

I tested that this at least works in the vsprintf case, -Wvla no longer
complains. fs/file.c also compiles with the MIN version of this.

I suppose MIN and MAX will collide with other uses in the tree. Hmm.

Rasmus

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 1/3] vsprintf: Remove accidental VLA usage
@ 2018-03-08  8:25     ` Rasmus Villemoes
  0 siblings, 0 replies; 32+ messages in thread
From: Rasmus Villemoes @ 2018-03-08  8:25 UTC (permalink / raw)
  To: Kees Cook, Andrew Morton
  Cc: linux-kernel, corbet, gustavo, rostedt, Chris Mason, Josef Bacik,
	David Sterba, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, Ingo Molnar, Peter Zijlstra, Thomas Gleixner,
	Masahiro Yamada, Borislav Petkov, Josh Poimboeuf, Randy Dunlap,
	Ian Abbott, Tobin C. Harding, Sergey Senozhatsky, Petr Mladek,
	Andy Shevchenko

On 2018-03-08 04:30, Kees Cook wrote:
> In the quest to remove all stack VLAs from the kernel[1], this introduces
> a new "simple max" macro, and changes the "sym" array size calculation to
> use it. The value is actually a fixed size, but since the max() macro uses
> some extensive tricks for safety, it ends up looking like a variable size
> to the compiler.
> 
> [1] https://lkml.org/lkml/2018/3/7/621
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  include/linux/kernel.h | 11 +++++++++++
>  lib/vsprintf.c         |  4 ++--
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 3fd291503576..1da554e9997f 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -820,6 +820,17 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
>  	      x, y)
>  
>  /**
> + * SIMPLE_MAX - return maximum of two values without any type checking
> + * @x: first value
> + * @y: second value
> + *
> + * This should only be used in stack array sizes, since the type-checking
> + * from max() confuses the compiler into thinking a VLA is being used.
> + */
> +#define SIMPLE_MAX(x, y)	((size_t)(x) > (size_t)(y) ? (size_t)(x) \
> +							   : (size_t)(y))

This will be abused at some point, leading to the usual double
evaluation etc. etc. problems. The name is also too long (and in general
we should avoid adjectives like "simple", "safe", people reading the
code won't know what is simple or safe about it). I think this should work

#define MAX(x, y) (__builtin_choose_expr((x) > (y), x, y))

That forces (x)>(y) to be a compile-time constant, so x and y must also
be; hence there can be no side effects. The MIN version of this could
replace the custom __const_min in fs/file.c, and probably other places
as well.

I tested that this at least works in the vsprintf case, -Wvla no longer
complains. fs/file.c also compiles with the MIN version of this.

I suppose MIN and MAX will collide with other uses in the tree. Hmm.

Rasmus

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 1/3] vsprintf: Remove accidental VLA usage
  2018-03-08  8:25     ` Rasmus Villemoes
@ 2018-03-08 11:21       ` Thomas Gleixner
  -1 siblings, 0 replies; 32+ messages in thread
From: Thomas Gleixner @ 2018-03-08 11:21 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: Kees Cook, Andrew Morton, linux-kernel, corbet, gustavo, rostedt,
	Chris Mason, Josef Bacik, David Sterba, David S. Miller,
	Alexey Kuznetsov, Hideaki YOSHIFUJI, Ingo Molnar, Peter Zijlstra,
	Masahiro Yamada, Borislav Petkov, Josh Poimboeuf, Randy Dunlap,
	Ian Abbott, Tobin C. Harding, Sergey Senozhatsky, Petr Mladek,
	Andy Shevchenko, Pantelis Antoniou, linux-btrfs, netdev,
	kernel-hardening

On Thu, 8 Mar 2018, Rasmus Villemoes wrote:
> On 2018-03-08 04:30, Kees Cook wrote:
> >  /**
> > + * SIMPLE_MAX - return maximum of two values without any type checking
> > + * @x: first value
> > + * @y: second value
> > + *
> > + * This should only be used in stack array sizes, since the type-checking
> > + * from max() confuses the compiler into thinking a VLA is being used.
> > + */
> > +#define SIMPLE_MAX(x, y)	((size_t)(x) > (size_t)(y) ? (size_t)(x) \
> > +							   : (size_t)(y))
> 
> This will be abused at some point, leading to the usual double
> evaluation etc. etc. problems. The name is also too long (and in general
> we should avoid adjectives like "simple", "safe", people reading the
> code won't know what is simple or safe about it). I think this should work
> 
> #define MAX(x, y) (__builtin_choose_expr((x) > (y), x, y))
> 
> That forces (x)>(y) to be a compile-time constant, so x and y must also
> be; hence there can be no side effects. The MIN version of this could
> replace the custom __const_min in fs/file.c, and probably other places
> as well.
> 
> I tested that this at least works in the vsprintf case, -Wvla no longer
> complains. fs/file.c also compiles with the MIN version of this.
> 
> I suppose MIN and MAX will collide with other uses in the tree. Hmm.

Make it CONST_MAX() or something like that which makes it entirely clear.

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 1/3] vsprintf: Remove accidental VLA usage
@ 2018-03-08 11:21       ` Thomas Gleixner
  0 siblings, 0 replies; 32+ messages in thread
From: Thomas Gleixner @ 2018-03-08 11:21 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: Kees Cook, Andrew Morton, linux-kernel, corbet, gustavo, rostedt,
	Chris Mason, Josef Bacik, David Sterba, David S. Miller,
	Alexey Kuznetsov, Hideaki YOSHIFUJI, Ingo Molnar, Peter Zijlstra,
	Masahiro Yamada, Borislav Petkov, Josh Poimboeuf, Randy Dunlap,
	Ian Abbott, Tobin C. Harding, Sergey Senozhatsky, Petr Mladek

On Thu, 8 Mar 2018, Rasmus Villemoes wrote:
> On 2018-03-08 04:30, Kees Cook wrote:
> >  /**
> > + * SIMPLE_MAX - return maximum of two values without any type checking
> > + * @x: first value
> > + * @y: second value
> > + *
> > + * This should only be used in stack array sizes, since the type-checking
> > + * from max() confuses the compiler into thinking a VLA is being used.
> > + */
> > +#define SIMPLE_MAX(x, y)	((size_t)(x) > (size_t)(y) ? (size_t)(x) \
> > +							   : (size_t)(y))
> 
> This will be abused at some point, leading to the usual double
> evaluation etc. etc. problems. The name is also too long (and in general
> we should avoid adjectives like "simple", "safe", people reading the
> code won't know what is simple or safe about it). I think this should work
> 
> #define MAX(x, y) (__builtin_choose_expr((x) > (y), x, y))
> 
> That forces (x)>(y) to be a compile-time constant, so x and y must also
> be; hence there can be no side effects. The MIN version of this could
> replace the custom __const_min in fs/file.c, and probably other places
> as well.
> 
> I tested that this at least works in the vsprintf case, -Wvla no longer
> complains. fs/file.c also compiles with the MIN version of this.
> 
> I suppose MIN and MAX will collide with other uses in the tree. Hmm.

Make it CONST_MAX() or something like that which makes it entirely clear.

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 3/3] btrfs: tree-checker: Avoid accidental stack VLA
  2018-03-08  3:30   ` Kees Cook
@ 2018-03-08 11:33     ` David Sterba
  -1 siblings, 0 replies; 32+ messages in thread
From: David Sterba @ 2018-03-08 11:33 UTC (permalink / raw)
  To: Kees Cook
  Cc: Andrew Morton, David S. Miller, gustavo, Chris Mason,
	Josef Bacik, Sergey Senozhatsky, rostedt, Peter Zijlstra,
	Randy Dunlap, Ingo Molnar, Pantelis Antoniou, Thomas Gleixner,
	Hideaki YOSHIFUJI, Andy Shevchenko, kernel-hardening, corbet,
	Ian Abbott, Alexey Kuznetsov, Josh Poimboeuf, Masahiro Yamada,
	Petr Mladek, Borislav Petkov, Tobin C. Harding, linux-btrfs,
	linux-kernel, netdev

On Wed, Mar 07, 2018 at 07:30:47PM -0800, Kees Cook wrote:
> In the quest to remove all stack VLAs from the kernel[1], this refactors
> the stack array size calculation to avoid using max(), which makes the
> compiler think the size isn't fixed.
> 
> [1] https://lkml.org/lkml/2018/3/7/621
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>

Acked-by: David Sterba <dsterba@suse.com>

for whatever name you decide for the max macro.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 3/3] btrfs: tree-checker: Avoid accidental stack VLA
@ 2018-03-08 11:33     ` David Sterba
  0 siblings, 0 replies; 32+ messages in thread
From: David Sterba @ 2018-03-08 11:33 UTC (permalink / raw)
  To: Kees Cook
  Cc: Andrew Morton, David S. Miller, gustavo, Chris Mason,
	Josef Bacik, Sergey Senozhatsky, rostedt, Peter Zijlstra,
	Randy Dunlap, Ingo Molnar, Pantelis Antoniou, Thomas Gleixner,
	Hideaki YOSHIFUJI, Andy Shevchenko, kernel-hardening, corbet,
	Ian Abbott, Alexey Kuznetsov, Josh Poimboeuf, Masahiro Yamada,
	Petr Mladek

On Wed, Mar 07, 2018 at 07:30:47PM -0800, Kees Cook wrote:
> In the quest to remove all stack VLAs from the kernel[1], this refactors
> the stack array size calculation to avoid using max(), which makes the
> compiler think the size isn't fixed.
> 
> [1] https://lkml.org/lkml/2018/3/7/621
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>

Acked-by: David Sterba <dsterba@suse.com>

for whatever name you decide for the max macro.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 0/3] Remove accidental VLA usage
  2018-03-08  3:30 ` Kees Cook
@ 2018-03-08 15:02   ` Josh Poimboeuf
  -1 siblings, 0 replies; 32+ messages in thread
From: Josh Poimboeuf @ 2018-03-08 15:02 UTC (permalink / raw)
  To: Kees Cook
  Cc: Andrew Morton, linux-kernel, corbet, gustavo, rostedt,
	Chris Mason, Josef Bacik, David Sterba, David S. Miller,
	Alexey Kuznetsov, Hideaki YOSHIFUJI, Ingo Molnar, Peter Zijlstra,
	Thomas Gleixner, Masahiro Yamada, Borislav Petkov, Randy Dunlap,
	Ian Abbott, Tobin C. Harding, Sergey Senozhatsky, Petr Mladek,
	Andy Shevchenko, Pantelis Antoniou, linux-btrfs, netdev,
	kernel-hardening

On Wed, Mar 07, 2018 at 07:30:44PM -0800, Kees Cook wrote:
> This series adds SIMPLE_MAX() to be used in places where a stack array
> is actually fixed, but the compiler still warns about VLA usage due to
> confusion caused by the safety checks in the max() macro.
> 
> I'm sending these via -mm since that's where I've introduced SIMPLE_MAX(),
> and they should all have no operational differences.

What if we instead simplify the max() macro's type checking so that GCC
can more easily fold the array size constants?  The below patch seems to
work:

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 3fd291503576..ec863726da29 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -782,42 +782,32 @@ ftrace_vprintk(const char *fmt, va_list ap)
 static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
 #endif /* CONFIG_TRACING */
 
-/*
- * min()/max()/clamp() macros that also do
- * strict type-checking.. See the
- * "unnecessary" pointer comparison.
- */
-#define __min(t1, t2, min1, min2, x, y) ({		\
-	t1 min1 = (x);					\
-	t2 min2 = (y);					\
-	(void) (&min1 == &min2);			\
-	min1 < min2 ? min1 : min2; })
+extern long __error_incompatible_types_in_min_macro;
+extern long __error_incompatible_types_in_max_macro;
+
+#define __min(t1, t2, x, y)						\
+	__builtin_choose_expr(__builtin_types_compatible_p(t1, t2),	\
+			      (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y),	\
+			      (t1)__error_incompatible_types_in_min_macro)
 
 /**
  * min - return minimum of two values of the same or compatible types
  * @x: first value
  * @y: second value
  */
-#define min(x, y)					\
-	__min(typeof(x), typeof(y),			\
-	      __UNIQUE_ID(min1_), __UNIQUE_ID(min2_),	\
-	      x, y)
+#define min(x, y) __min(typeof(x), typeof(y), x, y)			\
 
-#define __max(t1, t2, max1, max2, x, y) ({		\
-	t1 max1 = (x);					\
-	t2 max2 = (y);					\
-	(void) (&max1 == &max2);			\
-	max1 > max2 ? max1 : max2; })
+#define __max(t1, t2, x, y)						\
+	__builtin_choose_expr(__builtin_types_compatible_p(t1, t2),	\
+			      (t1)(x) > (t2)(y) ? (t1)(x) : (t2)(y),	\
+			      (t1)__error_incompatible_types_in_max_macro)
 
 /**
  * max - return maximum of two values of the same or compatible types
  * @x: first value
  * @y: second value
  */
-#define max(x, y)					\
-	__max(typeof(x), typeof(y),			\
-	      __UNIQUE_ID(max1_), __UNIQUE_ID(max2_),	\
-	      x, y)
+#define max(x, y) __max(typeof(x), typeof(y), x, y)
 
 /**
  * min3 - return minimum of three values
@@ -869,10 +859,7 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
  * @x: first value
  * @y: second value
  */
-#define min_t(type, x, y)				\
-	__min(type, type,				\
-	      __UNIQUE_ID(min1_), __UNIQUE_ID(min2_),	\
-	      x, y)
+#define min_t(type, x, y) __min(type, type, x, y)
 
 /**
  * max_t - return maximum of two values, using the specified type
@@ -880,10 +867,7 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
  * @x: first value
  * @y: second value
  */
-#define max_t(type, x, y)				\
-	__max(type, type,				\
-	      __UNIQUE_ID(min1_), __UNIQUE_ID(min2_),	\
-	      x, y)
+#define max_t(type, x, y) __max(type, type, x, y)				\
 
 /**
  * clamp_t - return a value clamped to a given range using a given type

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH 0/3] Remove accidental VLA usage
@ 2018-03-08 15:02   ` Josh Poimboeuf
  0 siblings, 0 replies; 32+ messages in thread
From: Josh Poimboeuf @ 2018-03-08 15:02 UTC (permalink / raw)
  To: Kees Cook
  Cc: Andrew Morton, linux-kernel, corbet, gustavo, rostedt,
	Chris Mason, Josef Bacik, David Sterba, David S. Miller,
	Alexey Kuznetsov, Hideaki YOSHIFUJI, Ingo Molnar, Peter Zijlstra,
	Thomas Gleixner, Masahiro Yamada, Borislav Petkov, Randy Dunlap,
	Ian Abbott, Tobin C. Harding, Sergey Senozhatsky, Petr Mladek,
	Andy Shevchenko

On Wed, Mar 07, 2018 at 07:30:44PM -0800, Kees Cook wrote:
> This series adds SIMPLE_MAX() to be used in places where a stack array
> is actually fixed, but the compiler still warns about VLA usage due to
> confusion caused by the safety checks in the max() macro.
> 
> I'm sending these via -mm since that's where I've introduced SIMPLE_MAX(),
> and they should all have no operational differences.

What if we instead simplify the max() macro's type checking so that GCC
can more easily fold the array size constants?  The below patch seems to
work:

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 3fd291503576..ec863726da29 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -782,42 +782,32 @@ ftrace_vprintk(const char *fmt, va_list ap)
 static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
 #endif /* CONFIG_TRACING */
 
-/*
- * min()/max()/clamp() macros that also do
- * strict type-checking.. See the
- * "unnecessary" pointer comparison.
- */
-#define __min(t1, t2, min1, min2, x, y) ({		\
-	t1 min1 = (x);					\
-	t2 min2 = (y);					\
-	(void) (&min1 == &min2);			\
-	min1 < min2 ? min1 : min2; })
+extern long __error_incompatible_types_in_min_macro;
+extern long __error_incompatible_types_in_max_macro;
+
+#define __min(t1, t2, x, y)						\
+	__builtin_choose_expr(__builtin_types_compatible_p(t1, t2),	\
+			      (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y),	\
+			      (t1)__error_incompatible_types_in_min_macro)
 
 /**
  * min - return minimum of two values of the same or compatible types
  * @x: first value
  * @y: second value
  */
-#define min(x, y)					\
-	__min(typeof(x), typeof(y),			\
-	      __UNIQUE_ID(min1_), __UNIQUE_ID(min2_),	\
-	      x, y)
+#define min(x, y) __min(typeof(x), typeof(y), x, y)			\
 
-#define __max(t1, t2, max1, max2, x, y) ({		\
-	t1 max1 = (x);					\
-	t2 max2 = (y);					\
-	(void) (&max1 == &max2);			\
-	max1 > max2 ? max1 : max2; })
+#define __max(t1, t2, x, y)						\
+	__builtin_choose_expr(__builtin_types_compatible_p(t1, t2),	\
+			      (t1)(x) > (t2)(y) ? (t1)(x) : (t2)(y),	\
+			      (t1)__error_incompatible_types_in_max_macro)
 
 /**
  * max - return maximum of two values of the same or compatible types
  * @x: first value
  * @y: second value
  */
-#define max(x, y)					\
-	__max(typeof(x), typeof(y),			\
-	      __UNIQUE_ID(max1_), __UNIQUE_ID(max2_),	\
-	      x, y)
+#define max(x, y) __max(typeof(x), typeof(y), x, y)
 
 /**
  * min3 - return minimum of three values
@@ -869,10 +859,7 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
  * @x: first value
  * @y: second value
  */
-#define min_t(type, x, y)				\
-	__min(type, type,				\
-	      __UNIQUE_ID(min1_), __UNIQUE_ID(min2_),	\
-	      x, y)
+#define min_t(type, x, y) __min(type, type, x, y)
 
 /**
  * max_t - return maximum of two values, using the specified type
@@ -880,10 +867,7 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
  * @x: first value
  * @y: second value
  */
-#define max_t(type, x, y)				\
-	__max(type, type,				\
-	      __UNIQUE_ID(min1_), __UNIQUE_ID(min2_),	\
-	      x, y)
+#define max_t(type, x, y) __max(type, type, x, y)				\
 
 /**
  * clamp_t - return a value clamped to a given range using a given type

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH 0/3] Remove accidental VLA usage
  2018-03-08 15:02   ` Josh Poimboeuf
@ 2018-03-08 18:02     ` Kees Cook
  -1 siblings, 0 replies; 32+ messages in thread
From: Kees Cook @ 2018-03-08 18:02 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Andrew Morton, LKML, Jonathan Corbet, Gustavo A. R. Silva,
	Steven Rostedt, Chris Mason, Josef Bacik, David Sterba,
	David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI,
	Ingo Molnar, Peter Zijlstra, Thomas Gleixner, Masahiro Yamada,
	Borislav Petkov, Randy Dunlap, Ian Abbott, Tobin C. Harding,
	Sergey Senozhatsky, Petr Mladek, Andy Shevchenko,
	Pantelis Antoniou, Linux Btrfs, Network Development,
	Kernel Hardening

On Thu, Mar 8, 2018 at 7:02 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Wed, Mar 07, 2018 at 07:30:44PM -0800, Kees Cook wrote:
>> This series adds SIMPLE_MAX() to be used in places where a stack array
>> is actually fixed, but the compiler still warns about VLA usage due to
>> confusion caused by the safety checks in the max() macro.
>>
>> I'm sending these via -mm since that's where I've introduced SIMPLE_MAX(),
>> and they should all have no operational differences.
>
> What if we instead simplify the max() macro's type checking so that GCC
> can more easily fold the array size constants?  The below patch seems to
> work:

Oooooh magic! Very nice. I couldn't figure out how to do this when I
stared at it. Yes, let me respin. (I assume I can add your S-o-b?)

-Kees

>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 3fd291503576..ec863726da29 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -782,42 +782,32 @@ ftrace_vprintk(const char *fmt, va_list ap)
>  static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
>  #endif /* CONFIG_TRACING */
>
> -/*
> - * min()/max()/clamp() macros that also do
> - * strict type-checking.. See the
> - * "unnecessary" pointer comparison.
> - */
> -#define __min(t1, t2, min1, min2, x, y) ({             \
> -       t1 min1 = (x);                                  \
> -       t2 min2 = (y);                                  \
> -       (void) (&min1 == &min2);                        \
> -       min1 < min2 ? min1 : min2; })
> +extern long __error_incompatible_types_in_min_macro;
> +extern long __error_incompatible_types_in_max_macro;
> +
> +#define __min(t1, t2, x, y)                                            \
> +       __builtin_choose_expr(__builtin_types_compatible_p(t1, t2),     \
> +                             (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y),    \
> +                             (t1)__error_incompatible_types_in_min_macro)
>
>  /**
>   * min - return minimum of two values of the same or compatible types
>   * @x: first value
>   * @y: second value
>   */
> -#define min(x, y)                                      \
> -       __min(typeof(x), typeof(y),                     \
> -             __UNIQUE_ID(min1_), __UNIQUE_ID(min2_),   \
> -             x, y)
> +#define min(x, y) __min(typeof(x), typeof(y), x, y)                    \
>
> -#define __max(t1, t2, max1, max2, x, y) ({             \
> -       t1 max1 = (x);                                  \
> -       t2 max2 = (y);                                  \
> -       (void) (&max1 == &max2);                        \
> -       max1 > max2 ? max1 : max2; })
> +#define __max(t1, t2, x, y)                                            \
> +       __builtin_choose_expr(__builtin_types_compatible_p(t1, t2),     \
> +                             (t1)(x) > (t2)(y) ? (t1)(x) : (t2)(y),    \
> +                             (t1)__error_incompatible_types_in_max_macro)
>
>  /**
>   * max - return maximum of two values of the same or compatible types
>   * @x: first value
>   * @y: second value
>   */
> -#define max(x, y)                                      \
> -       __max(typeof(x), typeof(y),                     \
> -             __UNIQUE_ID(max1_), __UNIQUE_ID(max2_),   \
> -             x, y)
> +#define max(x, y) __max(typeof(x), typeof(y), x, y)
>
>  /**
>   * min3 - return minimum of three values
> @@ -869,10 +859,7 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
>   * @x: first value
>   * @y: second value
>   */
> -#define min_t(type, x, y)                              \
> -       __min(type, type,                               \
> -             __UNIQUE_ID(min1_), __UNIQUE_ID(min2_),   \
> -             x, y)
> +#define min_t(type, x, y) __min(type, type, x, y)
>
>  /**
>   * max_t - return maximum of two values, using the specified type
> @@ -880,10 +867,7 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
>   * @x: first value
>   * @y: second value
>   */
> -#define max_t(type, x, y)                              \
> -       __max(type, type,                               \
> -             __UNIQUE_ID(min1_), __UNIQUE_ID(min2_),   \
> -             x, y)
> +#define max_t(type, x, y) __max(type, type, x, y)                              \
>
>  /**
>   * clamp_t - return a value clamped to a given range using a given type



-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 0/3] Remove accidental VLA usage
@ 2018-03-08 18:02     ` Kees Cook
  0 siblings, 0 replies; 32+ messages in thread
From: Kees Cook @ 2018-03-08 18:02 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Andrew Morton, LKML, Jonathan Corbet, Gustavo A. R. Silva,
	Steven Rostedt, Chris Mason, Josef Bacik, David Sterba,
	David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI,
	Ingo Molnar, Peter Zijlstra, Thomas Gleixner, Masahiro Yamada,
	Borislav Petkov, Randy Dunlap, Ian Abbott, Tobin C. Harding,
	Sergey Senozhatsky

On Thu, Mar 8, 2018 at 7:02 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Wed, Mar 07, 2018 at 07:30:44PM -0800, Kees Cook wrote:
>> This series adds SIMPLE_MAX() to be used in places where a stack array
>> is actually fixed, but the compiler still warns about VLA usage due to
>> confusion caused by the safety checks in the max() macro.
>>
>> I'm sending these via -mm since that's where I've introduced SIMPLE_MAX(),
>> and they should all have no operational differences.
>
> What if we instead simplify the max() macro's type checking so that GCC
> can more easily fold the array size constants?  The below patch seems to
> work:

Oooooh magic! Very nice. I couldn't figure out how to do this when I
stared at it. Yes, let me respin. (I assume I can add your S-o-b?)

-Kees

>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 3fd291503576..ec863726da29 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -782,42 +782,32 @@ ftrace_vprintk(const char *fmt, va_list ap)
>  static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
>  #endif /* CONFIG_TRACING */
>
> -/*
> - * min()/max()/clamp() macros that also do
> - * strict type-checking.. See the
> - * "unnecessary" pointer comparison.
> - */
> -#define __min(t1, t2, min1, min2, x, y) ({             \
> -       t1 min1 = (x);                                  \
> -       t2 min2 = (y);                                  \
> -       (void) (&min1 == &min2);                        \
> -       min1 < min2 ? min1 : min2; })
> +extern long __error_incompatible_types_in_min_macro;
> +extern long __error_incompatible_types_in_max_macro;
> +
> +#define __min(t1, t2, x, y)                                            \
> +       __builtin_choose_expr(__builtin_types_compatible_p(t1, t2),     \
> +                             (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y),    \
> +                             (t1)__error_incompatible_types_in_min_macro)
>
>  /**
>   * min - return minimum of two values of the same or compatible types
>   * @x: first value
>   * @y: second value
>   */
> -#define min(x, y)                                      \
> -       __min(typeof(x), typeof(y),                     \
> -             __UNIQUE_ID(min1_), __UNIQUE_ID(min2_),   \
> -             x, y)
> +#define min(x, y) __min(typeof(x), typeof(y), x, y)                    \
>
> -#define __max(t1, t2, max1, max2, x, y) ({             \
> -       t1 max1 = (x);                                  \
> -       t2 max2 = (y);                                  \
> -       (void) (&max1 == &max2);                        \
> -       max1 > max2 ? max1 : max2; })
> +#define __max(t1, t2, x, y)                                            \
> +       __builtin_choose_expr(__builtin_types_compatible_p(t1, t2),     \
> +                             (t1)(x) > (t2)(y) ? (t1)(x) : (t2)(y),    \
> +                             (t1)__error_incompatible_types_in_max_macro)
>
>  /**
>   * max - return maximum of two values of the same or compatible types
>   * @x: first value
>   * @y: second value
>   */
> -#define max(x, y)                                      \
> -       __max(typeof(x), typeof(y),                     \
> -             __UNIQUE_ID(max1_), __UNIQUE_ID(max2_),   \
> -             x, y)
> +#define max(x, y) __max(typeof(x), typeof(y), x, y)
>
>  /**
>   * min3 - return minimum of three values
> @@ -869,10 +859,7 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
>   * @x: first value
>   * @y: second value
>   */
> -#define min_t(type, x, y)                              \
> -       __min(type, type,                               \
> -             __UNIQUE_ID(min1_), __UNIQUE_ID(min2_),   \
> -             x, y)
> +#define min_t(type, x, y) __min(type, type, x, y)
>
>  /**
>   * max_t - return maximum of two values, using the specified type
> @@ -880,10 +867,7 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
>   * @x: first value
>   * @y: second value
>   */
> -#define max_t(type, x, y)                              \
> -       __max(type, type,                               \
> -             __UNIQUE_ID(min1_), __UNIQUE_ID(min2_),   \
> -             x, y)
> +#define max_t(type, x, y) __max(type, type, x, y)                              \
>
>  /**
>   * clamp_t - return a value clamped to a given range using a given type



-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 0/3] Remove accidental VLA usage
  2018-03-08 15:02   ` Josh Poimboeuf
@ 2018-03-08 18:06     ` Steven Rostedt
  -1 siblings, 0 replies; 32+ messages in thread
From: Steven Rostedt @ 2018-03-08 18:06 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Kees Cook, Andrew Morton, linux-kernel, corbet, gustavo,
	Chris Mason, Josef Bacik, David Sterba, David S. Miller,
	Alexey Kuznetsov, Hideaki YOSHIFUJI, Ingo Molnar, Peter Zijlstra,
	Thomas Gleixner, Masahiro Yamada, Borislav Petkov, Randy Dunlap,
	Ian Abbott, Tobin C. Harding, Sergey Senozhatsky, Petr Mladek,
	Andy Shevchenko, Pantelis Antoniou, linux-btrfs, netdev,
	kernel-hardening

On Thu, 8 Mar 2018 09:02:36 -0600
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Wed, Mar 07, 2018 at 07:30:44PM -0800, Kees Cook wrote:
> > This series adds SIMPLE_MAX() to be used in places where a stack array
> > is actually fixed, but the compiler still warns about VLA usage due to
> > confusion caused by the safety checks in the max() macro.
> > 
> > I'm sending these via -mm since that's where I've introduced SIMPLE_MAX(),
> > and they should all have no operational differences.  
> 
> What if we instead simplify the max() macro's type checking so that GCC
> can more easily fold the array size constants?  The below patch seems to
> work:

Nice. Have you tried to do a allmodconfig and build on various archs?

Of course pushing it to kernel.org will have the zero day bot do it for
you ;-)

-- Steve


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 0/3] Remove accidental VLA usage
@ 2018-03-08 18:06     ` Steven Rostedt
  0 siblings, 0 replies; 32+ messages in thread
From: Steven Rostedt @ 2018-03-08 18:06 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Kees Cook, Andrew Morton, linux-kernel, corbet, gustavo,
	Chris Mason, Josef Bacik, David Sterba, David S. Miller,
	Alexey Kuznetsov, Hideaki YOSHIFUJI, Ingo Molnar, Peter Zijlstra,
	Thomas Gleixner, Masahiro Yamada, Borislav Petkov, Randy Dunlap,
	Ian Abbott, Tobin C. Harding, Sergey Senozhatsky, Petr Mladek,
	Andy

On Thu, 8 Mar 2018 09:02:36 -0600
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Wed, Mar 07, 2018 at 07:30:44PM -0800, Kees Cook wrote:
> > This series adds SIMPLE_MAX() to be used in places where a stack array
> > is actually fixed, but the compiler still warns about VLA usage due to
> > confusion caused by the safety checks in the max() macro.
> > 
> > I'm sending these via -mm since that's where I've introduced SIMPLE_MAX(),
> > and they should all have no operational differences.  
> 
> What if we instead simplify the max() macro's type checking so that GCC
> can more easily fold the array size constants?  The below patch seems to
> work:

Nice. Have you tried to do a allmodconfig and build on various archs?

Of course pushing it to kernel.org will have the zero day bot do it for
you ;-)

-- Steve

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 0/3] Remove accidental VLA usage
  2018-03-08 18:02     ` Kees Cook
@ 2018-03-08 18:11       ` Josh Poimboeuf
  -1 siblings, 0 replies; 32+ messages in thread
From: Josh Poimboeuf @ 2018-03-08 18:11 UTC (permalink / raw)
  To: Kees Cook
  Cc: Andrew Morton, LKML, Jonathan Corbet, Gustavo A. R. Silva,
	Steven Rostedt, Chris Mason, Josef Bacik, David Sterba,
	David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI,
	Ingo Molnar, Peter Zijlstra, Thomas Gleixner, Masahiro Yamada,
	Borislav Petkov, Randy Dunlap, Ian Abbott, Tobin C. Harding,
	Sergey Senozhatsky, Petr Mladek, Andy Shevchenko,
	Pantelis Antoniou, Linux Btrfs, Network Development,
	Kernel Hardening

On Thu, Mar 08, 2018 at 10:02:01AM -0800, Kees Cook wrote:
> On Thu, Mar 8, 2018 at 7:02 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Wed, Mar 07, 2018 at 07:30:44PM -0800, Kees Cook wrote:
> >> This series adds SIMPLE_MAX() to be used in places where a stack array
> >> is actually fixed, but the compiler still warns about VLA usage due to
> >> confusion caused by the safety checks in the max() macro.
> >>
> >> I'm sending these via -mm since that's where I've introduced SIMPLE_MAX(),
> >> and they should all have no operational differences.
> >
> > What if we instead simplify the max() macro's type checking so that GCC
> > can more easily fold the array size constants?  The below patch seems to
> > work:
> 
> Oooooh magic! Very nice. I couldn't figure out how to do this when I
> stared at it. Yes, let me respin. (I assume I can add your S-o-b?)

I'm going to be traveling for a few days, so I bequeath the patch to
you.  You can add my SOB.  I agree with Steve's suggestion to run it
through 0-day.

> 
> -Kees
> 
> >
> > diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> > index 3fd291503576..ec863726da29 100644
> > --- a/include/linux/kernel.h
> > +++ b/include/linux/kernel.h
> > @@ -782,42 +782,32 @@ ftrace_vprintk(const char *fmt, va_list ap)
> >  static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
> >  #endif /* CONFIG_TRACING */
> >
> > -/*
> > - * min()/max()/clamp() macros that also do
> > - * strict type-checking.. See the
> > - * "unnecessary" pointer comparison.
> > - */
> > -#define __min(t1, t2, min1, min2, x, y) ({             \
> > -       t1 min1 = (x);                                  \
> > -       t2 min2 = (y);                                  \
> > -       (void) (&min1 == &min2);                        \
> > -       min1 < min2 ? min1 : min2; })
> > +extern long __error_incompatible_types_in_min_macro;
> > +extern long __error_incompatible_types_in_max_macro;
> > +
> > +#define __min(t1, t2, x, y)                                            \
> > +       __builtin_choose_expr(__builtin_types_compatible_p(t1, t2),     \
> > +                             (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y),    \
> > +                             (t1)__error_incompatible_types_in_min_macro)
> >
> >  /**
> >   * min - return minimum of two values of the same or compatible types
> >   * @x: first value
> >   * @y: second value
> >   */
> > -#define min(x, y)                                      \
> > -       __min(typeof(x), typeof(y),                     \
> > -             __UNIQUE_ID(min1_), __UNIQUE_ID(min2_),   \
> > -             x, y)
> > +#define min(x, y) __min(typeof(x), typeof(y), x, y)                    \
> >
> > -#define __max(t1, t2, max1, max2, x, y) ({             \
> > -       t1 max1 = (x);                                  \
> > -       t2 max2 = (y);                                  \
> > -       (void) (&max1 == &max2);                        \
> > -       max1 > max2 ? max1 : max2; })
> > +#define __max(t1, t2, x, y)                                            \
> > +       __builtin_choose_expr(__builtin_types_compatible_p(t1, t2),     \
> > +                             (t1)(x) > (t2)(y) ? (t1)(x) : (t2)(y),    \
> > +                             (t1)__error_incompatible_types_in_max_macro)
> >
> >  /**
> >   * max - return maximum of two values of the same or compatible types
> >   * @x: first value
> >   * @y: second value
> >   */
> > -#define max(x, y)                                      \
> > -       __max(typeof(x), typeof(y),                     \
> > -             __UNIQUE_ID(max1_), __UNIQUE_ID(max2_),   \
> > -             x, y)
> > +#define max(x, y) __max(typeof(x), typeof(y), x, y)
> >
> >  /**
> >   * min3 - return minimum of three values
> > @@ -869,10 +859,7 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
> >   * @x: first value
> >   * @y: second value
> >   */
> > -#define min_t(type, x, y)                              \
> > -       __min(type, type,                               \
> > -             __UNIQUE_ID(min1_), __UNIQUE_ID(min2_),   \
> > -             x, y)
> > +#define min_t(type, x, y) __min(type, type, x, y)
> >
> >  /**
> >   * max_t - return maximum of two values, using the specified type
> > @@ -880,10 +867,7 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
> >   * @x: first value
> >   * @y: second value
> >   */
> > -#define max_t(type, x, y)                              \
> > -       __max(type, type,                               \
> > -             __UNIQUE_ID(min1_), __UNIQUE_ID(min2_),   \
> > -             x, y)
> > +#define max_t(type, x, y) __max(type, type, x, y)                              \
> >
> >  /**
> >   * clamp_t - return a value clamped to a given range using a given type
> 
> 
> 
> -- 
> Kees Cook
> Pixel Security

-- 
Josh

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 0/3] Remove accidental VLA usage
@ 2018-03-08 18:11       ` Josh Poimboeuf
  0 siblings, 0 replies; 32+ messages in thread
From: Josh Poimboeuf @ 2018-03-08 18:11 UTC (permalink / raw)
  To: Kees Cook
  Cc: Andrew Morton, LKML, Jonathan Corbet, Gustavo A. R. Silva,
	Steven Rostedt, Chris Mason, Josef Bacik, David Sterba,
	David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI,
	Ingo Molnar, Peter Zijlstra, Thomas Gleixner, Masahiro Yamada,
	Borislav Petkov, Randy Dunlap, Ian Abbott, Tobin C. Harding,
	Sergey Senozhatsky

On Thu, Mar 08, 2018 at 10:02:01AM -0800, Kees Cook wrote:
> On Thu, Mar 8, 2018 at 7:02 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Wed, Mar 07, 2018 at 07:30:44PM -0800, Kees Cook wrote:
> >> This series adds SIMPLE_MAX() to be used in places where a stack array
> >> is actually fixed, but the compiler still warns about VLA usage due to
> >> confusion caused by the safety checks in the max() macro.
> >>
> >> I'm sending these via -mm since that's where I've introduced SIMPLE_MAX(),
> >> and they should all have no operational differences.
> >
> > What if we instead simplify the max() macro's type checking so that GCC
> > can more easily fold the array size constants?  The below patch seems to
> > work:
> 
> Oooooh magic! Very nice. I couldn't figure out how to do this when I
> stared at it. Yes, let me respin. (I assume I can add your S-o-b?)

I'm going to be traveling for a few days, so I bequeath the patch to
you.  You can add my SOB.  I agree with Steve's suggestion to run it
through 0-day.

> 
> -Kees
> 
> >
> > diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> > index 3fd291503576..ec863726da29 100644
> > --- a/include/linux/kernel.h
> > +++ b/include/linux/kernel.h
> > @@ -782,42 +782,32 @@ ftrace_vprintk(const char *fmt, va_list ap)
> >  static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
> >  #endif /* CONFIG_TRACING */
> >
> > -/*
> > - * min()/max()/clamp() macros that also do
> > - * strict type-checking.. See the
> > - * "unnecessary" pointer comparison.
> > - */
> > -#define __min(t1, t2, min1, min2, x, y) ({             \
> > -       t1 min1 = (x);                                  \
> > -       t2 min2 = (y);                                  \
> > -       (void) (&min1 == &min2);                        \
> > -       min1 < min2 ? min1 : min2; })
> > +extern long __error_incompatible_types_in_min_macro;
> > +extern long __error_incompatible_types_in_max_macro;
> > +
> > +#define __min(t1, t2, x, y)                                            \
> > +       __builtin_choose_expr(__builtin_types_compatible_p(t1, t2),     \
> > +                             (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y),    \
> > +                             (t1)__error_incompatible_types_in_min_macro)
> >
> >  /**
> >   * min - return minimum of two values of the same or compatible types
> >   * @x: first value
> >   * @y: second value
> >   */
> > -#define min(x, y)                                      \
> > -       __min(typeof(x), typeof(y),                     \
> > -             __UNIQUE_ID(min1_), __UNIQUE_ID(min2_),   \
> > -             x, y)
> > +#define min(x, y) __min(typeof(x), typeof(y), x, y)                    \
> >
> > -#define __max(t1, t2, max1, max2, x, y) ({             \
> > -       t1 max1 = (x);                                  \
> > -       t2 max2 = (y);                                  \
> > -       (void) (&max1 == &max2);                        \
> > -       max1 > max2 ? max1 : max2; })
> > +#define __max(t1, t2, x, y)                                            \
> > +       __builtin_choose_expr(__builtin_types_compatible_p(t1, t2),     \
> > +                             (t1)(x) > (t2)(y) ? (t1)(x) : (t2)(y),    \
> > +                             (t1)__error_incompatible_types_in_max_macro)
> >
> >  /**
> >   * max - return maximum of two values of the same or compatible types
> >   * @x: first value
> >   * @y: second value
> >   */
> > -#define max(x, y)                                      \
> > -       __max(typeof(x), typeof(y),                     \
> > -             __UNIQUE_ID(max1_), __UNIQUE_ID(max2_),   \
> > -             x, y)
> > +#define max(x, y) __max(typeof(x), typeof(y), x, y)
> >
> >  /**
> >   * min3 - return minimum of three values
> > @@ -869,10 +859,7 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
> >   * @x: first value
> >   * @y: second value
> >   */
> > -#define min_t(type, x, y)                              \
> > -       __min(type, type,                               \
> > -             __UNIQUE_ID(min1_), __UNIQUE_ID(min2_),   \
> > -             x, y)
> > +#define min_t(type, x, y) __min(type, type, x, y)
> >
> >  /**
> >   * max_t - return maximum of two values, using the specified type
> > @@ -880,10 +867,7 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
> >   * @x: first value
> >   * @y: second value
> >   */
> > -#define max_t(type, x, y)                              \
> > -       __max(type, type,                               \
> > -             __UNIQUE_ID(min1_), __UNIQUE_ID(min2_),   \
> > -             x, y)
> > +#define max_t(type, x, y) __max(type, type, x, y)                              \
> >
> >  /**
> >   * clamp_t - return a value clamped to a given range using a given type
> 
> 
> 
> -- 
> Kees Cook
> Pixel Security

-- 
Josh

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 0/3] Remove accidental VLA usage
  2018-03-08 15:02   ` Josh Poimboeuf
@ 2018-03-08 19:57     ` Rasmus Villemoes
  -1 siblings, 0 replies; 32+ messages in thread
From: Rasmus Villemoes @ 2018-03-08 19:57 UTC (permalink / raw)
  To: Josh Poimboeuf, Kees Cook
  Cc: Andrew Morton, linux-kernel, corbet, gustavo, rostedt,
	Chris Mason, Josef Bacik, David Sterba, David S. Miller,
	Alexey Kuznetsov, Hideaki YOSHIFUJI, Ingo Molnar, Peter Zijlstra,
	Thomas Gleixner, Masahiro Yamada, Borislav Petkov, Randy Dunlap,
	Ian Abbott, Tobin C. Harding, Sergey Senozhatsky, Petr Mladek,
	Andy Shevchenko, Pantelis Antoniou, linux-btrfs, netdev,
	kernel-hardening

On 2018-03-08 16:02, Josh Poimboeuf wrote:
> On Wed, Mar 07, 2018 at 07:30:44PM -0800, Kees Cook wrote:
>> This series adds SIMPLE_MAX() to be used in places where a stack array
>> is actually fixed, but the compiler still warns about VLA usage due to
>> confusion caused by the safety checks in the max() macro.
>>
>> I'm sending these via -mm since that's where I've introduced SIMPLE_MAX(),
>> and they should all have no operational differences.
> 
> What if we instead simplify the max() macro's type checking so that GCC
> can more easily fold the array size constants?  The below patch seems to
> work:
> 

> +extern long __error_incompatible_types_in_min_macro;
> +extern long __error_incompatible_types_in_max_macro;
> +
> +#define __min(t1, t2, x, y)						\
> +	__builtin_choose_expr(__builtin_types_compatible_p(t1, t2),	\
> +			      (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y),	\
> +			      (t1)__error_incompatible_types_in_min_macro)
>  
>  /**
>   * min - return minimum of two values of the same or compatible types
>   * @x: first value
>   * @y: second value
>   */
> -#define min(x, y)					\
> -	__min(typeof(x), typeof(y),			\
> -	      __UNIQUE_ID(min1_), __UNIQUE_ID(min2_),	\
> -	      x, y)
> +#define min(x, y) __min(typeof(x), typeof(y), x, y)			\
>  

But this introduces the the-chosen-one-of-x-and-y-gets-evaluated-twice
problem. Maybe we don't care? But until we get a
__builtin_assert_this_has_no_side_effects() I think that's a little
dangerous.

Rasmus

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 0/3] Remove accidental VLA usage
@ 2018-03-08 19:57     ` Rasmus Villemoes
  0 siblings, 0 replies; 32+ messages in thread
From: Rasmus Villemoes @ 2018-03-08 19:57 UTC (permalink / raw)
  To: Josh Poimboeuf, Kees Cook
  Cc: Andrew Morton, linux-kernel, corbet, gustavo, rostedt,
	Chris Mason, Josef Bacik, David Sterba, David S. Miller,
	Alexey Kuznetsov, Hideaki YOSHIFUJI, Ingo Molnar, Peter Zijlstra,
	Thomas Gleixner, Masahiro Yamada, Borislav Petkov, Randy Dunlap,
	Ian Abbott, Tobin C. Harding, Sergey Senozhatsky, Petr Mladek,
	Andy Shevchenko

On 2018-03-08 16:02, Josh Poimboeuf wrote:
> On Wed, Mar 07, 2018 at 07:30:44PM -0800, Kees Cook wrote:
>> This series adds SIMPLE_MAX() to be used in places where a stack array
>> is actually fixed, but the compiler still warns about VLA usage due to
>> confusion caused by the safety checks in the max() macro.
>>
>> I'm sending these via -mm since that's where I've introduced SIMPLE_MAX(),
>> and they should all have no operational differences.
> 
> What if we instead simplify the max() macro's type checking so that GCC
> can more easily fold the array size constants?  The below patch seems to
> work:
> 

> +extern long __error_incompatible_types_in_min_macro;
> +extern long __error_incompatible_types_in_max_macro;
> +
> +#define __min(t1, t2, x, y)						\
> +	__builtin_choose_expr(__builtin_types_compatible_p(t1, t2),	\
> +			      (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y),	\
> +			      (t1)__error_incompatible_types_in_min_macro)
>  
>  /**
>   * min - return minimum of two values of the same or compatible types
>   * @x: first value
>   * @y: second value
>   */
> -#define min(x, y)					\
> -	__min(typeof(x), typeof(y),			\
> -	      __UNIQUE_ID(min1_), __UNIQUE_ID(min2_),	\
> -	      x, y)
> +#define min(x, y) __min(typeof(x), typeof(y), x, y)			\
>  

But this introduces the the-chosen-one-of-x-and-y-gets-evaluated-twice
problem. Maybe we don't care? But until we get a
__builtin_assert_this_has_no_side_effects() I think that's a little
dangerous.

Rasmus

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 0/3] Remove accidental VLA usage
  2018-03-08 19:57     ` Rasmus Villemoes
@ 2018-03-08 20:39       ` Kees Cook
  -1 siblings, 0 replies; 32+ messages in thread
From: Kees Cook @ 2018-03-08 20:39 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: Josh Poimboeuf, Andrew Morton, LKML, Jonathan Corbet,
	Gustavo A. R. Silva, Steven Rostedt, Chris Mason, Josef Bacik,
	David Sterba, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, Ingo Molnar, Peter Zijlstra, Thomas Gleixner,
	Masahiro Yamada, Borislav Petkov, Randy Dunlap, Ian Abbott,
	Tobin C. Harding, Sergey Senozhatsky, Petr Mladek,
	Andy Shevchenko, Pantelis Antoniou, Linux Btrfs,
	Network Development, Kernel Hardening

On Thu, Mar 8, 2018 at 11:57 AM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> On 2018-03-08 16:02, Josh Poimboeuf wrote:
>> On Wed, Mar 07, 2018 at 07:30:44PM -0800, Kees Cook wrote:
>>> This series adds SIMPLE_MAX() to be used in places where a stack array
>>> is actually fixed, but the compiler still warns about VLA usage due to
>>> confusion caused by the safety checks in the max() macro.
>>>
>>> I'm sending these via -mm since that's where I've introduced SIMPLE_MAX(),
>>> and they should all have no operational differences.
>>
>> What if we instead simplify the max() macro's type checking so that GCC
>> can more easily fold the array size constants?  The below patch seems to
>> work:
>>
>
>> +extern long __error_incompatible_types_in_min_macro;
>> +extern long __error_incompatible_types_in_max_macro;
>> +
>> +#define __min(t1, t2, x, y)                                          \
>> +     __builtin_choose_expr(__builtin_types_compatible_p(t1, t2),     \
>> +                           (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y),    \
>> +                           (t1)__error_incompatible_types_in_min_macro)
>>
>>  /**
>>   * min - return minimum of two values of the same or compatible types
>>   * @x: first value
>>   * @y: second value
>>   */
>> -#define min(x, y)                                    \
>> -     __min(typeof(x), typeof(y),                     \
>> -           __UNIQUE_ID(min1_), __UNIQUE_ID(min2_),   \
>> -           x, y)
>> +#define min(x, y) __min(typeof(x), typeof(y), x, y)                  \
>>
>
> But this introduces the the-chosen-one-of-x-and-y-gets-evaluated-twice
> problem. Maybe we don't care? But until we get a
> __builtin_assert_this_has_no_side_effects() I think that's a little
> dangerous.

Eek, yes, we can't do the double-eval. The proposed change breaks
things badly. :)

a:   20
b:   40
max(a++, b++): 40
a:   21
b:   41

a:   20
b:   40
new_max(a++, b++): 41
a:   21
b:   42

However, this works for me:

#define __new_max(t1, t2, max1, max2, x, y)                    \
       __builtin_choose_expr(__builtin_constant_p(x) && \
                             __builtin_constant_p(y) && \
                             __builtin_types_compatible_p(t1, t2),     \
                             (t1)(x) > (t2)(y) ? (t1)(x) : (t2)(y),    \
                             __max(t1, t2, max1, max2, x, y))

#define new_max(x, y) \
        __new_max(typeof(x), typeof(y),                 \
              __UNIQUE_ID(max1_), __UNIQUE_ID(max2_),   \
              x, y)

(pardon the whitespace damage...)

Let me spin a sane patch and test it...

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 0/3] Remove accidental VLA usage
@ 2018-03-08 20:39       ` Kees Cook
  0 siblings, 0 replies; 32+ messages in thread
From: Kees Cook @ 2018-03-08 20:39 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: Josh Poimboeuf, Andrew Morton, LKML, Jonathan Corbet,
	Gustavo A. R. Silva, Steven Rostedt, Chris Mason, Josef Bacik,
	David Sterba, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, Ingo Molnar, Peter Zijlstra, Thomas Gleixner,
	Masahiro Yamada, Borislav Petkov, Randy Dunlap, Ian Abbott,
	Tobin C. Harding, Sergey

On Thu, Mar 8, 2018 at 11:57 AM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> On 2018-03-08 16:02, Josh Poimboeuf wrote:
>> On Wed, Mar 07, 2018 at 07:30:44PM -0800, Kees Cook wrote:
>>> This series adds SIMPLE_MAX() to be used in places where a stack array
>>> is actually fixed, but the compiler still warns about VLA usage due to
>>> confusion caused by the safety checks in the max() macro.
>>>
>>> I'm sending these via -mm since that's where I've introduced SIMPLE_MAX(),
>>> and they should all have no operational differences.
>>
>> What if we instead simplify the max() macro's type checking so that GCC
>> can more easily fold the array size constants?  The below patch seems to
>> work:
>>
>
>> +extern long __error_incompatible_types_in_min_macro;
>> +extern long __error_incompatible_types_in_max_macro;
>> +
>> +#define __min(t1, t2, x, y)                                          \
>> +     __builtin_choose_expr(__builtin_types_compatible_p(t1, t2),     \
>> +                           (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y),    \
>> +                           (t1)__error_incompatible_types_in_min_macro)
>>
>>  /**
>>   * min - return minimum of two values of the same or compatible types
>>   * @x: first value
>>   * @y: second value
>>   */
>> -#define min(x, y)                                    \
>> -     __min(typeof(x), typeof(y),                     \
>> -           __UNIQUE_ID(min1_), __UNIQUE_ID(min2_),   \
>> -           x, y)
>> +#define min(x, y) __min(typeof(x), typeof(y), x, y)                  \
>>
>
> But this introduces the the-chosen-one-of-x-and-y-gets-evaluated-twice
> problem. Maybe we don't care? But until we get a
> __builtin_assert_this_has_no_side_effects() I think that's a little
> dangerous.

Eek, yes, we can't do the double-eval. The proposed change breaks
things badly. :)

a:   20
b:   40
max(a++, b++): 40
a:   21
b:   41

a:   20
b:   40
new_max(a++, b++): 41
a:   21
b:   42

However, this works for me:

#define __new_max(t1, t2, max1, max2, x, y)                    \
       __builtin_choose_expr(__builtin_constant_p(x) && \
                             __builtin_constant_p(y) && \
                             __builtin_types_compatible_p(t1, t2),     \
                             (t1)(x) > (t2)(y) ? (t1)(x) : (t2)(y),    \
                             __max(t1, t2, max1, max2, x, y))

#define new_max(x, y) \
        __new_max(typeof(x), typeof(y),                 \
              __UNIQUE_ID(max1_), __UNIQUE_ID(max2_),   \
              x, y)

(pardon the whitespace damage...)

Let me spin a sane patch and test it...

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 0/3] Remove accidental VLA usage
  2018-03-08 15:02   ` Josh Poimboeuf
@ 2018-03-08 20:49     ` Andrew Morton
  -1 siblings, 0 replies; 32+ messages in thread
From: Andrew Morton @ 2018-03-08 20:49 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Kees Cook, linux-kernel, corbet, gustavo, rostedt, Chris Mason,
	Josef Bacik, David Sterba, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, Ingo Molnar, Peter Zijlstra, Thomas Gleixner,
	Masahiro Yamada, Borislav Petkov, Randy Dunlap, Ian Abbott,
	Tobin C. Harding, Sergey Senozhatsky, Petr Mladek,
	Andy Shevchenko, Pantelis Antoniou, linux-btrfs, netdev,
	kernel-hardening

On Thu, 8 Mar 2018 09:02:36 -0600 Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Wed, Mar 07, 2018 at 07:30:44PM -0800, Kees Cook wrote:
> > This series adds SIMPLE_MAX() to be used in places where a stack array
> > is actually fixed, but the compiler still warns about VLA usage due to
> > confusion caused by the safety checks in the max() macro.
> > 
> > I'm sending these via -mm since that's where I've introduced SIMPLE_MAX(),
> > and they should all have no operational differences.
> 
> What if we instead simplify the max() macro's type checking so that GCC
> can more easily fold the array size constants?  The below patch seems to
> work:
> 
> -/*
> - * min()/max()/clamp() macros that also do
> - * strict type-checking.. See the
> - * "unnecessary" pointer comparison.
> - */
> -#define __min(t1, t2, min1, min2, x, y) ({		\
> -	t1 min1 = (x);					\
> -	t2 min2 = (y);					\
> -	(void) (&min1 == &min2);			\
> -	min1 < min2 ? min1 : min2; })
> +extern long __error_incompatible_types_in_min_macro;
> +extern long __error_incompatible_types_in_max_macro;
> +
> +#define __min(t1, t2, x, y)						\
> +	__builtin_choose_expr(__builtin_types_compatible_p(t1, t2),	\
> +			      (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y),	\
> +			      (t1)__error_incompatible_types_in_min_macro)

This will move the error detection from compile-time to link-time. 
That's tolerable I guess, but a bit sad and should be flagged in the
changelog at least.


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 0/3] Remove accidental VLA usage
@ 2018-03-08 20:49     ` Andrew Morton
  0 siblings, 0 replies; 32+ messages in thread
From: Andrew Morton @ 2018-03-08 20:49 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Kees Cook, linux-kernel, corbet, gustavo, rostedt, Chris Mason,
	Josef Bacik, David Sterba, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, Ingo Molnar, Peter Zijlstra, Thomas Gleixner,
	Masahiro Yamada, Borislav Petkov, Randy Dunlap, Ian Abbott,
	Tobin C. Harding, Sergey Senozhatsky, Petr Mladek,
	Andy Shevchenko

On Thu, 8 Mar 2018 09:02:36 -0600 Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Wed, Mar 07, 2018 at 07:30:44PM -0800, Kees Cook wrote:
> > This series adds SIMPLE_MAX() to be used in places where a stack array
> > is actually fixed, but the compiler still warns about VLA usage due to
> > confusion caused by the safety checks in the max() macro.
> > 
> > I'm sending these via -mm since that's where I've introduced SIMPLE_MAX(),
> > and they should all have no operational differences.
> 
> What if we instead simplify the max() macro's type checking so that GCC
> can more easily fold the array size constants?  The below patch seems to
> work:
> 
> -/*
> - * min()/max()/clamp() macros that also do
> - * strict type-checking.. See the
> - * "unnecessary" pointer comparison.
> - */
> -#define __min(t1, t2, min1, min2, x, y) ({		\
> -	t1 min1 = (x);					\
> -	t2 min2 = (y);					\
> -	(void) (&min1 == &min2);			\
> -	min1 < min2 ? min1 : min2; })
> +extern long __error_incompatible_types_in_min_macro;
> +extern long __error_incompatible_types_in_max_macro;
> +
> +#define __min(t1, t2, x, y)						\
> +	__builtin_choose_expr(__builtin_types_compatible_p(t1, t2),	\
> +			      (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y),	\
> +			      (t1)__error_incompatible_types_in_min_macro)

This will move the error detection from compile-time to link-time. 
That's tolerable I guess, but a bit sad and should be flagged in the
changelog at least.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 0/3] Remove accidental VLA usage
  2018-03-08 20:39       ` Kees Cook
@ 2018-03-08 22:12         ` Rasmus Villemoes
  -1 siblings, 0 replies; 32+ messages in thread
From: Rasmus Villemoes @ 2018-03-08 22:12 UTC (permalink / raw)
  To: Kees Cook
  Cc: Josh Poimboeuf, Andrew Morton, LKML, Jonathan Corbet,
	Gustavo A. R. Silva, Steven Rostedt, Chris Mason, Josef Bacik,
	David Sterba, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, Ingo Molnar, Peter Zijlstra, Thomas Gleixner,
	Masahiro Yamada, Borislav Petkov, Randy Dunlap, Ian Abbott,
	Tobin C. Harding, Sergey Senozhatsky, Petr Mladek,
	Andy Shevchenko, Pantelis Antoniou, Linux Btrfs,
	Network Development, Kernel Hardening

On 8 March 2018 at 21:39, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Mar 8, 2018 at 11:57 AM, Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>> On 2018-03-08 16:02, Josh Poimboeuf wrote:
>>> On Wed, Mar 07, 2018 at 07:30:44PM -0800, Kees Cook wrote:
>>> +extern long __error_incompatible_types_in_min_macro;
>>> +extern long __error_incompatible_types_in_max_macro;
>>> +
>>> +#define __min(t1, t2, x, y)                                          \
>>> +     __builtin_choose_expr(__builtin_types_compatible_p(t1, t2),     \
>>> +                           (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y),    \
>>> +                           (t1)__error_incompatible_types_in_min_macro)
>>>
>>>  /**
>>>   * min - return minimum of two values of the same or compatible types
>>>   * @x: first value
>>>   * @y: second value
>>>   */
>>> -#define min(x, y)                                    \
>>> -     __min(typeof(x), typeof(y),                     \
>>> -           __UNIQUE_ID(min1_), __UNIQUE_ID(min2_),   \
>>> -           x, y)
>>> +#define min(x, y) __min(typeof(x), typeof(y), x, y)                  \
>>>
>>
>> But this introduces the the-chosen-one-of-x-and-y-gets-evaluated-twice
>> problem. Maybe we don't care? But until we get a
>> __builtin_assert_this_has_no_side_effects() I think that's a little
>> dangerous.
>
> Eek, yes, we can't do the double-eval. The proposed change breaks
> things badly. :)
>
> a:   20
> b:   40
> max(a++, b++): 40
> a:   21
> b:   41
>
> a:   20
> b:   40
> new_max(a++, b++): 41
> a:   21
> b:   42
>
> However, this works for me:
>
> #define __new_max(t1, t2, max1, max2, x, y)                    \
>        __builtin_choose_expr(__builtin_constant_p(x) && \
>                              __builtin_constant_p(y) && \
>                              __builtin_types_compatible_p(t1, t2),     \
>                              (t1)(x) > (t2)(y) ? (t1)(x) : (t2)(y),    \
>                              __max(t1, t2, max1, max2, x, y))
>
> #define new_max(x, y) \
>         __new_max(typeof(x), typeof(y),                 \
>               __UNIQUE_ID(max1_), __UNIQUE_ID(max2_),   \
>               x, y)

Yes, that would seem to do the trick.

Thinking out loud: do we really want or need the
__builtin_types_compatible condition when x and y are compile-time
constants? I think it would be nice to be able to use max(16,
sizeof(bla)) without having to cast either the literal or the sizeof.
Just omitting the type compatibility check might be dangerous, but
perhaps it could be relaxed to a check that both values are
representable in their common promoted type. Something like

(type_signed(t1) == type_signed(t2)) || ((t1)x >= 0 && (t2)y >= 0)

should be safe (if the types have same signedness, or the value of
signed type is positive), though it doesn't allow a few corner cases
(e.g. short vs. unsigned char is always ok due to promotion to int,
and also if the signed type is strictly wider than the unsigned type).

Rasmus

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 0/3] Remove accidental VLA usage
@ 2018-03-08 22:12         ` Rasmus Villemoes
  0 siblings, 0 replies; 32+ messages in thread
From: Rasmus Villemoes @ 2018-03-08 22:12 UTC (permalink / raw)
  To: Kees Cook
  Cc: Josh Poimboeuf, Andrew Morton, LKML, Jonathan Corbet,
	Gustavo A. R. Silva, Steven Rostedt, Chris Mason, Josef Bacik,
	David Sterba, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, Ingo Molnar, Peter Zijlstra, Thomas Gleixner,
	Masahiro Yamada, Borislav Petkov, Randy Dunlap, Ian Abbott,
	Tobin C. Harding, Sergey

On 8 March 2018 at 21:39, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Mar 8, 2018 at 11:57 AM, Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>> On 2018-03-08 16:02, Josh Poimboeuf wrote:
>>> On Wed, Mar 07, 2018 at 07:30:44PM -0800, Kees Cook wrote:
>>> +extern long __error_incompatible_types_in_min_macro;
>>> +extern long __error_incompatible_types_in_max_macro;
>>> +
>>> +#define __min(t1, t2, x, y)                                          \
>>> +     __builtin_choose_expr(__builtin_types_compatible_p(t1, t2),     \
>>> +                           (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y),    \
>>> +                           (t1)__error_incompatible_types_in_min_macro)
>>>
>>>  /**
>>>   * min - return minimum of two values of the same or compatible types
>>>   * @x: first value
>>>   * @y: second value
>>>   */
>>> -#define min(x, y)                                    \
>>> -     __min(typeof(x), typeof(y),                     \
>>> -           __UNIQUE_ID(min1_), __UNIQUE_ID(min2_),   \
>>> -           x, y)
>>> +#define min(x, y) __min(typeof(x), typeof(y), x, y)                  \
>>>
>>
>> But this introduces the the-chosen-one-of-x-and-y-gets-evaluated-twice
>> problem. Maybe we don't care? But until we get a
>> __builtin_assert_this_has_no_side_effects() I think that's a little
>> dangerous.
>
> Eek, yes, we can't do the double-eval. The proposed change breaks
> things badly. :)
>
> a:   20
> b:   40
> max(a++, b++): 40
> a:   21
> b:   41
>
> a:   20
> b:   40
> new_max(a++, b++): 41
> a:   21
> b:   42
>
> However, this works for me:
>
> #define __new_max(t1, t2, max1, max2, x, y)                    \
>        __builtin_choose_expr(__builtin_constant_p(x) && \
>                              __builtin_constant_p(y) && \
>                              __builtin_types_compatible_p(t1, t2),     \
>                              (t1)(x) > (t2)(y) ? (t1)(x) : (t2)(y),    \
>                              __max(t1, t2, max1, max2, x, y))
>
> #define new_max(x, y) \
>         __new_max(typeof(x), typeof(y),                 \
>               __UNIQUE_ID(max1_), __UNIQUE_ID(max2_),   \
>               x, y)

Yes, that would seem to do the trick.

Thinking out loud: do we really want or need the
__builtin_types_compatible condition when x and y are compile-time
constants? I think it would be nice to be able to use max(16,
sizeof(bla)) without having to cast either the literal or the sizeof.
Just omitting the type compatibility check might be dangerous, but
perhaps it could be relaxed to a check that both values are
representable in their common promoted type. Something like

(type_signed(t1) == type_signed(t2)) || ((t1)x >= 0 && (t2)y >= 0)

should be safe (if the types have same signedness, or the value of
signed type is positive), though it doesn't allow a few corner cases
(e.g. short vs. unsigned char is always ok due to promotion to int,
and also if the signed type is strictly wider than the unsigned type).

Rasmus

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 0/3] Remove accidental VLA usage
  2018-03-08 22:12         ` Rasmus Villemoes
@ 2018-03-08 23:33           ` Kees Cook
  -1 siblings, 0 replies; 32+ messages in thread
From: Kees Cook @ 2018-03-08 23:33 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: Josh Poimboeuf, Andrew Morton, LKML, Jonathan Corbet,
	Gustavo A. R. Silva, Steven Rostedt, Chris Mason, Josef Bacik,
	David Sterba, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, Ingo Molnar, Peter Zijlstra, Thomas Gleixner,
	Masahiro Yamada, Borislav Petkov, Randy Dunlap, Ian Abbott,
	Tobin C. Harding, Sergey Senozhatsky, Petr Mladek,
	Andy Shevchenko, Pantelis Antoniou, Linux Btrfs,
	Network Development, Kernel Hardening

On Thu, Mar 8, 2018 at 2:12 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> On 8 March 2018 at 21:39, Kees Cook <keescook@chromium.org> wrote:
>> However, this works for me:
>>
>> #define __new_max(t1, t2, max1, max2, x, y)                    \
>>        __builtin_choose_expr(__builtin_constant_p(x) && \
>>                              __builtin_constant_p(y) && \
>>                              __builtin_types_compatible_p(t1, t2),     \
>>                              (t1)(x) > (t2)(y) ? (t1)(x) : (t2)(y),    \
>>                              __max(t1, t2, max1, max2, x, y))
>>
>> #define new_max(x, y) \
>>         __new_max(typeof(x), typeof(y),                 \
>>               __UNIQUE_ID(max1_), __UNIQUE_ID(max2_),   \
>>               x, y)
>
> Yes, that would seem to do the trick.
>
> Thinking out loud: do we really want or need the
> __builtin_types_compatible condition when x and y are compile-time
> constants? I think it would be nice to be able to use max(16,
> sizeof(bla)) without having to cast either the literal or the sizeof.
> Just omitting the type compatibility check might be dangerous, but
> perhaps it could be relaxed to a check that both values are
> representable in their common promoted type. Something like
>
> (type_signed(t1) == type_signed(t2)) || ((t1)x >= 0 && (t2)y >= 0)
>
> should be safe (if the types have same signedness, or the value of
> signed type is positive), though it doesn't allow a few corner cases
> (e.g. short vs. unsigned char is always ok due to promotion to int,
> and also if the signed type is strictly wider than the unsigned type).

I agree, it would be nice. However, I think it'd be better to continue
to depend on max_t() for these kinds of cases though. For example:

char foo[max_t(size_t, 6, sizeof(something))];

Works with the proposed patch.

Also, I think this mismatch would already be triggering warnings, so
we shouldn't have any currently.

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 0/3] Remove accidental VLA usage
@ 2018-03-08 23:33           ` Kees Cook
  0 siblings, 0 replies; 32+ messages in thread
From: Kees Cook @ 2018-03-08 23:33 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: Josh Poimboeuf, Andrew Morton, LKML, Jonathan Corbet,
	Gustavo A. R. Silva, Steven Rostedt, Chris Mason, Josef Bacik,
	David Sterba, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, Ingo Molnar, Peter Zijlstra, Thomas Gleixner,
	Masahiro Yamada, Borislav Petkov, Randy Dunlap, Ian Abbott,
	Tobin C. Harding, Sergey

On Thu, Mar 8, 2018 at 2:12 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> On 8 March 2018 at 21:39, Kees Cook <keescook@chromium.org> wrote:
>> However, this works for me:
>>
>> #define __new_max(t1, t2, max1, max2, x, y)                    \
>>        __builtin_choose_expr(__builtin_constant_p(x) && \
>>                              __builtin_constant_p(y) && \
>>                              __builtin_types_compatible_p(t1, t2),     \
>>                              (t1)(x) > (t2)(y) ? (t1)(x) : (t2)(y),    \
>>                              __max(t1, t2, max1, max2, x, y))
>>
>> #define new_max(x, y) \
>>         __new_max(typeof(x), typeof(y),                 \
>>               __UNIQUE_ID(max1_), __UNIQUE_ID(max2_),   \
>>               x, y)
>
> Yes, that would seem to do the trick.
>
> Thinking out loud: do we really want or need the
> __builtin_types_compatible condition when x and y are compile-time
> constants? I think it would be nice to be able to use max(16,
> sizeof(bla)) without having to cast either the literal or the sizeof.
> Just omitting the type compatibility check might be dangerous, but
> perhaps it could be relaxed to a check that both values are
> representable in their common promoted type. Something like
>
> (type_signed(t1) == type_signed(t2)) || ((t1)x >= 0 && (t2)y >= 0)
>
> should be safe (if the types have same signedness, or the value of
> signed type is positive), though it doesn't allow a few corner cases
> (e.g. short vs. unsigned char is always ok due to promotion to int,
> and also if the signed type is strictly wider than the unsigned type).

I agree, it would be nice. However, I think it'd be better to continue
to depend on max_t() for these kinds of cases though. For example:

char foo[max_t(size_t, 6, sizeof(something))];

Works with the proposed patch.

Also, I think this mismatch would already be triggering warnings, so
we shouldn't have any currently.

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 32+ messages in thread

end of thread, other threads:[~2018-03-08 23:33 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-08  3:30 [PATCH 0/3] Remove accidental VLA usage Kees Cook
2018-03-08  3:30 ` Kees Cook
2018-03-08  3:30 ` [PATCH v2 1/3] vsprintf: " Kees Cook
2018-03-08  3:30   ` Kees Cook
2018-03-08  8:25   ` Rasmus Villemoes
2018-03-08  8:25     ` Rasmus Villemoes
2018-03-08 11:21     ` Thomas Gleixner
2018-03-08 11:21       ` Thomas Gleixner
2018-03-08  3:30 ` [PATCH 2/3] net: Remove accidental VLAs from proc buffers Kees Cook
2018-03-08  3:30   ` Kees Cook
2018-03-08  3:30 ` [PATCH 3/3] btrfs: tree-checker: Avoid accidental stack VLA Kees Cook
2018-03-08  3:30   ` Kees Cook
2018-03-08 11:33   ` David Sterba
2018-03-08 11:33     ` David Sterba
2018-03-08 15:02 ` [PATCH 0/3] Remove accidental VLA usage Josh Poimboeuf
2018-03-08 15:02   ` Josh Poimboeuf
2018-03-08 18:02   ` Kees Cook
2018-03-08 18:02     ` Kees Cook
2018-03-08 18:11     ` Josh Poimboeuf
2018-03-08 18:11       ` Josh Poimboeuf
2018-03-08 18:06   ` Steven Rostedt
2018-03-08 18:06     ` Steven Rostedt
2018-03-08 19:57   ` Rasmus Villemoes
2018-03-08 19:57     ` Rasmus Villemoes
2018-03-08 20:39     ` Kees Cook
2018-03-08 20:39       ` Kees Cook
2018-03-08 22:12       ` Rasmus Villemoes
2018-03-08 22:12         ` Rasmus Villemoes
2018-03-08 23:33         ` Kees Cook
2018-03-08 23:33           ` Kees Cook
2018-03-08 20:49   ` Andrew Morton
2018-03-08 20:49     ` Andrew Morton

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.