LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Kees Cook <keescook@chromium.org>
To: Borislav Petkov <bp@alien8.de>
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: Mon, 11 Nov 2019 09:56:16 -0800
Message-ID: <201911110934.AC5BA313@keescook> (raw)
In-Reply-To: <20191108213307.GI4503@zn.tnic>

[this wasn't being discussed on a list... CCing lkml]

On Fri, Nov 08, 2019 at 10:33:07PM +0100, Borislav Petkov wrote:
> On Fri, Nov 08, 2019 at 01:22:50PM -0800, Kees Cook wrote:
> > The correct pattern for these kinds of things is to do the checks at
> > open time, yes. (Which is why I perked up at this patch when I noticed
> > it.)
> 
> I would move it there but...
> 
> > Well, I'm not entirely sure what the issue here is. I saw the patch also
> > changed the DAC permissions to 0600, so wouldn't that alone fix things?
> > But the capable checks moved around... is there an "unprivileged" use of
> > this file any more? If so, why keep at capable() checks and just use
> > DAC?
> 
> ... yes, that would be even better because it would kill all the checks,
> so less code.
> 
> How's that?

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):


diff --git a/arch/x86/kernel/cpu/mtrr/if.c b/arch/x86/kernel/cpu/mtrr/if.c
index 4d36dcc1cf87..7ccc3e290338 100644
--- a/arch/x86/kernel/cpu/mtrr/if.c
+++ b/arch/x86/kernel/cpu/mtrr/if.c
@@ -34,6 +34,11 @@ const char *mtrr_attrib_to_str(int x)
 
 #ifdef CONFIG_PROC_FS
 
+static bool has_mtrr_privs(struct file *file)
+{
+	return file_ns_capable(file, &init_user_ns, CAP_SYS_ADMIN);
+}
+
 static int
 mtrr_file_add(unsigned long base, unsigned long size,
 	      unsigned int type, bool increment, struct file *file, int page)
@@ -101,7 +106,7 @@ mtrr_write(struct file *file, const char __user *buf, size_t len, loff_t * ppos)
 	int length;
 	size_t linelen;
 
-	if (!capable(CAP_SYS_ADMIN))
+	if (!has_mtrr_privs(file))
 		return -EPERM;
 
 	memset(line, 0, LINE_SIZE);
@@ -226,7 +231,7 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
 #ifdef CONFIG_COMPAT
 	case MTRRIOC32_ADD_ENTRY:
 #endif
-		if (!capable(CAP_SYS_ADMIN))
+		if (!has_mtrr_privs(file))
 			return -EPERM;
 		err =
 		    mtrr_file_add(sentry.base, sentry.size, sentry.type, true,
@@ -236,7 +241,7 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
 #ifdef CONFIG_COMPAT
 	case MTRRIOC32_SET_ENTRY:
 #endif
-		if (!capable(CAP_SYS_ADMIN))
+		if (!has_mtrr_privs(file))
 			return -EPERM;
 		err = mtrr_add(sentry.base, sentry.size, sentry.type, false);
 		break;
@@ -244,7 +249,7 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
 #ifdef CONFIG_COMPAT
 	case MTRRIOC32_DEL_ENTRY:
 #endif
-		if (!capable(CAP_SYS_ADMIN))
+		if (!has_mtrr_privs(file))
 			return -EPERM;
 		err = mtrr_file_del(sentry.base, sentry.size, file, 0);
 		break;
@@ -252,7 +257,7 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
 #ifdef CONFIG_COMPAT
 	case MTRRIOC32_KILL_ENTRY:
 #endif
-		if (!capable(CAP_SYS_ADMIN))
+		if (!has_mtrr_privs(file))
 			return -EPERM;
 		err = mtrr_del(-1, sentry.base, sentry.size);
 		break;
@@ -279,7 +284,7 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
 #ifdef CONFIG_COMPAT
 	case MTRRIOC32_ADD_PAGE_ENTRY:
 #endif
-		if (!capable(CAP_SYS_ADMIN))
+		if (!has_mtrr_privs(file))
 			return -EPERM;
 		err =
 		    mtrr_file_add(sentry.base, sentry.size, sentry.type, true,
@@ -289,7 +294,7 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
 #ifdef CONFIG_COMPAT
 	case MTRRIOC32_SET_PAGE_ENTRY:
 #endif
-		if (!capable(CAP_SYS_ADMIN))
+		if (!has_mtrr_privs(file))
 			return -EPERM;
 		err =
 		    mtrr_add_page(sentry.base, sentry.size, sentry.type, false);
@@ -298,7 +303,7 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
 #ifdef CONFIG_COMPAT
 	case MTRRIOC32_DEL_PAGE_ENTRY:
 #endif
-		if (!capable(CAP_SYS_ADMIN))
+		if (!has_mtrr_privs(file))
 			return -EPERM;
 		err = mtrr_file_del(sentry.base, sentry.size, file, 1);
 		break;
@@ -306,7 +311,7 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
 #ifdef CONFIG_COMPAT
 	case MTRRIOC32_KILL_PAGE_ENTRY:
 #endif
-		if (!capable(CAP_SYS_ADMIN))
+		if (!has_mtrr_privs(file))
 			return -EPERM;
 		err = mtrr_del_page(-1, sentry.base, sentry.size);
 		break;
@@ -401,6 +406,7 @@ static int mtrr_seq_show(struct seq_file *seq, void *offset)
 	int i, max;
 	mtrr_type type;
 	unsigned long base, size;
+	int usage;
 
 	max = num_var_ranges;
 	for (i = 0; i < max; i++) {
@@ -409,6 +415,15 @@ static int mtrr_seq_show(struct seq_file *seq, void *offset)
 			mtrr_usage_table[i] = 0;
 			continue;
 		}
+		usage = mtrr_usage_table[i];
+		type_str = mtrr_attrib_to_str(type);
+
+		if (!has_mtrr_privs(seq->file)) {
+			base = 0;
+			size = 0;
+			usage = 0;
+			type_str = "?";
+		}
 		if (size < (0x100000 >> PAGE_SHIFT)) {
 			/* less than 1MB */
 			factor = 'K';
@@ -420,8 +435,7 @@ static int mtrr_seq_show(struct seq_file *seq, void *offset)
 		/* Base can be > 32bit */
 		seq_printf(seq, "reg%02i: base=0x%06lx000 (%5luMB), size=%5lu%cB, count=%d: %s\n",
 			   i, base, base >> (20 - PAGE_SHIFT),
-			   size, factor,
-			   mtrr_usage_table[i], mtrr_attrib_to_str(type));
+			   size, factor, usage, type_str);
 	}
 	return 0;
 }


If we want to risk breaking stuff, here is the "just check capable at open time" patch:

diff --git a/arch/x86/kernel/cpu/mtrr/if.c b/arch/x86/kernel/cpu/mtrr/if.c
index 4d36dcc1cf87..a65e5c6686d0 100644
--- a/arch/x86/kernel/cpu/mtrr/if.c
+++ b/arch/x86/kernel/cpu/mtrr/if.c
@@ -101,9 +101,6 @@ mtrr_write(struct file *file, const char __user *buf, size_t len, loff_t * ppos)
 	int length;
 	size_t linelen;
 
-	if (!capable(CAP_SYS_ADMIN))
-		return -EPERM;
-
 	memset(line, 0, LINE_SIZE);
 
 	len = min_t(size_t, len, LINE_SIZE - 1);
@@ -226,8 +223,6 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
 #ifdef CONFIG_COMPAT
 	case MTRRIOC32_ADD_ENTRY:
 #endif
-		if (!capable(CAP_SYS_ADMIN))
-			return -EPERM;
 		err =
 		    mtrr_file_add(sentry.base, sentry.size, sentry.type, true,
 				  file, 0);
@@ -236,24 +231,18 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
 #ifdef CONFIG_COMPAT
 	case MTRRIOC32_SET_ENTRY:
 #endif
-		if (!capable(CAP_SYS_ADMIN))
-			return -EPERM;
 		err = mtrr_add(sentry.base, sentry.size, sentry.type, false);
 		break;
 	case MTRRIOC_DEL_ENTRY:
 #ifdef CONFIG_COMPAT
 	case MTRRIOC32_DEL_ENTRY:
 #endif
-		if (!capable(CAP_SYS_ADMIN))
-			return -EPERM;
 		err = mtrr_file_del(sentry.base, sentry.size, file, 0);
 		break;
 	case MTRRIOC_KILL_ENTRY:
 #ifdef CONFIG_COMPAT
 	case MTRRIOC32_KILL_ENTRY:
 #endif
-		if (!capable(CAP_SYS_ADMIN))
-			return -EPERM;
 		err = mtrr_del(-1, sentry.base, sentry.size);
 		break;
 	case MTRRIOC_GET_ENTRY:
@@ -279,8 +268,6 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
 #ifdef CONFIG_COMPAT
 	case MTRRIOC32_ADD_PAGE_ENTRY:
 #endif
-		if (!capable(CAP_SYS_ADMIN))
-			return -EPERM;
 		err =
 		    mtrr_file_add(sentry.base, sentry.size, sentry.type, true,
 				  file, 1);
@@ -289,8 +276,6 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
 #ifdef CONFIG_COMPAT
 	case MTRRIOC32_SET_PAGE_ENTRY:
 #endif
-		if (!capable(CAP_SYS_ADMIN))
-			return -EPERM;
 		err =
 		    mtrr_add_page(sentry.base, sentry.size, sentry.type, false);
 		break;
@@ -298,16 +283,12 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
 #ifdef CONFIG_COMPAT
 	case MTRRIOC32_DEL_PAGE_ENTRY:
 #endif
-		if (!capable(CAP_SYS_ADMIN))
-			return -EPERM;
 		err = mtrr_file_del(sentry.base, sentry.size, file, 1);
 		break;
 	case MTRRIOC_KILL_PAGE_ENTRY:
 #ifdef CONFIG_COMPAT
 	case MTRRIOC32_KILL_PAGE_ENTRY:
 #endif
-		if (!capable(CAP_SYS_ADMIN))
-			return -EPERM;
 		err = mtrr_del_page(-1, sentry.base, sentry.size);
 		break;
 	case MTRRIOC_GET_PAGE_ENTRY:
@@ -381,6 +362,9 @@ static int mtrr_open(struct inode *inode, struct file *file)
 		return -EIO;
 	if (!mtrr_if->get)
 		return -ENXIO;
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
 	return single_open(file, mtrr_seq_show, NULL);
 }
 


Thoughts?

-Kees

> 
> ---
> From: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
> Date: Tue, 5 Nov 2019 15:17:14 +0800
> Subject: [PATCH] x86/mtrr: Restrict MTRR ranges dumping and ioctl()
> 
> /proc/mtrr dumps the physical memory ranges of the variable range MTRRs
> along with their respective sizes and caching attributes. Since that
> file is world-readable, it presents a small information leak about the
> physical address ranges of a system which should be blocked.
> 
> Make that file root-only and get rid of all the capability checks as
> they're not needed anymore.
> 
>  [ bp: rewrite commit message. ]
> 
> Suggested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Colin Ian King <colin.king@canonical.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Tyler Hicks <tyhicks@canonical.com>
> Cc: x86-ml <x86@kernel.org>
> Cc: zhangxiaoxu@huawei.com
> Link: https://lkml.kernel.org/r/20191105071714.27376-1-zhangxiaoxu5@huawei.com
> ---
>  arch/x86/kernel/cpu/mtrr/if.c | 18 +-----------------
>  1 file changed, 1 insertion(+), 17 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mtrr/if.c b/arch/x86/kernel/cpu/mtrr/if.c
> index 4d36dcc1cf87..7ff865f2b150 100644
> --- a/arch/x86/kernel/cpu/mtrr/if.c
> +++ b/arch/x86/kernel/cpu/mtrr/if.c
> @@ -226,8 +226,6 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
>  #ifdef CONFIG_COMPAT
>  	case MTRRIOC32_ADD_ENTRY:
>  #endif
> -		if (!capable(CAP_SYS_ADMIN))
> -			return -EPERM;
>  		err =
>  		    mtrr_file_add(sentry.base, sentry.size, sentry.type, true,
>  				  file, 0);
> @@ -236,24 +234,18 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
>  #ifdef CONFIG_COMPAT
>  	case MTRRIOC32_SET_ENTRY:
>  #endif
> -		if (!capable(CAP_SYS_ADMIN))
> -			return -EPERM;
>  		err = mtrr_add(sentry.base, sentry.size, sentry.type, false);
>  		break;
>  	case MTRRIOC_DEL_ENTRY:
>  #ifdef CONFIG_COMPAT
>  	case MTRRIOC32_DEL_ENTRY:
>  #endif
> -		if (!capable(CAP_SYS_ADMIN))
> -			return -EPERM;
>  		err = mtrr_file_del(sentry.base, sentry.size, file, 0);
>  		break;
>  	case MTRRIOC_KILL_ENTRY:
>  #ifdef CONFIG_COMPAT
>  	case MTRRIOC32_KILL_ENTRY:
>  #endif
> -		if (!capable(CAP_SYS_ADMIN))
> -			return -EPERM;
>  		err = mtrr_del(-1, sentry.base, sentry.size);
>  		break;
>  	case MTRRIOC_GET_ENTRY:
> @@ -279,8 +271,6 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
>  #ifdef CONFIG_COMPAT
>  	case MTRRIOC32_ADD_PAGE_ENTRY:
>  #endif
> -		if (!capable(CAP_SYS_ADMIN))
> -			return -EPERM;
>  		err =
>  		    mtrr_file_add(sentry.base, sentry.size, sentry.type, true,
>  				  file, 1);
> @@ -289,8 +279,6 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
>  #ifdef CONFIG_COMPAT
>  	case MTRRIOC32_SET_PAGE_ENTRY:
>  #endif
> -		if (!capable(CAP_SYS_ADMIN))
> -			return -EPERM;
>  		err =
>  		    mtrr_add_page(sentry.base, sentry.size, sentry.type, false);
>  		break;
> @@ -298,16 +286,12 @@ mtrr_ioctl(struct file *file, unsigned int cmd, unsigned long __arg)
>  #ifdef CONFIG_COMPAT
>  	case MTRRIOC32_DEL_PAGE_ENTRY:
>  #endif
> -		if (!capable(CAP_SYS_ADMIN))
> -			return -EPERM;
>  		err = mtrr_file_del(sentry.base, sentry.size, file, 1);
>  		break;
>  	case MTRRIOC_KILL_PAGE_ENTRY:
>  #ifdef CONFIG_COMPAT
>  	case MTRRIOC32_KILL_PAGE_ENTRY:
>  #endif
> -		if (!capable(CAP_SYS_ADMIN))
> -			return -EPERM;
>  		err = mtrr_del_page(-1, sentry.base, sentry.size);
>  		break;
>  	case MTRRIOC_GET_PAGE_ENTRY:
> @@ -436,7 +420,7 @@ static int __init mtrr_if_init(void)
>  	    (!cpu_has(c, X86_FEATURE_CENTAUR_MCR)))
>  		return -ENODEV;
>  
> -	proc_create("mtrr", S_IWUSR | S_IRUGO, NULL, &mtrr_fops);
> +	proc_create("mtrr", 0600, NULL, &mtrr_fops);
>  	return 0;
>  }
>  arch_initcall(mtrr_if_init);
> -- 
> 2.21.0
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

-- 
Kees Cook

  parent reply index

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20191105071714.27376-1-zhangxiaoxu5@huawei.com>
2019-11-08 20:06 ` [tip: x86/mtrr] x86/mtrr: Restrict MTRR ranges dumping and ioctl() tip-bot2 for Zhang Xiaoxu
     [not found] ` <201911081236.57A127A@keescook>
     [not found]   ` <20191108205031.GH4503@zn.tnic>
     [not found]     ` <201911081320.5D3CD1A4CD@keescook>
     [not found]       ` <20191108213307.GI4503@zn.tnic>
2019-11-11 17:56         ` Kees Cook [this message]
2019-11-12 17:49           ` [PATCH] x86/mtrr: only administrator can read the configurations Borislav Petkov
2019-11-12 22:35             ` Kees Cook
2019-11-13 21:47               ` Thomas Gleixner

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=201911110934.AC5BA313@keescook \
    --to=keescook@chromium.org \
    --cc=bp@alien8.de \
    --cc=colin.king@canonical.com \
    --cc=hpa@zytor.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=mjg59@google.com \
    --cc=tglx@linutronix.de \
    --cc=tyhicks@canonical.com \
    --cc=x86@kernel.org \
    --cc=zhangxiaoxu5@huawei.com \
    --cc=zhangxiaoxu@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git