Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Tue, Mar 20, 2018 at 4:23 PM, Linus Torvalds <torva...@linux-foundation.org> wrote: > On Sat, Mar 17, 2018 at 1:07 PM, Kees Cook <keesc...@chromium.org> wrote: >> >> No luck! :( gcc 4.4 refuses to play along. And, hilariously, not only >> does it not change the complaint about __builtin_choose_expr(), it >> also thinks that's a VLA now. > > Hmm. So thanks to the diseased mind of Martin Uecker, there's a better > test for "__is_constant()": > > /* Glory to Martin Uecker <martin.uec...@med.uni-goettingen.de> */ > #define __is_constant(a) \ > (sizeof(int) == sizeof(*(1 ? ((void*)((a) * 0l)) : (int*)1))) > > that is actually *specified* by the C standard to work, and doesn't > even depend on any gcc extensions. I feel we risk awakening Cthulhu with this. :) > The reason is some really subtle pointer conversion rules, where the > type of the ternary operator will depend on whether one of the > pointers is NULL or not. > > And the definition of NULL, in turn, very much depends on "integer > constant expression that has the value 0". > > Are you willing to do one final try on a generic min/max? Same as my > last patch, but using the above __is_constant() test instead of > __builtin_constant_p? So, this time it's not a catastrophic failure with gcc 4.4. Instead it fails in 11 distinct places: $ grep "first argument to ‘__builtin_choose_expr’ not a constant" log | cut -d: -f1-2 crypto/ablkcipher.c:71 crypto/blkcipher.c:70 crypto/skcipher.c:95 mm/percpu.c:2453 net/ceph/osdmap.c:1545 net/ceph/osdmap.c:1756 net/ceph/osdmap.c:1763 mm/kmemleak.c:1371 mm/kmemleak.c:1403 drivers/infiniband/hw/hfi1/pio_copy.c:421 drivers/infiniband/hw/hfi1/pio_copy.c:547 Seems like it doesn't like void * arguments: mm/percpu.c: void *ptr; ... base = min(ptr, base); mm/kmemleak.c: static void scan_large_block(void *start, void *end) ... next = min(start + MAX_SCAN_SIZE, end); I'll poke a bit more... -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Sat, Mar 17, 2018 at 11:52 AM, Linus Torvalds <torva...@linux-foundation.org> wrote: > So the above is completely insane, bit there is actually a chance that > using that completely crazy "x -> sizeof(char[x])" conversion actually > helps, because it really does have a (very odd) evaluation-time > change. sizeof() has to be evaluated as part of the constant > expression evaluation, in ways that "__builtin_constant_p()" isn't > specified to be done. > > But it is also definitely me grasping at straws. If that doesn't work > for 4.4, there's nothing else I can possibly see. No luck! :( gcc 4.4 refuses to play along. And, hilariously, not only does it not change the complaint about __builtin_choose_expr(), it also thinks that's a VLA now. ./include/linux/mm.h: In function ‘get_mm_hiwater_rss’: ./include/linux/mm.h:1567: warning: variable length array is used ./include/linux/mm.h:1567: error: first argument to ‘__builtin_choose_expr’ not a constant 6.8 is happy with it (of course). I do think the earlier version (without the sizeof-hiding-builting_constant_p) provides a template for a const_max() that both you and Rasmus would be happy with, though! -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
On Fri, Mar 16, 2018 at 12:27 PM, Linus Torvalds
<torva...@linux-foundation.org> wrote:
> Kees - is there some online "gcc-4.4 checker" somewhere? This does
> seem to work with my gcc. I actually tested some of those files you
> pointed at now.
Unfortunately my 4.4 test fails quickly:
./include/linux/jiffies.h: In function ‘jiffies_delta_to_clock_t’:
./include/linux/jiffies.h:444: error: first argument to
‘__builtin_choose_expr’ not a constant
static inline clock_t jiffies_delta_to_clock_t(long delta)
{
return jiffies_to_clock_t(max(0L, delta));
}
I think this is the same problem of using __builtin_constant_p() in
4.4 that we hit earlier? :(
-Kees
--
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 2/2] Remove false-positive VLAs when using max()
As part of removing VLAs from the kernel[1], we want to build with -Wvla,
but it is overly pessimistic and only accepts constant expressions for
stack array sizes, instead of also constant values. The max() macro
triggers the warning, so this refactors these uses of max() to use the
new const_max() instead.
[1] https://lkml.org/lkml/2018/3/7/621
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
drivers/input/touchscreen/cyttsp4_core.c | 2 +-
fs/btrfs/tree-checker.c | 3 ++-
lib/vsprintf.c | 5 +++--
net/ipv4/proc.c | 8
net/ipv6/proc.c | 11 +--
5 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/drivers/input/touchscreen/cyttsp4_core.c
b/drivers/input/touchscreen/cyttsp4_core.c
index 727c3232517c..7fb9bd48e41c 100644
--- a/drivers/input/touchscreen/cyttsp4_core.c
+++ b/drivers/input/touchscreen/cyttsp4_core.c
@@ -868,7 +868,7 @@ static void cyttsp4_get_mt_touches(struct cyttsp4_mt_data
*md, int num_cur_tch)
struct cyttsp4_touch tch;
int sig;
int i, j, t = 0;
- int ids[max(CY_TMA1036_MAX_TCH, CY_TMA4XX_MAX_TCH)];
+ int ids[const_max_t(size_t, CY_TMA1036_MAX_TCH, CY_TMA4XX_MAX_TCH)];
memset(ids, 0, si->si_ofs.tch_abs[CY_TCH_T].max * sizeof(int));
for (i = 0; i < num_cur_tch; i++) {
diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index c3c8d48f6618..d83244e3821f 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[const_max_t(size_t, BTRFS_NAME_LEN,
+XATTR_NAME_MAX)];
read_extent_buffer(leaf, namebuf,
(unsigned long)(di + 1), name_len);
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d7a708f82559..12ff57a36171 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -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)];
+ char sym[const_max_t(size_t,
+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;
diff --git a/net/ipv4/proc.c b/net/ipv4/proc.c
index dc5edc8f7564..7f5c3b40dac9 100644
--- a/net/ipv4/proc.c
+++ b/net/ipv4/proc.c
@@ -46,7 +46,7 @@
#include
#include
-#define TCPUDP_MIB_MAX max_t(u32, UDP_MIB_MAX, TCP_MIB_MAX)
+#define TCPUDP_MIB_MAX const_max_t(size_t, UDP_MIB_MAX, TCP_MIB_MAX)
/*
* Report socket allocation statistics [m...@utu.fi]
@@ -404,7 +404,7 @@ static int snmp_seq_show_tcp_udp(struct seq_file *seq, void
*v)
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 +421,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 +432,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..b68c233de296 100644
--- a/net/ipv6/proc.c
+++ b/net/ipv6/proc.c
@@ -30,10 +30,9 @@
#include
#include
-#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 const_max_t(u32, \
+ const_max_t(u32, UDP_MIB_MAX, TCP_MIB_MAX), \
+
[PATCH v5 0/2] Remove false-positive VLAs when using max()
Patch 1 adds const_max_t(), patch 2 uses it in all the places max() was used for stack arrays. Commit log from patch 1: ---snip--- kernel.h: Introduce const_max_t() for VLA removal In the effort to remove all VLAs from the kernel[1], it is desirable to build with -Wvla. However, this warning is overly pessimistic, in that it is only happy with stack array sizes that are declared as constant expressions, and not constant values. One case of this is the evaluation of the max() macro which, due to its construction, ends up converting constant expression arguments into a constant value result. Attempts to adjust the behavior of max() ran afoul of version-dependent compiler behavior[2]. To work around this and still gain -Wvla coverage, this patch introduces a new macro, const_max_t(), for use in these cases of stack array size declaration, where the constant expressions are retained. Since this means losing the double-evaluation protections of the max() macro, this macro is designed to explicitly fail if used on non-constant arguments. Older compilers will fail with the unhelpful message: error: first argument to ‘__builtin_choose_expr’ not a constant Newer compilers will fail with a hopefully more helpful message: error: call to ‘__error_non_const_arg’ declared with attribute error: const_max_t() used with non-constant expression To gain the ability to compare differing types, the desired type must be explicitly declared, as with the existing max_t() macro. This is needed when comparing different enum types and to allow things like: int foo[const_max_t(size_t, 6, sizeof(something))]; [1] https://lkml.org/lkml/2018/3/7/621 [2] https://lkml.org/lkml/2018/3/10/170 ---eol--- Hopefully this reads well as a summary from all the things that got tried. I've tested this on allmodconfig builds with gcc 4.4.4 and 6.3.0, with and without -Wvla. -Kees v5: explicit type argument v4: forced size_t type -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/2] kernel.h: Introduce const_max() for VLA removal
On Thu, Mar 15, 2018 at 4:46 PM, Linus Torvalds <torva...@linux-foundation.org> wrote: > What I'm *not* so much ok with is "const_max(5,sizeof(x))" erroring > out, or silently causing insane behavior due to hidden subtle type > casts.. Yup! I like it as an explicit argument. Thanks! -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/2] kernel.h: Introduce const_max() for VLA removal
On Thu, Mar 15, 2018 at 4:34 PM, Linus Torvalds <torva...@linux-foundation.org> wrote: > On Thu, Mar 15, 2018 at 3:46 PM, Kees Cook <keesc...@chromium.org> wrote: >> >> So, AIUI, I can either get strict type checking, in which case, this >> is rejected (which I assume there is still a desire to have): >> >> int foo[const_max(6, sizeof(whatever))]; > > Ehh, yes, that looks fairly sane, and erroring out would be annoying. > > But maybe we should just make the type explicit, and make it "const_max_t()"? > > I think all the existing users are of type "max_t()" anyway due to the > very same issue, no? All but one are using max()[1]. One case uses max_t() to get u32. > At least if there's an explicit type like 'size_t', then passing in > "-1" becoming a large unsigned integer is understandable and clear, > not just some odd silent behavior. > > Put another way: I think it's unacceptable that > > const_max(-1,6) > > magically becomes a huge positive number like in that patch of yours, but > > const_max_t(size_t, -1, 6) > > *obviously* is a huge positive number. > > The two things would *do* the same thing, but in the second case the > type is explicit and visible. > >> due to __builtin_types_compatible_p() rejecting it, or I can construct >> a "positive arguments only" test, in which the above is accepted, but >> this is rejected: > > That sounds acceptable too, although the "const_max_t()" thing is > presumably going to be simpler? I much prefer explicit typing, but both you and Rasmus mentioned wanting the int/sizeof_t mixing. I'm totally happy with const_max_t() -- even if it makes my line-wrapping harder due to the longer name. ;) I'll resend in a moment... -Kees [1] https://patchwork.kernel.org/patch/10285709/ -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/2] kernel.h: Introduce const_max() for VLA removal
On Thu, Mar 15, 2018 at 4:17 PM, Miguel Ojeda
<miguel.ojeda.sando...@gmail.com> wrote:
>> The full one, using your naming convention:
>>
>> #define const_max(x, y) \
>> ({ \
>> if (!__builtin_constant_p(x))\
>> __error_not_const_arg(); \
>> if (!__builtin_constant_p(y))\
>> __error_not_const_arg(); \
>> if (!__builtin_types_compatible_p(typeof(x), typeof(y))) \
>> __error_incompatible_types();\
>> if ((x) < 0) \
>> __error_not_positive_arg(); \
>> if ((y) < 0) \
>> __error_not_positive_arg(); \
>> __builtin_choose_expr((x) > (y), (x), (y)); \
>> })
>>
>
> Nevermind... gcc doesn't take that as a constant expr, even if it
> compiles as one at -O0.
Yeah, unfortunately. :(
-Kees
--
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/2] kernel.h: Introduce const_max() for VLA removal
On Thu, Mar 15, 2018 at 3:23 PM, Linus Torvalds
<torva...@linux-foundation.org> wrote:
> On Thu, Mar 15, 2018 at 3:16 PM, Kees Cook <keesc...@chromium.org> wrote:
>>
>> size_t __error_not_const_arg(void) \
>> __compiletime_error("const_max() used with non-compile-time constant arg");
>> #define const_max(x, y) \
>> __builtin_choose_expr(__builtin_constant_p(x) &&\
>> __builtin_constant_p(y), \
>> (typeof(x))(x) > (typeof(y))(y) ? \
>> (x) : (y), \
>> __error_not_const_arg())
>>
>> Is typeof() forcing enums to int? Regardless, I'll put this through
>> larger testing. How does that look?
>
> Ok, that alleviates my worry about one class of insane behavior, but
> it does raise a few other questions:
>
> - what drugs is gcc on where (typeof(x)(x)) makes a difference? Funky.
Yeah, that's why I didn't even try that originally. But in looking
back at max() again, it seemed to be the only thing missing that would
handle the enum evaluation, which turned out to be true.
> - this does have the usual "what happen if you do
>
> const_max(-1,sizeof(x))
>
> where the comparison will now be done in 'size_t', and -1 ends up
> being a very very big unsigned integer.
>
> Is there no way to get that type checking inserted? Maybe now is a
> good point for that __builtin_types_compatible(), and add it to the
> constness checking (and change the name of that error case function)?
So, AIUI, I can either get strict type checking, in which case, this
is rejected (which I assume there is still a desire to have):
int foo[const_max(6, sizeof(whatever))];
due to __builtin_types_compatible_p() rejecting it, or I can construct
a "positive arguments only" test, in which the above is accepted, but
this is rejected:
int foo[const_max(-1, sizeof(whatever))];
By using this eye-bleed:
size_t __error_not_const_arg(void) \
__compiletime_error("const_max() used with non-compile-time constant arg");
size_t __error_not_positive_arg(void) \
__compiletime_error("const_max() used with negative arg");
#define const_max(x, y) \
__builtin_choose_expr(__builtin_constant_p(x) &&\
__builtin_constant_p(y), \
__builtin_choose_expr((x) >= 0 && (y) >= 0, \
(typeof(x))(x) > (typeof(y))(y) ? \
(x) : (y), \
__error_not_positive_arg()), \
__error_not_const_arg())
-Kees
--
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/2] kernel.h: Introduce const_max() for VLA removal
On Thu, Mar 15, 2018 at 2:42 PM, Linus Torvalds
<torva...@linux-foundation.org> wrote:
> On Thu, Mar 15, 2018 at 12:47 PM, Kees Cook <keesc...@chromium.org> wrote:
>>
>> To gain the ability to compare differing types, the arguments are
>> explicitly cast to size_t.
>
> Ugh, I really hate this.
>
> It silently does insane things if you do
>
>const_max(-1,6)
>
> and there is nothing in the name that implies that you can't use
> negative constants.
Yeah, I didn't like that effect either. I was seeing this:
./include/linux/kernel.h:836:14: warning: comparison between ‘enum
’ and ‘enum ’ [-Wenum-compare]
(x) > (y) ? \
^
./include/linux/kernel.h:838:7: note: in definition of macro ‘const_max’
(y), \
^
net/ipv6/proc.c:34:11: note: in expansion of macro ‘const_max’
const_max(IPSTATS_MIB_MAX, ICMP_MIB_MAX))
^
But it turns out that just doing a typeof() fixes this, and there's no
need for the hard cast to size_t:
size_t __error_not_const_arg(void) \
__compiletime_error("const_max() used with non-compile-time constant arg");
#define const_max(x, y) \
__builtin_choose_expr(__builtin_constant_p(x) &&\
__builtin_constant_p(y), \
(typeof(x))(x) > (typeof(y))(y) ? \
(x) : (y), \
__error_not_const_arg())
Is typeof() forcing enums to int? Regardless, I'll put this through
larger testing. How does that look?
-Kees
--
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 2/2] Remove false-positive VLAs when using max()
As part of removing VLAs from the kernel[1], we want to build with -Wvla,
but it is overly pessimistic and only accepts constant expressions for
stack array sizes, instead of also constant values. The max() macro
triggers the warning, so this refactors these uses of max() to use the
new const_max() instead.
[1] https://lkml.org/lkml/2018/3/7/621
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
drivers/input/touchscreen/cyttsp4_core.c | 2 +-
fs/btrfs/tree-checker.c | 3 ++-
lib/vsprintf.c | 4 ++--
net/ipv4/proc.c | 8
net/ipv6/proc.c | 10 --
5 files changed, 13 insertions(+), 14 deletions(-)
diff --git a/drivers/input/touchscreen/cyttsp4_core.c
b/drivers/input/touchscreen/cyttsp4_core.c
index 727c3232517c..f89497940051 100644
--- a/drivers/input/touchscreen/cyttsp4_core.c
+++ b/drivers/input/touchscreen/cyttsp4_core.c
@@ -868,7 +868,7 @@ static void cyttsp4_get_mt_touches(struct cyttsp4_mt_data
*md, int num_cur_tch)
struct cyttsp4_touch tch;
int sig;
int i, j, t = 0;
- int ids[max(CY_TMA1036_MAX_TCH, CY_TMA4XX_MAX_TCH)];
+ int ids[const_max(CY_TMA1036_MAX_TCH, CY_TMA4XX_MAX_TCH)];
memset(ids, 0, si->si_ofs.tch_abs[CY_TCH_T].max * sizeof(int));
for (i = 0; i < num_cur_tch; i++) {
diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index c3c8d48f6618..1ddd6cc3c4fc 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[const_max(BTRFS_NAME_LEN,
+ XATTR_NAME_MAX)];
read_extent_buffer(leaf, namebuf,
(unsigned long)(di + 1), name_len);
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d7a708f82559..9d5610b643ce 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[const_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;
diff --git a/net/ipv4/proc.c b/net/ipv4/proc.c
index dc5edc8f7564..fad6f989004e 100644
--- a/net/ipv4/proc.c
+++ b/net/ipv4/proc.c
@@ -46,7 +46,7 @@
#include
#include
-#define TCPUDP_MIB_MAX max_t(u32, UDP_MIB_MAX, TCP_MIB_MAX)
+#define TCPUDP_MIB_MAX const_max(UDP_MIB_MAX, TCP_MIB_MAX)
/*
* Report socket allocation statistics [m...@utu.fi]
@@ -404,7 +404,7 @@ static int snmp_seq_show_tcp_udp(struct seq_file *seq, void
*v)
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 +421,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 +432,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..58bbfc4fa7fa 100644
--- a/net/ipv6/proc.c
+++ b/net/ipv6/proc.c
@@ -30,10 +30,8 @@
#include
#include
-#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 const_max(const_max(UDP_MIB_MAX, TCP_MIB_MAX), \
+ const_max(IPSTATS_MIB_MAX, ICMP_MIB_MAX))
static int sockstat6_seq_show(struct seq_file *seq, void *v)
{
@@ -199,7 +197,7 @@ stati
[PATCH v4 0/2] Remove false-positive VLAs when using max()
I'm calling this "v4" since the last effort at this was v3, even if it's a different approach. Patch 1 adds const_max(), patch 2 uses it in all the places max() was used for stack arrays. Commit log from patch 1: ---snip--- kernel.h: Introduce const_max() for VLA removal In the effort to remove all VLAs from the kernel[1], it is desirable to build with -Wvla. However, this warning is overly pessimistic, in that it is only happy with stack array sizes that are declared as constant expressions, and not constant values. One case of this is the evaluation of the max() macro which, due to its construction, ends up converting constant expression arguments into a constant value result. Attempts to adjust the behavior of max() ran afoul of version-dependent compiler behavior[2]. To work around this and still gain -Wvla coverage, this patch introduces a new macro, const_max(), for use in these cases of stack array size declaration, where the constant expressions are retained. Since this means losing the double-evaluation protections of the max() macro, this macro is designed to explicitly fail if used on non-constant arguments. Older compilers will fail with the unhelpful message: error: first argument to ‘__builtin_choose_expr’ not a constant Newer compilers will fail with a hopefully more helpful message: error: call to ‘__error_not_const_arg’ declared with attribute error: const_max() used with non-compile-time constant arg To gain the ability to compare differing types, the arguments are explicitly cast to size_t. Without this, some compiler versions will fail when comparing different enum types or similar constant expression cases. With the casting, it's possible to do things like: int foo[const_max(6, sizeof(something))]; [1] https://lkml.org/lkml/2018/3/7/621 [2] https://lkml.org/lkml/2018/3/10/170 ---eol--- Hopefully this reads well as a summary from all the things that got tried. I've tested this on allmodconfig builds with gcc 4.4.4 and 6.3.0, with and without -Wvla. -Kees -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 1/2] kernel.h: Introduce const_max() for VLA removal
In the effort to remove all VLAs from the kernel[1], it is desirable to
build with -Wvla. However, this warning is overly pessimistic, in that
it is only happy with stack array sizes that are declared as constant
expressions, and not constant values. One case of this is the evaluation
of the max() macro which, due to its construction, ends up converting
constant expression arguments into a constant value result. Attempts
to adjust the behavior of max() ran afoul of version-dependent compiler
behavior[2].
To work around this and still gain -Wvla coverage, this patch introduces
a new macro, const_max(), for use in these cases of stack array size
declaration, where the constant expressions are retained. Since this means
losing the double-evaluation protections of the max() macro, this macro is
designed to explicitly fail if used on non-constant arguments.
Older compilers will fail with the unhelpful message:
error: first argument to ‘__builtin_choose_expr’ not a constant
Newer compilers will fail with a hopefully more helpful message:
error: call to ‘__error_not_const_arg’ declared with attribute error:
const_max() used with non-compile-time constant arg
To gain the ability to compare differing types, the arguments are
explicitly cast to size_t. Without this, some compiler versions will
fail when comparing different enum types or similar constant expression
cases. With the casting, it's possible to do things like:
int foo[const_max(6, sizeof(something))];
[1] https://lkml.org/lkml/2018/3/7/621
[2] https://lkml.org/lkml/2018/3/10/170
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
include/linux/kernel.h | 19 +++
1 file changed, 19 insertions(+)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 3fd291503576..012f588b5a25 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -820,6 +820,25 @@ static inline void ftrace_dump(enum ftrace_dump_mode
oops_dump_mode) { }
x, y)
/**
+ * const_max - return maximum of two positive compile-time constant values
+ * @x: first compile-time constant value
+ * @y: second compile-time constant value
+ *
+ * This has no type checking nor multi-evaluation defenses, and must
+ * only ever be used with positive compile-time constant values (for
+ * example when calculating a stack array size).
+ */
+size_t __error_not_const_arg(void) \
+__compiletime_error("const_max() used with non-compile-time constant arg");
+#define const_max(x, y)\
+ __builtin_choose_expr(__builtin_constant_p(x) &&\
+ __builtin_constant_p(y), \
+ (size_t)(x) > (size_t)(y) ? \
+ (size_t)(x) : \
+ (size_t)(y),\
+ __error_not_const_arg())
+
+/**
* min3 - return minimum of three values
* @x: first value
* @y: second value
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()
On Tue, Mar 13, 2018 at 2:02 PM, Andrew Morton <a...@linux-foundation.org> wrote: > On Mon, 12 Mar 2018 21:28:57 -0700 Kees Cook <keesc...@chromium.org> wrote: > >> On Mon, Mar 12, 2018 at 4:57 PM, Linus Torvalds >> <torva...@linux-foundation.org> wrote: >> > On Mon, Mar 12, 2018 at 3:55 PM, Andrew Morton >> > <a...@linux-foundation.org> wrote: >> >> >> >> Replacing the __builtin_choose_expr() with ?: works of course. >> > >> > Hmm. That sounds like the right thing to do. We were so myopically >> > staring at the __builtin_choose_expr() problem that we overlooked the >> > obvious solution. >> > >> > Using __builtin_constant_p() together with a ?: is in fact our common >> > pattern, so that should be fine. The only real reason to use >> > __builtin_choose_expr() is if you want to get the *type* to vary >> > depending on which side you choose, but that's not an issue for >> > min/max. >> >> This doesn't solve it for -Wvla, unfortunately. That was the point of >> Josh's original suggestion of __builtin_choose_expr(). >> >> Try building with KCFLAGS=-Wval and checking net/ipv6/proc.c: >> >> net/ipv6/proc.c: In function ‘snmp6_seq_show_item’: >> net/ipv6/proc.c:198:2: warning: ISO C90 forbids array ‘buff’ whose >> size can’t be evaluated [-Wvla] >> unsigned long buff[SNMP_MIB_MAX]; >> ^~~~ > > PITA. Didn't we once have a different way of detecting VLAs? Some > post-compilation asm parser, iirc. > > I suppose the world wouldn't end if we had a gcc version ifdef in > kernel.h. We'll get to remove it in, oh, ten years. For fixing only 6 VLAs, we don't need all this effort. When it looked like we could get away with just a "better" max(), sure. ;) I'll send a "const_max()" which will refuse to work on non-constant-values (so it doesn't get accidentally used on variables that could be exposed to double-evaluation), and will work for stack array declarations (to avoid the overly-sensitive -Wvla checks). -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()
On Mon, Mar 12, 2018 at 4:57 PM, Linus Torvalds <torva...@linux-foundation.org> wrote: > On Mon, Mar 12, 2018 at 3:55 PM, Andrew Morton > <a...@linux-foundation.org> wrote: >> >> Replacing the __builtin_choose_expr() with ?: works of course. > > Hmm. That sounds like the right thing to do. We were so myopically > staring at the __builtin_choose_expr() problem that we overlooked the > obvious solution. > > Using __builtin_constant_p() together with a ?: is in fact our common > pattern, so that should be fine. The only real reason to use > __builtin_choose_expr() is if you want to get the *type* to vary > depending on which side you choose, but that's not an issue for > min/max. This doesn't solve it for -Wvla, unfortunately. That was the point of Josh's original suggestion of __builtin_choose_expr(). Try building with KCFLAGS=-Wval and checking net/ipv6/proc.c: net/ipv6/proc.c: In function ‘snmp6_seq_show_item’: net/ipv6/proc.c:198:2: warning: ISO C90 forbids array ‘buff’ whose size can’t be evaluated [-Wvla] unsigned long buff[SNMP_MIB_MAX]; ^~~~ -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()
On Fri, Mar 9, 2018 at 10:10 PM, Miguel Ojeda <miguel.ojeda.sando...@gmail.com> wrote: > On Sat, Mar 10, 2018 at 4:11 AM, Randy Dunlap <rdun...@infradead.org> wrote: >> On 03/09/2018 04:07 PM, Andrew Morton wrote: >>> On Fri, 9 Mar 2018 12:05:36 -0800 Kees Cook <keesc...@chromium.org> wrote: >>> >>>> When max() is used in stack array size calculations from literal values >>>> (e.g. "char foo[max(sizeof(struct1), sizeof(struct2))]", the compiler >>>> thinks this is a dynamic calculation due to the single-eval logic, which >>>> is not needed in the literal case. This change removes several accidental >>>> stack VLAs from an x86 allmodconfig build: >>>> >>>> $ diff -u before.txt after.txt | grep ^- >>>> -drivers/input/touchscreen/cyttsp4_core.c:871:2: warning: ISO C90 forbids >>>> variable length array ‘ids’ [-Wvla] >>>> -fs/btrfs/tree-checker.c:344:4: warning: ISO C90 forbids variable length >>>> array ‘namebuf’ [-Wvla] >>>> -lib/vsprintf.c:747:2: warning: ISO C90 forbids variable length array >>>> ‘sym’ [-Wvla] >>>> -net/ipv4/proc.c:403:2: warning: ISO C90 forbids variable length array >>>> ‘buff’ [-Wvla] >>>> -net/ipv6/proc.c:198:2: warning: ISO C90 forbids variable length array >>>> ‘buff’ [-Wvla] >>>> -net/ipv6/proc.c:218:2: warning: ISO C90 forbids variable length array >>>> ‘buff64’ [-Wvla] >>>> >>>> Based on an earlier patch from Josh Poimboeuf. >>> >>> v1, v2 and v3 of this patch all fail with gcc-4.4.4: >>> >>> ./include/linux/jiffies.h: In function 'jiffies_delta_to_clock_t': >>> ./include/linux/jiffies.h:444: error: first argument to >>> '__builtin_choose_expr' not a constant >> >> >> I'm seeing that problem with >>> gcc --version >> gcc (SUSE Linux) 4.8.5 > > Same here, 4.8.5 fails. gcc 5.4.1 seems to work. I compiled a minimal > 5.1.0 and it seems to work as well. And sparse freaks out too: drivers/net/ethernet/via/via-velocity.c:97:26: sparse: incorrect type in initializer (different address spaces) @@ expected void *addr @@got struct mac_regs [noderef] *mac_regs drivers/net/ethernet/via/via-velocity.c:100:49: sparse: incorrect type in argument 2 (different base types) @@expected restricted pci_power_t [usertype] state @@got _t [usertype] state @@ Alright, I'm giving up on fixing max(). I'll go back to STACK_MAX() or some other name for the simple macro. Bleh. -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()
On Fri, Mar 9, 2018 at 5:30 PM, Kees Cook <keesc...@chromium.org> wrote: > -- > Kees Cook > Pixel SecurityOn > [...] WTF, gmail just blasted HTML into my explicitly plain-text email?! Apologies... -- Kees Cook Pixel SecurityOn Fri, Mar 9, 2018 at 5:30 PM, Kees Cook mailto:keesc...@chromium.org; target="_blank">keesc...@chromium.org wrote:On Fri, Mar 9, 2018 at 4:38 PM, Linus Torvalds mailto:torva...@linux-foundation.org;>torva...@linux-foundation.org wrote: On Fri, Mar 9, 2018 at 4:32 PM, Andrew Morton mailto:a...@linux-foundation.org;>a...@linux-foundation.org wrote: I wonder which gcc versions actually accept Kees's addition. Ah, my old nemesis, gcc 4.4.4. *sob* Note that we already do have this pattern, as seen by: git grep -2 __builtin_choose_expr | grep -2 __builtin_constant_p which show drivers/pinctrl/intel/pinctrl-intel.h, so the pattern already exists current kernels and _works_ - it apparently just doesn't work in slightly more complicated cases. Fun. Yeah, so all the PIN_GROUP() callers have either a literal or an array name for that argument, so the argument to __builtin_constant_p() isn't complex. git grep '\bPIN_GROUP\b' | egrep -v '(1|2|3|true|false)\)' It's one reason why I wondered if simplifying the expression to have just that single __builtin_constant_p() might not end up working.. Yeah, it seems like it doesn't bail out as "false" for complex expressions given to __builtin_constant_p(). If no magic solution, then which of these? - go back to my original "multi-eval max only for constants" macro (meh) - add gcc version checks around this and similarly for -Wvla in the future (eww) - raise gcc version (yikes) -Kees -- Kees Cook Pixel Securitydiv class="gmail_extra"brdiv class="gmail_quote"On Fri, Mar 9, 2018 at 4:38 PM, Linus Torvalds span dir="ltr"lt;a href="mailto:mailto:torva...@linux-foundation.org;>torvalds@linux-foundation.org" target="_blank"mailto:torva...@linux-foundation.org;>torvalds@linux-foundation.org/agt;/span wrote:brblockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"span class=""On Fri, Mar 9, 2018 at 4:32 PM, Andrew Morton lt;a href="mailto:mailto:a...@linux-foundation.org;>akpm@linux-foundation.org"mailto:a...@linux-foundation.org;>akpm@linux-foundation.org/agt; wrote:br gt;br gt; I wonder which gcc versions actually accept Kees's addition.br br /spanNote that we already do have this pattern, as seen by:br br nbsp; git grep -2nbsp; __builtin_choose_expr | grep -2 __builtin_constant_pbr br which show drivers/pinctrl/intel/pinctrl-wbrintel.h, so the patternbr already exists current kernels and _works_ - it apparently justbr doesn't work in slightly more complicated cases.br br It's one reason why I wondered if simplifying the expression to havebr just that single __builtin_constant_p() might not end up working..br span class="HOEnZb"font color="#88"br nbsp; nbsp; nbsp; nbsp; nbsp; nbsp; nbsp; Linusbr /font/span/blockquote/divbrbr clear="all"divbr/div-- brdiv class="gmail_signature" data-smartmail="gmail_signature"Kees CookbrPixel Security/div /div -- Kees CookPixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()
On Fri, Mar 9, 2018 at 4:38 PM, Linus Torvalds <torva...@linux-foundation.org> wrote: > On Fri, Mar 9, 2018 at 4:32 PM, Andrew Morton <a...@linux-foundation.org> > wrote: >> >> I wonder which gcc versions actually accept Kees's addition. Ah, my old nemesis, gcc 4.4.4. *sob* > Note that we already do have this pattern, as seen by: > > git grep -2 __builtin_choose_expr | grep -2 __builtin_constant_p > > which show drivers/pinctrl/intel/pinctrl-intel.h, so the pattern > already exists current kernels and _works_ - it apparently just > doesn't work in slightly more complicated cases. Fun. Yeah, so all the PIN_GROUP() callers have either a literal or an array name for that argument, so the argument to __builtin_constant_p() isn't complex. git grep '\bPIN_GROUP\b' | egrep -v '(1|2|3|true|false)\)' > It's one reason why I wondered if simplifying the expression to have > just that single __builtin_constant_p() might not end up working.. Yeah, it seems like it doesn't bail out as "false" for complex expressions given to __builtin_constant_p(). If no magic solution, then which of these? - go back to my original "multi-eval max only for constants" macro (meh) - add gcc version checks around this and similarly for -Wvla in the future (eww) - raise gcc version (yikes) -Kees -- Kees Cook Pixel SecurityOn Fri, Mar 9, 2018 at 4:38 PM, Linus Torvalds mailto:torva...@linux-foundation.org; target="_blank">torva...@linux-foundation.org wrote:On Fri, Mar 9, 2018 at 4:32 PM, Andrew Morton mailto:a...@linux-foundation.org;>a...@linux-foundation.org wrote: I wonder which gcc versions actually accept Kees's addition. Note that we already do have this pattern, as seen by: git grep -2 __builtin_choose_expr | grep -2 __builtin_constant_p which show drivers/pinctrl/intel/pinctrl-intel.h, so the pattern already exists current kernels and _works_ - it apparently just doesn't work in slightly more complicated cases. It's one reason why I wondered if simplifying the expression to have just that single __builtin_constant_p() might not end up working.. Linus -- Kees CookPixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()
On Fri, Mar 9, 2018 at 1:10 PM, Linus Torvalds
<torva...@linux-foundation.org> wrote:
> On Fri, Mar 9, 2018 at 12:05 PM, Kees Cook <keesc...@chromium.org> wrote:
>> When max() is used in stack array size calculations from literal values
>> (e.g. "char foo[max(sizeof(struct1), sizeof(struct2))]", the compiler
>> thinks this is a dynamic calculation due to the single-eval logic, which
>> is not needed in the literal case. This change removes several accidental
>> stack VLAs from an x86 allmodconfig build:
>
> Ok, looks good.
>
> I just have a couple of questions about applying it.
>
> In particular, if this will help people working on getting rid of
> VLA's in the short term, I can apply it directly.
AFAIK, all the VLAs effected by max() are fixed with this patch. i.e.
no other VLA work I'm aware of depends on this (famous last words).
> But if people who are looking at it (anybody else than Kees?)
FWIW, I've seen at least 6 other people helping out now with VLA clean
up patches. Whee. :)
> don't much care, then this
> might be a 4.17 thing or at least "random -mm queue"?
Andrew has this (v2) queued in -mm for 4.17. I didn't view VLA clean
up (or this macro change) as urgent by any means.
> #define __single_eval_max(max1, max2, x, y) ({ \
> typeof (x) max1 = (x); \
> typeof (y) max2 = (y); \
> max1 > max2 ? max1 : max2; })
>
> actually looks more natural to me than passing the two types in as
> arguments to the macro.
I (obviously) didn't design this macro originally, but my take on this
was that it's safer to keep the type check together with the
comparison so that it is never possible for someone to run off and use
__single_eval_max() directly and accidentally bypass the type check.
While it does look silly from the max_t()/min_t() perspective, it just
seems more defensive.
> (That form also is amenable to things like "__auto_type" etc simplifications).
Agreed, I think that's the biggest reason to lift the types. However,
since we're still not actually doing anything with the types (i.e.
this change doesn't weaken the type checking), I would think it would
be orthogonal. While it would be nice to resolve differing types
sanely, there doesn't appear to be a driving reason to make this
change. The example case of max(5, sizeof(thing)) doesn't currently
exist in the code and I don't see how making min()/max() _less_ strict
would be generically useful (in fact, it has proven useful to have
strict type checking).
> Side note: do we *really* need the unique variable names? That's what
> makes those things _really_ illegible. I thgink it's done just for a
> sparse warning that we should probably ignore. It's off by default
> anyway exactly because it doesn't work that well due to nested macro
> expansions like this.
That I'm not sure about. I'd be fine to remove them; I left them in
place because it seemed quite intentional. :)
> There is very real value to keeping our odd macros legible, I feel.
> Even when they are complicated by issues like this, it would be good
> to keep them as simple as possible.
>
> Comments?
I'm open to whatever! I'm just glad this gets to kill a handful of
"accidental" stack VLAs. :)
-Kees
--
Kees Cook
Pixel SecurityOn
Fri, Mar 9, 2018 at 1:10 PM, Linus Torvalds mailto:torva...@linux-foundation.org;
target="_blank">torva...@linux-foundation.org
wrote:On
Fri, Mar 9, 2018 at 12:05 PM, Kees Cook mailto:keesc...@chromium.org;>keesc...@chromium.org
wrote:
When max() is used in stack array size calculations from literal values
(e.g. "char foo[max(sizeof(struct1), sizeof(struct2))]", the compiler
thinks this is a dynamic calculation due to the single-eval
logic, which
is not needed in the literal case. This change removes several
accidental
stack VLAs from an x86 allmodconfig build:
Ok, looks good.
I just have a couple of questions about applying it.
In particular, if this will help people working on getting rid of
VLA's in the short term, I can apply it directly. But if people who
are looking at it (anybody else than Kees?) don't much care, then this
might be a 4.17 thing or at least "random -mm queue"?
The other unrelated reaction I had to this was that "we're passing
those types down very deep, even though nobody _cares_ about them all
that much at that deep level".
Honestly, the only case that really cares is the very top level, and
the rest could just take the properly cast versions.
For example, "max_t/min_t" really don't care at all, since they - by
definition - just take the single specified type.
So I'm wondering if we should just drop the types from __max/__min
(and everything they call) entirely, and instead do
#define __check_type(x,y)
((void)((typeof(x)*)1==(typeof(y)
[PATCH v3] kernel.h: Skip single-eval logic on literals in min()/max()
When max() is used in stack array size calculations from literal values
(e.g. "char foo[max(sizeof(struct1), sizeof(struct2))]", the compiler
thinks this is a dynamic calculation due to the single-eval logic, which
is not needed in the literal case. This change removes several accidental
stack VLAs from an x86 allmodconfig build:
$ diff -u before.txt after.txt | grep ^-
-drivers/input/touchscreen/cyttsp4_core.c:871:2: warning: ISO C90 forbids
variable length array ‘ids’ [-Wvla]
-fs/btrfs/tree-checker.c:344:4: warning: ISO C90 forbids variable length array
‘namebuf’ [-Wvla]
-lib/vsprintf.c:747:2: warning: ISO C90 forbids variable length array ‘sym’
[-Wvla]
-net/ipv4/proc.c:403:2: warning: ISO C90 forbids variable length array ‘buff’
[-Wvla]
-net/ipv6/proc.c:198:2: warning: ISO C90 forbids variable length array ‘buff’
[-Wvla]
-net/ipv6/proc.c:218:2: warning: ISO C90 forbids variable length array ‘buff64’
[-Wvla]
Based on an earlier patch from Josh Poimboeuf.
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
v3:
- drop __builtin_types_compatible_p() (Rasmus, Linus)
v2:
- fix copy/paste-o max1_/max2_ (ijc)
- clarify "compile-time" constant in comment (Rasmus)
- clean up formatting on min_t()/max_t()
---
include/linux/kernel.h | 48 ++--
1 file changed, 30 insertions(+), 18 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 3fd291503576..a0fca4deb3ab 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -787,37 +787,55 @@ static inline void ftrace_dump(enum ftrace_dump_mode
oops_dump_mode) { }
* strict type-checking.. See the
* "unnecessary" pointer comparison.
*/
-#define __min(t1, t2, min1, min2, x, y) ({ \
+#define __single_eval_min(t1, t2, min1, min2, x, y) ({ \
t1 min1 = (x); \
t2 min2 = (y); \
(void) ( == );\
min1 < min2 ? min1 : min2; })
+/*
+ * In the case of compile-time constant values, there is no need to do
+ * the double-evaluation protection, so the raw comparison can be made.
+ * This allows min()/max() to be used in stack array allocations and
+ * avoid the compiler thinking it is a dynamic value leading to an
+ * accidental VLA.
+ */
+#define __min(t1, t2, x, y)\
+ __builtin_choose_expr(__builtin_constant_p(x) &&\
+ __builtin_constant_p(y), \
+ (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y),\
+ __single_eval_min(t1, t2, \
+ __UNIQUE_ID(min1_), \
+ __UNIQUE_ID(min2_), \
+ x, y))
+
/**
* 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) ({ \
+#define __single_eval_max(t1, t2, max1, max2, x, y) ({ \
t1 max1 = (x); \
t2 max2 = (y); \
(void) ( == );\
max1 > max2 ? max1 : max2; })
+#define __max(t1, t2, x, y)\
+ __builtin_choose_expr(__builtin_constant_p(x) &&\
+ __builtin_constant_p(y), \
+ (t1)(x) > (t2)(y) ? (t1)(x) : (t2)(y),\
+ __single_eval_max(t1, t2, \
+ __UNIQUE_ID(max1_), \
+ __UNIQUE_ID(max2_), \
+ x, y))
/**
* 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 +887,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_), \
-
Re: [PATCH] kernel.h: Skip single-eval logic on literals in min()/max()
On Thu, Mar 8, 2018 at 5:35 PM, Linus Torvalds <torva...@linux-foundation.org> wrote: > I don't want to weaken the type enforcement, and I _thought_ you had > done that __builtin_types_compatible_p() to keep it in place. I thought so too (that originally came from Josh), but on removal, I was surprised that the checking was retained. :) > But if that's not why you did it, then why was it there at all? If the > type warning shows through even if it's in the other expression, then > just a > > > #define __min(t1, t2, x, y) \ > __builtin_choose_expr( \ > __builtin_constant_p(x) & \ > __builtin_constant_p(y),\ > (t1)(x) < (t2)(y) ? (t1)(x) : (t2)(y), \ > __single_eval_min(t1, t2, \ >... > > would seem to be sufficient? > > Because logically, the only thing that matters is that x and y don't > have any side effects and can be evaluated twice, and > "__builtin_constant_p()" is already a much stronger version of that. > > Hmm? The __builtin_types_compatible_p() just doesn't seem to matter > for the only thing I thought it was there for. Yup, agreed. I'll drop it. -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] kernel.h: Skip single-eval logic on literals in min()/max()
On Thu, Mar 8, 2018 at 3:48 PM, Linus Torvalds
<torva...@linux-foundation.org> wrote:
> On Thu, Mar 8, 2018 at 1:40 PM, Kees Cook <keesc...@chromium.org> wrote:
>> +#define __min(t1, t2, 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),\
>
> I understand why you use __builtin_types_compatible_p(), but please don't.
>
> It will mean that trivial constants like "5" and "sizeof(x)" won't
> simplify, because they have different types.
Rasmus mentioned this too. What I said there was that I was shy to
make that change, since we already can't mix that kind of thing with
the existing min()/max() implementation. The existing min()/max() is
already extremely strict, so there are no instances of this in the
tree. If I explicitly add one, I see this with or without the patch:
In file included from drivers/misc/lkdtm.h:7:0,
from drivers/misc/lkdtm_core.c:33:
drivers/misc/lkdtm_core.c: In function ‘lkdtm_module_exit’:
./include/linux/kernel.h:809:16: warning: comparison of distinct
pointer types lacks a cast
(void) ( == ); \
^
./include/linux/kernel.h:818:2: note: in expansion of macro ‘__max’
__max(typeof(x), typeof(y), \
^
./include/linux/printk.h:308:34: note: in expansion of macro ‘max’
printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
^~~
drivers/misc/lkdtm_core.c:500:2: note: in expansion of macro ‘pr_info’
pr_info("%lu\n", max(16, sizeof(unsigned long)));
^~~
> The ?: will give the right combined type anyway, and if you want the
> type comparison warning, just add a comma-expression with something
> like like
>
>(t1 *)1 == (t2 *)1
>
> to get the type compatibility warning.
When I tried removing __builtin_types_compatible_p(), I still got the
type-check warning because I think the preprocessor still sees the
"(void) ( == )" before optimizing? So, I technically _can_
drop the __builtin_types_compatible_p(), and still keep the type
warning. :P
> Yeah, yeah, maybe none of the VLA cases triggered that, but it seems
> silly to not just get that obvious constant case right.
>
> Hmm?
So are you saying you _want_ the type enforcement weakened here, or
that I should just not use __builtin_types_compatible_p()?
Thanks!
-Kees
--
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] kernel.h: Skip single-eval logic on literals in min()/max()
When max() is used in stack array size calculations from literal values
(e.g. "char foo[max(sizeof(struct1), sizeof(struct2))]", the compiler
thinks this is a dynamic calculation due to the single-eval logic, which
is not needed in the literal case. This change removes several accidental
stack VLAs from an x86 allmodconfig build:
$ diff -u before.txt after.txt | grep ^-
-drivers/input/touchscreen/cyttsp4_core.c:871:2: warning: ISO C90 forbids
variable length array ‘ids’ [-Wvla]
-fs/btrfs/tree-checker.c:344:4: warning: ISO C90 forbids variable length array
‘namebuf’ [-Wvla]
-lib/vsprintf.c:747:2: warning: ISO C90 forbids variable length array ‘sym’
[-Wvla]
-net/ipv4/proc.c:403:2: warning: ISO C90 forbids variable length array ‘buff’
[-Wvla]
-net/ipv6/proc.c:198:2: warning: ISO C90 forbids variable length array ‘buff’
[-Wvla]
-net/ipv6/proc.c:218:2: warning: ISO C90 forbids variable length array ‘buff64’
[-Wvla]
Based on an earlier patch from Josh Poimboeuf.
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
v2:
- fix copy/paste-o max1_/max2_ (ijc)
- clarify "compile-time" constant in comment (Rasmus)
- clean up formatting on min_t()/max_t()
---
include/linux/kernel.h | 50 --
1 file changed, 32 insertions(+), 18 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 3fd291503576..108cdf7bd484 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -787,37 +787,57 @@ static inline void ftrace_dump(enum ftrace_dump_mode
oops_dump_mode) { }
* strict type-checking.. See the
* "unnecessary" pointer comparison.
*/
-#define __min(t1, t2, min1, min2, x, y) ({ \
+#define __single_eval_min(t1, t2, min1, min2, x, y) ({ \
t1 min1 = (x); \
t2 min2 = (y); \
(void) ( == );\
min1 < min2 ? min1 : min2; })
+/*
+ * In the case of compile-time constant values, there is no need to do
+ * the double-evaluation protection, so the raw comparison can be made.
+ * This allows min()/max() to be used in stack array allocations and
+ * avoid the compiler thinking it is a dynamic value leading to an
+ * accidental VLA.
+ */
+#define __min(t1, t2, 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),\
+ __single_eval_min(t1, t2, \
+ __UNIQUE_ID(min1_), \
+ __UNIQUE_ID(min2_), \
+ x, y))
+
/**
* 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) ({ \
+#define __single_eval_max(t1, t2, max1, max2, x, y) ({ \
t1 max1 = (x); \
t2 max2 = (y); \
(void) ( == );\
max1 > max2 ? max1 : max2; })
+#define __max(t1, t2, 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),\
+ __single_eval_max(t1, t2, \
+ __UNIQUE_ID(max1_), \
+ __UNIQUE_ID(max2_), \
+ x, y))
/**
* 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 +889,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,
Re: [PATCH 0/3] Remove accidental VLA usage
On Thu, Mar 8, 2018 at 2:12 PM, Rasmus Villemoes <li...@rasmusvillemoes.dk> wrote: > On 8 March 2018 at 21:39, Kees Cook <keesc...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] kernel.h: Skip single-eval logic on literals in min()/max()
On Thu, Mar 8, 2018 at 2:18 PM, Andrew Morton <a...@linux-foundation.org> wrote:
> On Thu, 8 Mar 2018 13:40:45 -0800 Kees Cook <keesc...@chromium.org> wrote:
>
>> When max() is used in stack array size calculations from literal values
>> (e.g. "char foo[max(sizeof(struct1), sizeof(struct2))]", the compiler
>> thinks this is a dynamic calculation due to the single-eval logic, which
>> is not needed in the literal case. This change removes several accidental
>> stack VLAs from an x86 allmodconfig build:
>>
>> $ diff -u before.txt after.txt | grep ^-
>> -drivers/input/touchscreen/cyttsp4_core.c:871:2: warning: ISO C90 forbids
>> variable length array ‘ids’ [-Wvla]
>> -fs/btrfs/tree-checker.c:344:4: warning: ISO C90 forbids variable length
>> array ‘namebuf’ [-Wvla]
>> -lib/vsprintf.c:747:2: warning: ISO C90 forbids variable length array ‘sym’
>> [-Wvla]
>> -net/ipv4/proc.c:403:2: warning: ISO C90 forbids variable length array
>> ‘buff’ [-Wvla]
>> -net/ipv6/proc.c:198:2: warning: ISO C90 forbids variable length array
>> ‘buff’ [-Wvla]
>> -net/ipv6/proc.c:218:2: warning: ISO C90 forbids variable length array
>> ‘buff64’ [-Wvla]
>>
>> Based on an earlier patch from Josh Poimboeuf.
>>
>> ...
>>
>> --- a/include/linux/kernel.h
>> +++ b/include/linux/kernel.h
>> @@ -787,37 +787,57 @@ static inline void ftrace_dump(enum ftrace_dump_mode
>> oops_dump_mode) { }
>> * strict type-checking.. See the
>> * "unnecessary" pointer comparison.
>> */
>> -#define __min(t1, t2, min1, min2, x, y) ({ \
>> +#define __single_eval_min(t1, t2, min1, min2, x, y) ({ \
>> t1 min1 = (x); \
>> t2 min2 = (y); \
>> (void) ( == );\
>> min1 < min2 ? min1 : min2; })
>>
>> +/*
>> + * In the case of builtin constant values, there is no need to do the
>> + * double-evaluation protection, so the raw comparison can be made.
>> + * This allows min()/max() to be used in stack array allocations and
>> + * avoid the compiler thinking it is a dynamic value leading to an
>> + * accidental VLA.
>> + */
>> +#define __min(t1, t2, 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),\
>> + __single_eval_min(t1, t2, \
>> + __UNIQUE_ID(max1_), \
>> + __UNIQUE_ID(max2_), \
>> + x, y))
>> +
>
> Holy crap.
>
> I suppose gcc will one day be fixed and we won't need this.
>
> Is there a good reason to convert min()? Surely nobody will be using
> min to dimension an array - always max? Just for symmetry, I guess.
I just went with symmetry. It seems like an ugly risk to implement min
and mix differently. :) In theory it may produce smaller code for rare
min() uses, but I haven't actually verified that.
I will send a v2 with the two nits mentioned...
-Kees
--
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] kernel.h: Skip single-eval logic on literals in min()/max()
When max() is used in stack array size calculations from literal values
(e.g. "char foo[max(sizeof(struct1), sizeof(struct2))]", the compiler
thinks this is a dynamic calculation due to the single-eval logic, which
is not needed in the literal case. This change removes several accidental
stack VLAs from an x86 allmodconfig build:
$ diff -u before.txt after.txt | grep ^-
-drivers/input/touchscreen/cyttsp4_core.c:871:2: warning: ISO C90 forbids
variable length array ‘ids’ [-Wvla]
-fs/btrfs/tree-checker.c:344:4: warning: ISO C90 forbids variable length array
‘namebuf’ [-Wvla]
-lib/vsprintf.c:747:2: warning: ISO C90 forbids variable length array ‘sym’
[-Wvla]
-net/ipv4/proc.c:403:2: warning: ISO C90 forbids variable length array ‘buff’
[-Wvla]
-net/ipv6/proc.c:198:2: warning: ISO C90 forbids variable length array ‘buff’
[-Wvla]
-net/ipv6/proc.c:218:2: warning: ISO C90 forbids variable length array ‘buff64’
[-Wvla]
Based on an earlier patch from Josh Poimboeuf.
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
include/linux/kernel.h | 42 ++
1 file changed, 30 insertions(+), 12 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 3fd291503576..e0b39d461582 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -787,37 +787,57 @@ static inline void ftrace_dump(enum ftrace_dump_mode
oops_dump_mode) { }
* strict type-checking.. See the
* "unnecessary" pointer comparison.
*/
-#define __min(t1, t2, min1, min2, x, y) ({ \
+#define __single_eval_min(t1, t2, min1, min2, x, y) ({ \
t1 min1 = (x); \
t2 min2 = (y); \
(void) ( == );\
min1 < min2 ? min1 : min2; })
+/*
+ * In the case of builtin constant values, there is no need to do the
+ * double-evaluation protection, so the raw comparison can be made.
+ * This allows min()/max() to be used in stack array allocations and
+ * avoid the compiler thinking it is a dynamic value leading to an
+ * accidental VLA.
+ */
+#define __min(t1, t2, 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),\
+ __single_eval_min(t1, t2, \
+ __UNIQUE_ID(max1_), \
+ __UNIQUE_ID(max2_), \
+ x, y))
+
/**
* 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) ({ \
+#define __single_eval_max(t1, t2, max1, max2, x, y) ({ \
t1 max1 = (x); \
t2 max2 = (y); \
(void) ( == );\
max1 > max2 ? max1 : max2; })
+#define __max(t1, t2, 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),\
+ __single_eval_max(t1, t2, \
+ __UNIQUE_ID(max1_), \
+ __UNIQUE_ID(max2_), \
+ x, y))
/**
* 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
@@ -871,7 +891,6 @@ static inline void ftrace_dump(enum ftrace_dump_mode
oops_dump_mode) { }
*/
#define min_t(type, x, y) \
__min(type, type, \
- __UNIQUE_ID(min1_), __UNIQUE_ID(min2_), \
x, y)
/**
@@ -882,7 +901,6 @@ static inline void ftrace_dump(enu
Re: [PATCH 0/3] Remove accidental VLA usage
On Thu, Mar 8, 2018 at 11:57 AM, Rasmus Villemoes <li...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Remove accidental VLA usage
On Thu, Mar 8, 2018 at 7:02 AM, Josh Poimboeuf <jpoim...@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:
Oh 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; })
> +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; })
> +#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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] net: Remove accidental VLAs from proc buffers
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 <keesc...@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
#include
-#define TCPUDP_MIB_MAX max_t(u32, UDP_MIB_MAX, TCP_MIB_MAX)
-
/*
* Report socket allocation statistics [m...@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
#include
-#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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] btrfs: tree-checker: Avoid accidental stack VLA
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 <keesc...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/3] vsprintf: Remove accidental VLA usage
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 <keesc...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3] Remove accidental VLA usage
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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] v3 block subsystem refcounter conversions
On Tue, Jun 27, 2017 at 6:26 AM, Jens Axboe <ax...@kernel.dk> wrote:
> On 06/27/2017 05:39 AM, Elena Reshetova wrote:
>> Changes in v3:
>> No changes in patches apart from trivial rebases, but now by
>> default refcount_t = atomic_t and uses all atomic standard operations
>> unless CONFIG_REFCOUNT_FULL is enabled. This is a compromize for the
>> systems that are critical on performance and cannot accept even
>> slight delay on the refcounter operations.
>
> Is that true in 4.12-rc, or is that true in a later release once
> Linus has pulled those changes in? If the latter, please resend
> this when those changes are in, thanks.
It's in -next currently ("locking/refcount: Create unchecked atomic_t
implementation")
-Kees
--
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] v2: block subsystem refcounter conversions
On Fri, Apr 21, 2017 at 2:27 PM, James Bottomley <james.bottom...@hansenpartnership.com> wrote: > On Fri, 2017-04-21 at 13:22 -0700, Kees Cook wrote: >> On Fri, Apr 21, 2017 at 12:55 PM, Eric Biggers <ebigge...@gmail.com> >> wrote: >> > > > Of course, having extra checks behind a debug option is fine. >> > > > But they should not be part of the base feature; the base >> > > > feature should just be mitigation of reference count >> > > > *overflows*. It would be nice to do more, of >> > > > course; but when the extra stuff prevents people from using >> > > > refcount_t for performance reasons, it defeats the point of the >> > > > feature in the first place. >> > > >> > > Sure, but as I said above, I think the smaller tricks and fixes >> > > won't be convincing enough, so their value is questionable. >> > >> > This makes no sense, as the main point of the feature is supposed >> > to be the security improvement. As-is, the extra debugging stuff >> > is actually preventing the security improvement from being adopted, >> > which is unfortunate. >> >> We've been trying to handle the conflicting desires of those wanting >> very precise refcounting implementation and gaining the security >> protections. Ultimately, the best way forward seemed to be to first >> land the precise refcounting implementation, and start conversion >> until we ran into concerns over performance. Now, since we're here, >> we can move forward with getting a fast implementation that provides >> the desired security protections without too greatly messing with the >> refcount API. > > But that's not what it really looks like. What it looks like is > someone came up with a new API and is now intent on forcing it through > the kernel in about 500 patches using security as the hammer. The intent is to split refcounting and statistical counters away from atomics, since they have distinct APIs. This will let us audit the remaining atomic uses much more easily. > If we were really concerned about security first, we'd have fixed the > atomic overflow problem in the atomics and then built the precise > refcounting on that and tried to persuade people of the merits. I agree, but this approach was NAKed by multiple atomics maintainers. > Why can't we still do this? It looks like the overflow protection will > add only a couple of cycles to the atomics. We can move the current > version to an unchecked variant which can be used either in truly > performance critical regions with no security implications or if > someone really needs the atomic to overflow. From my point of view it > would give us the 90% case (security) and stop this endless argument > over the fast paths. Subsystems which have already moved to refcount > would stay there and the rest get to evaluate a migration on the merits > of the operational utility. -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] v2: block subsystem refcounter conversions
On Fri, Apr 21, 2017 at 12:55 PM, Eric Biggers <ebigge...@gmail.com> wrote: > Hi Elena, > > On Fri, Apr 21, 2017 at 10:55:29AM +, Reshetova, Elena wrote: >> > >> > At the very least, what is there now could probably be made about twice as >> > fast >> > by removing the checks that don't actually help mitigate refcount overflow >> > bugs, >> > specifically all the checks in refcount_dec(), and the part of >> > refcount_inc() >> > where it doesn't allow incrementing a 0 refcount. Hint: if a refcount is >> > 0, the >> > object has already been freed, so the attacker will have had the >> > opportunity to >> > allocate it with contents they control already. >> >> refcount_dec() is used very little through the code actually, it is more >> like an exception >> case since in order to use it one must really be sure that refcounter >> doesn't drop to zero. >> Removing the warn around it wouldn't likely affect much overall and thus it >> is better to >> stay to discourage people of API itself :) >> >> refcount_inc() is of course a different story, it is extensively used. I >> guess the perf issue >> on checking increment from zero might only come from WARNs being printed, >> but not really from an additional check here for zero since it is trivial >> and part of >> the needed loop anyway. So, I think only removing the >> WARNs might have any visible impact, but even this is probably not really >> that big. >> >> So, I think these changes won't really help adoption of interface if >> arguments against >> is performance. If we do have a performance issue, I think arch. specific >> implementation >> is likely the only way to considerably speed it up. > > I should have used refcount_dec_and_test() as the example, as the same applies > to both refcount_dec() and refcount_dec_and_test(). There is negligible > security benefit to have these refcount release operations checked vs. just > calling through to atomic_dec() and atomic_dec_and_test(). It's unfortunate, > but there is no known way to detect ahead of time (i.e. before exploitation) > if > there are too many refcount releases, only too many refcount acquires. > > The WARN is only executed if there is a bug, so naturally it's only a problem > if > the functions are to be inlined, creating bloat. The actual performance > problem > is the overhead associated with using comparisons and cmpxchg's to avoid > changing a refcount that is 0 or UINT_MAX. The more efficient approach would > be > to (a) drop the check for 0, and (b) don't require full operation to be > atomic, > but rather do something like "lock incl %[counter]; js ". Yes > it's not "atomic", and people have complained about this, but there is no > technical reason why it needs to be atomic. Attackers do *not* care whether > your exploit mitigation is theoretically "atomic" or not, they only care > whether > it works or not. And besides, it's not even "atomic_t" anymore, it's > "refcount_t". > >> > Of course, having extra checks behind a debug option is fine. But they >> > should >> > not be part of the base feature; the base feature should just be >> > mitigation of >> > reference count *overflows*. It would be nice to do more, of course; but >> > when >> > the extra stuff prevents people from using refcount_t for performance >> > reasons, >> > it defeats the point of the feature in the first place. >> >> Sure, but as I said above, I think the smaller tricks and fixes won't be >> convincing enough, >> so their value is questionable. > > This makes no sense, as the main point of the feature is supposed to be the > security improvement. As-is, the extra debugging stuff is actually preventing > the security improvement from being adopted, which is unfortunate. We've been trying to handle the conflicting desires of those wanting very precise refcounting implementation and gaining the security protections. Ultimately, the best way forward seemed to be to first land the precise refcounting implementation, and start conversion until we ran into concerns over performance. Now, since we're here, we can move forward with getting a fast implementation that provides the desired security protections without too greatly messing with the refcount API. -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] v2: block subsystem refcounter conversions
On Fri, Apr 21, 2017 at 3:55 AM, Reshetova, Elena <elena.reshet...@intel.com> wrote: > On Thu, Apr 20, 2017 at 11:33 AM, Eric Biggers <ebigge...@gmail.com> wrote: >> Of course, having extra checks behind a debug option is fine. But they >> should >> not be part of the base feature; the base feature should just be mitigation >> of >> reference count *overflows*. It would be nice to do more, of course; but >> when >> the extra stuff prevents people from using refcount_t for performance >> reasons, >> it defeats the point of the feature in the first place. > > Sure, but as I said above, I think the smaller tricks and fixes won't be > convincing enough, > so their value is questionable. > >> I strongly encourage anyone who has been involved in refcount_t to experiment >> with removing a reference count decrement somewhere in their kernel, then >> trying >> to exploit it to gain code execution. If you don't know what you're trying >> to >> protect against, you will not know which defences work and which don't. > > Well, we had successful CVEs and even exploits on this in the past. > @Kees, do you store a list of them in the project? Here are two from last year: http://perception-point.io/2016/01/14/analysis-and-exploitation-of-a-linux-kernel-vulnerability-cve-2016-0728/ http://cyseclabs.com/page?n=02012016 I don't disagree with Eric on the threat model: the primary concern for reference count protection is the overflow condition. Detecting a prior use-after-free is certainly nice to have, but the more important case is the initial overflow. How about we introduce something like CONFIG_HAVE_ARCH_FAST_REFCOUNT_T for per-arch implementations and CONFIG_FAST_REFCOUNT_T that trades coverage for speed, and checks only the overflow condition. This gets us the critical coverage without the changes in performance. This is basically what PaX/grsecurity already did: there is a tiny change to the atomic inc functions to detect the wrap. -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: fs/btrfs/extent-tree.c:8430:9: error: format not a string literal and no format arguments
On Tue, Nov 19, 2013 at 8:05 PM, Fengguang Wu fengguang...@intel.com wrote:
On Tue, Nov 19, 2013 at 07:56:35PM -0800, Kees Cook wrote:
Hi!
Which tree is 'devel-snb'? I don't see that on the kernel.org trees.
It's my local merge branch, based on the latest upstream release.
Hm, which release? I don't see it in 3.12, Linus's tree, nor linux-next.
Let's CC the btrfs developers for this warning. :)
Sounds good. I wanted to see what get_raid_name() uses for return
values in case gcc was being dumb, but adding %s before it should
fix the problem.
-Kees
Thanks,
Fengguang
On Tue, Nov 19, 2013 at 5:01 PM, kbuild test robot
fengguang...@intel.com wrote:
tree:devel-snb-x86_64-201311200240
head: 1a985a0807ea34f37a4c5287089abd1cd2f65049
commit: a9b93a3684dd6ebfb7cfa173f78a79c09de81207 Merge
'kees/format-security' into devel-snb-x86_64-201311200240
date: 6 hours ago
config: make ARCH=x86_64 allmodconfig
All error/warnings:
fs/btrfs/extent-tree.c:6201:12: sparse: symbol 'get_raid_name' was not
declared. Should it be static?
fs/btrfs/extent-tree.c:2469:28: sparse: context imbalance in
'run_clustered_refs' - unexpected unlock
fs/btrfs/extent-tree.c:8304:9: sparse: context imbalance in
'btrfs_put_block_group_cache' - wrong count at exit
fs/btrfs/extent-tree.c: In function '__link_block_group':
fs/btrfs/extent-tree.c:8430:9: error: format not a string literal and no
format arguments [-Werror=format-security]
get_raid_name(index));
^
cc1: some warnings being treated as errors
vim +8430 fs/btrfs/extent-tree.c
8414 return 0;
8415 }
8416
8417 static void __link_block_group(struct btrfs_space_info *space_info,
8418 struct btrfs_block_group_cache
*cache)
8419 {
8420 int index = get_block_group_index(cache);
8421
8422 down_write(space_info-groups_sem);
8423 if (list_empty(space_info-block_groups[index])) {
8424 struct kobject *kobj =
space_info-block_group_kobjs[index];
8425 int ret;
8426
8427 kobject_get(space_info-kobj); /* put in release
*/
8428 ret = kobject_init_and_add(kobj, btrfs_raid_ktype,
8429 space_info-kobj,
8430 get_raid_name(index));
8431 if (ret) {
8432 pr_warn(btrfs: failed to add kobject for
block cache. ignoring.\n);
8433 kobject_put(space_info-kobj);
8434 }
8435 }
8436 list_add_tail(cache-list,
space_info-block_groups[index]);
8437 up_write(space_info-groups_sem);
8438 }
---
0-DAY kernel build testing backend Open Source Technology
Center
http://lists.01.org/mailman/listinfo/kbuild Intel
Corporation
--
Kees Cook
Chrome OS Security
--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: fs/btrfs/extent-tree.c:8430:9: error: format not a string literal and no format arguments
On Wed, Nov 20, 2013 at 10:05 AM, Jeff Mahoney je...@suse.com wrote: On 11/20/13, 12:30 PM, Chris Mason wrote: Quoting Fengguang Wu (2013-11-19 23:05:51) On Tue, Nov 19, 2013 at 07:56:35PM -0800, Kees Cook wrote: Hi! Which tree is 'devel-snb'? I don't see that on the kernel.org trees. It's my local merge branch, based on the latest upstream release. Let's CC the btrfs developers for this warning. :) Thanks, Fengguang On Tue, Nov 19, 2013 at 5:01 PM, kbuild test robot fengguang...@intel.com wrote: tree:devel-snb-x86_64-201311200240 head: 1a985a0807ea34f37a4c5287089abd1cd2f65049 commit: a9b93a3684dd6ebfb7cfa173f78a79c09de81207 Merge 'kees/format-security' into devel-snb-x86_64-201311200240 date: 6 hours ago config: make ARCH=x86_64 allmodconfig All error/warnings: fs/btrfs/extent-tree.c:6201:12: sparse: symbol 'get_raid_name' was not declared. Should it be static? fs/btrfs/extent-tree.c:2469:28: sparse: context imbalance in 'run_clustered_refs' - unexpected unlock fs/btrfs/extent-tree.c:8304:9: sparse: context imbalance in 'btrfs_put_block_group_cache' - wrong count at exit fs/btrfs/extent-tree.c: In function '__link_block_group': fs/btrfs/extent-tree.c:8430:9: error: format not a string literal and no format arguments [-Werror=format-security] get_raid_name(index)); This comes from the btrfs-next tree, with Jeff's sysfs patches, so I've added Jeff. Ah, found it: http://git.kernel.org/cgit/linux/kernel/git/josef/btrfs-next.git/tree/fs/btrfs/extent-tree.c Thanks for the heads up. I have a few other fixes that need to go into that patch set WRT cleanup too. It looks like gcc is being stupid (const strings again). If you don't want to change the call from kobject_init_and_add(..., get_raid_name(index)); to kobject_init_and_add(..., %s, get_raid_name(index)); that's fine -- I can whitelist it since it's a string constant. But since it's not critical path, it would be nice to gain the %s just to be defensive if get_raid_name ever changes, etc. Thanks for taking a look! -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] btrfs: fix static checker warnings
On Wed, Nov 20, 2013 at 1:50 PM, Jeff Mahoney je...@suse.com wrote: This patch fixes the following warnings: fs/btrfs/extent-tree.c:6201:12: sparse: symbol 'get_raid_name' was not declared. Should it be static? fs/btrfs/extent-tree.c:8430:9: error: format not a string literal and no format arguments [-Werror=format-security] get_raid_name(index)); Signed-off-by: Jeff Mahoney je...@suse.com Reviewed-by: Kees Cook keesc...@chromium.org Thanks! -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
concerns about non-root subvol snapshots
Hi, I have some concerns about how brtfs access-checks the creation of subvol snapshots. AIUI, only the destination is validated, which basically results in a super-powerful hardlink ability. Normally, hardlinking is possible to individual files in the same way, which results in creation a small attack surface (i.e. if /etc and /home are in the same fs, link /etc/shadow to ~/.bad-admin-app.rc and exploit a vulnerability in bad-admin-app to stomp on /etc/shadow). In the case of a btrfs subvol snapshot, a user could duplicate an entire tree of their choice, even stuff that the user cannot see (/var/log/audit could now be linked into ~/.bad-admin-app/var/log/audit). I'm aware that due to DAC and MAC, when being enforced, these duplicated trees are generally considered safe, but my concerns come from the looming specter of misbehaving admin tools (which have in the past been tricked by hardlinks from time to time). In this case, that tool couldn't even check hardlink count. :) My knee-jerk reaction is that using subvol snapshot should require CAP_SYS_RESOURCE or CAP_SYS_ADMIN instead of just anyone. Though perhaps this should be a mount option, I'm not entirely sure. Any thoughts on how to accomplish this? Thanks, -Kees -- Kees Cook Ubuntu Security Team -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
