Date: Mon, 12 Mar 2018 21:05:36 -0700 From: Kees Cook <keescook@...omium.org> To: "Tobin C. Harding" <tobin@...orbit.com> Cc: "Gustavo A. R. Silva" <gustavo@...eddedor.com>, Tycho Andersen <tycho@...ho.ws>, Kernel Hardening <kernel-hardening@...ts.openwall.com> Subject: Re: VLA commit log On Sun, Mar 11, 2018 at 10:44 PM, Tobin C. Harding <tobin@...orbit.com> wrote: > On Mon, Mar 12, 2018 at 12:38:04AM -0500, Gustavo A. R. Silva wrote: >> >> >> On 03/12/2018 12:26 AM, Tobin C. Harding wrote: >> >Hi, >> > >> >I got some push back on the commit log we have all started to use >> >(copying Kees' initial commit log). If we are going to do hundreds of >> >these patches should we write a perfectly correct commit log that can be >> >included as the start of the 'why' of each VLA removal patch? Here is >> >my attempt, I am quite bad at writing commit logs so would love someone >> >to fix it up. >> > >> >> The same thing happened to me once and then I wrote this: > > I had a feeling this had happened to you but I couldn't find the patch > that made me think that when writing this. > >> In preparation to enabling -Wvla, remove VLA and replace it >> with a fixed-length array instead. > > --- > >> From a security viewpoint, the use of Variable Length Arrays can be I avoid starting lines with "From" due to age-old mbox-format email hilarity. Also, if we're going to be pedantic, we should say "stack VLA". >> a vector for stack overflow attacks. Also, in general, as the code Many people confused "stack buffer overflow" with "stack exhaustion". In this case, we should use the latter terminology. >> evolves it is easy to lose track of how big a VLA can get. Thus, we >> can end up having segfaults that are hard to debug. >> >> Also, fixed as part of the directive to remove all VLAs from >> the kernel: https://lkml.org/lkml/2018/3/7/621 > > --- > > Cool, I like this (between ---). Kees can you ack this since I'm going > to cut and paste it about 300 times :) We've got two cases we're dealing with: 1) Actual VLA in use, etc. 2) -Wvla warns about a non-constant-expression, but it's a constant-value ("const int foo = 5"). In this case, it appears no VLA is generated, but we get the warning. So we're fixing VLA warnings that aren't real. But we still want the coverage of -Wvla, so we need to refactor these cases too. So, for 1, sure: --- The use of stack Variable Length Arrays needs to be avoided, as they can be a vector for stack exhaustion, which can be both a runtime bug (kernel Oops) or a security flaw (overwriting memory beyond the stack). Also, in general, as code evolves it is easy to lose track of how big a VLA can get. Thus, we can end up having runtime failures that are hard to debug. As part of the directive to remove all VLAs from the kernel, and build with -Wvla, this patch [does whatever, with analysis of alternatives, runtime impact, or memory usage].  https://lkml.org/lkml/2018/3/7/621 --- For 2, how about something like this: --- As part of the directive to remove all VLAs from the kernel, it would be nice to build with -Wvla. This warning is, however, overly pessimistic in that it warns about not using constant expressions when declaring stack array size: it still warns about constant values, even though those do not appear to generate actual VLAs. This patch adjusts the stack array declarations to use a constant expression to remove this false positive by [whatever, with analysys of alternatives, etc].  https://lkml.org/lkml/2018/3/7/621 --- What do you think? -Kees -- Kees Cook Pixel Security
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.