| Message ID | 1509580240-28314-1-git-send-email-me@tobin.cc (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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.
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.
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.
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.
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
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
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