From 2e30dd9e0698d5a04698f38e932a6e29893d8a93 Mon Sep 17 00:00:00 2001 From: Borislav Petkov Date: Fri, 8 Nov 2019 21:05:45 +0100 Subject: x86/mtrr: Get rid of mtrr_seq_show() forward declaration ... by moving the function up in the file. No functional changes. Signed-off-by: Borislav Petkov Cc: x86@kernel.org Link: https://lkml.kernel.org/r/20191108200815.24589-1-bp@alien8.de --- arch/x86/kernel/cpu/mtrr/if.c | 42 ++++++++++++++++++++---------------------- 1 file changed, 20 insertions(+), 22 deletions(-) (limited to 'arch') diff --git a/arch/x86/kernel/cpu/mtrr/if.c b/arch/x86/kernel/cpu/mtrr/if.c index 4d36dcc1cf87..268d31879557 100644 --- a/arch/x86/kernel/cpu/mtrr/if.c +++ b/arch/x86/kernel/cpu/mtrr/if.c @@ -373,28 +373,6 @@ static int mtrr_close(struct inode *ino, struct file *file) return single_release(ino, file); } -static int mtrr_seq_show(struct seq_file *seq, void *offset); - -static int mtrr_open(struct inode *inode, struct file *file) -{ - if (!mtrr_if) - return -EIO; - if (!mtrr_if->get) - return -ENXIO; - return single_open(file, mtrr_seq_show, NULL); -} - -static const struct file_operations mtrr_fops = { - .owner = THIS_MODULE, - .open = mtrr_open, - .read = seq_read, - .llseek = seq_lseek, - .write = mtrr_write, - .unlocked_ioctl = mtrr_ioctl, - .compat_ioctl = mtrr_ioctl, - .release = mtrr_close, -}; - static int mtrr_seq_show(struct seq_file *seq, void *offset) { char factor; @@ -426,6 +404,26 @@ static int mtrr_seq_show(struct seq_file *seq, void *offset) return 0; } +static int mtrr_open(struct inode *inode, struct file *file) +{ + if (!mtrr_if) + return -EIO; + if (!mtrr_if->get) + return -ENXIO; + return single_open(file, mtrr_seq_show, NULL); +} + +static const struct file_operations mtrr_fops = { + .owner = THIS_MODULE, + .open = mtrr_open, + .read = seq_read, + .llseek = seq_lseek, + .write = mtrr_write, + .unlocked_ioctl = mtrr_ioctl, + .compat_ioctl = mtrr_ioctl, + .release = mtrr_close, +}; + static int __init mtrr_if_init(void) { struct cpuinfo_x86 *c = &boot_cpu_data; -- cgit v1.2.3-59-g8ed1b From 4fc265a9c9b258ddd7eafbd0dbfca66687c3d8aa Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Mon, 18 Nov 2019 13:09:21 -0800 Subject: x86/mtrr: Require CAP_SYS_ADMIN for all access Zhang Xiaoxu noted that physical address locations for MTRR were visible to non-root users, which could be considered an information leak. In discussing[1] the options for solving this, it sounded like just moving the capable check into open() was the first step. If this breaks userspace, then we will have a test case for the more conservative approaches discussed in the thread. In summary: - MTRR 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 open 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 desired protection. (i.e. check against CAP_SYS_ADMIN not just the wider uid 0.) - if that still breaks things, as in userspace expects to be able to read other parts of the file as non-uid-0 and non-CAP_SYS_ADMIN, then we need to censor the contents using the opener's permissions. For example, as done in other /proc cases, like commit 51d7b120418e ("/proc/iomem: only expose physical resource addresses to privileged users"). [1] https://lore.kernel.org/lkml/201911110934.AC5BA313@keescook/ Reported-by: Zhang Xiaoxu Signed-off-by: Kees Cook Signed-off-by: Borislav Petkov Acked-by: James Morris Cc: "H. Peter Anvin" Cc: Colin Ian King Cc: Ingo Molnar Cc: linux-security-module@vger.kernel.org Cc: Matthew Garrett Cc: Thomas Gleixner Cc: Tyler Hicks Cc: x86-ml Cc: Thomas Gleixner Link: https://lkml.kernel.org/r/201911181308.63F06502A1@keescook --- arch/x86/kernel/cpu/mtrr/if.c | 21 ++------------------- 1 file changed, 2 insertions(+), 19 deletions(-) (limited to 'arch') diff --git a/arch/x86/kernel/cpu/mtrr/if.c b/arch/x86/kernel/cpu/mtrr/if.c index 268d31879557..da532f656a7b 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: @@ -410,6 +391,8 @@ 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); } -- cgit v1.2.3-59-g8ed1b