[LLVMdev] Adding diversity for security (and testing)

[Per Larsen]

+david chisnall


On Mon, Jan 27, 2014 at 3:28 PM, Geremy Condra <gcondra at google.com> wrote:

> +jfb for completeness
>
>
> On Thu, Jan 23, 2014 at 5:50 PM, Stephen Crane <sjcrane at uci.edu> wrote:
>
>> Hi all,
>>
>> For anyone who's interested in this work, we have a pending patch
>> discussion on the llvm-commits list:
>> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140120/202449.htmlWe'd really appreciate input from anyone interested, especially everyone
>> involved in this discussion thread.
>>
>> Thanks,
>> Stephen
>>
>>
>> On Fri, Sep 20, 2013 at 2:36 PM, Stephen Crane <sjcrane at uci.edu> wrote:
>>
>>> Nick,
>>>
>>> Thanks so much for such a detailed review. I definitely missed a few of
>>> the details of the LLVM standards. Sorry. Here's a new patch that should
>>> resolve the issues you pointed out. I've also included a few comments below
>>> -- anything I haven't replied to has been fixed.
>>>
>>> In particular, I'd like to discuss RNG seeding with the list. I
>>> currently use a static singleton to make sure there is only a single RNG
>>> ever instantiated, but this is obviously undesirable. Is there a better
>>> place where we can initialize the RNG in a thread-safe manner? We really
>>> need an RNG for each thread in that case, so that the results are
>>> consistent, regardless of thread ordering. I'm not familiar enough with
>>> LLVM to know if such a place even exists.
>>>
>>>
>>> On Sep 20, 2013, at 00:52 , Nick Lewycky <nicholas at mxc.ca> wrote:
>>> > Awesome! I'll jump right in to reviewing. Note that my reviews often
>>> focus on low-level stuff (formatting, typos). I understand that these
>>> patches may not be ready for that, and rather expect a higher-level review,
>>> but I'll do what I do anyways.
>>>
>>> Works for me! You're completely right that I was initially expecting
>>> some higher-level feedback, but this is great.
>>>
>>> > You have separate LLVM_WITH_OPENSSL and LLVM_ENABLE_RNG settings. Why?
>>> Either LLVM_ENABLE_RNG should always be on and then people can choose to
>>> link in openssl or not. Or maybe you're worried that this is insecure and
>>> you should remove LLVM_ENABLE_RNG and make it driven solely on
>>> LLVM_ENABLE_RNG? If you think we can put a good enough RNG into LLVM, we
>>> should go with turning on LLVM_ENABLE_RNG permanently, but if not then we
>>> should make the whole thing conditional on whether LLVM_WITH_OPENSSL is set.
>>>
>>> I had initially included this because I expected resistance to adding an
>>> RNG at all, but it now appears that we're all on the same page that some
>>> RNG is required. I'll go ahead and remove the ENABLE_RNG defines.
>>>
>>> > Are you familiar with std::random_device and the random number
>>> generator C++ 11 standard libary components? Yep, I'm going to ask you to
>>> expose a compatible API, or reasons why that doesn't work (or is silly).
>>> std::random_device is defined to be non-deterministic, and you needn't
>>> provide methods that you don't call (entropy()) but it would be convenient
>>> for everyone reading the API if it were a subset of an existing standard,
>>> or with clearly documented deviations from the standard.
>>>
>>> I had not actually thought about using the std::random_device interface.
>>> If we were able to use C++11 features, this would be quite nice, since that
>>> includes builtin things like shuffle over RandomAccessIterators and such
>>> niceties. However, since this is not the case, I think complying to the
>>> <random> API (which is quite odd, there's not even an overarching
>>> interface) might be more complication than is necessary. We would have to
>>> emulate not only the generator, but also the distribution classes and the
>>> generic shuffle method. Even using operator() makes the client code
>>> particularly unreadable, since it currently uses a static Generator()
>>> getter to get the instance of the RNG. I'd like to resolve this first, then
>>> we can look at whether using the operator() interface makes sense.
>>>
>>> > +void RandomNumberGenerator::ReadStateFile(StringRef StateFilename) {
>>> >
>>> > There's nothing wrong with this function per se. It isn't buggy. I'm
>>> just wondering how this API fits with the general "llvm as a library"
>>> approach. LLVM doesn't generally bake in the assumption that it's running
>>> on a system with a filesystem. For example, the bitcode reader has a
>>> convenience method which reads a .bc file, sure, but that just does the
>>> action of loading it into a MemoryBuffer, and forwards it along to the API
>>> using MemoryBuffer that does the work. I think you should follow that
>>> pattern here too.
>>>
>>> On thinking this over further, I think this feature is simply not needed
>>> any more. We had some trouble with getting 64-bit command-line options
>>> working, but since this works now, the state file really shouldn't be
>>> needed. Nuked.
>>>
>>> > Overall this looks like a great start. However, I would like some
>>> other people to review things:
>>> >  - I don't actually approve lib/CodeGen and lib/Target changes.
>>> Somebody else is going to have to think about whether "InsertedNOP" belongs
>>> as an MIFlag.
>>>
>>> The reasoning behind making this an MIFlag was to be able to skip over
>>> NOPs in the terminators of a block. For uniform security we want to be able
>>> to place NOPs in the terminators, but still need to skip over them when
>>> finding the first terminator.
>>>
>>> >  - I'm concerned about seeding the RNG. I'm especially concerned about
>>> seeding it in the LTO case, I hadn't thought of that until I saw it in the
>>> patch. I'm appreciate if somebody with a security background could ponder
>>> that one.
>>> >  - There's also a clang patch which should be reviewed by cfe-commits.
>>> >
>>> > Nick
>>>
>>>
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140128/385787b2/attachment.html>