Re: [PATCH v2 1/3] Add the latent_entropy gcc plugin
From: Emese Revfy
Date: Mon Jun 13 2016 - 17:42:24 EST
On Thu, 9 Jun 2016 14:51:45 -0700
Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> On Mon, May 30, 2016 at 4:31 PM, Emese Revfy <re.emese@xxxxxxxxx> 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.
> > + * 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
> > +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
> > +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
> > + 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
> > +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
> > +{
> > + 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
> > +
> > + 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
> > +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.
> > +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
> 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
--
Emese