Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Sat, 4 Nov 2017 09:02:00 +1100
From: "Tobin C. Harding" <me@...in.cc>
To: Tycho Andersen <tycho@...ker.com>
Cc: kernel-hardening@...ts.openwall.com, Kees Cook <keescook@...omium.org>
Subject: Re: [RFC] vla: add VLA macro and testing

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@...in.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.

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.