* Re: [RFC PATCH v4 21/27] x86/cet/shstk: ELF header parsing of Shadow Stack
From: Eugene Syromiatnikov @ 2018-10-03 23:27 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <20180921150351.20898-22-yu-cheng.yu@intel.com>
On Fri, Sep 21, 2018 at 08:03:45AM -0700, Yu-cheng Yu wrote:
> Look in .note.gnu.property of an ELF file and check if Shadow Stack needs
> to be enabled for the task.
>
> Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
> arch/x86/Kconfig | 4 +
> arch/x86/include/asm/elf.h | 5 +
> arch/x86/include/uapi/asm/elf_property.h | 15 +
> arch/x86/kernel/Makefile | 2 +
> arch/x86/kernel/elf.c | 340 +++++++++++++++++++++++
> fs/binfmt_elf.c | 15 +
> include/uapi/linux/elf.h | 1 +
> 7 files changed, 382 insertions(+)
> create mode 100644 arch/x86/include/uapi/asm/elf_property.h
> create mode 100644 arch/x86/kernel/elf.c
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 808aa3aecf3c..6377125543cc 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1919,12 +1919,16 @@ config X86_INTEL_CET
> config ARCH_HAS_SHSTK
> def_bool n
>
> +config ARCH_HAS_PROGRAM_PROPERTIES
> + def_bool n
> +
> config X86_INTEL_SHADOW_STACK_USER
> prompt "Intel Shadow Stack for user-mode"
> def_bool n
> depends on CPU_SUP_INTEL && X86_64
> select X86_INTEL_CET
> select ARCH_HAS_SHSTK
> + select ARCH_HAS_PROGRAM_PROPERTIES
> ---help---
> Shadow stack provides hardware protection against program stack
> corruption. Only when all the following are true will an application
> diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
> index 0d157d2a1e2a..5b5f169c5c07 100644
> --- a/arch/x86/include/asm/elf.h
> +++ b/arch/x86/include/asm/elf.h
> @@ -382,4 +382,9 @@ struct va_alignment {
>
> extern struct va_alignment va_align;
> extern unsigned long align_vdso_addr(unsigned long);
> +
> +#ifdef CONFIG_ARCH_HAS_PROGRAM_PROPERTIES
> +extern int arch_setup_features(void *ehdr, void *phdr, struct file *file,
> + bool interp);
> +#endif
> #endif /* _ASM_X86_ELF_H */
> diff --git a/arch/x86/include/uapi/asm/elf_property.h b/arch/x86/include/uapi/asm/elf_property.h
> new file mode 100644
> index 000000000000..af361207718c
> --- /dev/null
> +++ b/arch/x86/include/uapi/asm/elf_property.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _UAPI_ASM_X86_ELF_PROPERTY_H
> +#define _UAPI_ASM_X86_ELF_PROPERTY_H
> +
> +/*
> + * pr_type
> + */
> +#define GNU_PROPERTY_X86_FEATURE_1_AND (0xc0000002)
> +
> +/*
> + * Bits for GNU_PROPERTY_X86_FEATURE_1_AND
> + */
> +#define GNU_PROPERTY_X86_FEATURE_1_SHSTK (0x00000002)
Hm, these defeinitions aren't much different comparing to NT_*
definitions in include/uapi/linux/elf.h, is it expected that those
properties have to be parsed individually for each architecture?
> +
> +#endif /* _UAPI_ASM_X86_ELF_PROPERTY_H */
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index fbb2d91fb756..36b14ef410c8 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -141,6 +141,8 @@ obj-$(CONFIG_UNWINDER_GUESS) += unwind_guess.o
>
> obj-$(CONFIG_X86_INTEL_CET) += cet.o
>
> +obj-$(CONFIG_ARCH_HAS_PROGRAM_PROPERTIES) += elf.o
Same thing here, enablement of program properties per se seems rather generic.
> diff --git a/arch/x86/kernel/elf.c b/arch/x86/kernel/elf.c
> new file mode 100644
> index 000000000000..2fddd0bc545b
> --- /dev/null
> +++ b/arch/x86/kernel/elf.c
> @@ -0,0 +1,340 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Look at an ELF file's .note.gnu.property and determine if the file
> + * supports shadow stack and/or indirect branch tracking.
> + * The path from the ELF header to the note section is the following:
> + * elfhdr->elf_phdr->elf_note->property[].
> + */
> +
> +#include <asm/cet.h>
> +#include <asm/elf_property.h>
> +#include <asm/prctl.h>
> +#include <asm/processor.h>
> +#include <uapi/linux/elf-em.h>
> +#include <uapi/linux/prctl.h>
> +#include <linux/binfmts.h>
> +#include <linux/elf.h>
> +#include <linux/slab.h>
> +#include <linux/fs.h>
> +#include <linux/uaccess.h>
> +#include <linux/string.h>
> +#include <linux/compat.h>
> +
> +/*
> + * The .note.gnu.property layout:
> + *
> + * struct elf_note {
> + * u32 n_namesz; --> sizeof(n_name[]); always (4)
> + * u32 n_ndescsz;--> sizeof(property[])
> + * u32 n_type; --> always NT_GNU_PROPERTY_TYPE_0
> + * };
> + * char n_name[4]; --> always 'GNU\0'
> + *
> + * struct {
> + * struct property_x86 {
> + * u32 pr_type;
> + * u32 pr_datasz;
> + * };
> + * u8 pr_data[pr_datasz];
> + * }[];
> + */
> +
> +#define BUF_SIZE (PAGE_SIZE / 4)
> +
> +struct property_x86 {
> + u32 pr_type;
> + u32 pr_datasz;
> +};
> +
> +typedef bool (test_fn)(void *buf, u32 *arg);
> +typedef void *(next_fn)(void *buf, u32 *arg);
> +
> +static inline bool test_note_type_0(void *buf, u32 *arg)
> +{
> + struct elf_note *n = buf;
> +
> + return ((n->n_namesz == 4) && (memcmp(n + 1, "GNU", 4) == 0) &&
> + (n->n_type == NT_GNU_PROPERTY_TYPE_0));
> +}
> +
> +static inline void *next_note(void *buf, u32 *arg)
> +{
> + struct elf_note *n = buf;
> + u32 align = *arg;
> + int size;
> +
> + size = round_up(sizeof(*n) + n->n_namesz, align);
> + size = round_up(size + n->n_descsz, align);
> +
> + if (buf + size < buf)
> + return NULL;
> + else
> + return (buf + size);
> +}
> +
> +static inline bool test_property_x86(void *buf, u32 *arg)
> +{
> + struct property_x86 *pr = buf;
> + u32 max_type = *arg;
> +
> + if (pr->pr_type > max_type)
> + *arg = pr->pr_type;
> +
> + return (pr->pr_type == GNU_PROPERTY_X86_FEATURE_1_AND);
> +}
> +
> +static inline void *next_property(void *buf, u32 *arg)
> +{
> + struct property_x86 *pr = buf;
> + u32 max_type = *arg;
> +
> + if ((buf + sizeof(*pr) + pr->pr_datasz < buf) ||
> + (pr->pr_type > GNU_PROPERTY_X86_FEATURE_1_AND) ||
> + (pr->pr_type > max_type))
> + return NULL;
> + else
> + return (buf + sizeof(*pr) + pr->pr_datasz);
> +}
> +
> +/*
> + * Scan 'buf' for a pattern; return true if found.
> + * *pos is the distance from the beginning of buf to where
> + * the searched item or the next item is located.
> + */
> +static int scan(u8 *buf, u32 buf_size, int item_size,
> + test_fn test, next_fn next, u32 *arg, u32 *pos)
> +{
> + int found = 0;
> + u8 *p, *max;
> +
> + max = buf + buf_size;
> + if (max < buf)
> + return 0;
> +
> + p = buf;
> +
> + while ((p + item_size < max) && (p + item_size > buf)) {
> + if (test(p, arg)) {
> + found = 1;
> + break;
> + }
> +
> + p = next(p, arg);
> + }
> +
> + *pos = (p + item_size <= buf) ? 0 : (u32)(p - buf);
> + return found;
> +}
> +
> +/*
> + * Search a NT_GNU_PROPERTY_TYPE_0 for GNU_PROPERTY_X86_FEATURE_1_AND.
> + */
> +static int find_feature_x86(struct file *file, unsigned long desc_size,
> + loff_t file_offset, u8 *buf, u32 *feature)
> +{
> + u32 buf_pos;
> + unsigned long read_size;
> + unsigned long done;
> + int found = 0;
> + int ret = 0;
> + u32 last_pr = 0;
> +
> + *feature = 0;
> + buf_pos = 0;
> +
> + for (done = 0; done < desc_size; done += buf_pos) {
> + read_size = desc_size - done;
> + if (read_size > BUF_SIZE)
> + read_size = BUF_SIZE;
> +
> + ret = kernel_read(file, buf, read_size, &file_offset);
> +
> + if (ret != read_size)
> + return (ret < 0) ? ret : -EIO;
> +
> + ret = 0;
> + found = scan(buf, read_size, sizeof(struct property_x86),
> + test_property_x86, next_property,
> + &last_pr, &buf_pos);
> +
> + if ((!buf_pos) || found)
> + break;
> +
> + file_offset += buf_pos - read_size;
> + }
> +
> + if (found) {
> + struct property_x86 *pr =
> + (struct property_x86 *)(buf + buf_pos);
> +
> + if (pr->pr_datasz == 4) {
> + u32 *max = (u32 *)(buf + read_size);
> + u32 *data = (u32 *)((u8 *)pr + sizeof(*pr));
> +
> + if (data + 1 <= max) {
> + *feature = *data;
> + } else {
> + file_offset += buf_pos - read_size;
> + file_offset += sizeof(*pr);
> + ret = kernel_read(file, feature, 4,
> + &file_offset);
> + }
> + }
> + }
> +
> + return ret;
> +}
> +
> +/*
> + * Search a PT_NOTE segment for the first NT_GNU_PROPERTY_TYPE_0.
> + */
> +static int find_note_type_0(struct file *file, unsigned long note_size,
> + loff_t file_offset, u32 align, u32 *feature)
> +{
> + u8 *buf;
> + u32 buf_pos;
> + unsigned long read_size;
> + unsigned long done;
> + int found = 0;
> + int ret = 0;
> +
> + buf = kmalloc(BUF_SIZE, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + *feature = 0;
> + buf_pos = 0;
> +
> + for (done = 0; done < note_size; done += buf_pos) {
> + read_size = note_size - done;
> + if (read_size > BUF_SIZE)
> + read_size = BUF_SIZE;
> +
> + ret = kernel_read(file, buf, read_size, &file_offset);
> +
> + if (ret != read_size) {
> + ret = (ret < 0) ? ret : -EIO;
> + kfree(buf);
> + return ret;
> + }
> +
> + /*
> + * item_size = sizeof(struct elf_note) + elf_note.n_namesz.
> + * n_namesz is 4 for the note type we look for.
> + */
> + ret = 0;
> + found += scan(buf, read_size, sizeof(struct elf_note) + 4,
> + test_note_type_0, next_note,
> + &align, &buf_pos);
> +
> + file_offset += buf_pos - read_size;
> +
> + if (found == 1) {
> + struct elf_note *n =
> + (struct elf_note *)(buf + buf_pos);
> + u32 start = round_up(sizeof(*n) + n->n_namesz, align);
> + u32 total = round_up(start + n->n_descsz, align);
> +
> + ret = find_feature_x86(file, n->n_descsz,
> + file_offset + start,
> + buf, feature);
> + file_offset += total;
> + buf_pos += total;
> + } else if (!buf_pos) {
> + *feature = 0;
> + break;
> + }
> + }
> +
> + kfree(buf);
> + return ret;
> +}
> +
> +#ifdef CONFIG_COMPAT
> +static int check_notes_32(struct file *file, struct elf32_phdr *phdr,
> + int phnum, u32 *feature)
> +{
> + int i;
> + int err = 0;
> +
> + for (i = 0; i < phnum; i++, phdr++) {
> + if ((phdr->p_type != PT_NOTE) || (phdr->p_align != 4))
> + continue;
> +
> + err = find_note_type_0(file, phdr->p_filesz, phdr->p_offset,
> + phdr->p_align, feature);
> + if (err)
> + return err;
> + }
> +
> + return 0;
> +}
> +#endif
> +
> +#ifdef CONFIG_X86_64
> +static int check_notes_64(struct file *file, struct elf64_phdr *phdr,
> + int phnum, u32 *feature)
> +{
> + int i;
> + int err = 0;
> +
> + for (i = 0; i < phnum; i++, phdr++) {
> + if ((phdr->p_type != PT_NOTE) || (phdr->p_align != 8))
> + continue;
> +
> + err = find_note_type_0(file, phdr->p_filesz, phdr->p_offset,
> + phdr->p_align, feature);
> + if (err)
> + return err;
> + }
> +
> + return 0;
> +}
> +#endif
> +
> +int arch_setup_features(void *ehdr_p, void *phdr_p,
> + struct file *file, bool interp)
> +{
> + int err = 0;
> + u32 feature = 0;
> +
> + struct elf64_hdr *ehdr64 = ehdr_p;
> +
> + if (!cpu_feature_enabled(X86_FEATURE_SHSTK))
> + return 0;
> +
> + if (ehdr64->e_ident[EI_CLASS] == ELFCLASS64) {
> + struct elf64_phdr *phdr64 = phdr_p;
> +
> + err = check_notes_64(file, phdr64, ehdr64->e_phnum,
> + &feature);
> + if (err < 0)
> + goto out;
> + } else {
> +#ifdef CONFIG_COMPAT
> + struct elf32_hdr *ehdr32 = ehdr_p;
> +
> + if (ehdr32->e_ident[EI_CLASS] == ELFCLASS32) {
> + struct elf32_phdr *phdr32 = phdr_p;
> +
> + err = check_notes_32(file, phdr32, ehdr32->e_phnum,
> + &feature);
> + if (err < 0)
> + goto out;
> + }
> +#endif
> + }
> +
> + memset(¤t->thread.cet, 0, sizeof(struct cet_status));
> +
> + if (cpu_feature_enabled(X86_FEATURE_SHSTK)) {
> + if (feature & GNU_PROPERTY_X86_FEATURE_1_SHSTK) {
> + err = cet_setup_shstk();
> + if (err < 0)
> + goto out;
> + }
> + }
> +
> +out:
> + return err;
> +}
There's a lot of similar code with bpf stackmap .build-id code (commit
v4.17-rc1~148^2~156^2~3^2~1), it might be worthy generalising some ELF
traversal routines, since there's general need of parsing ELF property
segments.
^ permalink raw reply
* Re: [RFC PATCH] mm, proc: report PR_SET_THP_DISABLE in proc
From: David Rientjes @ 2018-10-03 22:51 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, Vlastimil Babka, Alexey Dobriyan,
Kirill A. Shutemov, linux-kernel, linux-mm, linux-api
In-Reply-To: <20181003073640.GF18290@dhcp22.suse.cz>
On Wed, 3 Oct 2018, Michal Hocko wrote:
> > > So how about this? (not tested yet but it should be pretty
> > > straightforward)
> >
> > Umm, prctl(PR_GET_THP_DISABLE)?
>
> /me confused. I thought you want to query for the flag on a
> _different_ process.
Why would we want to check three locations (system wide setting, prctl
setting, madvise setting) to determine if a heap can be backed by thp?
If the nh flag being exported to VmFlag is to be extended beyond what my
patch did, I suggest (1) it does it for the system wide setting as well
and/or (2) calling a helper function to determine if the vma could be
backed by thp in the first place regardless of any setting to determine if
nh/hg is important.
The last thing I suggest is done is adding a third place to check.
^ permalink raw reply
* [RFC v3 1/1] ns: add binfmt_misc to the user namespace
From: Laurent Vivier @ 2018-10-03 22:50 UTC (permalink / raw)
To: linux-kernel
Cc: Dmitry Safonov, Alexander Viro, Jann Horn, linux-fsdevel,
James Bottomley, Andrei Vagin, containers, Eric Biederman,
linux-api, Laurent Vivier
In-Reply-To: <20181003225022.32033-1-laurent@vivier.eu>
This patch allows to have a different binfmt_misc configuration
for each new user namespace. By default, the binfmt_misc configuration
is the one of the host, but if the binfmt_misc filesystem is mounted
in the new namespace a new empty binfmt instance is created and used
in this namespace.
For instance, using "unshare" we can start a chroot of an another
architecture and configure the binfmt_misc interpreter without being root
to run the binaries in this chroot.
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
fs/binfmt_misc.c | 85 +++++++++++++++++++++++-----------
include/linux/user_namespace.h | 15 ++++++
kernel/user.c | 14 ++++++
kernel/user_namespace.c | 9 ++++
4 files changed, 95 insertions(+), 28 deletions(-)
diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index aa4a7a23ff99..78780bc87506 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -38,9 +38,6 @@ enum {
VERBOSE_STATUS = 1 /* make it zero to save 400 bytes kernel memory */
};
-static LIST_HEAD(entries);
-static int enabled = 1;
-
enum {Enabled, Magic};
#define MISC_FMT_PRESERVE_ARGV0 (1 << 31)
#define MISC_FMT_OPEN_BINARY (1 << 30)
@@ -60,10 +57,7 @@ typedef struct {
struct file *interp_file;
} Node;
-static DEFINE_RWLOCK(entries_lock);
static struct file_system_type bm_fs_type;
-static struct vfsmount *bm_mnt;
-static int entry_count;
/*
* Max length of the register string. Determined by:
@@ -85,13 +79,13 @@ static int entry_count;
* if we do, return the node, else NULL
* locking is done in load_misc_binary
*/
-static Node *check_file(struct linux_binprm *bprm)
+static Node *check_file(struct user_namespace *ns, struct linux_binprm *bprm)
{
char *p = strrchr(bprm->interp, '.');
struct list_head *l;
/* Walk all the registered handlers. */
- list_for_each(l, &entries) {
+ list_for_each(l, &ns->binfmt_ns->entries) {
Node *e = list_entry(l, Node, list);
char *s;
int j;
@@ -133,17 +127,18 @@ static int load_misc_binary(struct linux_binprm *bprm)
struct file *interp_file = NULL;
int retval;
int fd_binary = -1;
+ struct user_namespace *ns = current_user_ns();
retval = -ENOEXEC;
- if (!enabled)
+ if (!ns->binfmt_ns->enabled)
return retval;
/* to keep locking time low, we copy the interpreter string */
- read_lock(&entries_lock);
- fmt = check_file(bprm);
+ read_lock(&ns->binfmt_ns->entries_lock);
+ fmt = check_file(ns ,bprm);
if (fmt)
dget(fmt->dentry);
- read_unlock(&entries_lock);
+ read_unlock(&ns->binfmt_ns->entries_lock);
if (!fmt)
return retval;
@@ -609,19 +604,19 @@ static void bm_evict_inode(struct inode *inode)
kfree(e);
}
-static void kill_node(Node *e)
+static void kill_node(struct user_namespace *ns, Node *e)
{
struct dentry *dentry;
- write_lock(&entries_lock);
+ write_lock(&ns->binfmt_ns->entries_lock);
list_del_init(&e->list);
- write_unlock(&entries_lock);
+ write_unlock(&ns->binfmt_ns->entries_lock);
dentry = e->dentry;
drop_nlink(d_inode(dentry));
d_drop(dentry);
dput(dentry);
- simple_release_fs(&bm_mnt, &entry_count);
+ simple_release_fs(&ns->binfmt_ns->bm_mnt, &ns->binfmt_ns->entry_count);
}
/* /<entry> */
@@ -651,6 +646,7 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer,
struct dentry *root;
Node *e = file_inode(file)->i_private;
int res = parse_command(buffer, count);
+ struct user_namespace *ns = file->f_path.dentry->d_sb->s_user_ns;
switch (res) {
case 1:
@@ -667,7 +663,7 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer,
inode_lock(d_inode(root));
if (!list_empty(&e->list))
- kill_node(e);
+ kill_node(ns, e);
inode_unlock(d_inode(root));
break;
@@ -693,6 +689,7 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
struct inode *inode;
struct super_block *sb = file_inode(file)->i_sb;
struct dentry *root = sb->s_root, *dentry;
+ struct user_namespace *ns = file->f_path.dentry->d_sb->s_user_ns;
int err = 0;
e = create_entry(buffer, count);
@@ -716,7 +713,8 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
if (!inode)
goto out2;
- err = simple_pin_fs(&bm_fs_type, &bm_mnt, &entry_count);
+ err = simple_pin_fs(&bm_fs_type, &ns->binfmt_ns->bm_mnt,
+ &ns->binfmt_ns->entry_count);
if (err) {
iput(inode);
inode = NULL;
@@ -725,12 +723,16 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
if (e->flags & MISC_FMT_OPEN_FILE) {
struct file *f;
+ const struct cred *old_cred;
+ old_cred = override_creds(file->f_cred);
f = open_exec(e->interpreter);
+ revert_creds(old_cred);
if (IS_ERR(f)) {
err = PTR_ERR(f);
pr_notice("register: failed to install interpreter file %s\n", e->interpreter);
- simple_release_fs(&bm_mnt, &entry_count);
+ simple_release_fs(&ns->binfmt_ns->bm_mnt,
+ &ns->binfmt_ns->entry_count);
iput(inode);
inode = NULL;
goto out2;
@@ -743,9 +745,9 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
inode->i_fop = &bm_entry_operations;
d_instantiate(dentry, inode);
- write_lock(&entries_lock);
- list_add(&e->list, &entries);
- write_unlock(&entries_lock);
+ write_lock(&ns->binfmt_ns->entries_lock);
+ list_add(&e->list, &ns->binfmt_ns->entries);
+ write_unlock(&ns->binfmt_ns->entries_lock);
err = 0;
out2:
@@ -770,7 +772,8 @@ static const struct file_operations bm_register_operations = {
static ssize_t
bm_status_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
{
- char *s = enabled ? "enabled\n" : "disabled\n";
+ struct user_namespace *ns = file->f_path.dentry->d_sb->s_user_ns;
+ char *s = ns->binfmt_ns->enabled ? "enabled\n" : "disabled\n";
return simple_read_from_buffer(buf, nbytes, ppos, s, strlen(s));
}
@@ -778,25 +781,27 @@ bm_status_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
static ssize_t bm_status_write(struct file *file, const char __user *buffer,
size_t count, loff_t *ppos)
{
+ struct user_namespace *ns = file->f_path.dentry->d_sb->s_user_ns;
int res = parse_command(buffer, count);
struct dentry *root;
switch (res) {
case 1:
/* Disable all handlers. */
- enabled = 0;
+ ns->binfmt_ns->enabled = 0;
break;
case 2:
/* Enable all handlers. */
- enabled = 1;
+ ns->binfmt_ns->enabled = 1;
break;
case 3:
/* Delete all handlers. */
root = file_inode(file)->i_sb->s_root;
inode_lock(d_inode(root));
- while (!list_empty(&entries))
- kill_node(list_first_entry(&entries, Node, list));
+ while (!list_empty(&ns->binfmt_ns->entries))
+ kill_node(ns, list_first_entry(&ns->binfmt_ns->entries,
+ Node, list));
inode_unlock(d_inode(root));
break;
@@ -838,7 +843,30 @@ static int bm_fill_super(struct super_block *sb, void *data, int silent)
static struct dentry *bm_mount(struct file_system_type *fs_type,
int flags, const char *dev_name, void *data)
{
- return mount_single(fs_type, flags, data, bm_fill_super);
+ struct user_namespace *ns = current_user_ns();
+
+ /* create a new binfmt namespace
+ * if we are not in the first user namespace
+ * but the binfmt namespace is the first one
+ */
+ if (ns != &init_user_ns && ns->binfmt_ns == &init_binfmt_ns) {
+ struct binfmt_namespace *binfmt_ns;
+
+ binfmt_ns = kmalloc(sizeof(struct binfmt_namespace),
+ GFP_KERNEL);
+ if (binfmt_ns == NULL) {
+ return ERR_PTR(-ENOMEM);
+ }
+ INIT_LIST_HEAD(&binfmt_ns->entries);
+ binfmt_ns->enabled = 1;
+ rwlock_init(&binfmt_ns->entries_lock);
+ binfmt_ns->bm_mnt = NULL;
+ binfmt_ns->entry_count = 0;
+ ns->binfmt_ns = binfmt_ns;
+ }
+
+ return mount_ns(fs_type, flags, data, ns, ns,
+ bm_fill_super);
}
static struct linux_binfmt misc_format = {
@@ -849,6 +877,7 @@ static struct linux_binfmt misc_format = {
static struct file_system_type bm_fs_type = {
.owner = THIS_MODULE,
.name = "binfmt_misc",
+ .fs_flags = FS_USERNS_MOUNT,
.mount = bm_mount,
.kill_sb = kill_litter_super,
};
diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index d6b74b91096b..319141da5315 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -52,6 +52,18 @@ enum ucount_type {
UCOUNT_COUNTS,
};
+#if IS_ENABLED(CONFIG_BINFMT_MISC)
+struct binfmt_namespace {
+ struct list_head entries;
+ rwlock_t entries_lock;
+ int enabled;
+ struct vfsmount *bm_mnt;
+ int entry_count;
+} __randomize_layout;
+
+extern struct binfmt_namespace init_binfmt_ns;
+#endif
+
struct user_namespace {
struct uid_gid_map uid_map;
struct uid_gid_map gid_map;
@@ -76,6 +88,9 @@ struct user_namespace {
#endif
struct ucounts *ucounts;
int ucount_max[UCOUNT_COUNTS];
+#if IS_ENABLED(CONFIG_BINFMT_MISC)
+ struct binfmt_namespace *binfmt_ns;
+#endif
} __randomize_layout;
struct ucounts {
diff --git a/kernel/user.c b/kernel/user.c
index 0df9b1640b2a..220ab2053d44 100644
--- a/kernel/user.c
+++ b/kernel/user.c
@@ -19,6 +19,17 @@
#include <linux/user_namespace.h>
#include <linux/proc_ns.h>
+#if IS_ENABLED(CONFIG_BINFMT_MISC)
+struct binfmt_namespace init_binfmt_ns = {
+ .entries = LIST_HEAD_INIT(init_binfmt_ns.entries),
+ .enabled = 1,
+ .entries_lock = __RW_LOCK_UNLOCKED(init_binfmt_ns.entries_lock),
+ .bm_mnt = NULL,
+ .entry_count = 0,
+};
+EXPORT_SYMBOL_GPL(init_binfmt_ns);
+#endif
+
/*
* userns count is 1 for root user, 1 for init_uts_ns,
* and 1 for... ?
@@ -66,6 +77,9 @@ struct user_namespace init_user_ns = {
.persistent_keyring_register_sem =
__RWSEM_INITIALIZER(init_user_ns.persistent_keyring_register_sem),
#endif
+#if IS_ENABLED(CONFIG_BINFMT_MISC)
+ .binfmt_ns = &init_binfmt_ns,
+#endif
};
EXPORT_SYMBOL_GPL(init_user_ns);
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index e5222b5fb4fe..dec0ab4a729a 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -140,6 +140,10 @@ int create_user_ns(struct cred *new)
if (!setup_userns_sysctls(ns))
goto fail_keyring;
+#if IS_ENABLED(CONFIG_BINFMT_MISC)
+ ns->binfmt_ns = &init_binfmt_ns;
+#endif
+
set_cred_user_ns(new, ns);
return 0;
fail_keyring:
@@ -195,6 +199,11 @@ static void free_user_ns(struct work_struct *work)
kfree(ns->projid_map.forward);
kfree(ns->projid_map.reverse);
}
+#if IS_ENABLED(CONFIG_BINFMT_MISC)
+ if (ns->binfmt_ns != &init_binfmt_ns) {
+ kfree(ns->binfmt_ns);
+ }
+#endif
retire_userns_sysctls(ns);
#ifdef CONFIG_PERSISTENT_KEYRINGS
key_put(ns->persistent_keyring_register);
--
2.17.1
^ permalink raw reply related
* [RFC v3 0/1] ns: introduce binfmt_misc namespace
From: Laurent Vivier @ 2018-10-03 22:50 UTC (permalink / raw)
To: linux-kernel
Cc: Dmitry Safonov, Alexander Viro, Jann Horn, linux-fsdevel,
James Bottomley, Andrei Vagin, containers, Eric Biederman,
linux-api, Laurent Vivier
v3: create a structure to store binfmt_misc data,
add a pointer to this structure in the user_namespace structure,
in init_user_ns structure this pointer points to an init_binfmt_ns
structure. And all new user namespaces point to this init structure.
A new binfmt namespace structure is allocated if the binfmt_misc
filesystem is mounted in a user namespace that is not the initial
one but its binfmt namespace pointer points to the initial one.
add override_creds()/revert_creds() around open_exec() in
bm_register_write()
v2: no new namespace, binfmt_misc data are now part of
the mount namespace
I put this in mount namespace instead of user namespace
because the mount namespace is already needed and
I don't want to force to have the user namespace for that.
As this is a filesystem, it seems logic to have it here.
This allows to define a new interpreter for each new container.
But the main goal is to be able to chroot to a directory
using a binfmt_misc interpreter without being root.
I have a modified version of unshare at:
git@github.com:vivier/util-linux.git branch unshare-chroot
with some new options to unshare binfmt_misc namespace and to chroot
to a directory.
If you have a directory /chroot/powerpc/jessie containing debian for powerpc
binaries and a qemu-ppc interpreter, you can do for instance:
$ uname -a
Linux fedora28-wor-2 4.19.0-rc5+ #18 SMP Mon Oct 1 00:32:34 CEST 2018 x86_64 x86_64 x86_64 GNU/Linux
$ ./unshare --map-root-user --fork --pid \
--load-interp ":qemu-ppc:M::\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x14:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff:/qemu-ppc:OC" \
--root=/chroot/powerpc/jessie /bin/bash -l
# uname -a
Linux fedora28-wor-2 4.19.0-rc5+ #18 SMP Mon Oct 1 00:32:34 CEST 2018 ppc GNU/Linux
# id
uid=0(root) gid=0(root) groups=0(root),65534(nogroup)
# ls -l
total 5940
drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:58 bin
drwxr-xr-x. 2 nobody nogroup 4096 Jun 17 20:26 boot
drwxr-xr-x. 4 nobody nogroup 4096 Aug 12 00:08 dev
drwxr-xr-x. 42 nobody nogroup 4096 Sep 28 07:25 etc
drwxr-xr-x. 3 nobody nogroup 4096 Sep 28 07:25 home
drwxr-xr-x. 9 nobody nogroup 4096 Aug 12 00:58 lib
drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:08 media
drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:08 mnt
drwxr-xr-x. 3 nobody nogroup 4096 Aug 12 13:09 opt
dr-xr-xr-x. 143 nobody nogroup 0 Sep 30 23:02 proc
-rwxr-xr-x. 1 nobody nogroup 6009712 Sep 28 07:22 qemu-ppc
drwx------. 3 nobody nogroup 4096 Aug 12 12:54 root
drwxr-xr-x. 3 nobody nogroup 4096 Aug 12 00:08 run
drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:58 sbin
drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:08 srv
drwxr-xr-x. 2 nobody nogroup 4096 Apr 6 2015 sys
drwxrwxrwt. 2 nobody nogroup 4096 Sep 28 10:31 tmp
drwxr-xr-x. 10 nobody nogroup 4096 Aug 12 00:08 usr
drwxr-xr-x. 11 nobody nogroup 4096 Aug 12 00:08 var
If you want to use the qemu binary provided by your distro, you can use
--load-interp ":qemu-ppc:M::\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x14:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff:/bin/qemu-ppc-static:OCF"
With the 'F' flag, qemu-ppc-static will be then loaded from the main root
filesystem before switching to the chroot.
Laurent Vivier (1):
ns: add binfmt_misc to the user namespace
fs/binfmt_misc.c | 85 +++++++++++++++++++++++-----------
include/linux/user_namespace.h | 15 ++++++
kernel/user.c | 14 ++++++
kernel/user_namespace.c | 9 ++++
4 files changed, 95 insertions(+), 28 deletions(-)
--
2.17.1
^ permalink raw reply
* Re: [PATCH 2/3] namei: implement AT_THIS_ROOT chroot-like path resolution
From: Andy Lutomirski @ 2018-10-03 22:09 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Jann Horn, Eric W. Biederman, Jeff Layton, J. Bruce Fields,
Al Viro, Arnd Bergmann, Shuah Khan, David Howells,
Andrew Lutomirski, Christian Brauner, Tycho Andersen, LKML,
Linux FS Devel, linux-arch, open list:KERNEL SELFTEST FRAMEWORK,
dev, Linux Containers, Linux API
In-Reply-To: <20181002073220.7mzndna4tdnxdvdt@ryuk>
On Tue, Oct 2, 2018 at 12:32 AM Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> On 2018-10-01, Andy Lutomirski <luto@amacapital.net> wrote:
> > >>> Currently most container runtimes try to do this resolution in
> > >>> userspace[1], causing many potential race conditions. In addition, the
> > >>> "obvious" alternative (actually performing a {ch,pivot_}root(2))
> > >>> requires a fork+exec which is *very* costly if necessary for every
> > >>> filesystem operation involving a container.
> > >>
> > >> Wait. fork() I understand, but why exec? And actually, you don't need
> > >> a full fork() either, clone() lets you do this with some process parts
> > >> shared. And then you also shouldn't need to use SCM_RIGHTS, just keep
> > >> the file descriptor table shared. And why chroot()/pivot_root(),
> > >> wouldn't you want to use setns()?
> > >
> > > You're right about this -- for C runtimes. In Go we cannot do a raw
> > > clone() or fork() (if you do it manually with RawSyscall you'll end with
> > > broken runtime state). So you're forced to do fork+exec (which then
> > > means that you can't use CLONE_FILES and must use SCM_RIGHTS). Same goes
> > > for CLONE_VFORK.
> >
> > I must admit that I’m not very sympathetic to the argument that “Go’s
> > runtime model is incompatible with the simpler solution.”
>
> Multi-threaded programs have a similar issue (though with Go it's much
> worse). If you fork a multi-threaded C program then you can only safely
> use AS-Safe glibc functions (those that are safe within a signal
> handler). But if you're just doing three syscalls this shouldn't be as
> big of a problem as Go where you can't even do said syscalls.
>
> > Anyway, it occurs to me that the real problem is that setns() and
> > chroot() are both overkill for this use case.
>
> I agree. My diversion to Go was to explain why it was particularly bad
> for cri-o/rkt/runc/Docker/etc.
>
> > What’s needed is to start your walk from /proc/pid-in-container/root,
> > with two twists:
> >
> > 1. Do the walk as though rooted at a directory. This is basically just
> > your AT_THIS_ROOT, but the footgun is avoided because the dirfd you
> > use is from a foreign namespace, and, except for symlinks to absolute
> > paths, no amount of .. racing is going to escape the *namespace*.
>
> This is quite clever and I'll admit I hadn't thought of this. This
> definitely fixes the ".." issue, but as you've said it won't handle
> absolute symlinks (which means userspace has the same races that we
> currently have even if you assume that you have a container process
> already running -- CVE-2018-15664 is one of many examples of this).
>
> (AT_THIS_ROOT using /proc/$container/root would in principle fix all of
> the mentioned issues -- but as I said before I'd like to see whether
> hardening ".." would be enough to solve the escape problem.)
Hmm. Good point.
^ permalink raw reply
* Re: [RFC PATCH v4 24/27] mm/mmap: Create a guard area between VMAs
From: Eugene Syromiatnikov @ 2018-10-03 21:21 UTC (permalink / raw)
To: Jann Horn
Cc: yu-cheng.yu, Andy Lutomirski, the arch/x86 maintainers,
H . Peter Anvin, Thomas Gleixner, Ingo Molnar, kernel list,
linux-doc, Linux-MM, linux-arch, Linux API, Arnd Bergmann,
Balbir Singh, Cyrill Gorcunov, Dave Hansen, Florian Weimer,
hjl.tools, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg
In-Reply-To: <CAG48ez0KZYv9BECMm0-BNypJ232jrdkFp6_4VYTA=WoB-8w89w@mail.gmail.com>
On Wed, Oct 03, 2018 at 06:52:40PM +0200, Jann Horn wrote:
> On Wed, Oct 3, 2018 at 6:32 PM Eugene Syromiatnikov <esyr@redhat.com> wrote:
> > I'm not sure, however, whether such a change that provides no ability
> > to configure or affect it will go well with all the supported
> > architectures.
>
> Is there a concrete reason why you think an architecture might not
> like this? As far as I can tell, the virtual address space overhead
> should be insignificant even for 32-bit systems.
Not really, and not architectures per se, but judging by some past
experiences with enabling ASLR, I would expect that all kinds of weird
applications may start to behave in all kinds of strange ways.
Not that I have anything more than this doubt, however; but this sort of
change without any ability to tune or revert it still looks unusual to me.
^ permalink raw reply
* Re: Problems with VM_MIXEDMAP removal from /proc/<pid>/smaps
From: Dan Williams @ 2018-10-03 21:13 UTC (permalink / raw)
To: Jan Kara
Cc: linux-xfs, linux-nvdimm, Linux API, Christoph Hellwig, Linux MM,
linux-fsdevel, linux-ext4
In-Reply-To: <20181003164407.GK24030-4I4JzKEfoa/jFM9bn6wA6Q@public.gmane.org>
On Wed, Oct 3, 2018 at 9:46 AM Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> wrote:
>
> On Wed 03-10-18 08:13:37, Dan Williams wrote:
> > On Wed, Oct 3, 2018 at 8:07 AM Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> wrote:
> > > WRT per-inode DAX property, AFAIU that inode flag is just going to be
> > > advisory thing - i.e., use DAX if possible. If you mount a filesystem with
> > > these inode flags set in a configuration which does not allow DAX to be
> > > used, you will still be able to access such inodes but the access will use
> > > page cache instead. And querying these flags should better show real
> > > on-disk status and not just whether DAX is used as that would result in an
> > > even bigger mess. So this feature seems to be somewhat orthogonal to the
> > > API I'm looking for.
> >
> > True, I imagine once we have that flag we will be able to distinguish
> > the "saved" property and the "effective / live" property of DAX...
> > Also it's really not DAX that applications care about as much as "is
> > there page-cache indirection / overhead for this mapping?". That seems
> > to be a narrower guarantee that we can make than what "DAX" might
> > imply.
>
> Right. So what do people think about my suggestion earlier in the thread to
> use madvise(MADV_DIRECT_ACCESS) for this? Currently it would return success
> when DAX is in use, failure otherwise. Later we could extend it to be also
> used as a hint for caching policy for the inode...
The only problem is that you can't use it purely as a query. If we
ever did plumb it to be a hint you could not read the state without
writing the state.
mincore(2) seems to be close the intent of discovering whether RAM is
being consumed for a given address range, but it currently is
implemented to only indicate if *any* mapping is established, not
whether RAM is consumed. I can see an argument that a dax mapped file
should always report an empty mincore vector.
^ permalink raw reply
* Re: [RFC PATCH v4 4/9] mm/mmap: Add IBT bitmap size to address space limit check
From: Eugene Syromiatnikov @ 2018-10-03 20:21 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <20180921150553.21016-5-yu-cheng.yu@intel.com>
On Fri, Sep 21, 2018 at 08:05:48AM -0700, Yu-cheng Yu wrote:
> The indirect branch tracking legacy bitmap takes a large address
> space. This causes may_expand_vm() failure on the address limit
> check. For a IBT-enabled task, add the bitmap size to the
> address limit.
>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
> arch/x86/include/uapi/asm/resource.h | 5 +++++
> include/uapi/asm-generic/resource.h | 3 +++
> mm/mmap.c | 12 +++++++++++-
> 3 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/uapi/asm/resource.h b/arch/x86/include/uapi/asm/resource.h
> index 04bc4db8921b..0741b2a6101a 100644
> --- a/arch/x86/include/uapi/asm/resource.h
> +++ b/arch/x86/include/uapi/asm/resource.h
> @@ -1 +1,6 @@
> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> +#ifdef CONFIG_X86_INTEL_CET
> +#define rlimit_as_extra() current->thread.cet.ibt_bitmap_size
> +#endif
Does this really belong to UAPI?
> +
> #include <asm-generic/resource.h>
> diff --git a/include/uapi/asm-generic/resource.h b/include/uapi/asm-generic/resource.h
> index f12db7a0da64..8a7608a09700 100644
> --- a/include/uapi/asm-generic/resource.h
> +++ b/include/uapi/asm-generic/resource.h
> @@ -58,5 +58,8 @@
> # define RLIM_INFINITY (~0UL)
> #endif
>
> +#ifndef rlimit_as_extra
> +#define rlimit_as_extra() 0
> +#endif
And this?
> #endif /* _UAPI_ASM_GENERIC_RESOURCE_H */
> diff --git a/mm/mmap.c b/mm/mmap.c
> index fa581ced3f56..397b8cb0b0af 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -3237,7 +3237,17 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
> */
> bool may_expand_vm(struct mm_struct *mm, vm_flags_t flags, unsigned long npages)
> {
> - if (mm->total_vm + npages > rlimit(RLIMIT_AS) >> PAGE_SHIFT)
> + unsigned long as_limit = rlimit(RLIMIT_AS);
> + unsigned long as_limit_plus = as_limit + rlimit_as_extra();
> +
> + /* as_limit_plus overflowed */
> + if (as_limit_plus < as_limit)
> + as_limit_plus = RLIM_INFINITY;
> +
> + if (as_limit_plus > as_limit)
> + as_limit = as_limit_plus;
> +
> + if (mm->total_vm + npages > as_limit >> PAGE_SHIFT)
I wonder, how realistic a scenario where a userspace application enables IBT,
configures a huge prefetchable IO memory region (that just ignores bits
of offset beyond 16, for example), and start repeatedly loading a legacy
library there at different linear addresses.
> return false;
>
> if (is_data_mapping(flags) &&
> --
> 2.17.1
>
^ permalink raw reply
* Re: [RFC PATCH v4 3/9] x86/cet/ibt: Add IBT legacy code bitmap allocation function
From: Eugene Syromiatnikov @ 2018-10-03 19:57 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <20180921150553.21016-4-yu-cheng.yu@intel.com>
On Fri, Sep 21, 2018 at 08:05:47AM -0700, Yu-cheng Yu wrote:
> Indirect branch tracking provides an optional legacy code bitmap
> that indicates locations of non-IBT compatible code. When set,
> each bit in the bitmap represents a page in the linear address is
> legacy code.
>
> We allocate the bitmap only when the application requests it.
> Most applications do not need the bitmap.
>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
> arch/x86/kernel/cet.c | 45 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
>
> diff --git a/arch/x86/kernel/cet.c b/arch/x86/kernel/cet.c
> index 6adfe795d692..a65d9745af08 100644
> --- a/arch/x86/kernel/cet.c
> +++ b/arch/x86/kernel/cet.c
> @@ -314,3 +314,48 @@ void cet_disable_ibt(void)
> wrmsrl(MSR_IA32_U_CET, r);
> current->thread.cet.ibt_enabled = 0;
> }
> +
> +int cet_setup_ibt_bitmap(void)
> +{
> + u64 r;
> + unsigned long bitmap;
> + unsigned long size;
> +
> + if (!cpu_feature_enabled(X86_FEATURE_IBT))
> + return -EOPNOTSUPP;
> +
> + if (!current->thread.cet.ibt_bitmap_addr) {
> + /*
> + * Calculate size and put in thread header.
> + * may_expand_vm() needs this information.
> + */
> + size = TASK_SIZE / PAGE_SIZE / BITS_PER_BYTE;
TASK_SIZE_MAX is likely needed here, as an application can easily switch
between long an 32-bit protected mode. And then the case of a CPU that
doesn't support 5LPT.
> + current->thread.cet.ibt_bitmap_size = size;
> + bitmap = do_mmap_locked(0, size, PROT_READ | PROT_WRITE,
> + MAP_ANONYMOUS | MAP_PRIVATE,
> + VM_DONTDUMP);
> +
> + if (bitmap >= TASK_SIZE) {
Shouldn't bitmap be unmapped here?
> + current->thread.cet.ibt_bitmap_size = 0;
> + return -ENOMEM;
> + }
> +
> + current->thread.cet.ibt_bitmap_addr = bitmap;
> + }
> +
> + /*
> + * Lower bits of MSR_IA32_CET_LEG_IW_EN are for IBT
> + * settings. Clear lower bits even bitmap is already
> + * page-aligned.
> + */
> + bitmap = current->thread.cet.ibt_bitmap_addr;
> + bitmap &= PAGE_MASK;
In a hypothetical situation of bitmap & PAGE_MASK < bitmap that would lead
to bitmap pointing to unmapped memory. A check that bitmap is sane would
probably be better.
> +
> + /*
> + * Turn on IBT legacy bitmap.
> + */
> + rdmsrl(MSR_IA32_U_CET, r);
> + r |= (MSR_IA32_CET_LEG_IW_EN | bitmap);
> + wrmsrl(MSR_IA32_U_CET, r);
> + return 0;
> +}
> --
> 2.17.1
>
^ permalink raw reply
* Re: [RFC v2 v2 1/1] ns: add binfmt_misc to the mount namespace
From: Jann Horn @ 2018-10-03 19:26 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Laurent Vivier, kernel list, dima, Andrei Vagin, Al Viro,
James Bottomley, containers, linux-fsdevel, Linux API
In-Reply-To: <87o9cbo96j.fsf@xmission.com>
On Wed, Oct 3, 2018 at 8:07 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
> Laurent Vivier <laurent@vivier.eu> writes:
> > This patch allows to have a different binftm_misc configuration
> > in each container we mount binfmt_misc filesystem with mount namespace
> > enabled.
> >
> > A container started without the CLONE_NEWNS will use the host binfmt_misc
> > configuration, otherwise the container starts with an empty binfmt_misc
> > interpreters list.
> >
> > For instance, using "unshare" we can start a chroot of an another
> > architecture and configure the binfmt_misc interpreted without being root
> > to run the binaries in this chroot.
>
> A couple of things.
> As has already been mentioned on your previous version anything that
> comes through the filesystem interface needs to lookup up the local
> binfmt context not through current but through file->f_dentry->d_sb.
> AKA the superblock of the mounted filesystem.
Something else: bm_register_write() currently calls into open_exec(),
which uses the credentials of current. That's not really allowed in
this context - but so far, it's not a big deal because only
init-namespace root can reach this code. Before you expose this stuff
to unprivileged userspace, this needs to get fixed; perhaps by
wrapping the open_exec() call in override_creds(file->f_cred) and
revert_creds().
^ permalink raw reply
* Re: [RFC PATCH v4 2/9] x86/cet/ibt: User-mode indirect branch tracking support
From: Eugene Syromiatnikov @ 2018-10-03 18:58 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <20180921150553.21016-3-yu-cheng.yu@intel.com>
On Fri, Sep 21, 2018 at 08:05:46AM -0700, Yu-cheng Yu wrote:
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index bffa9ef47832..230f65ee881e 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -434,6 +435,23 @@ static __init int setup_disable_shstk(char *s)
> __setup("no_cet_shstk", setup_disable_shstk);
> #endif
>
> +#ifdef CONFIG_X86_INTEL_BRANCH_TRACKING_USER
> +static __init int setup_disable_ibt(char *s)
> +{
> + /* require an exact match without trailing characters */
> + if (strlen(s))
> + return 0;
"s[0] (!= '\0')" will do the same check in constant time.
^ permalink raw reply
* Re: [RFC PATCH v4 26/27] x86/cet/shstk: Add arch_prctl functions for Shadow Stack
From: Eugene Syromiatnikov @ 2018-10-03 17:57 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <20180921150351.20898-27-yu-cheng.yu@intel.com>
On Fri, Sep 21, 2018 at 08:03:50AM -0700, Yu-cheng Yu wrote:
> arch_prctl(ARCH_CET_STATUS, unsigned long *addr)
> Return CET feature status.
>
> The parameter 'addr' is a pointer to a user buffer.
> On returning to the caller, the kernel fills the following
> information:
>
> *addr = SHSTK/IBT status
> *(addr + 1) = SHSTK base address
> *(addr + 2) = SHSTK size
The subtle detail here is that x32 binaries will get 64-bit value, which
is not entirely obvious. I think, it might be better to define
a structure type for it as a part of UAPI, for example:
struct user_cet_status {
__u32 struct_size;
__u32 features;
__kernel_ulong_t shstk_base;
__kernel_ulong_t shstk_size;
};
Adding "struct_size" field along with appropriate checks will also
allow for possible extensions, if they ever appear.
> arch_prctl(ARCH_CET_DISABLE, unsigned long features)
> Disable CET features specified in 'features'. Return
> -EPERM if CET is locked.
While x86_64 and x32 will have 64-bit space for feature bits, IA-32 will
have only 32 bits.
> arch_prctl(ARCH_CET_LOCK)
> Lock in CET feature.
>
> arch_prctl(ARCH_CET_ALLOC_SHSTK, unsigned long *addr)
> Allocate a new SHSTK.
>
> The parameter 'addr' is a pointer to a user buffer and indicates
> the desired SHSTK size to allocate. On returning to the caller
> the buffer contains the address of the new SHSTK.
Again, on x32 that will be a pointer to a 64-bit value, which is not
entirely obvious from this description.
It's not clear whether inability to enable some CET feature in runtime
is unavailable by design or by omission; same for setting (an allocated)
shadow stack as task's shadow stack.
>
> Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
> arch/x86/include/asm/cet.h | 5 ++
> arch/x86/include/uapi/asm/prctl.h | 5 ++
> arch/x86/kernel/Makefile | 2 +-
> arch/x86/kernel/cet.c | 27 +++++++++++
> arch/x86/kernel/cet_prctl.c | 79 +++++++++++++++++++++++++++++++
> arch/x86/kernel/process.c | 5 ++
> 6 files changed, 122 insertions(+), 1 deletion(-)
> create mode 100644 arch/x86/kernel/cet_prctl.c
>
> diff --git a/arch/x86/include/asm/cet.h b/arch/x86/include/asm/cet.h
> index b7b33e1026bb..212bd68e31d3 100644
> --- a/arch/x86/include/asm/cet.h
> +++ b/arch/x86/include/asm/cet.h
> @@ -12,19 +12,24 @@ struct task_struct;
> struct cet_status {
> unsigned long shstk_base;
> unsigned long shstk_size;
> + unsigned int locked:1;
> unsigned int shstk_enabled:1;
> };
>
> #ifdef CONFIG_X86_INTEL_CET
> +int prctl_cet(int option, unsigned long arg2);
> int cet_setup_shstk(void);
> int cet_setup_thread_shstk(struct task_struct *p);
> +int cet_alloc_shstk(unsigned long *arg);
> void cet_disable_shstk(void);
> void cet_disable_free_shstk(struct task_struct *p);
> int cet_restore_signal(unsigned long ssp);
> int cet_setup_signal(bool ia32, unsigned long rstor, unsigned long *new_ssp);
> #else
> +static inline int prctl_cet(int option, unsigned long arg2) { return 0; }
Why 0 and not -EINVAL?
> static inline int cet_setup_shstk(void) { return 0; }
0 here also looks strange.
> static inline int cet_setup_thread_shstk(struct task_struct *p) { return 0; }
And here.
> +static inline int cet_alloc_shstk(unsigned long *arg) { return -EINVAL; }
> static inline void cet_disable_shstk(void) {}
> static inline void cet_disable_free_shstk(struct task_struct *p) {}
> static inline int cet_restore_signal(unsigned long ssp) { return 0; }
> diff --git a/arch/x86/include/uapi/asm/prctl.h b/arch/x86/include/uapi/asm/prctl.h
> index 5a6aac9fa41f..3aec1088e01d 100644
> --- a/arch/x86/include/uapi/asm/prctl.h
> +++ b/arch/x86/include/uapi/asm/prctl.h
> @@ -14,4 +14,9 @@
> #define ARCH_MAP_VDSO_32 0x2002
> #define ARCH_MAP_VDSO_64 0x2003
>
> +#define ARCH_CET_STATUS 0x3001
> +#define ARCH_CET_DISABLE 0x3002
> +#define ARCH_CET_LOCK 0x3003
> +#define ARCH_CET_ALLOC_SHSTK 0x3004
> +
> #endif /* _ASM_X86_PRCTL_H */
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index 36b14ef410c8..b9e6cdc6b4f7 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -139,7 +139,7 @@ obj-$(CONFIG_UNWINDER_ORC) += unwind_orc.o
> obj-$(CONFIG_UNWINDER_FRAME_POINTER) += unwind_frame.o
> obj-$(CONFIG_UNWINDER_GUESS) += unwind_guess.o
>
> -obj-$(CONFIG_X86_INTEL_CET) += cet.o
> +obj-$(CONFIG_X86_INTEL_CET) += cet.o cet_prctl.o
>
> obj-$(CONFIG_ARCH_HAS_PROGRAM_PROPERTIES) += elf.o
>
> diff --git a/arch/x86/kernel/cet.c b/arch/x86/kernel/cet.c
> index ce0b3b7b1160..1c2689738604 100644
> --- a/arch/x86/kernel/cet.c
> +++ b/arch/x86/kernel/cet.c
> @@ -110,6 +110,33 @@ static int create_rstor_token(bool ia32, unsigned long ssp,
> return 0;
> }
>
> +int cet_alloc_shstk(unsigned long *arg)
> +{
> + unsigned long len = *arg;
> + unsigned long addr;
> + unsigned long token;
> + unsigned long ssp;
> +
> + addr = do_mmap_locked(0, len, PROT_READ,
> + MAP_ANONYMOUS | MAP_PRIVATE, VM_SHSTK);
> + if (addr >= TASK_SIZE_MAX)
> + return -ENOMEM;
> +
> + /* Restore token is 8 bytes and aligned to 8 bytes */
> + ssp = addr + len;
> + token = ssp;
> +
> + if (!in_ia32_syscall())
> + token |= 1;
This pair of check and bit or'ing definitely asks for a macro or a
wrapper function.
> + ssp -= 8;
> +
> + if (write_user_shstk_64(ssp, token))
> + return -EINVAL;
Shouldn't addr be unmapped on error?
> + *arg = addr;
> + return 0;
> +}
> +
> int cet_setup_shstk(void)
> {
> unsigned long addr, size;
> diff --git a/arch/x86/kernel/cet_prctl.c b/arch/x86/kernel/cet_prctl.c
> new file mode 100644
> index 000000000000..c4b7c19f5040
> --- /dev/null
> +++ b/arch/x86/kernel/cet_prctl.c
> @@ -0,0 +1,79 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#include <linux/errno.h>
> +#include <linux/uaccess.h>
> +#include <linux/prctl.h>
> +#include <linux/compat.h>
> +#include <asm/processor.h>
> +#include <asm/prctl.h>
> +#include <asm/elf.h>
> +#include <asm/elf_property.h>
> +#include <asm/cet.h>
> +
> +/* See Documentation/x86/intel_cet.txt. */
> +
> +static int handle_get_status(unsigned long arg2)
> +{
> + unsigned int features = 0;
> + unsigned long shstk_base, shstk_size;
> + unsigned long buf[3];
> +
> + if (current->thread.cet.shstk_enabled)
> + features |= GNU_PROPERTY_X86_FEATURE_1_SHSTK;
> +
> + shstk_base = current->thread.cet.shstk_base;
> + shstk_size = current->thread.cet.shstk_size;
> +
> + buf[0] = (unsigned long)features;
> + buf[1] = shstk_base;
> + buf[2] = shstk_size;
> + return copy_to_user((unsigned long __user *)arg2, buf,
> + sizeof(buf));
> +}
> +
> +static int handle_alloc_shstk(unsigned long arg2)
> +{
> + int err = 0;
> + unsigned long shstk_size = 0;
> +
> + if (get_user(shstk_size, (unsigned long __user *)arg2))
> + return -EFAULT;
> +
> + err = cet_alloc_shstk(&shstk_size);
> + if (err)
> + return err;
> +
> + if (put_user(shstk_size, (unsigned long __user *)arg2))
Again, leaking allocated stack.
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +int prctl_cet(int option, unsigned long arg2)
> +{
> + if (!cpu_feature_enabled(X86_FEATURE_SHSTK))
> + return -EINVAL;
> +
> + switch (option) {
> + case ARCH_CET_STATUS:
> + return handle_get_status(arg2);
> +
> + case ARCH_CET_DISABLE:
> + if (current->thread.cet.locked)
> + return -EPERM;
> + if (arg2 & GNU_PROPERTY_X86_FEATURE_1_SHSTK)
> + cet_disable_free_shstk(current);
The rest of bits in arg2 should be 0, otherwise this interface won't be
possible to extend.
> + return 0;
> +
> + case ARCH_CET_LOCK:
> + current->thread.cet.locked = 1;
> + return 0;
> +
> + case ARCH_CET_ALLOC_SHSTK:
> + return handle_alloc_shstk(arg2);
> +
> + default:
> + return -EINVAL;
> + }
> +}
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index 440f012ef925..251b8714f9a3 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -792,6 +792,11 @@ long do_arch_prctl_common(struct task_struct *task, int option,
> return get_cpuid_mode();
> case ARCH_SET_CPUID:
> return set_cpuid_mode(task, cpuid_enabled);
> + case ARCH_CET_STATUS:
> + case ARCH_CET_DISABLE:
> + case ARCH_CET_LOCK:
> + case ARCH_CET_ALLOC_SHSTK:
> + return prctl_cet(option, cpuid_enabled);
It's probably a good opportunity to change the strange name for an argument
of a dispatch call.
^ permalink raw reply
* Re: [RFC PATCH] mm, proc: report PR_SET_THP_DISABLE in proc
From: Mike Rapoport @ 2018-10-03 17:33 UTC (permalink / raw)
To: David Rientjes
Cc: Michal Hocko, Andrew Morton, Vlastimil Babka, Alexey Dobriyan,
Kirill A. Shutemov, linux-kernel, linux-mm, linux-api
In-Reply-To: <alpine.DEB.2.21.1810021329260.87409@chino.kir.corp.google.com>
On Tue, Oct 02, 2018 at 01:29:42PM -0700, David Rientjes wrote:
> On Tue, 2 Oct 2018, Michal Hocko wrote:
>
> > On Wed 26-09-18 08:06:24, Michal Hocko wrote:
> > > On Tue 25-09-18 15:04:06, Andrew Morton wrote:
> > > > On Tue, 25 Sep 2018 14:45:19 -0700 (PDT) David Rientjes <rientjes@google.com> wrote:
> > > >
> > > > > > > It is also used in
> > > > > > > automated testing to ensure that vmas get disabled for thp appropriately
> > > > > > > and we used "nh" since that is how PR_SET_THP_DISABLE previously enforced
> > > > > > > this, and those tests now break.
> > > > > >
> > > > > > This sounds like a bit of an abuse to me. It shows how an internal
> > > > > > implementation detail leaks out to the userspace which is something we
> > > > > > should try to avoid.
> > > > > >
> > > > >
> > > > > Well, it's already how this has worked for years before commit
> > > > > 1860033237d4 broke it. Changing the implementation in the kernel is fine
> > > > > as long as you don't break userspace who relies on what is exported to it
> > > > > and is the only way to determine if MADV_NOHUGEPAGE is preventing it from
> > > > > being backed by hugepages.
> > > >
> > > > 1860033237d4 was over a year ago so perhaps we don't need to be
> > > > too worried about restoring the old interface. In which case
> > > > we have an opportunity to make improvements such as that suggested
> > > > by Michal?
> > >
> > > Yeah, can we add a way to export PR_SET_THP_DISABLE to userspace
> > > somehow? E.g. /proc/<pid>/status. It is a process wide thing so
> > > reporting it per VMA sounds strange at best.
> >
> > So how about this? (not tested yet but it should be pretty
> > straightforward)
>
> Umm, prctl(PR_GET_THP_DISABLE)?
>
~/git/linux$ git grep PR_GET_THP_DISABLE
include/uapi/linux/prctl.h:#define PR_GET_THP_DISABLE 42
kernel/sys.c: case PR_GET_THP_DISABLE:
tools/include/uapi/linux/prctl.h:#define PR_GET_THP_DISABLE 42
--
Sincerely yours,
Mike.
^ permalink raw reply
* Re: [RFC PATCH v4 24/27] mm/mmap: Create a guard area between VMAs
From: Jann Horn @ 2018-10-03 16:52 UTC (permalink / raw)
To: Eugene Syromiatnikov
Cc: yu-cheng.yu, Andy Lutomirski, the arch/x86 maintainers,
H . Peter Anvin, Thomas Gleixner, Ingo Molnar, kernel list,
linux-doc, Linux-MM, linux-arch, Linux API, Arnd Bergmann,
Balbir Singh, Cyrill Gorcunov, Dave Hansen, Florian Weimer,
hjl.tools, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg
In-Reply-To: <20181003163226.GC9449@asgard.redhat.com>
On Wed, Oct 3, 2018 at 6:32 PM Eugene Syromiatnikov <esyr@redhat.com> wrote:
> On Wed, Oct 03, 2018 at 09:00:04AM -0700, Yu-cheng Yu wrote:
> > On Tue, 2018-10-02 at 22:36 -0700, Andy Lutomirski wrote:
> > > On Tue, Oct 2, 2018 at 9:55 PM Eugene Syromiatnikov <esyr@redhat.com> wrote:
> > > >
> > > > On Fri, Sep 21, 2018 at 08:03:48AM -0700, Yu-cheng Yu wrote:
> > > > > Create a guard area between VMAs, to detect memory corruption.
> > > >
> > > > Do I understand correctly that with this patch a user space program
> > > > no longer be able to place two mappings back to back? If it is so,
> > > > it will likely break a lot of things; for example, it's a common ring
> > > > buffer implementations technique, to map buffer memory twice back
> > > > to back in order to avoid special handling of items wrapping its end.
> > >
> > > I haven't checked what the patch actually does, but it shouldn't have
> > > any affect on MAP_FIXED or the new no-replace MAP_FIXED variant.
> > >
> > > --Andy
> >
> > I did some mmap tests with/without MAP_FIXED, and it works as intended.
> > In addition to the ring buffer, are there other test cases?
>
> Right, after some more code reading I figured out that it indeed
> shouldn't affect MAP_FIXED, thank you for confirmation.
>
> I'm not sure, however, whether such a change that provides no ability
> to configure or affect it will go well with all the supported
> architectures.
Is there a concrete reason why you think an architecture might not
like this? As far as I can tell, the virtual address space overhead
should be insignificant even for 32-bit systems.
^ permalink raw reply
* Re: [RFC PATCH v4 20/27] x86/cet/shstk: Signal handling for shadow stack
From: Jann Horn @ 2018-10-03 16:46 UTC (permalink / raw)
To: yu-cheng.yu
Cc: the arch/x86 maintainers, H . Peter Anvin, Thomas Gleixner,
Ingo Molnar, kernel list, linux-doc, Linux-MM, linux-arch,
Linux API, Arnd Bergmann, Andy Lutomirski, Balbir Singh,
Cyrill Gorcunov, Dave Hansen, Florian Weimer, hjl.tools,
Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov
In-Reply-To: <20180921150351.20898-21-yu-cheng.yu@intel.com>
On Fri, Sep 21, 2018 at 5:09 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> When setting up a signal, the kernel creates a shadow stack
> restore token at the current SHSTK address and then stores the
> token's address in the signal frame, right after the FPU state.
> Before restoring a signal, the kernel verifies and then uses the
> restore token to set the SHSTK pointer.
[...]
> +#ifdef CONFIG_X86_64
> +static int copy_ext_from_user(struct sc_ext *ext, void __user *fpu)
> +{
> + void __user *p;
> +
> + if (!fpu)
> + return -EINVAL;
> +
> + p = fpu + fpu_user_xstate_size + FP_XSTATE_MAGIC2_SIZE;
> + p = (void __user *)ALIGN((unsigned long)p, 8);
> +
> + if (!access_ok(VERIFY_READ, p, sizeof(*ext)))
> + return -EFAULT;
> +
> + if (__copy_from_user(ext, p, sizeof(*ext)))
> + return -EFAULT;
Why do you first manually call access_ok(), then call
__copy_from_user() with the same size? Just use "if
(copy_from_user(ext, p, sizeof(*ext)))" (without underscores) and get
rid of the access_ok().
> + if (ext->total_size != sizeof(*ext))
> + return -EINVAL;
> + return 0;
> +}
> +
> +static int copy_ext_to_user(void __user *fpu, struct sc_ext *ext)
> +{
> + void __user *p;
> +
> + if (!fpu)
> + return -EINVAL;
> +
> + if (ext->total_size != sizeof(*ext))
> + return -EINVAL;
> +
> + p = fpu + fpu_user_xstate_size + FP_XSTATE_MAGIC2_SIZE;
> + p = (void __user *)ALIGN((unsigned long)p, 8);
> +
> + if (!access_ok(VERIFY_WRITE, p, sizeof(*ext)))
> + return -EFAULT;
> +
> + if (__copy_to_user(p, ext, sizeof(*ext)))
> + return -EFAULT;
Same as above.
> + return 0;
> +}
^ permalink raw reply
* Re: Problems with VM_MIXEDMAP removal from /proc/<pid>/smaps
From: Jan Kara @ 2018-10-03 16:44 UTC (permalink / raw)
To: Dan Williams
Cc: linux-xfs, linux-nvdimm, Linux API, Christoph Hellwig, Linux MM,
linux-fsdevel, Jan Kara, linux-ext4
In-Reply-To: <CAPcyv4iJvN6_Cf6tw=5a=Uh99LfMFKU7n8QkGcz1ZaxL0Oi-3w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed 03-10-18 08:13:37, Dan Williams wrote:
> On Wed, Oct 3, 2018 at 8:07 AM Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> wrote:
> > WRT per-inode DAX property, AFAIU that inode flag is just going to be
> > advisory thing - i.e., use DAX if possible. If you mount a filesystem with
> > these inode flags set in a configuration which does not allow DAX to be
> > used, you will still be able to access such inodes but the access will use
> > page cache instead. And querying these flags should better show real
> > on-disk status and not just whether DAX is used as that would result in an
> > even bigger mess. So this feature seems to be somewhat orthogonal to the
> > API I'm looking for.
>
> True, I imagine once we have that flag we will be able to distinguish
> the "saved" property and the "effective / live" property of DAX...
> Also it's really not DAX that applications care about as much as "is
> there page-cache indirection / overhead for this mapping?". That seems
> to be a narrower guarantee that we can make than what "DAX" might
> imply.
Right. So what do people think about my suggestion earlier in the thread to
use madvise(MADV_DIRECT_ACCESS) for this? Currently it would return success
when DAX is in use, failure otherwise. Later we could extend it to be also
used as a hint for caching policy for the inode...
Honza
--
Jan Kara <jack-IBi9RG/b67k@public.gmane.org>
SUSE Labs, CR
^ permalink raw reply
* Re: [RFC PATCH v4 24/27] mm/mmap: Create a guard area between VMAs
From: Yu-cheng Yu @ 2018-10-03 16:40 UTC (permalink / raw)
To: Eugene Syromiatnikov
Cc: Andy Lutomirski, X86 ML, H. Peter Anvin, Thomas Gleixner,
Ingo Molnar, LKML, linux-doc, Linux-MM, linux-arch, Linux API,
Arnd Bergmann, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H. J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg
In-Reply-To: <20181003163226.GC9449@asgard.redhat.com>
On Wed, 2018-10-03 at 18:32 +0200, Eugene Syromiatnikov wrote:
> On Wed, Oct 03, 2018 at 09:00:04AM -0700, Yu-cheng Yu wrote:
> > On Tue, 2018-10-02 at 22:36 -0700, Andy Lutomirski wrote:
> > > On Tue, Oct 2, 2018 at 9:55 PM Eugene Syromiatnikov <esyr@redhat.com>
> > > wrote:
> > > >
> > > > On Fri, Sep 21, 2018 at 08:03:48AM -0700, Yu-cheng Yu wrote:
> > > > > Create a guard area between VMAs, to detect memory corruption.
> > > >
> > > > Do I understand correctly that with this patch a user space program
> > > > no longer be able to place two mappings back to back? If it is so,
> > > > it will likely break a lot of things; for example, it's a common ring
> > > > buffer implementations technique, to map buffer memory twice back
> > > > to back in order to avoid special handling of items wrapping its end.
> > >
> > > I haven't checked what the patch actually does, but it shouldn't have
> > > any affect on MAP_FIXED or the new no-replace MAP_FIXED variant.
> > >
> > > --Andy
> >
> > I did some mmap tests with/without MAP_FIXED, and it works as intended.
> > In addition to the ring buffer, are there other test cases?
>
> Right, after some more code reading I figured out that it indeed
> shouldn't affect MAP_FIXED, thank you for confirmation.
>
> I'm not sure, however, whether such a change that provides no ability
> to configure or affect it will go well with all the supported
> architectures.
Can we do CONFIG_MMAP_GUARD_GAP?
^ permalink raw reply
* Re: [RFC PATCH v4 24/27] mm/mmap: Create a guard area between VMAs
From: Eugene Syromiatnikov @ 2018-10-03 16:32 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: Andy Lutomirski, X86 ML, H. Peter Anvin, Thomas Gleixner,
Ingo Molnar, LKML, linux-doc, Linux-MM, linux-arch, Linux API,
Arnd Bergmann, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H. J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg
In-Reply-To: <5ddb0ad33298d1858e530fce9c9ea2788b2fac81.camel@intel.com>
On Wed, Oct 03, 2018 at 09:00:04AM -0700, Yu-cheng Yu wrote:
> On Tue, 2018-10-02 at 22:36 -0700, Andy Lutomirski wrote:
> > On Tue, Oct 2, 2018 at 9:55 PM Eugene Syromiatnikov <esyr@redhat.com> wrote:
> > >
> > > On Fri, Sep 21, 2018 at 08:03:48AM -0700, Yu-cheng Yu wrote:
> > > > Create a guard area between VMAs, to detect memory corruption.
> > >
> > > Do I understand correctly that with this patch a user space program
> > > no longer be able to place two mappings back to back? If it is so,
> > > it will likely break a lot of things; for example, it's a common ring
> > > buffer implementations technique, to map buffer memory twice back
> > > to back in order to avoid special handling of items wrapping its end.
> >
> > I haven't checked what the patch actually does, but it shouldn't have
> > any affect on MAP_FIXED or the new no-replace MAP_FIXED variant.
> >
> > --Andy
>
> I did some mmap tests with/without MAP_FIXED, and it works as intended.
> In addition to the ring buffer, are there other test cases?
Right, after some more code reading I figured out that it indeed
shouldn't affect MAP_FIXED, thank you for confirmation.
I'm not sure, however, whether such a change that provides no ability
to configure or affect it will go well with all the supported
architectures.
^ permalink raw reply
* Re: [RFC PATCH v4 24/27] mm/mmap: Create a guard area between VMAs
From: Andy Lutomirski @ 2018-10-03 16:18 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: Eugene Syromiatnikov, X86 ML, H. Peter Anvin, Thomas Gleixner,
Ingo Molnar, LKML, linux-doc, Linux-MM, linux-arch, Linux API,
Arnd Bergmann, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H. J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg
In-Reply-To: <5ddb0ad33298d1858e530fce9c9ea2788b2fac81.camel@intel.com>
On Wed, Oct 3, 2018 at 9:06 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
> On Tue, 2018-10-02 at 22:36 -0700, Andy Lutomirski wrote:
> > On Tue, Oct 2, 2018 at 9:55 PM Eugene Syromiatnikov <esyr@redhat.com> wrote:
> > >
> > > On Fri, Sep 21, 2018 at 08:03:48AM -0700, Yu-cheng Yu wrote:
> > > > Create a guard area between VMAs, to detect memory corruption.
> > >
> > > Do I understand correctly that with this patch a user space program
> > > no longer be able to place two mappings back to back? If it is so,
> > > it will likely break a lot of things; for example, it's a common ring
> > > buffer implementations technique, to map buffer memory twice back
> > > to back in order to avoid special handling of items wrapping its end.
> >
> > I haven't checked what the patch actually does, but it shouldn't have
> > any affect on MAP_FIXED or the new no-replace MAP_FIXED variant.
> >
> > --Andy
>
> I did some mmap tests with/without MAP_FIXED, and it works as intended.
> In addition to the ring buffer, are there other test cases?
>
Various ELF loaders, perhaps? Do they use MAP_FIXED or do they just
use address hints?
^ permalink raw reply
* Re: [RFC PATCH v4 06/27] x86/cet: Control protection exception handler
From: Yu-cheng Yu @ 2018-10-03 16:11 UTC (permalink / raw)
To: Eugene Syromiatnikov
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <20181003103959.GB7111@asgard.redhat.com>
On Wed, 2018-10-03 at 12:39 +0200, Eugene Syromiatnikov wrote:
> On Fri, Sep 21, 2018 at 08:03:30AM -0700, Yu-cheng Yu wrote:
> > +dotraplinkage void
> > +do_control_protection(struct pt_regs *regs, long error_code)
> > +{
> > + struct task_struct *tsk;
> > +
> > + RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU");
> > + if (notify_die(DIE_TRAP, "control protection fault", regs,
> > + error_code, X86_TRAP_CP, SIGSEGV) == NOTIFY_STOP)
> > + return;
> > + cond_local_irq_enable(regs);
> > +
> > + if (!user_mode(regs))
> > + die("kernel control protection fault", regs, error_code);
> > +
> > + if (!static_cpu_has(X86_FEATURE_SHSTK) &&
> > + !static_cpu_has(X86_FEATURE_IBT))
> > + WARN_ONCE(1, "CET is disabled but got control "
> > + "protection fault\n");
> > +
> > + tsk = current;
> > + tsk->thread.error_code = error_code;
> > + tsk->thread.trap_nr = X86_TRAP_CP;
> > +
> > + if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV) &&
> > + printk_ratelimit()) {
> > + unsigned int max_err;
> > +
> > + max_err = ARRAY_SIZE(control_protection_err) - 1;
> > + if ((error_code < 0) || (error_code > max_err))
> > + error_code = 0;
> > + pr_info("%s[%d] control protection ip:%lx sp:%lx
> > error:%lx(%s)",
> > + tsk->comm, task_pid_nr(tsk),
> > + regs->ip, regs->sp, error_code,
> > + control_protection_err[error_code]);
> > + print_vma_addr(KERN_CONT " in ", regs->ip);
> > + pr_cont("\n");
> > + }
> > +
> > + force_sig_info(SIGSEGV, SEND_SIG_PRIV, tsk);
>
> That way, no information is provided to userspace (both application and
> debugger), which is rather unfortunate. It would be nice if a new SEGV_*
> code was added at least, and CET error (with error code constant provided
> in UAPI) is passed via si_errno. (Having ip/sp/*ssp would be even
> better, but I'm not exactly sure about ramifications of providing this
> kind of information to user space).
Ok, I will add that.
Yu-cheng
^ permalink raw reply
* Re: [RFC PATCH v4 09/27] x86/mm: Change _PAGE_DIRTY to _PAGE_DIRTY_HW
From: Yu-cheng Yu @ 2018-10-03 16:07 UTC (permalink / raw)
To: Matthew Wilcox
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <20181003133856.GA24782@bombadil.infradead.org>
On Wed, 2018-10-03 at 06:38 -0700, Matthew Wilcox wrote:
> On Fri, Sep 21, 2018 at 08:03:33AM -0700, Yu-cheng Yu wrote:
> > We are going to create _PAGE_DIRTY_SW for non-hardware, memory
> > management purposes. Rename _PAGE_DIRTY to _PAGE_DIRTY_HW and
> > _PAGE_BIT_DIRTY to _PAGE_BIT_DIRTY_HW to make these PTE dirty
> > bits more clear. There are no functional changes in this
> > patch.
>
> I would like there to be some documentation in this patchset which
> explains the difference between PAGE_SOFT_DIRTY and PAGE_DIRTY_SW.
I will add some comments for the difference between PAGE_SOFT_DIRTY and
PAGE_DIRTY_SW.
Yu-cheng
^ permalink raw reply
* Re: [RFC PATCH v4 24/27] mm/mmap: Create a guard area between VMAs
From: Yu-cheng Yu @ 2018-10-03 16:00 UTC (permalink / raw)
To: Andy Lutomirski, Eugene Syromiatnikov
Cc: X86 ML, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, LKML,
linux-doc, Linux-MM, linux-arch, Linux API, Arnd Bergmann,
Balbir Singh, Cyrill Gorcunov, Dave Hansen, Florian Weimer,
H. J. Lu, Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz,
Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <CALCETrU-Ny-uC1NqRedQwNKe2MMhsFEqZ08TtHJwbLfCACMmLw@mail.gmail.com>
On Tue, 2018-10-02 at 22:36 -0700, Andy Lutomirski wrote:
> On Tue, Oct 2, 2018 at 9:55 PM Eugene Syromiatnikov <esyr@redhat.com> wrote:
> >
> > On Fri, Sep 21, 2018 at 08:03:48AM -0700, Yu-cheng Yu wrote:
> > > Create a guard area between VMAs, to detect memory corruption.
> >
> > Do I understand correctly that with this patch a user space program
> > no longer be able to place two mappings back to back? If it is so,
> > it will likely break a lot of things; for example, it's a common ring
> > buffer implementations technique, to map buffer memory twice back
> > to back in order to avoid special handling of items wrapping its end.
>
> I haven't checked what the patch actually does, but it shouldn't have
> any affect on MAP_FIXED or the new no-replace MAP_FIXED variant.
>
> --Andy
I did some mmap tests with/without MAP_FIXED, and it works as intended.
In addition to the ring buffer, are there other test cases?
Yu-cheng
^ permalink raw reply
* Re: Problems with VM_MIXEDMAP removal from /proc/<pid>/smaps
From: Dan Williams @ 2018-10-03 15:13 UTC (permalink / raw)
To: Jan Kara
Cc: linux-xfs, linux-nvdimm, Linux API, Christoph Hellwig, Linux MM,
linux-fsdevel, linux-ext4
In-Reply-To: <20181003150658.GC24030-4I4JzKEfoa/jFM9bn6wA6Q@public.gmane.org>
On Wed, Oct 3, 2018 at 8:07 AM Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> wrote:
>
> On Wed 03-10-18 07:38:50, Dan Williams wrote:
> > On Wed, Oct 3, 2018 at 5:51 AM Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> wrote:
> > >
> > > On Tue 02-10-18 13:18:54, Dan Williams wrote:
> > > > On Tue, Oct 2, 2018 at 8:32 AM Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> wrote:
> > > > >
> > > > > On Tue 02-10-18 07:52:06, Christoph Hellwig wrote:
> > > > > > On Tue, Oct 02, 2018 at 04:44:13PM +0200, Johannes Thumshirn wrote:
> > > > > > > On Tue, Oct 02, 2018 at 07:37:13AM -0700, Christoph Hellwig wrote:
> > > > > > > > No, it should not. DAX is an implementation detail thay may change
> > > > > > > > or go away at any time.
> > > > > > >
> > > > > > > Well we had an issue with an application checking for dax, this is how
> > > > > > > we landed here in the first place.
> > > > > >
> > > > > > So what exacty is that "DAX" they are querying about (and no, I'm not
> > > > > > joking, nor being philosophical).
> > > > >
> > > > > I believe the application we are speaking about is mostly concerned about
> > > > > the memory overhead of the page cache. Think of a machine that has ~ 1TB of
> > > > > DRAM, the database running on it is about that size as well and they want
> > > > > database state stored somewhere persistently - which they may want to do by
> > > > > modifying mmaped database files if they do small updates... So they really
> > > > > want to be able to use close to all DRAM for the DB and not leave slack
> > > > > space for the kernel page cache to cache 1TB of database files.
> > > >
> > > > VM_MIXEDMAP was never a reliable indication of DAX because it could be
> > > > set for random other device-drivers that use vm_insert_mixed(). The
> > > > MAP_SYNC flag positively indicates that page cache is disabled for a
> > > > given mapping, although whether that property is due to "dax" or some
> > > > other kernel mechanics is purely an internal detail.
> > > >
> > > > I'm not opposed to faking out VM_MIXEDMAP if this broken check has
> > > > made it into production, but again, it's unreliable.
> > >
> > > So luckily this particular application wasn't widely deployed yet so we
> > > will likely get away with the vendor asking customers to update to a
> > > version not looking into smaps and parsing /proc/mounts instead.
> > >
> > > But I don't find parsing /proc/mounts that beautiful either and I'd prefer
> > > if we had a better interface for applications to query whether they can
> > > avoid page cache for mmaps or not.
> >
> > Yeah, the mount flag is not a good indicator either. I think we need
> > to follow through on the per-inode property of DAX. Darrick and I
> > discussed just allowing the property to be inherited from the parent
> > directory at file creation time. That avoids the dynamic set-up /
> > teardown races that seem intractable at this point.
> >
> > What's wrong with MAP_SYNC as a page-cache detector in the meantime?
>
> So IMHO checking for MAP_SYNC is about as reliable as checking for 'dax'
> mount option. It works now but nobody promises it will reliably detect DAX in
> future - e.g. there's nothing that prevents MAP_SYNC to work for mappings
> using pagecache if we find a sensible usecase for that.
Fair enough.
> WRT per-inode DAX property, AFAIU that inode flag is just going to be
> advisory thing - i.e., use DAX if possible. If you mount a filesystem with
> these inode flags set in a configuration which does not allow DAX to be
> used, you will still be able to access such inodes but the access will use
> page cache instead. And querying these flags should better show real
> on-disk status and not just whether DAX is used as that would result in an
> even bigger mess. So this feature seems to be somewhat orthogonal to the
> API I'm looking for.
True, I imagine once we have that flag we will be able to distinguish
the "saved" property and the "effective / live" property of DAX...
Also it's really not DAX that applications care about as much as "is
there page-cache indirection / overhead for this mapping?". That seems
to be a narrower guarantee that we can make than what "DAX" might
imply.
^ permalink raw reply
* Re: [RFC PATCH v4 18/27] x86/cet/shstk: User-mode shadow stack support
From: Yu-cheng Yu @ 2018-10-03 15:12 UTC (permalink / raw)
To: Eugene Syromiatnikov
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <20181003150754.GC32759@asgard.redhat.com>
On Wed, 2018-10-03 at 17:08 +0200, Eugene Syromiatnikov wrote:
> On Fri, Sep 21, 2018 at 08:03:42AM -0700, Yu-cheng Yu wrote:
>
> > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > index 5ea1d64cb0b4..b20450dde5b7 100644
> > --- a/fs/proc/task_mmu.c
> > +++ b/fs/proc/task_mmu.c
> > @@ -652,6 +652,9 @@ static void show_smap_vma_flags(struct seq_file *m,
> > struct vm_area_struct *vma)
> > [ilog2(VM_PKEY_BIT4)] = "",
> > #endif
> > #endif /* CONFIG_ARCH_HAS_PKEYS */
> > +#ifdef CONFIG_X86_INTEL_SHADOW_STACK_USER
> > + [ilog2(VM_SHSTK)] = "ss"
> > +#endif
>
> It's probably makes sense to have this hunk as a part of "x86/cet/shstk:
> Add Kconfig option for user-mode shadow stack", where VM_SHSTK was
> initially introduced.
Yes, move it to "mm/Introduce VM_SHSTK for shadow stack memory".
Yu-cheng
^ permalink raw reply
* Re: [RFC PATCH v4 18/27] x86/cet/shstk: User-mode shadow stack support
From: Eugene Syromiatnikov @ 2018-10-03 15:08 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <20180921150351.20898-19-yu-cheng.yu@intel.com>
On Fri, Sep 21, 2018 at 08:03:42AM -0700, Yu-cheng Yu wrote:
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 5ea1d64cb0b4..b20450dde5b7 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -652,6 +652,9 @@ static void show_smap_vma_flags(struct seq_file *m, struct vm_area_struct *vma)
> [ilog2(VM_PKEY_BIT4)] = "",
> #endif
> #endif /* CONFIG_ARCH_HAS_PKEYS */
> +#ifdef CONFIG_X86_INTEL_SHADOW_STACK_USER
> + [ilog2(VM_SHSTK)] = "ss"
> +#endif
It's probably makes sense to have this hunk as a part of "x86/cet/shstk:
Add Kconfig option for user-mode shadow stack", where VM_SHSTK was
initially introduced.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox