lkml.org 
[lkml]   [2018]   [Mar]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectVLA removal (was Re: [RFC 2/2] lustre: use VLA_SAFE)
On Wed, Mar 7, 2018 at 2:10 AM, Tobin C. Harding <tobin@apporbit.com> wrote:
> On Tue, Mar 06, 2018 at 09:46:02PM -0800, Kees Cook wrote:
>> On Tue, Mar 6, 2018 at 9:27 PM, Tobin C. Harding <me@tobin.cc> wrote:
>> > Currently lustre uses a VLA to store a string on the stack. We can use
>> > the newly define VLA_SAFE macro to make this declaration safer.
>> >
>> > Use VLA_SAFE to declare VLA.
>>
>> VLA_SAFE implements a max, which is nice, but I think we're just
>> digging ourselves into a bigger hole with this, since now all the
>> maxes must be validated (which isn't done here, what happens if
>> VLA_DEFAULT_MAX is smaller than the strlen() math? We'll overflow the
>> stack buffer in the later sprintf).
>
> ok, lets drop this.
>
> Memory on the stack is always going to be faster than memory from the
> slub allocator, right? Do you think using kasprintf() is going to be
> acceptable? Isn't it only going to be acceptable on non-time critical
> paths? I'm still trying to get my head around how we get rid of VLA
> when the stack is faster? Is this a speed vs safety trade off that must
> be tackled on a case by case basis?

It really does need to be a case-by-case basis. It'll be a balance of
speed, safety, and sanity. :) In the lustre case, that's both a bug
fix (buffer over-run) and an unbounded VLA removal. Putting a string
of unknown length on the stack tends not to be sensible, so the
kmalloc/kfree is reasonable, IMO.

Building with -Wvla, I see 209 unique locations reported in 60 directories:
http://paste.ubuntu.com/p/srQxwPQS9s/

In the case of the crypto, my past thoughts have included either
adding a buffer to some already-allocated context, or using an upper
bound on the VLAs, since there's a fixed number of implementations
built in at any given time. Though, I suspect neither will work
without more examination. Usually, if it were easy, it'd be done
already. ;)

To try to keep from adding new VLAs, maybe we could add -Wvla to the
W=n level in scripts/Makefile.extrawarn. Likely W=2:

# W=1 - warnings that may be relevant and does not occur too often
# W=2 - warnings that occur quite often but may still be relevant
# W=3 - the more obscure warnings, can most likely be ignored

And frankly, maybe -Wformat-security -- and perhaps format-truncation
and format-overflow -- should get added to W=2 too... they've gotten
it much less noisy over time, though still very noisy. ;)

Or, as mentioned in another thread, disable -Wvla in certain
directories but enable it at the top-level. I'm less of a fan of that,
though, since it tends to lead to the problem just getting forgotten.

-Kees

--
Kees Cook
Pixel Security

\
 
 \ /
  Last update: 2018-03-07 18:38    [W:0.050 / U:0.732 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site