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

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

[Kees Cook]

[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

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

[Borislav Petkov]

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

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

[Kees Cook]

On Tue, Nov 12, 2019 at 06:49:56PM +0100, Borislav Petkov wrote:
> 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.

Shall I send a patch for just moving the capable() checks into open()
and if someone yells we switch to the other option on the assumption
that then we'll have a real-world case we can test the other solution
against?

-- 
Kees Cook

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

[Thomas Gleixner]

On Tue, 12 Nov 2019, Kees Cook wrote:
> On Tue, Nov 12, 2019 at 06:49:56PM +0100, Borislav Petkov wrote:
> > 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.
> 
> Shall I send a patch for just moving the capable() checks into open()
> and if someone yells we switch to the other option on the assumption
> that then we'll have a real-world case we can test the other solution
> against?

Makes sense.

Thanks,

	tglx