On Mon, Jun 13, 2016 at 2:49 PM, Emese Revfy <re.emese@xxxxxxxxx> wrote: > 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. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
