Re: [PATCH v2 1/3] Add the latent_entropy gcc plugin

[Kees Cook <keescook@chromium.org>]

On Mon, Jun 13, 2016 at 2:49 PM, Emese Revfy <re.emese@gmail.com> wrote:
> On Thu, 9 Jun 2016 14:51:45 -0700
> Kees Cook <keescook@chromium.org> wrote:
>
>> On Mon, May 30, 2016 at 4:31 PM, Emese Revfy <re.emese@gmail.com> wrote:
>> > -  GCC_PLUGINS_CFLAGS := $(addprefix -fplugin=$(objtree)/scripts/gcc-plugins/, $(gcc-plugin-y))
>> > +  GCC_PLUGINS_CFLAGS := $(strip $(addprefix -fplugin=$(objtree)/scripts/gcc-plugins/, $(gcc-plugin-y)) $(gcc-plugin-cflags-y))
>>
>> Is this change part of latent_entropy, or a general fix to the gcc
>> plugin infrastructure?
>
> This is a new feature in the gcc plugin infrastructure. The latent_entropy plugin has an argument and
> we must add it (with a space) to the cflags.

Okay, can you break this out into a separate commit?

>
>> > + * gcc plugin to help generate a little bit of entropy from program state,
>> > + * used throughout the uptime of the kernel
>>
>> I think this comment needs a lot of expanding. What are all the ways
>> that this plugin makes changes to code? Things I think I see are:
>> pre-filling data variables with randomness, creating a local_entropy
>> variable (local to what?), mixing stack pointer (into what?), updating
>> latent_entropy global.
>
> I demonstrated the details here:
> https://github.com/ephox-gcc-plugins/latent_entropy/commit/049acd9f478d47ee6526d8e93ab8cfcc3ff91b13

That helps, thanks. Can you also mention how __latent_entropy changes
non-functions? (i.e. initializes them with random data.)

Also, I think this isn't accurate:

 * local_entropy ^= get_random_long();

Looking at the disassembly, it seems that static random values (i.e.
randomly chosen at gcc runtime) are added, rather than making calls to
the kernel's get_random_long() function.

>> > +static unsigned HOST_WIDE_INT seed;
>> > +static unsigned HOST_WIDE_INT get_random_const(void)
>> > +{
>> > +       unsigned int i;
>> > +       unsigned HOST_WIDE_INT ret = 0;
>> > +
>> > +       for (i = 0; i < 8 * sizeof ret; i++) {
>> > +               ret = (ret << 1) | (seed & 1);
>> > +               seed >>= 1;
>> > +               if (ret & 1)
>> > +                       seed ^= 0xD800000000000000ULL;
>> > +       }
>> > +
>> > +       return ret;
>> > +}
>>
>> Please add some comments above this function about why the seed is
>> chosen this way, how it is expected to change over the lifetime of the
>> plugin, etc.
>
> You can see the comments here:
> https://github.com/ephox-gcc-plugins/latent_entropy/commit/4999276e866271c69186a8e3112c265b6a0f3205

Ah-ha, thanks. I missed the assignment of "seed" during the init phase.

>> > +static tree handle_latent_entropy_attribute(tree *node, tree name, tree args __unused, int flags __unused, bool *no_add_attrs)
>>
>> Can you add comments to each section below describing what's being
>> checked for? Or describe above the function what specific situations
>> are valid for using the attribute? (The latter patch says "functions",
>> but also marks other kinds of things.)
>
> I think the error messages already describe all the wrong situations.
> What would you like to see in addition to the existing error messages?
>
> You can find a description about the attribute here:
> https://github.com/ephox-gcc-plugins/latent_entropy/commit/f0ec66810682579109469b862ac5169aa2a743ca

I think that clears it up nicely.

>
>> > +                       mask = 1ULL << (TREE_INT_CST_LOW(TYPE_SIZE(type)) - 1);
>> > +                       mask = 2 * (mask - 1) + 1;
>> > +
>> > +                       if (TYPE_UNSIGNED(type))
>> > +                               DECL_INITIAL(*node) = build_int_cstu(type, mask & get_random_const());
>> > +                       else
>> > +                               DECL_INITIAL(*node) = build_int_cst(type, mask & get_random_const());
>> > +                       break;
>>
>> What is happening here? Is this populating integers with the random
>> const? (I assume the ARRAY_TYPE version of this is the same thing,
>> only multiple times. Could that be made into a function instead of
>> cut/paste with a loop in the ARRAY_TYPE case below?
>
> https://github.com/ephox-gcc-plugins/latent_entropy/commit/d65864b6ca2e61cc73cd28309ba0779fde75b4f2

Great! This is much more readable to me.

>> > +static enum tree_code get_op(tree *rhs)
>>
>> Please describe this state machine, and why it does what it does. :)
>
> https://github.com/ephox-gcc-plugins/latent_entropy/commit/c4cc18cfb5d37121fe62907bed6b5aaafb84fff8

Great!

>
>> > +{
>> > +       static enum tree_code op;
>> > +       unsigned HOST_WIDE_INT random_const;
>> > +
>> > +       random_const = get_random_const();
>> > +
>> > +       switch (op) {
>> > +       case BIT_XOR_EXPR:
>> > +               op = PLUS_EXPR;
>> > +               break;
>> > +
>> > +       case PLUS_EXPR:
>> > +               if (rhs) {
>> > +                       op = LROTATE_EXPR;
>> > +                       random_const &= HOST_BITS_PER_WIDE_INT - 1;
>> > +                       break;
>> > +               }
>>
>> What's happening here with the random_const?
>
> I wrote a comment, you can find it here:
> https://github.com/ephox-gcc-plugins/latent_entropy/commit/da452fdbc0247095fdf2b1f52eb4ddd368fad640
>

Thanks!

>> > +
>> > +       case LROTATE_EXPR:
>> > +       default:
>> > +               op = BIT_XOR_EXPR;
>> > +               break;
>> > +       }
>> > +       if (rhs)
>> > +               *rhs = build_int_cstu(unsigned_intDI_type_node, random_const);
>> > +       return op;
>> > +}
>> > +
>> > +static void perturb_local_entropy(basic_block bb, tree local_entropy)
>>
>> What effect does this function have on the resulting code output?
>
> Would you like to see more on top of this comment:
> https://github.com/ephox-gcc-plugins/latent_entropy/commit/049acd9f478d47ee6526d8e93ab8cfcc3ff91b13

I think that comment should be fine.

>> > +static void perturb_latent_entropy(basic_block bb, tree rhs)
>>
>> Same for this. I assume this is effectively:
>>
>>    u64 temp_latent_entropy;
>>
>>    temp_latent_entropy = latent_entropy;
>>    temp_latent_entropy = temp_latent_entropy OP rhs
>>    latent_entropy = temp_latent_entropy;
>>
>> Where does rhs come from? (Is this the "local_entropy" below?)
>
> Sure, I'll rename the rhs parameter to local_entropy.

Thanks.

>
>> > +static void mix_in_sp(basic_block bb, tree local_entropy)
>>
>> What is the stack pointer mixed into?
>
> I already wrote some comments:
> https://github.com/ephox-gcc-plugins/latent_entropy/commit/d2781819d774b4370c248cdb8d0dd2b47308b6f4

Awesome.

>> This below needs a bit more detail in comments. IIUC, it's creating a
>> local (to the .o file? the basic block?) variable, initializing it
>> with the stack, perturbing it with random operations, then updating
>> the latent_entropy with it?
>> > +       /* create local entropy variable */
>> > +       local_entropy = create_a_tmp_var(unsigned_intDI_type_node, "local_entropy");
>>
>> What value does local_entropy have initially?
>
> You can see it here:
> https://github.com/ephox-gcc-plugins/latent_entropy/commit/049acd9f478d47ee6526d8e93ab8cfcc3ff91b13

Got it, thanks. These changes look great. If you can send an updated
series for latent_entropy, I'll add it to -next.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security