Re: [GIT PULL] gcc-plugins update for v4.8-rc1

[Linus Torvalds]

On Mon, Aug 8, 2016 at 4:18 PM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> In theory, this tree is the plugin support tree (now that the core
> infrastructure landed by way of the kbuild tree). Perhaps I'm
> misunderstanding the segregation of responsibilities on this, though.

So at this point, I'd rather not mix in the actual plugin code with
the infrastructure code itself.

Yes, yes, I know we have a couple of "actual plugin code" already, but
those were pretty simple and I considered them examples (thinking of
that complexity plugin in particular).

So my reaction here was that (a) your very short description about the
pull was about the new plugin, but then (b) the actual branch itself
was in my opinion *not* so much about the new plugin, but mostly about
improving the kbuild plugin infrastructure.

In other words, I'd much rather see the plugin infrastructure changes
entirely independently of the plugins themselves. And without the
pointless repeated back-merges.

>>  (c) Of the four remaining commits that actually add entropy, the
>> "extra_latent_entropy" thing isn't about the gcc plugin at all, but
>> has a pointless "klet's mix it into the plugin pool" code under an
>> #ifdef. For no possible reason I can see.
>
> Since it had some dependency on the plugin, this seemed the right
> place to land it. I could certainly clean up the code to avoid an
> #ifdef in the .c file, if that is of particular note. The reason for
> the hash mixing there is to further perturb the latent_entropy value
> based on the (possibly warm or otherwise unpredictable) physical
> memory contents.

So I really don't see the point in mixing up the gcc plugin code with
the "hash the memory contents" code.

I think they are entirely independent issues, and the mix-up is just
confusing without adding any actual value.

Why should it matter to the memory hashing whether the gcc plugin is
enabled or not?

> It is certainly hard to measure, but given the instrumentation of
> things like interrupt code, loops, and other control flow primitives,
> there is unpredictability being added even on identical builds on the
> same hardware.

Sure. But we actually do interrupt-related entropy already.

The add_interrupt_randomness() function may not be perfect, but it
should add the instruction pointer at the time of the interrupt into
the pool, for example, and the cycle counter if the CPU has any.

(Of course, that does depend on the architecture getting all the
get_irq_regs() things right).

So I agree that there is some entropy to be had, I'm just not clear on
how much more the plugin actually adds.

I agree that the plugin changes the random pool. There is no question
of that. The question is more about "does it actually change the pool
in a meaningful way".

Anyway, my main issue is that I'm definitely not taking that branch as-is.

I'd take a cleaned-up branch with just the infrastructure updates (ie
not the repeated merges).

I'd also (separately, and later) take a branch that has the gcc-plugin
with more explanations. I'm not entirely convinced it really adds that
much real randomness, but it doesn't seem to be horribly detrimental
either, and it's perhaps a more interesting example of a plugin than
some.

I'd also take the memory hashing change (I've suggested that in the
past, in fact), but I really don't see the point of mixing that up
with that whole notion of "latent_entropy".

                      Linus