[kernel-hardening] [RFC] vla: add VLA macro and testing

[kernel-hardening] [RFC] vla: add VLA macro and testing

[Tobin C. Harding]

Variable Length Arrays (VLA) pose a risk to the stack if the variable
passed into the array declaration is too large. If the variable used can
be controlled by a malicious party then this poses a security risk to
the kernel.

Add a macro for declaring VLA's. Macro includes a requested size and a
maximum size, if requested size is larger than maximum size then
requested size is capped at maximum. Requested size is passed by
reference and updated by macro so caller has access to size of array
after declaration.

Signed-off-by: Tobin C. Harding <me@tobin.cc>

---

I was unable to get the test module to integrate with the kernel build system
correctly. The attempt was to mirror the way `lib/test_printf.c` functions. This
effort was unsuccessful, it is included in the patch in the hope of getting
better suggestions. To test, the test module was built out of tree and all tests
pass.  

The macro needs some work. It functions as intended but

Checkpatch emits ERROR: Macros with multiple statements should be enclosed in a
do - while loop.

Also for each use of VLA() checkpatch emits WARNING: Missing a blank line after
declarations.

Also I was unsure where to put the macro definition, appreciate any suggestions.

thanks,
Tobin.

 include/linux/vla.h                | 24 ++++++++++
 lib/Kconfig.debug                  |  3 ++
 lib/Makefile                       |  1 +
 lib/test_vla.c                     | 98 ++++++++++++++++++++++++++++++++++++++
 tools/testing/selftests/lib/config |  1 +
 5 files changed, 127 insertions(+)
 create mode 100644 include/linux/vla.h
 create mode 100644 lib/test_vla.c

diff --git a/include/linux/vla.h b/include/linux/vla.h
new file mode 100644
index 000000000000..e8f1572bbf42
--- /dev/null
+++ b/include/linux/vla.h
@@ -0,0 +1,24 @@
+/*
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+#ifndef __LINUX_VLA_H
+#define __LINUX_VLA_H
+
+/*
+ * When declaring a local variable using VLA(), VLA() must be the last
+ * declaration otherwise a compiler warning [-Wdeclaration-after-statement] will
+ * be generated
+ */
+#define VLA(type, name, size, max) \
+	type name[(*size < max ? *size : max)]; \
+	*size = (*size < max ? *size : max)
+
+#endif	/* __LINUX_VLA_H */
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index dfdad67d8f6c..3b8eb2789235 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1917,6 +1917,9 @@ config TEST_DEBUG_VIRTUAL
 
 	  If unsure, say N.
 
+config TEST_VLA
+	tristate "Test VLA macro at runtime"
+
 endmenu # runtime tests
 
 config MEMTEST
diff --git a/lib/Makefile b/lib/Makefile
index dafa79613fb4..b1e8f7c54c5b 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -63,6 +63,7 @@ obj-$(CONFIG_TEST_UUID) += test_uuid.o
 obj-$(CONFIG_TEST_PARMAN) += test_parman.o
 obj-$(CONFIG_TEST_KMOD) += test_kmod.o
 obj-$(CONFIG_TEST_DEBUG_VIRTUAL) += test_debug_virtual.o
+obj-$(CONFIG_TEST_VLA) += test_vla.o
 
 ifeq ($(CONFIG_DEBUG_KOBJECT),y)
 CFLAGS_kobject.o += -DDEBUG
diff --git a/lib/test_vla.c b/lib/test_vla.c
new file mode 100644
index 000000000000..033aeb219062
--- /dev/null
+++ b/lib/test_vla.c
@@ -0,0 +1,98 @@
+/*
+ * Tests for VLA macro.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/printk.h>
+
+#include <linux/vla.h>
+
+#define MAX_VLA_SIZE 32
+
+static unsigned total_tests __initdata;
+static unsigned failed_tests __initdata;
+
+static int __init test_vla_less_than_max(int want)
+{
+	int *size = &want;
+	VLA(char, myvla, size, MAX_VLA_SIZE);
+
+	total_tests++;
+
+	if (*size != want) {
+		pr_warn("VLA() declaration error: size=%d want=%d max=%d\n",
+			*size, want, MAX_VLA_SIZE);
+		return 1;
+	}
+
+	/* quiet compiler warning */
+	myvla[0] = 'a';
+
+	return 0;
+}
+
+static int __init test_vla_greater_than_max(int want)
+{
+	int *size = &want;
+	VLA(char, myvla, size, MAX_VLA_SIZE);
+
+	total_tests++;
+
+	if (*size != MAX_VLA_SIZE) {
+		pr_warn("VLA() declaration error: size=%d want=%d max=%d\n",
+			*size, want, MAX_VLA_SIZE);
+		return 1;
+	}
+
+	/* quiet compiler warning */
+	myvla[0] = 'a';
+
+	return 0;
+}
+
+static void __init test_vla_int_type_usage(int want)
+{
+	int *size = &want;
+	int i;
+	VLA(int, myvla, size, MAX_VLA_SIZE);
+
+	total_tests++;
+
+	for (i = 0; i < *size; i++)
+		myvla[i] = 0;
+}
+
+static int __init test_vla_init(void)
+{
+	int lt_max = MAX_VLA_SIZE / 2;
+	int gt_max = MAX_VLA_SIZE * 2;
+	int err;
+
+	err = test_vla_less_than_max(lt_max);
+	if (err)
+		failed_tests++;
+
+	err = test_vla_greater_than_max(gt_max);
+	if (err)
+		failed_tests++;
+
+	test_vla_int_type_usage(lt_max);
+	test_vla_int_type_usage(gt_max);
+
+	if (failed_tests == 0)
+		pr_info("all %u tests passed\n", total_tests);
+	else
+		pr_warn("failed %u out of %u tests\n",
+			failed_tests, total_tests);
+
+	return failed_tests ? -EINVAL : 0;
+}
+
+module_init(test_vla_init);
+
+MODULE_AUTHOR("Tobin C. Harding <me@tobin.cc>");
+MODULE_LICENSE("GPL");
diff --git a/tools/testing/selftests/lib/config b/tools/testing/selftests/lib/config
index 126933bcc950..0776156f881d 100644
--- a/tools/testing/selftests/lib/config
+++ b/tools/testing/selftests/lib/config
@@ -1,3 +1,4 @@
 CONFIG_TEST_PRINTF=m
 CONFIG_TEST_BITMAP=m
 CONFIG_PRIME_NUMBERS=m
+CONFIG_TEST_VLA=m
-- 
2.7.4

Re: [kernel-hardening] [RFC] vla: add VLA macro and testing

[Tycho Andersen]

On Thu, Nov 02, 2017 at 10:50:40AM +1100, Tobin C. Harding wrote:
> Variable Length Arrays (VLA) pose a risk to the stack if the variable
> passed into the array declaration is too large. If the variable used can
> be controlled by a malicious party then this poses a security risk to
> the kernel.
> 
> Add a macro for declaring VLA's. Macro includes a requested size and a
> maximum size, if requested size is larger than maximum size then
> requested size is capped at maximum. Requested size is passed by
> reference and updated by macro so caller has access to size of array
> after declaration.
> 
> Signed-off-by: Tobin C. Harding <me@tobin.cc>
> 
> ---
> 
> I was unable to get the test module to integrate with the kernel build system
> correctly. The attempt was to mirror the way `lib/test_printf.c` functions. This
> effort was unsuccessful, it is included in the patch in the hope of getting
> better suggestions. To test, the test module was built out of tree and all tests
> pass.  
> 
> The macro needs some work. It functions as intended but
> 
> Checkpatch emits ERROR: Macros with multiple statements should be enclosed in a
> do - while loop.
> 
> Also for each use of VLA() checkpatch emits WARNING: Missing a blank line after
> declarations.
> 
> Also I was unsure where to put the macro definition, appreciate any suggestions.
> 
> thanks,
> Tobin.
> 
>  include/linux/vla.h                | 24 ++++++++++
>  lib/Kconfig.debug                  |  3 ++
>  lib/Makefile                       |  1 +
>  lib/test_vla.c                     | 98 ++++++++++++++++++++++++++++++++++++++
>  tools/testing/selftests/lib/config |  1 +
>  5 files changed, 127 insertions(+)
>  create mode 100644 include/linux/vla.h
>  create mode 100644 lib/test_vla.c
> 
> diff --git a/include/linux/vla.h b/include/linux/vla.h
> new file mode 100644
> index 000000000000..e8f1572bbf42
> --- /dev/null
> +++ b/include/linux/vla.h
> @@ -0,0 +1,24 @@
> +/*
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +#ifndef __LINUX_VLA_H
> +#define __LINUX_VLA_H
> +
> +/*
> + * When declaring a local variable using VLA(), VLA() must be the last
> + * declaration otherwise a compiler warning [-Wdeclaration-after-statement] will
> + * be generated
> + */
> +#define VLA(type, name, size, max) \
> +	type name[(*size < max ? *size : max)]; \
> +	*size = (*size < max ? *size : max)

This requires you to have your VLAs at the end of the declarations,
which is annoying and potentially limiting (limited to one VLA, etc.).
I also don't think you need the pointer. Something like:

#define VLA(type, name, size, max) \
        type name[(size < max ? max : (size = max))]

should work I think.

It would also be nice to see some users of the VLA macro as part of
this series.

Cheers,

Tycho

Re: [kernel-hardening] [RFC] vla: add VLA macro and testing

[Tobin C. Harding]

On Fri, Nov 03, 2017 at 11:44:44AM +0300, Tycho Andersen wrote:
> On Thu, Nov 02, 2017 at 10:50:40AM +1100, Tobin C. Harding wrote:
> > Variable Length Arrays (VLA) pose a risk to the stack if the variable
> > passed into the array declaration is too large. If the variable used can
> > be controlled by a malicious party then this poses a security risk to
> > the kernel.
> > 
> > Add a macro for declaring VLA's. Macro includes a requested size and a
> > maximum size, if requested size is larger than maximum size then
> > requested size is capped at maximum. Requested size is passed by
> > reference and updated by macro so caller has access to size of array
> > after declaration.
> > 
> > Signed-off-by: Tobin C. Harding <me@tobin.cc>
> > 
> > ---
> > 
> > I was unable to get the test module to integrate with the kernel build system
> > correctly. The attempt was to mirror the way `lib/test_printf.c` functions. This
> > effort was unsuccessful, it is included in the patch in the hope of getting
> > better suggestions. To test, the test module was built out of tree and all tests
> > pass.  
> > 
> > The macro needs some work. It functions as intended but
> > 
> > Checkpatch emits ERROR: Macros with multiple statements should be enclosed in a
> > do - while loop.
> > 
> > Also for each use of VLA() checkpatch emits WARNING: Missing a blank line after
> > declarations.
> > 
> > Also I was unsure where to put the macro definition, appreciate any suggestions.
> > 
> > thanks,
> > Tobin.
> > 
> >  include/linux/vla.h                | 24 ++++++++++
> >  lib/Kconfig.debug                  |  3 ++
> >  lib/Makefile                       |  1 +
> >  lib/test_vla.c                     | 98 ++++++++++++++++++++++++++++++++++++++
> >  tools/testing/selftests/lib/config |  1 +
> >  5 files changed, 127 insertions(+)
> >  create mode 100644 include/linux/vla.h
> >  create mode 100644 lib/test_vla.c
> > 
> > diff --git a/include/linux/vla.h b/include/linux/vla.h
> > new file mode 100644
> > index 000000000000..e8f1572bbf42
> > --- /dev/null
> > +++ b/include/linux/vla.h
> > @@ -0,0 +1,24 @@
> > +/*
> > + * This software is licensed under the terms of the GNU General Public
> > + * License version 2, as published by the Free Software Foundation, and
> > + * may be copied, distributed, and modified under those terms.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + */
> > +#ifndef __LINUX_VLA_H
> > +#define __LINUX_VLA_H
> > +
> > +/*
> > + * When declaring a local variable using VLA(), VLA() must be the last
> > + * declaration otherwise a compiler warning [-Wdeclaration-after-statement] will
> > + * be generated
> > + */
> > +#define VLA(type, name, size, max) \
> > +	type name[(*size < max ? *size : max)]; \
> > +	*size = (*size < max ? *size : max)
> 
> This requires you to have your VLAs at the end of the declarations,
> which is annoying and potentially limiting (limited to one VLA, etc.).
> I also don't think you need the pointer. Something like:
> 
> #define VLA(type, name, size, max) \
>         type name[(size < max ? max : (size = max))]
>
> should work I think.

         type name[(size < max ? size : (size = max))]

Oh, cool. Thanks Tycho. I'll test out your suggestion. At first glance I
think that will remove both the checkpatch issues and the declaration
annoyance. Nice one!

> It would also be nice to see some users of the VLA macro as part of
> this series.

Righto, I wondered if that was the done thing. Will add one for the next
spin.

thanks,
Tobin.

[kernel-hardening] Re: [RFC] vla: add VLA macro and testing

[Kees Cook]

On Wed, Nov 1, 2017 at 4:50 PM, Tobin C. Harding <me@tobin.cc> wrote:
> Variable Length Arrays (VLA) pose a risk to the stack if the variable
> passed into the array declaration is too large. If the variable used can
> be controlled by a malicious party then this poses a security risk to
> the kernel.
>
> Add a macro for declaring VLA's. Macro includes a requested size and a
> maximum size, if requested size is larger than maximum size then
> requested size is capped at maximum. Requested size is passed by
> reference and updated by macro so caller has access to size of array
> after declaration.
>
> Signed-off-by: Tobin C. Harding <me@tobin.cc>
>
> ---
>
> I was unable to get the test module to integrate with the kernel build system
> correctly. The attempt was to mirror the way `lib/test_printf.c` functions. This
> effort was unsuccessful, it is included in the patch in the hope of getting
> better suggestions. To test, the test module was built out of tree and all tests
> pass.
>
> The macro needs some work. It functions as intended but
>
> Checkpatch emits ERROR: Macros with multiple statements should be enclosed in a
> do - while loop.
>
> Also for each use of VLA() checkpatch emits WARNING: Missing a blank line after
> declarations.
>
> Also I was unsure where to put the macro definition, appreciate any suggestions.

While I'd certainly like to see SOME kind of sanity checking for VLAs,
my preference would be to entirely eliminate them at get -Werror=vla
added to the Makefile. Even Linus has expressed irritation over the
idea of VLAs in structs in the past:

https://lkml.org/lkml/2013/9/23/500

I imagine VLAs make Josh's life harder too. Would objtool have an
easier time if there were no VLAs?

Could we just fix the existing VLAs instead? How many are there really?

-Kees

-- 
Kees Cook
Pixel Security

[kernel-hardening] Re: [RFC] vla: add VLA macro and testing

[Nick Desaulniers]

On Wed, Nov 8, 2017 at 2:58 PM, Kees Cook <keescook@chromium.org> wrote:
> While I'd certainly like to see SOME kind of sanity checking for VLAs,
> my preference would be to entirely eliminate them at get -Werror=vla
> added to the Makefile. Even Linus has expressed irritation over the
> idea of VLAs in structs in the past:
>
> https://lkml.org/lkml/2013/9/23/500

VLAIS makes it difficult (impossible) to compile allyesconfig with
clang.  VLA on the stack is fine, but VLAIS is not.

[kernel-hardening] Re: [RFC] vla: add VLA macro and testing

[Josh Poimboeuf]

On Wed, Nov 08, 2017 at 02:58:19PM -0800, Kees Cook wrote:
> On Wed, Nov 1, 2017 at 4:50 PM, Tobin C. Harding <me@tobin.cc> wrote:
> > Variable Length Arrays (VLA) pose a risk to the stack if the variable
> > passed into the array declaration is too large. If the variable used can
> > be controlled by a malicious party then this poses a security risk to
> > the kernel.
> >
> > Add a macro for declaring VLA's. Macro includes a requested size and a
> > maximum size, if requested size is larger than maximum size then
> > requested size is capped at maximum. Requested size is passed by
> > reference and updated by macro so caller has access to size of array
> > after declaration.
> >
> > Signed-off-by: Tobin C. Harding <me@tobin.cc>
> >
> > ---
> >
> > I was unable to get the test module to integrate with the kernel build system
> > correctly. The attempt was to mirror the way `lib/test_printf.c` functions. This
> > effort was unsuccessful, it is included in the patch in the hope of getting
> > better suggestions. To test, the test module was built out of tree and all tests
> > pass.
> >
> > The macro needs some work. It functions as intended but
> >
> > Checkpatch emits ERROR: Macros with multiple statements should be enclosed in a
> > do - while loop.
> >
> > Also for each use of VLA() checkpatch emits WARNING: Missing a blank line after
> > declarations.
> >
> > Also I was unsure where to put the macro definition, appreciate any suggestions.
> 
> While I'd certainly like to see SOME kind of sanity checking for VLAs,
> my preference would be to entirely eliminate them at get -Werror=vla
> added to the Makefile. Even Linus has expressed irritation over the
> idea of VLAs in structs in the past:
> 
> https://lkml.org/lkml/2013/9/23/500
> 
> I imagine VLAs make Josh's life harder too. Would objtool have an
> easier time if there were no VLAs?

As it turns out, it doesn't make objtool's life that much harder.  GCC
sets up a frame pointer before doing the stack allocation anyway.

But, I strongly agree that VLAs should just go away.

-- 
Josh

[kernel-hardening] Re: [RFC] vla: add VLA macro and testing

[Tobin C. Harding]

On Wed, Nov 08, 2017 at 03:04:25PM -0800, Nick Desaulniers wrote:
> On Wed, Nov 8, 2017 at 2:58 PM, Kees Cook <keescook@chromium.org> wrote:
> > While I'd certainly like to see SOME kind of sanity checking for VLAs,
> > my preference would be to entirely eliminate them at get -Werror=vla
> > added to the Makefile. Even Linus has expressed irritation over the
> > idea of VLAs in structs in the past:
> >
> > https://lkml.org/lkml/2013/9/23/500
> 
> VLAIS makes it difficult (impossible) to compile allyesconfig with
> clang.  VLA on the stack is fine, but VLAIS is not.

What is VLAIS please? I don't get any results when grepping the kernel
tree.

$ git grep VLAIS

thanks,
Tobin.

[kernel-hardening] Re: [RFC] vla: add VLA macro and testing

[Nick Desaulniers]

Variable Length Array In Struct, a GNU C extension.

Think of a variable length array, as a member of a struct, but not as
the final member.

On Sun, Nov 12, 2017 at 3:54 PM, Tobin C. Harding <me@tobin.cc> wrote:
> On Wed, Nov 08, 2017 at 03:04:25PM -0800, Nick Desaulniers wrote:
>> On Wed, Nov 8, 2017 at 2:58 PM, Kees Cook <keescook@chromium.org> wrote:
>> > While I'd certainly like to see SOME kind of sanity checking for VLAs,
>> > my preference would be to entirely eliminate them at get -Werror=vla
>> > added to the Makefile. Even Linus has expressed irritation over the
>> > idea of VLAs in structs in the past:
>> >
>> > https://lkml.org/lkml/2013/9/23/500
>>
>> VLAIS makes it difficult (impossible) to compile allyesconfig with
>> clang.  VLA on the stack is fine, but VLAIS is not.
>
> What is VLAIS please? I don't get any results when grepping the kernel
> tree.
>
> $ git grep VLAIS
>
> thanks,
> Tobin.



-- 
Thanks,
~Nick Desaulniers

[kernel-hardening] Re: [RFC] vla: add VLA macro and testing

[Tobin C. Harding]

On Mon, Nov 13, 2017 at 08:50:25AM -0800, Nick Desaulniers wrote:
> Variable Length Array In Struct, a GNU C extension.
> 
> Think of a variable length array, as a member of a struct, but not as
> the final member.

thanks Nick