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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191112174956.GB32336@zn.tnic>
Date:   Tue, 12 Nov 2019 18:49:56 +0100
From:   Borislav Petkov <bp@...en8.de>
To:     Kees Cook <keescook@...omium.org>
Cc:     Zhang Xiaoxu <zhangxiaoxu5@...wei.com>, zhangxiaoxu@...wei.com,
        mingo@...hat.com, hpa@...or.com, x86@...nel.org,
        tyhicks@...onical.com, colin.king@...onical.com,
        tglx@...utronix.de, linux-kernel@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        Matthew Garrett <mjg59@...gle.com>
Subject: Re: [PATCH] x86/mtrr: only administrator can read the configurations.

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