On Mon, Nov 11, 2019 at 09:56:16AM -0800, Kees Cook wrote:

Some recap from being accidentally offlist:

- this patch should check capabilities at open time (or retain the
  checks on the opener's permissions for later checks).

- changing the DAC permissions might break something that expects to
  read mtrr when not uid 0.

- if we leave the DAC permissions alone and just move the capable check
  to the opener, we should get the intent of the original patch. (i.e.
  check against CAP_SYS_ADMIN not just the wider uid 0.)

- *this may still break things* if userspace expects to be able to
  read other parts of the file as non-uid-0 and non-CAP_SYS_ADMIN.
  If *that* is the case, then we need to censor the contents using
  the opener's permissions (as done in other /proc cases).

I think the most cautious way forward is something like
51d7b120418e ("/proc/iomem: only expose physical resource addresses to
privileged users"). Untested (and should likely be expanded to know
about read vs write for lockdown interaction):

I'm back'n'forth on this.

So tglx and I agree that it doesn't make a whole lotta sense for
non-privileged luserspace to be able to read /proc/mtrr because it is a
small leak and normal users shouldn't care about the caching attributes
of memory regions in the first place.

So maybe we should do the second variant.

But then we're not supposed to break luserspace.

But then we can revert it if we do...

Ugh.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette