On Mon, Jun 13, 2016 at 2:49 PM, Emese Revfy [off-list ref] wrote:

On Thu, 9 Jun 2016 14:51:45 -0700
Kees Cook [off-list ref] wrote:

quoted

On Mon, May 30, 2016 at 4:31 PM, Emese Revfy [off-list ref] wrote:

quoted

-  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?

quoted

quoted

+ * 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.

quoted

quoted

+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.

quoted

quoted

+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.

quoted

quoted

+                       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.

quoted

quoted

+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!

quoted

quoted

+{
+       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!

quoted

quoted

+
+       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.

quoted

quoted

+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.

quoted

quoted

+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.

quoted

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?

quoted

+       /* 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, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>