Re: [PATCH] x86/mtrr: only administrator can read the configurations.

From: Borislav Petkov <bp@alien8.de>
To: Kees Cook <keescook@chromium.org>
Cc: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>,
	zhangxiaoxu@huawei.com, mingo@redhat.com, hpa@zytor.com,
	x86@kernel.org, tyhicks@canonical.com, colin.king@canonical.com,
	tglx@linutronix.de, linux-kernel@vger.kernel.org,
	linux-security-module@vger.kernel.org,
	Matthew Garrett <mjg59@google.com>
Subject: Re: [PATCH] x86/mtrr: only administrator can read the configurations.
Date: Tue, 12 Nov 2019 18:49:56 +0100	[thread overview]
Message-ID: <20191112174956.GB32336@zn.tnic> (raw)
In-Reply-To: <201911110934.AC5BA313@keescook>

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