* Re: [PATCH v9 00/24] ILP32 for ARM64
From: Arnd Bergmann @ 2018-10-14 19:49 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Yury Norov, Catalin Marinas, Linux ARM, Linux Kernel Mailing List,
open list:DOCUMENTATION, linux-arch, Linux API, Adam Borowski,
Alexander Graf, Alexey Klimov, Andreas Schwab, Andrew Pinski,
Bamvor Zhangjian, Chris Metcalf, Christoph Muellner, Dave Martin,
David Miller, Florian Weimer
In-Reply-To: <CALCETrVuabkb5ZQOSgQQTEq8CrS602NBpAkE2=4n982r1f5reQ@mail.gmail.com>
On Sat, Oct 13, 2018 at 9:36 PM Andy Lutomirski <luto@amacapital.net> wrote:
>
> On Wed, May 16, 2018 at 1:19 AM Yury Norov <ynorov@caviumnetworks.com> wrote:
> >
> > This series enables AARCH64 with ILP32 mode.
> >
> > As supporting work, it introduces ARCH_32BIT_OFF_T configuration
> > option that is enabled for existing 32-bit architectures but disabled
> > for new arches (so 64-bit off_t userspace type is used by new userspace).
> > Also it deprecates getrlimit and setrlimit syscalls prior to prlimit64.
>
> Second, ILP32 user code is highly unlikely
> to end up with the same struct layout as ILP64 code. The latter seems
> like it should be solved entirely in userspace by adding a way to
> annotate a structure as being a kernel ABI structure and getting the
> toolchain to lay it out as if it were ILP64 even though the target is
> ILP32.
The syscall ABI could be almost completely abstracted in glibc, the
main issue is ioctl and a couple of related interfaces that pass data
structures (read() on /dev/input/*, mmap on /dev/snd/*
or raw sockets, fcntl).
The question whether a data type is laid out like a 64-bit architecture
would cannot be a property of the type in most of those cases,
because the same types are used elsewhere. Many ioctls just
take a pointer to a 'long' or similar, and then you have structures
like 'timespec' that are used both in syscall/ioctl ABI and in normal
user space code, but are required to be laid out differently there.
(timespec is a bad example because y2038 of course, but it
illustrates the point).
> 2. I think you should make a conscious decision as to whether the
> ILP32-ness of a syscall is a property of the task or of the syscall.
> On x86, x32-ness is a property of the syscall, but historically it
> also got rather entangled with the state of the task, and the result
> was a mess. It looks like you're making it be a property of the task,
> which is fine, but you're making it impossible for very clever ILP32
> libraries to include little ILP64 stubs that do fancy things with full
> 64-bit syscalls.
>
> 3. Make very certain that you aren't exploitable by malicious
> processes that set the high bits in ILP32 syscall args. x86 compat
> has issues like that in the past.
This point was actually the most important one for keeping the
aarch64 ilp32 interface as restricted as it is: it doesn't allow
anything that the normal aarch32/armv7 emulation doesn't
already provide.
Arnd
^ permalink raw reply
* Re: [PATCH v9 00/24] ILP32 for ARM64
From: Arnd Bergmann @ 2018-10-14 19:53 UTC (permalink / raw)
To: Eugene Syromiatnikov
Cc: Szabolcs Nagy, Yury Norov, nd, Catalin Marinas, Linux ARM,
Linux Kernel Mailing List, open list:DOCUMENTATION, linux-arch,
Linux API, Adam Borowski, Alexander Graf, Alexey Klimov,
Andreas Schwab, Andrew Pinski, Bamvor Zhangjian, Chris Metcalf,
Christoph Muellner, Dave Martin, David Miller
In-Reply-To: <20181013020731.GD21972@asgard.redhat.com>
On Sat, Oct 13, 2018 at 4:07 AM Eugene Syromiatnikov <esyr@redhat.com> wrote:
>
> On Wed, Oct 10, 2018 at 03:39:07PM +0100, Szabolcs Nagy wrote:
> > On 10/10/18 15:10, Eugene Syromiatnikov wrote:
> > > * What's the reasoning behind capping syscall arguments to 32 bit? x32
> > > and MIPS N32 do not have such a restriction (and do not need special
> > > wrappers for syscalls that pass 64-bit values as a result, except
> > > when they do, as it is the case for preadv2 on x32); moreover, that
> > > would lead to insurmountable difficulties for AArch64 ILP32 tracers
> > > that try to trace LP64 tracees, as it would be impossible to pass
> > > 64-bit addresses to process_vm_{read,write} or ptrace PEEK/POKE.
> >
> > but that's necessarily the case for all ilp32 abis:
> > the userspace syscall function receives 32bit
> > arguments so even if the kernel abi takes 64bit
> > args you cannot use that from c code. (the libc
> > does not even know which args should be sign or
> > zero extended.)
>
> glibc's syscall() prototype has kernel_ulong_t as its arguments (more
> specifically, to __syscall_ulong_t, which is 64-bit wide on x32; it
> should also have kernel_long_t as its return type instead of long,
> but that's another story), so it works perfectly fine in case of x32.
>
> > process_vm_readv/writev is limited by the ilp32
> > iovec struct, not by the syscall arguments.
>
> Right, on x32/N32 this issue is worked around by the usage of the respective
> x86_64/N64 call, and it looks like another thing that is impossible
> with AArch64 ilp32.
>
> > ptrace is specified to take void* addr argument,
> > and void* is 32bit on all ilp32 targets.
> > so again on the c language level there is no
> > way around the 32bit limitation.
>
> Which is an issue.
I have no idea why you think this is a problem specific to aarch64-ilp32:
If we want to be able to debug 64-bit tasks from a 32-bit task on any
architecture that has compat mode, we should solve it once and
extend the ptrace interface to allow it on *all* of them. We certainly
don't need /more/ special cases for the x32 hack, there should really
be fewer of them.
Arnd
^ permalink raw reply
* Re: rseq system call incompletely wired up in microblaze
From: Michal Simek @ 2018-10-15 7:37 UTC (permalink / raw)
To: Mathieu Desnoyers, Michal Simek
Cc: Thomas Gleixner, Paul E. McKenney, Peter Zijlstra, Boqun Feng,
linux-kernel, linux-api
In-Reply-To: <1913468383.3666.1539535618042.JavaMail.zimbra@efficios.com>
Hi Mathieu,
On 14.10.2018 18:46, Mathieu Desnoyers wrote:
> Hi Michal,
>
> I notice the following commit went into 4.18:
>
> commit 54b0a2011dfcd2e3fe2c28062694fbbe3eb377a3
> Author: Michal Simek <michal.simek@xilinx.com>
> Date: Thu Jun 14 08:24:39 2018 +0200
>
> microblaze: Add new syscalls io_pgetevents and rseq
>
> Wire up new syscalls io_pgetevents and rseq.
>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>
> It adds the rseq system call to the microblaze syscall list, but
> does not add the required hooks in the architecture code, nor does
> it select HAVE_RSEQ. So effectively, even though sys_rseq is reserved
> on microblaze, it currently always returns ENOSYS.
>
> Is your intent to simply reserve the system call number, or to
> also get rseq to work on microblaze ?
Yes. my indentation is allocate syscall numbers for all these new
syscalls because it takes some time when new syscalls numbers are
propagated to toolchain builds.
I can't see a problem if ENOSYS is returned because it is proper
reaction when syscall is not implemented.
Thanks,
Michal
^ permalink raw reply
* Re: [RFC PATCH] mm, proc: report PR_SET_THP_DISABLE in proc
From: Michal Hocko @ 2018-10-15 15:03 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, Vlastimil Babka, Alexey Dobriyan,
Kirill A. Shutemov, linux-kernel, linux-mm, linux-api
In-Reply-To: <20181009083326.GG8528@dhcp22.suse.cz>
On Tue 09-10-18 10:33:26, Michal Hocko wrote:
> On Thu 04-10-18 11:34:11, David Rientjes wrote:
> > On Thu, 4 Oct 2018, Michal Hocko wrote:
> >
> > > > And prior to the offending commit, there were three ways to control thp
> > > > but two ways to determine if a mapping was eligible for thp based on the
> > > > implementation detail of one of those ways.
> > >
> > > Yes, it is really unfortunate that we have ever allowed to leak such an
> > > internal stuff like VMA flags to userspace.
> > >
> >
> > Right, I don't like userspace dependencies on VmFlags in smaps myself, but
> > it's the only way we have available that shows whether a single mapping is
> > eligible to be backed by thp :/
>
> Which is not the case due to reasons mentioned earlier. It only speaks
> about madvise status on the VMA.
>
> > > > If there are three ways to
> > > > control thp, userspace is still in the dark wrt which takes precedence
> > > > over the other: we have PR_SET_THP_DISABLE but globally sysfs has it set
> > > > to "always", or we have MADV_HUGEPAGE set per smaps but PR_SET_THP_DISABLE
> > > > shown in /proc/pid/status, etc.
> > > >
> > > > Which one is the ultimate authority?
> > >
> > > Isn't our documentation good enough? If not then we should document it
> > > properly.
> > >
> >
> > No, because the offending commit actually changed the precedence itself:
> > PR_SET_THP_DISABLE used to be honored for future mappings and the commit
> > changed that for all current mappings.
>
> Which is the actual and the full point of the fix as described in the
> changelog. The original implementation was poor and inconsistent.
>
> > So as a result of the commit
> > itself we would have had to change the documentation and userspace can't
> > be expected to keep up with yet a fourth variable: kernel version. It
> > really needs to be simpler, just a per-mapping specifier.
>
> As I've said, if you really need a per-vma granularity then make it a
> dedicated line in the output with a clear semantic. Do not make VMA
> flags even more confusing.
Can we settle with something please?
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v5 02/27] x86/fpu/xstate: Change names to separate XSAVES system and user states
From: Borislav Petkov @ 2018-10-15 17:03 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,
Eugene Syromiatnikov, Florian Weimer, H.J. Lu, Jann Horn,
Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pa
In-Reply-To: <20181011151523.27101-3-yu-cheng.yu@intel.com>
On Thu, Oct 11, 2018 at 08:14:58AM -0700, Yu-cheng Yu wrote:
> Control Flow Enforcement (CET) MSRs are XSAVES system/supervisor
> states. To support CET, we introduce XSAVES system states first.
>
> XSAVES is a "supervisor" instruction and, comparing to XSAVE, saves
> additional "supervisor" states that can be modified only from CPL 0.
> However, these states are per-task and not kernel's own. Rename
> "supervisor" states to "system" states to clearly separate them from
> "user" states.
>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
> arch/x86/include/asm/fpu/internal.h | 4 +-
> arch/x86/include/asm/fpu/xstate.h | 20 +++----
> arch/x86/kernel/fpu/core.c | 4 +-
> arch/x86/kernel/fpu/init.c | 2 +-
> arch/x86/kernel/fpu/signal.c | 6 +--
> arch/x86/kernel/fpu/xstate.c | 82 ++++++++++++++---------------
> 6 files changed, 57 insertions(+), 61 deletions(-)
...
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 87a57b7642d3..e7cbaed12ef1 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -51,13 +51,14 @@ static short xsave_cpuid_features[] __initdata = {
> };
>
> /*
> - * Mask of xstate features supported by the CPU and the kernel:
> + * Mask of supported 'user' xstate features derived from boot_cpu_has() and
> + * SUPPORTED_XFEATURES_MASK.
<--- This comment here looks like a good place to put some blurb about
user and system states, what they are, what the distinction is and so
on.
> */
> -u64 xfeatures_mask __read_mostly;
> +u64 xfeatures_mask_user __read_mostly;
>
> static unsigned int xstate_offsets[XFEATURE_MAX] = { [ 0 ... XFEATURE_MAX - 1] = -1};
> static unsigned int xstate_sizes[XFEATURE_MAX] = { [ 0 ... XFEATURE_MAX - 1] = -1};
> -static unsigned int xstate_comp_offsets[sizeof(xfeatures_mask)*8];
> +static unsigned int xstate_comp_offsets[sizeof(xfeatures_mask_user)*8];
>
> /*
> * The XSAVE area of kernel can be in standard or compacted format;
> @@ -82,7 +83,7 @@ void fpu__xstate_clear_all_cpu_caps(void)
> */
> int cpu_has_xfeatures(u64 xfeatures_needed, const char **feature_name)
> {
> - u64 xfeatures_missing = xfeatures_needed & ~xfeatures_mask;
> + u64 xfeatures_missing = xfeatures_needed & ~xfeatures_mask_user;
>
> if (unlikely(feature_name)) {
> long xfeature_idx, max_idx;
> @@ -113,14 +114,11 @@ int cpu_has_xfeatures(u64 xfeatures_needed, const char **feature_name)
> }
> EXPORT_SYMBOL_GPL(cpu_has_xfeatures);
>
> -static int xfeature_is_supervisor(int xfeature_nr)
> +static int xfeature_is_system(int xfeature_nr)
> {
> /*
> - * We currently do not support supervisor states, but if
> - * we did, we could find out like this.
> - *
> * SDM says: If state component 'i' is a user state component,
> - * ECX[0] return 0; if state component i is a supervisor
> + * ECX[0] return 0; if state component i is a system
is 0
> * state component, ECX[0] returns 1.
is 1.
> */
> u32 eax, ebx, ecx, edx;
...
> @@ -242,7 +238,7 @@ void fpu__init_cpu_xstate(void)
> */
> static int xfeature_enabled(enum xfeature xfeature)
> {
> - return !!(xfeatures_mask & (1UL << xfeature));
> + return !!(xfeatures_mask_user & BIT_ULL(xfeature));
> }
>
> /*
> @@ -272,7 +268,7 @@ static void __init setup_xstate_features(void)
> cpuid_count(XSTATE_CPUID, i, &eax, &ebx, &ecx, &edx);
>
> /*
> - * If an xfeature is supervisor state, the offset
> + * If an xfeature is system state, the offset
is a system state, ...
> * in EBX is invalid. We leave it to -1.
> */
> if (xfeature_is_user(i))
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply
* Re: [RFC PATCH] mm, proc: report PR_SET_THP_DISABLE in proc
From: David Rientjes @ 2018-10-15 22:25 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: <20181015150325.GN18839@dhcp22.suse.cz>
On Mon, 15 Oct 2018, Michal Hocko wrote:
> > > No, because the offending commit actually changed the precedence itself:
> > > PR_SET_THP_DISABLE used to be honored for future mappings and the commit
> > > changed that for all current mappings.
> >
> > Which is the actual and the full point of the fix as described in the
> > changelog. The original implementation was poor and inconsistent.
> >
> > > So as a result of the commit
> > > itself we would have had to change the documentation and userspace can't
> > > be expected to keep up with yet a fourth variable: kernel version. It
> > > really needs to be simpler, just a per-mapping specifier.
> >
> > As I've said, if you really need a per-vma granularity then make it a
> > dedicated line in the output with a clear semantic. Do not make VMA
> > flags even more confusing.
>
> Can we settle with something please?
I don't understand the point of extending smaps with yet another line.
The only way for a different process to determine if a single vma from
another process is thp disabled is by the "nh" flag, so it is reasonable
that userspace reads this. My patch fixes that. If smaps is extended
with another line per your patch, it doesn't change the fact that previous
binaries are built to check for "nh" so it does not deprecate that.
("THP_Enabled" is also ambiguous since it only refers to prctl and not the
default thp setting or madvise.)
^ permalink raw reply
* Re: [RFC PATCH v4 21/27] x86/cet/shstk: ELF header parsing of Shadow Stack
From: Kees Cook @ 2018-10-15 23:40 UTC (permalink / raw)
To: Yu-cheng Yu, Andy Lutomirski
Cc: X86 ML, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, LKML,
open list:DOCUMENTATION, Linux-MM, linux-arch, Linux API,
Arnd Bergmann, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, 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 8:03 AM, Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> Look in .note.gnu.property of an ELF file and check if Shadow Stack needs
> to be enabled for the task.
Ah, I've been wanting this for other things too (see below).
>
> 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)
> +
> +#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
> +
> ###
> # 64 bit specific files
> ifeq ($(CONFIG_X86_64),y)
> 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];
> + * }[];
> + */
Does NT_GNU_PROPERTY_TYPE_0 only ever contain property_x86 bytes? (I
assume not, since there is a pr_type?)
> +
> +#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));
Cheaper to test n_type first...
> +}
> +
> +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);
I think this could overflow: n_namesz can be u64 for elf64_note.
> + size = round_up(size + n->n_descsz, align);
Same here. You may want to use check_add_overflow(), etc, an u64 types.
> +
> + if (buf + size < buf)
> + return NULL;
I don't understand this. You want to check size not exceeding the
allocation, which isn't passed into this function. Checking for a full
unsigned address wrap around is not sufficient to detect overflow.
> + 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;
Why is *arg being updated? I don't see last_pr used outside of here --
are properties required to be pr_type-ordered?
> +
> + 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) ||
Again, this "< buf" test doesn't look at all correct to me.
> + (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)
I'm not a fan of the short "scan", "test" and "next" names, and I
really don't like an arg named "arg". Something slightly more
descriptive for all of these would be nice, please.
> +{
> + 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)) {
These comparisons are safe due to the BUF_SIZE limit of buf_size and
the only used size of item_size, but if this becomes more generic, it
should be more defensive on the size calculations (e.g. make sure than
"item_size < max" and then here "p < max - item_size", etc).
I'd kind of rather this code walked the base type and check each for
the matching feature. What is the general specification for what
NT_GNU_PROPERTY_TYPE_0 contains?
> + 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;
Why kmalloc over stack variable? (Or, does BUF_SIZE here really need
to be 1024?)
> +
> + *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);
Same overflow notes from earlier...
> +
> + 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;
Instead of a separate parser here, wouldn't it be a bit nicer to
attach this to the existing binfmt_elf program header parsing loop:
elf_ppnt = elf_phdata;
for (i = 0; i < loc->elf_ex.e_phnum; i++, elf_ppnt++)
switch (elf_ppnt->p_type) {
case PT_GNU_STACK:
...
case PT_LOPROC ... PT_HIPROC:
...
> +
> + 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
Should there be an #else error here?
> + }
> +
> + memset(¤t->thread.cet, 0, sizeof(struct cet_status));
> +
> + if (cpu_feature_enabled(X86_FEATURE_SHSTK)) {
The CPU feature was already tested at arch_setup_features() entry.
> + if (feature & GNU_PROPERTY_X86_FEATURE_1_SHSTK) {
> + err = cet_setup_shstk();
> + if (err < 0)
> + goto out;
> + }
> + }
> +
> +out:
> + return err;
> +}
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index efae2fb0930a..b891aa292b46 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -1081,6 +1081,21 @@ static int load_elf_binary(struct linux_binprm *bprm)
> goto out_free_dentry;
> }
>
> +#ifdef CONFIG_ARCH_HAS_PROGRAM_PROPERTIES
> + if (interpreter) {
> + retval = arch_setup_features(&loc->interp_elf_ex,
> + interp_elf_phdata,
> + interpreter, true);
> + } else {
> + retval = arch_setup_features(&loc->elf_ex,
> + elf_phdata,
> + bprm->file, false);
> + }
> +
> + if (retval < 0)
> + goto out_free_dentry;
> +#endif
> +
> if (elf_interpreter) {
> unsigned long interp_map_addr = 0;
>
> diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
> index c5358e0ae7c5..5ef25a565e88 100644
> --- a/include/uapi/linux/elf.h
> +++ b/include/uapi/linux/elf.h
> @@ -372,6 +372,7 @@ typedef struct elf64_shdr {
> #define NT_PRFPREG 2
> #define NT_PRPSINFO 3
> #define NT_TASKSTRUCT 4
> +#define NT_GNU_PROPERTY_TYPE_0 5
> #define NT_AUXV 6
> /*
> * Note to userspace developers: size of NT_SIGINFO note may increase
> --
> 2.17.1
>
I'd like to be using this code for a few other cases too (not just
x86-specific). For example, for marking KASan binaries as needing a
"legacy" memory layouts[1]. Others might be setting things like
no_new_privs at exec time, etc.
-Kees
[1] https://lkml.kernel.org/r/CAGXu5jL1HRG7Dn9vraw8Hu7LF+69k3EDpztt1Ju7ijEzmvRdhA@mail.gmail.com
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: [RFC PATCH for 4.21 06/16] cpu_opv: Provide cpu_opv system call (v8)
From: Sergey Senozhatsky @ 2018-10-16 8:10 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Peter Zijlstra, Paul E . McKenney, Boqun Feng, linux-kernel,
linux-api, Thomas Gleixner, Andy Lutomirski, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H . Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer,
Steven Rostedt, Josh Triplett, Linus Torvalds, Catalin Marinas,
Will Deacon
In-Reply-To: <20181010191936.7495-7-mathieu.desnoyers@efficios.com>
Hi Mathieu,
On (10/10/18 15:19), Mathieu Desnoyers wrote:
[..]
> +SYSCALL_DEFINE4(cpu_opv, struct cpu_op __user *, ucpuopv, int, cpuopcnt,
> + int, cpu, int, flags)
> +{
[..]
> +again:
> + ret = cpu_opv_pin_pages(cpuopv, cpuopcnt, &vaddr_ptrs);
> + if (ret)
> + goto end;
> + ret = do_cpu_opv(cpuopv, cpuopcnt, &vaddr_ptrs, cpu);
> + if (ret == -EAGAIN)
> + retry = true;
> +end:
> + for (i = 0; i < vaddr_ptrs.nr_vaddr; i++) {
> + struct vaddr *vaddr = &vaddr_ptrs.addr[i];
> + int j;
> +
> + vm_unmap_user_ram((void *)vaddr->mem, vaddr->nr_pages);
A dumb question.
Both vm_unmap_user_ram() and vm_map_user_ram() can BUG_ON().
So this is
userspace -> syscall -> cpu_opv() -> vm_unmap_user_ram() -> BUG_ON()
Any chance someone can exploit it?
-ss
^ permalink raw reply
* Re: [PATCH v6 0/1] ns: introduce binfmt_misc namespace
From: Laurent Vivier @ 2018-10-16 9:52 UTC (permalink / raw)
To: linux-kernel
Cc: Jann Horn, James Bottomley, linux-api, linux-fsdevel,
Andrei Vagin, Alexander Viro, Eric Biederman, containers,
Dmitry Safonov
In-Reply-To: <20181010161430.11633-1-laurent@vivier.eu>
Hi,
Any comment on this last version?
Any chance to be merged?
Thanks,
Laurent
Le 10/10/2018 à 18:14, Laurent Vivier a écrit :
> v6: Return &init_binfmt_ns instead of NULL in binfmt_ns()
> This should never happen, but to stay safe return a
> value we can use.
> change subject from "RFC" to "PATCH"
>
> v5: Use READ_ONCE()/WRITE_ONCE()
> move mount pointer struct init to bm_fill_super() and add smp_wmb()
> remove useless NULL value init
> add WARN_ON_ONCE()
>
> v4: first user namespace is initialized with &init_binfmt_ns,
> all new user namespaces are initialized with a NULL and use
> the one of the first parent that is not NULL. The pointer
> is initialized to a valid value the first time the binfmt_misc
> fs is mounted in the current user namespace.
> This allows to not change the way it was working before:
> new ns inherits values from its parent, and if parent value is modified
> (or parent creates its own binfmt entry by mounting the fs) child
> inherits it (unless it has itself mounted the fs).
>
> 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 | 111 ++++++++++++++++++++++++---------
> include/linux/user_namespace.h | 15 +++++
> kernel/user.c | 14 +++++
> kernel/user_namespace.c | 3 +
> 4 files changed, 115 insertions(+), 28 deletions(-)
>
^ permalink raw reply
* Re: [PATCH v6 1/1] ns: add binfmt_misc to the user namespace
From: Rasmus Villemoes @ 2018-10-16 10:13 UTC (permalink / raw)
To: Laurent Vivier, linux-kernel
Cc: Jann Horn, James Bottomley, linux-api, linux-fsdevel,
Andrei Vagin, Alexander Viro, Eric Biederman, containers,
Dmitry Safonov
In-Reply-To: <20181010161430.11633-2-laurent@vivier.eu>
On 2018-10-10 18:14, Laurent Vivier wrote:
> + /* create a new binfmt namespace
> + * if we are not in the first user namespace
> + * but the binfmt namespace is the first one
> + */
> + if (READ_ONCE(ns->binfmt_ns) == NULL) {
> + struct binfmt_namespace *new_ns;
> +
> + new_ns = kmalloc(sizeof(struct binfmt_namespace),
> + GFP_KERNEL);
> + if (new_ns == NULL)
> + return -ENOMEM;
> + INIT_LIST_HEAD(&new_ns->entries);
> + new_ns->enabled = 1;
> + rwlock_init(&new_ns->entries_lock);
> + new_ns->bm_mnt = NULL;
> + new_ns->entry_count = 0;
> + /* ensure new_ns is completely initialized before sharing it */
> + smp_wmb();
> + WRITE_ONCE(ns->binfmt_ns, new_ns);
> + }
If ns->binfmt_ns can really change under us (given you use READ_ONCE),
what prevents two instances of this code running at the same time, in
which case one of them would leak its new_ns instance? Also, there
doesn't seem to be any smp_rmb() buddy to that wmb(), I don't think
that's implied by READ_ONCE() in binfmt_ns().
Rasmus
^ permalink raw reply
* Re: [RFC PATCH] mm, proc: report PR_SET_THP_DISABLE in proc
From: Michal Hocko @ 2018-10-16 10:48 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, Vlastimil Babka, Alexey Dobriyan,
Kirill A. Shutemov, linux-kernel, linux-mm, linux-api
In-Reply-To: <alpine.DEB.2.21.1810151519250.247641@chino.kir.corp.google.com>
On Mon 15-10-18 15:25:14, David Rientjes wrote:
> On Mon, 15 Oct 2018, Michal Hocko wrote:
>
> > > > No, because the offending commit actually changed the precedence itself:
> > > > PR_SET_THP_DISABLE used to be honored for future mappings and the commit
> > > > changed that for all current mappings.
> > >
> > > Which is the actual and the full point of the fix as described in the
> > > changelog. The original implementation was poor and inconsistent.
> > >
> > > > So as a result of the commit
> > > > itself we would have had to change the documentation and userspace can't
> > > > be expected to keep up with yet a fourth variable: kernel version. It
> > > > really needs to be simpler, just a per-mapping specifier.
> > >
> > > As I've said, if you really need a per-vma granularity then make it a
> > > dedicated line in the output with a clear semantic. Do not make VMA
> > > flags even more confusing.
> >
> > Can we settle with something please?
>
> I don't understand the point of extending smaps with yet another line.
Because abusing a vma flag part is just wrong. What are you going to do
when a next bug report states that the flag is set even though no
userspace has set it and that leads to some malfunctioning? Can you rule
that out? Even your abuse of the flag is surprising so why others
wouldn't be?
> The only way for a different process to determine if a single vma from
> another process is thp disabled is by the "nh" flag, so it is reasonable
> that userspace reads this. My patch fixes that. If smaps is extended
> with another line per your patch, it doesn't change the fact that previous
> binaries are built to check for "nh" so it does not deprecate that.
> ("THP_Enabled" is also ambiguous since it only refers to prctl and not the
> default thp setting or madvise.)
As I've said there are two things. Exporting PR_SET_THP_DISABLE to
userspace so that a 3rd party process can query it. I've already
explained why that might be useful. If you really insist on having
a per-vma field then let's do it properly now. Are you going to agree on
that? If yes, I am willing to spend my time on that but I am not going
to bother if this will lead to "I want my vma field abuse anyway".
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v6 1/1] ns: add binfmt_misc to the user namespace
From: Laurent Vivier @ 2018-10-16 10:53 UTC (permalink / raw)
To: Rasmus Villemoes, linux-kernel
Cc: Jann Horn, James Bottomley, linux-api, linux-fsdevel,
Andrei Vagin, Alexander Viro, Eric Biederman, containers,
Dmitry Safonov
In-Reply-To: <97280bb2-933e-9281-bd91-99748e1dd653@rasmusvillemoes.dk>
Le 16/10/2018 à 12:13, Rasmus Villemoes a écrit :
> On 2018-10-10 18:14, Laurent Vivier wrote:
>
>> + /* create a new binfmt namespace
>> + * if we are not in the first user namespace
>> + * but the binfmt namespace is the first one
>> + */
>> + if (READ_ONCE(ns->binfmt_ns) == NULL) {
>> + struct binfmt_namespace *new_ns;
>> +
>> + new_ns = kmalloc(sizeof(struct binfmt_namespace),
>> + GFP_KERNEL);
>> + if (new_ns == NULL)
>> + return -ENOMEM;
>> + INIT_LIST_HEAD(&new_ns->entries);
>> + new_ns->enabled = 1;
>> + rwlock_init(&new_ns->entries_lock);
>> + new_ns->bm_mnt = NULL;
>> + new_ns->entry_count = 0;
>> + /* ensure new_ns is completely initialized before sharing it */
>> + smp_wmb();
>> + WRITE_ONCE(ns->binfmt_ns, new_ns);
>> + }
>
> If ns->binfmt_ns can really change under us (given you use READ_ONCE),
> what prevents two instances of this code running at the same time, in
> which case one of them would leak its new_ns instance? Also, there
See
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1782780.html
> doesn't seem to be any smp_rmb() buddy to that wmb(), I don't think
> that's implied by READ_ONCE() in binfmt_ns().
See
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1783049.html
Thanks,
Laurent
^ permalink raw reply
* Re: [PATCH v6 1/1] ns: add binfmt_misc to the user namespace
From: Andrei Vagin @ 2018-10-16 16:22 UTC (permalink / raw)
To: Laurent Vivier
Cc: linux-kernel, Jann Horn, James Bottomley, linux-api,
linux-fsdevel, Alexander Viro, Eric Biederman, containers,
Dmitry Safonov
In-Reply-To: <20181010161430.11633-2-laurent@vivier.eu>
On Wed, Oct 10, 2018 at 06:14:30PM +0200, Laurent Vivier wrote:
> 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 previous level, 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 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>
Acked-by: Andrei Vagin <avagin@gmail.com>
Thanks,
Andrei
> ---
> fs/binfmt_misc.c | 111 ++++++++++++++++++++++++---------
> include/linux/user_namespace.h | 15 +++++
> kernel/user.c | 14 +++++
> kernel/user_namespace.c | 3 +
> 4 files changed, 115 insertions(+), 28 deletions(-)
>
> diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
> index aa4a7a23ff99..df9dc3248b7b 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:
> @@ -80,18 +74,37 @@ static int entry_count;
> */
> #define MAX_REGISTER_LENGTH 1920
>
> +static struct binfmt_namespace *binfmt_ns(struct user_namespace *ns)
> +{
> + struct binfmt_namespace *b_ns;
> +
> + while (ns) {
> + b_ns = READ_ONCE(ns->binfmt_ns);
> + if (b_ns)
> + return b_ns;
> + ns = ns->parent;
> + }
> + /* as the first user namespace is initialized with
> + * &init_binfmt_ns we should never come here
> + * but we try to stay safe by logging a warning
> + * and returning a sane value
> + */
> + WARN_ON_ONCE(1);
> + return &init_binfmt_ns;
> +}
> +
> /*
> * Check if we support the binfmt
> * 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 binfmt_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->entries) {
> Node *e = list_entry(l, Node, list);
> char *s;
> int j;
> @@ -133,17 +146,18 @@ static int load_misc_binary(struct linux_binprm *bprm)
> struct file *interp_file = NULL;
> int retval;
> int fd_binary = -1;
> + struct binfmt_namespace *ns = binfmt_ns(current_user_ns());
>
> retval = -ENOEXEC;
> - if (!enabled)
> + if (!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->entries_lock);
> + fmt = check_file(ns, bprm);
> if (fmt)
> dget(fmt->dentry);
> - read_unlock(&entries_lock);
> + read_unlock(&ns->entries_lock);
> if (!fmt)
> return retval;
>
> @@ -609,19 +623,19 @@ static void bm_evict_inode(struct inode *inode)
> kfree(e);
> }
>
> -static void kill_node(Node *e)
> +static void kill_node(struct binfmt_namespace *ns, Node *e)
> {
> struct dentry *dentry;
>
> - write_lock(&entries_lock);
> + write_lock(&ns->entries_lock);
> list_del_init(&e->list);
> - write_unlock(&entries_lock);
> + write_unlock(&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->bm_mnt, &ns->entry_count);
> }
>
> /* /<entry> */
> @@ -651,6 +665,9 @@ 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 binfmt_namespace *ns;
> +
> + ns = binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
>
> switch (res) {
> case 1:
> @@ -667,7 +684,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 +710,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 binfmt_namespace *ns;
> int err = 0;
>
> e = create_entry(buffer, count);
> @@ -716,7 +734,9 @@ 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);
> + ns = binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
> + err = simple_pin_fs(&bm_fs_type, &ns->bm_mnt,
> + &ns->entry_count);
> if (err) {
> iput(inode);
> inode = NULL;
> @@ -725,12 +745,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->bm_mnt,
> + &ns->entry_count);
> iput(inode);
> inode = NULL;
> goto out2;
> @@ -743,9 +767,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->entries_lock);
> + list_add(&e->list, &ns->entries);
> + write_unlock(&ns->entries_lock);
>
> err = 0;
> out2:
> @@ -770,7 +794,9 @@ 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 binfmt_namespace *ns =
> + binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
> + char *s = ns->enabled ? "enabled\n" : "disabled\n";
>
> return simple_read_from_buffer(buf, nbytes, ppos, s, strlen(s));
> }
> @@ -778,25 +804,28 @@ 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 binfmt_namespace *ns;
> int res = parse_command(buffer, count);
> struct dentry *root;
>
> + ns = binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
> switch (res) {
> case 1:
> /* Disable all handlers. */
> - enabled = 0;
> + ns->enabled = 0;
> break;
> case 2:
> /* Enable all handlers. */
> - enabled = 1;
> + 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->entries))
> + kill_node(ns, list_first_entry(&ns->entries,
> + Node, list));
>
> inode_unlock(d_inode(root));
> break;
> @@ -823,12 +852,34 @@ static const struct super_operations s_ops = {
> static int bm_fill_super(struct super_block *sb, void *data, int silent)
> {
> int err;
> + struct user_namespace *ns = sb->s_user_ns;
> static const struct tree_descr bm_files[] = {
> [2] = {"status", &bm_status_operations, S_IWUSR|S_IRUGO},
> [3] = {"register", &bm_register_operations, S_IWUSR},
> /* last one */ {""}
> };
>
> + /* create a new binfmt namespace
> + * if we are not in the first user namespace
> + * but the binfmt namespace is the first one
> + */
> + if (READ_ONCE(ns->binfmt_ns) == NULL) {
> + struct binfmt_namespace *new_ns;
> +
> + new_ns = kmalloc(sizeof(struct binfmt_namespace),
> + GFP_KERNEL);
> + if (new_ns == NULL)
> + return -ENOMEM;
> + INIT_LIST_HEAD(&new_ns->entries);
> + new_ns->enabled = 1;
> + rwlock_init(&new_ns->entries_lock);
> + new_ns->bm_mnt = NULL;
> + new_ns->entry_count = 0;
> + /* ensure new_ns is completely initialized before sharing it */
> + smp_wmb();
> + WRITE_ONCE(ns->binfmt_ns, new_ns);
> + }
> +
> err = simple_fill_super(sb, BINFMTFS_MAGIC, bm_files);
> if (!err)
> sb->s_op = &s_ops;
> @@ -838,7 +889,10 @@ 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();
> +
> + return mount_ns(fs_type, flags, data, ns, ns,
> + bm_fill_super);
> }
>
> static struct linux_binfmt misc_format = {
> @@ -849,6 +903,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..990cf5950a89 100644
> --- a/kernel/user_namespace.c
> +++ b/kernel/user_namespace.c
> @@ -195,6 +195,9 @@ 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)
> + 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
* Re: [RFC PATCH v4 21/27] x86/cet/shstk: ELF header parsing of Shadow Stack
From: Yu-cheng Yu @ 2018-10-16 17:23 UTC (permalink / raw)
To: Kees Cook, Andy Lutomirski
Cc: X86 ML, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, LKML,
open list:DOCUMENTATION, Linux-MM, linux-arch, Linux API,
Arnd Bergmann, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Mike Kravetz,
Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <CAGXu5jKO5Ot5VAJBMHudgx40g4N2tqhLKHeCdS7rkFj1bPaHig@mail.gmail.com>
On Mon, 2018-10-15 at 16:40 -0700, Kees Cook wrote:
> On Fri, Sep 21, 2018 at 8:03 AM, Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> > Look in .note.gnu.property of an ELF file and check if Shadow Stack needs
> > to be enabled for the task.
[...]
> > +/*
> > + * 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];
> > + * }[];
> > + */
>
> Does NT_GNU_PROPERTY_TYPE_0 only ever contain property_x86 bytes? (I
> assume not, since there is a pr_type?)
There are other property types, but we only look for NT_GNU_PROPERTY_TYPE_0.
> > +
> > +#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));
>
> Cheaper to test n_type first...
Yes, Thanks!
>
> > +}
> > +
> > +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);
>
> I think this could overflow: n_namesz can be u64 for elf64_note.
>
> > + size = round_up(size + n->n_descsz, align);
>
> Same here. You may want to use check_add_overflow(), etc, an u64 types.
Note->n_namesz is always four-byte. I should have used u32.
>
> > +
> > + if (buf + size < buf)
> > + return NULL;
>
> I don't understand this. You want to check size not exceeding the
> allocation, which isn't passed into this function. Checking for a full
> unsigned address wrap around is not sufficient to detect overflow.
Here we only detect the warp around. After this returns we then check other
types of overflow in scan().
>
> > + 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;
>
> Why is *arg being updated? I don't see last_pr used outside of here --
> are properties required to be pr_type-ordered?
Yes, they need to be in ascending order.
>
> > +
> > + 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) ||
>
> Again, this "< buf" test doesn't look at all correct to me.
>
> > + (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)
>
> I'm not a fan of the short "scan", "test" and "next" names, and I
> really don't like an arg named "arg". Something slightly more
> descriptive for all of these would be nice, please.
I need to work on that :-) What would you suggest?
>
> > +{
> > + 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)) {
>
> These comparisons are safe due to the BUF_SIZE limit of buf_size and
> the only used size of item_size, but if this becomes more generic, it
> should be more defensive on the size calculations (e.g. make sure than
> "item_size < max" and then here "p < max - item_size", etc).
>
> I'd kind of rather this code walked the base type and check each for
> the matching feature. What is the general specification for what
> NT_GNU_PROPERTY_TYPE_0 contains?
There are other property types, but the kernel does not look at most of them.
If the kernel needs to look at others, we need to rewrite this.
[...]
> > +
> > +/*
> > + * 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;
>
> Why kmalloc over stack variable? (Or, does BUF_SIZE here really need
> to be 1024?)
BUF_SIZE can be smaller, for example 64. If it is too small, we need to do
kernel_read() too often.
>
> > +
> > + *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);
>
> Same overflow notes from earlier...
>
> > +
> > + 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;
>
> Instead of a separate parser here, wouldn't it be a bit nicer to
> attach this to the existing binfmt_elf program header parsing loop:
We need to wait until SET_PERSONALITY2() is done.
[...]
> > +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
>
> Should there be an #else error here?
Yes, thanks.
> I'd like to be using this code for a few other cases too (not just
> x86-specific). For example, for marking KASan binaries as needing a
> "legacy" memory layouts[1]. Others might be setting things like
> no_new_privs at exec time, etc.
If the item is a bit of GNU_PROPERTY_X86_FEATURE_1_AND, then this code would
work. Has it been finalized?
Yu-cheng
^ permalink raw reply
* Re: [RFC PATCH for 4.21 04/16] mm: Introduce vm_map_user_ram, vm_unmap_user_ram
From: Steven Rostedt @ 2018-10-16 18:30 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Peter Zijlstra, Paul E . McKenney, Boqun Feng, linux-kernel,
linux-api, Thomas Gleixner, Andy Lutomirski, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H . Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer,
Josh Triplett, Linus Torvalds, Catalin Marinas, Will Deacon,
Michael Kerrisk
In-Reply-To: <20181010191936.7495-5-mathieu.desnoyers@efficios.com>
On Wed, 10 Oct 2018 15:19:24 -0400
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> + * vm_unmap_user_ram - unmap linear kernel address space set up by vm_map_user_ram
> + * @mem: the pointer returned by vm_map_user_ram
> + * @count: the count passed to that vm_map_user_ram call (cannot unmap partial)
> + */
> +void vm_unmap_user_ram(const void *mem, unsigned int count)
> +{
> + unsigned long size = (unsigned long)count << PAGE_SHIFT;
> + unsigned long addr = (unsigned long)mem;
> + struct vmap_area *va;
> +
> + might_sleep();
> + BUG_ON(!addr);
> + BUG_ON(addr < VMALLOC_START);
> + BUG_ON(addr > VMALLOC_END);
> + BUG_ON(!PAGE_ALIGNED(addr));
> +
> + debug_check_no_locks_freed(mem, size);
> + va = find_vmap_area(addr);
> + BUG_ON(!va);
> + free_unmap_vmap_area(va);
> +}
> +EXPORT_SYMBOL(vm_unmap_user_ram);
> +
Noticing this from Sergey's question in another patch, why are you
using BUG_ON()? That's rather extreme and something we are trying to
avoid adding more of (I still need to remove the BUG_ON()s I've added
over ten years ago). I don't see why all these BUG_ON's can't be turned
into:
if (WARN_ON(x))
return;
-- Steve
^ permalink raw reply
* Re: [RFC PATCH for 4.21 06/16] cpu_opv: Provide cpu_opv system call (v8)
From: Mathieu Desnoyers @ 2018-10-16 19:17 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Peter Zijlstra, Paul E. McKenney, Boqun Feng, linux-kernel,
linux-api, Thomas Gleixner, Andy Lutomirski, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Linus Torvalds, Catalin Marinas, Will
In-Reply-To: <20181016081029.GA30363@jagdpanzerIV>
----- On Oct 16, 2018, at 4:10 AM, Sergey Senozhatsky sergey.senozhatsky.work@gmail.com wrote:
> Hi Mathieu,
>
> On (10/10/18 15:19), Mathieu Desnoyers wrote:
> [..]
>> +SYSCALL_DEFINE4(cpu_opv, struct cpu_op __user *, ucpuopv, int, cpuopcnt,
>> + int, cpu, int, flags)
>> +{
> [..]
>> +again:
>> + ret = cpu_opv_pin_pages(cpuopv, cpuopcnt, &vaddr_ptrs);
>> + if (ret)
>> + goto end;
>> + ret = do_cpu_opv(cpuopv, cpuopcnt, &vaddr_ptrs, cpu);
>> + if (ret == -EAGAIN)
>> + retry = true;
>> +end:
>> + for (i = 0; i < vaddr_ptrs.nr_vaddr; i++) {
>> + struct vaddr *vaddr = &vaddr_ptrs.addr[i];
>> + int j;
>> +
>> + vm_unmap_user_ram((void *)vaddr->mem, vaddr->nr_pages);
>
> A dumb question.
>
> Both vm_unmap_user_ram() and vm_map_user_ram() can BUG_ON().
> So this is
> userspace -> syscall -> cpu_opv() -> vm_unmap_user_ram() -> BUG_ON()
>
> Any chance someone can exploit it?
Hi Sergey,
Let's look at vm_unmap_user_ram() and vm_map_user_ram() separately.
If we look at the input from vm_unmap_user_ram, it's called with the
following parameters by the cpu_opv system call:
for (i = 0; i < vaddr_ptrs.nr_vaddr; i++) {
struct vaddr *vaddr = &vaddr_ptrs.addr[i];
int j;
vm_unmap_user_ram((void *)vaddr->mem, vaddr->nr_pages);
[...]
}
The vaddr_ptrs array content is filled by the call to cpu_opv_pin_pages above:
ret = cpu_opv_pin_pages(cpuopv, cpuopcnt, &vaddr_ptrs);
if (ret)
goto end;
by passing the array to cpu_op_pin_pages(), which appends a virtual address at
the end of the array (on success) and increments nr_vaddr. Those virtual
addresses are returned by vm_map_user_ram(), so they are not user-controlled.
Therefore, only an internal kernel bug between vm_map_user_ram() and
vm_unmap_user_ram() should trigger the BUG_ON(). No user input is passed
to vm_unmap_user_ram().
Now, let's look at vm_map_user_ram(). It calls alloc_vmap_area(), which returns
a vmap_area. Then if vmap_page_range failed, vm_unmap_user_ram is called on the
memory that has just been returned by vm_map_user_ram. Again, only an internal
bug between map/unmap can trigger the BUG_ON() in vm_unmap_user_ram.
Is there another scenario I missed ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.21 04/16] mm: Introduce vm_map_user_ram, vm_unmap_user_ram
From: Mathieu Desnoyers @ 2018-10-16 19:21 UTC (permalink / raw)
To: rostedt
Cc: Peter Zijlstra, Paul E. McKenney, Boqun Feng, linux-kernel,
linux-api, Thomas Gleixner, Andy Lutomirski, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer,
Josh Triplett, Linus Torvalds, Catalin Marinas, Will Deacon,
Michae
In-Reply-To: <20181016143016.10da89bd@gandalf.local.home>
----- On Oct 16, 2018, at 2:30 PM, rostedt rostedt@goodmis.org wrote:
> On Wed, 10 Oct 2018 15:19:24 -0400
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>
>> + * vm_unmap_user_ram - unmap linear kernel address space set up by
>> vm_map_user_ram
>> + * @mem: the pointer returned by vm_map_user_ram
>> + * @count: the count passed to that vm_map_user_ram call (cannot unmap partial)
>> + */
>> +void vm_unmap_user_ram(const void *mem, unsigned int count)
>> +{
>> + unsigned long size = (unsigned long)count << PAGE_SHIFT;
>> + unsigned long addr = (unsigned long)mem;
>> + struct vmap_area *va;
>> +
>> + might_sleep();
>> + BUG_ON(!addr);
>> + BUG_ON(addr < VMALLOC_START);
>> + BUG_ON(addr > VMALLOC_END);
>> + BUG_ON(!PAGE_ALIGNED(addr));
>> +
>> + debug_check_no_locks_freed(mem, size);
>> + va = find_vmap_area(addr);
>> + BUG_ON(!va);
>> + free_unmap_vmap_area(va);
>> +}
>> +EXPORT_SYMBOL(vm_unmap_user_ram);
>> +
>
> Noticing this from Sergey's question in another patch, why are you
> using BUG_ON()? That's rather extreme and something we are trying to
> avoid adding more of (I still need to remove the BUG_ON()s I've added
> over ten years ago). I don't see why all these BUG_ON's can't be turned
> into:
>
> if (WARN_ON(x))
> return;
I borrowed the code from vm_unmap_ram(), which has the following checks:
BUG_ON(!addr);
BUG_ON(addr < VMALLOC_START);
BUG_ON(addr > VMALLOC_END);
BUG_ON(!PAGE_ALIGNED(addr));
[...]
va = find_vmap_area(addr);
BUG_ON(!va);
The expectation here is that inputs to vm_unmap_ram() should always come from
vm_map_ram(), so an erroneous input is an internal kernel bug. I applied the
same logic to vm_unmap_user_ram() and vm_map_user_ram().
Should we turn all those BUG_ON() into if (WARN_ON(x)) return; in vm_{map,unmap}_ram
as well ?
Thanks,
Mathieu
>
> -- Steve
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.21 04/16] mm: Introduce vm_map_user_ram, vm_unmap_user_ram
From: Steven Rostedt @ 2018-10-16 19:40 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Peter Zijlstra, Paul E. McKenney, Boqun Feng, linux-kernel,
linux-api, Thomas Gleixner, Andy Lutomirski, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer,
Josh Triplett, Linus Torvalds, Catalin Marinas, Will Deacon,
Michae
In-Reply-To: <1635827931.274.1539717691833.JavaMail.zimbra@efficios.com>
On Tue, 16 Oct 2018 15:21:31 -0400 (EDT)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> ----- On Oct 16, 2018, at 2:30 PM, rostedt rostedt@goodmis.org wrote:
>
> > On Wed, 10 Oct 2018 15:19:24 -0400
> > Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> >
> >> + * vm_unmap_user_ram - unmap linear kernel address space set up by
> >> vm_map_user_ram
> >> + * @mem: the pointer returned by vm_map_user_ram
> >> + * @count: the count passed to that vm_map_user_ram call (cannot unmap partial)
> >> + */
> >> +void vm_unmap_user_ram(const void *mem, unsigned int count)
> >> +{
> >> + unsigned long size = (unsigned long)count << PAGE_SHIFT;
> >> + unsigned long addr = (unsigned long)mem;
> >> + struct vmap_area *va;
> >> +
> >> + might_sleep();
> >> + BUG_ON(!addr);
> >> + BUG_ON(addr < VMALLOC_START);
> >> + BUG_ON(addr > VMALLOC_END);
> >> + BUG_ON(!PAGE_ALIGNED(addr));
> >> +
> >> + debug_check_no_locks_freed(mem, size);
> >> + va = find_vmap_area(addr);
> >> + BUG_ON(!va);
> >> + free_unmap_vmap_area(va);
> >> +}
> >> +EXPORT_SYMBOL(vm_unmap_user_ram);
> >> +
> >
> > Noticing this from Sergey's question in another patch, why are you
> > using BUG_ON()? That's rather extreme and something we are trying to
> > avoid adding more of (I still need to remove the BUG_ON()s I've added
> > over ten years ago). I don't see why all these BUG_ON's can't be turned
> > into:
> >
> > if (WARN_ON(x))
> > return;
>
> I borrowed the code from vm_unmap_ram(), which has the following checks:
>
> BUG_ON(!addr);
> BUG_ON(addr < VMALLOC_START);
> BUG_ON(addr > VMALLOC_END);
> BUG_ON(!PAGE_ALIGNED(addr));
> [...]
> va = find_vmap_area(addr);
> BUG_ON(!va);
>
> The expectation here is that inputs to vm_unmap_ram() should always come from
> vm_map_ram(), so an erroneous input is an internal kernel bug. I applied the
> same logic to vm_unmap_user_ram() and vm_map_user_ram().
>
> Should we turn all those BUG_ON() into if (WARN_ON(x)) return; in vm_{map,unmap}_ram
> as well ?
>
>
I would argue yes! That code was added in 2008 (which is also the same
year I added BUG_ON() to my code). Back then it wasn't such an issue,
but today we are finding (and Linus has been complaining) that BUG_ON
really shouldn't be necessary. Especially if you can get out of the
function with a simple return.
-- Steve
^ permalink raw reply
* [PATCH v2 0/2] sysctl: handle overflow for file-max
From: Christian Brauner @ 2018-10-16 19:53 UTC (permalink / raw)
To: keescook, linux-kernel
Cc: ebiederm, mcgrof, akpm, joe.lawrence, longman, linux, viro,
adobriyan, linux-api, Christian Brauner
Hey,
Here is v2 of this patchset. Changelogs are in the individual commits.
This is a new approach after I have done more reasoning about it.
Currently, when writing
echo 18446744073709551616 > /proc/sys/fs/file-max
/proc/sys/fs/file-max will overflow and be set to 0. That quickly
crashes the system.
The first version of this patch intended to detect the overflow and cap
at ULONG_MAX. However, we should not do this and rather return EINVAL on
overflow. The reasons are:
- this aligns with other sysctl handlers that simply reject overflows
(cf. [1], [2], and a bunch of others)
- we already do a partial fail on overflow right now
Namely, when the TMPBUFLEN is exceeded. So we already reject values
such as 184467440737095516160 (21 chars) but accept values such as
18446744073709551616 (20 chars) but both are overflows. So we should
just always reject 64bit overflows and not special-case this based on
the number of chars.
(This patchset is in reference to https://lkml.org/lkml/2018/10/11/585.)
Thanks!
Christian
[1]: fb910c42cceb ("sysctl: check for UINT_MAX before unsigned int min/max")
[2]: 196851bed522 ("s390/topology: correct topology mode proc handler")
Christian Brauner (2):
sysctl: handle overflow in proc_get_long
sysctl: handle overflow for file-max
kernel/sysctl.c | 31 ++++++++++++++++++++++++++++++-
1 file changed, 30 insertions(+), 1 deletion(-)
--
2.17.1
^ permalink raw reply
* [PATCH v2 1/2] sysctl: handle overflow in proc_get_long
From: Christian Brauner @ 2018-10-16 19:53 UTC (permalink / raw)
To: keescook, linux-kernel
Cc: ebiederm, mcgrof, akpm, joe.lawrence, longman, linux, viro,
adobriyan, linux-api, Christian Brauner
In-Reply-To: <20181016195337.2440-1-christian@brauner.io>
proc_get_long() is a funny function. It uses simple_strtoul() and for a
good reason. proc_get_long() wants to always succeed the parse and return
the maybe incorrect value and the trailing characters to check against a
pre-defined list of acceptable trailing values.
However, simple_strtoul() explicitly ignores overflows which can cause
funny things like the following to happen:
echo 18446744073709551616 > /proc/sys/fs/file-max
cat /proc/sys/fs/file-max
0
(Which will cause your system to silently die behind your back.)
On the other hand kstrtoul() does do overflow detection but does not return
the trailing characters, and also fails the parse when anything other than
'\n' is a trailing character whereas proc_get_long() wants to be more
lenient.
Now, before adding another kstrtoul() function let's simply add a static
parse strtoul_lenient() which:
- fails on overflow with -ERANGE
- returns the trailing characters to the caller
The reason why we should fail on ERANGE is that we already do a partial
fail on overflow right now. Namely, when the TMPBUFLEN is exceeded. So we
already reject values such as 184467440737095516160 (21 chars) but accept
values such as 18446744073709551616 (20 chars) but both are overflows. So
we should just always reject 64bit overflows and not special-case this
based on the number of chars.
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Christian Brauner <christian@brauner.io>
---
v1->v2:
- s/sysctl_cap_erange/sysctl_lenient/g
- consistenly fail on overflow
v0->v1:
- s/sysctl_strtoul_lenient/strtoul_cap_erange/g
- (Al) remove bool overflow return argument from strtoul_cap_erange
- (Al) return ULONG_MAX on ERANGE from strtoul_cap_erange
- (Dominik) fix spelling in commit message
---
kernel/sysctl.c | 24 +++++++++++++++++++++++-
1 file changed, 23 insertions(+), 1 deletion(-)
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index cc02050fd0c4..7d98e02e5d72 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -67,6 +67,7 @@
#include <linux/bpf.h>
#include <linux/mount.h>
#include <linux/pipe_fs_i.h>
+#include <../lib/kstrtox.h>
#include <linux/uaccess.h>
#include <asm/processor.h>
@@ -2065,6 +2066,26 @@ static void proc_skip_char(char **buf, size_t *size, const char v)
}
}
+static int strtoul_lenient(const char *cp, char **endp, unsigned int base,
+ unsigned long *res)
+{
+ unsigned long long result;
+ unsigned int rv;
+
+ cp = _parse_integer_fixup_radix(cp, &base);
+ rv = _parse_integer(cp, base, &result);
+ if ((rv & KSTRTOX_OVERFLOW) || (result != (unsigned long)result))
+ return -ERANGE;
+
+ cp += rv;
+
+ if (endp)
+ *endp = (char *)cp;
+
+ *res = (unsigned long)result;
+ return 0;
+}
+
#define TMPBUFLEN 22
/**
* proc_get_long - reads an ASCII formatted integer from a user buffer
@@ -2108,7 +2129,8 @@ static int proc_get_long(char **buf, size_t *size,
if (!isdigit(*p))
return -EINVAL;
- *val = simple_strtoul(p, &p, 0);
+ if (strtoul_lenient(p, &p, 0, val))
+ return -EINVAL;
len = p - tmp;
--
2.17.1
^ permalink raw reply related
* [PATCH v2 2/2] sysctl: handle overflow for file-max
From: Christian Brauner @ 2018-10-16 19:53 UTC (permalink / raw)
To: keescook, linux-kernel
Cc: ebiederm, mcgrof, akpm, joe.lawrence, longman, linux, viro,
adobriyan, linux-api, Christian Brauner
In-Reply-To: <20181016195337.2440-1-christian@brauner.io>
Currently, when writing
echo 18446744073709551616 > /proc/sys/fs/file-max
/proc/sys/fs/file-max will overflow and be set to 0. That quickly
crashes the system.
This commit sets the max and min value for file-max and returns -EINVAL
when a long int is exceeded. Any higher value cannot currently be used as
the percpu counters are long ints and not unsigned integers. This behavior
also aligns with other tuneables that return -EINVAL when their range is
exceeded. See e.g. [1], [2] and others.
[1]: fb910c42cceb ("sysctl: check for UINT_MAX before unsigned int min/max")
[2]: 196851bed522 ("s390/topology: correct topology mode proc handler")
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Christian Brauner <christian@brauner.io>
---
v2->v1:
- consistenly fail on overflow
v0->v1:
- if max value is < than ULONG_MAX use max as upper bound
- (Dominik) remove double "the" from commit message
---
kernel/sysctl.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 7d98e02e5d72..0874001e5435 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -127,6 +127,7 @@ static int __maybe_unused one = 1;
static int __maybe_unused two = 2;
static int __maybe_unused four = 4;
static unsigned long one_ul = 1;
+static unsigned long long_max = LONG_MAX;
static int one_hundred = 100;
static int one_thousand = 1000;
#ifdef CONFIG_PRINTK
@@ -1696,6 +1697,8 @@ static struct ctl_table fs_table[] = {
.maxlen = sizeof(files_stat.max_files),
.mode = 0644,
.proc_handler = proc_doulongvec_minmax,
+ .extra1 = &zero,
+ .extra2 = &long_max,
},
{
.procname = "nr_open",
@@ -2797,6 +2800,10 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
break;
if (neg)
continue;
+ if ((max && val > *max) || (min && val < *min)) {
+ err = -EINVAL;
+ break;
+ }
val = convmul * val / convdiv;
if ((min && val < *min) || (max && val > *max))
continue;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v2 2/2] sysctl: handle overflow for file-max
From: Waiman Long @ 2018-10-16 20:49 UTC (permalink / raw)
To: Christian Brauner, keescook, linux-kernel
Cc: ebiederm, mcgrof, akpm, joe.lawrence, linux, viro, adobriyan,
linux-api
In-Reply-To: <20181016195337.2440-3-christian@brauner.io>
On 10/16/2018 03:53 PM, Christian Brauner wrote:
> Currently, when writing
>
> echo 18446744073709551616 > /proc/sys/fs/file-max
>
> /proc/sys/fs/file-max will overflow and be set to 0. That quickly
> crashes the system.
> This commit sets the max and min value for file-max and returns -EINVAL
> when a long int is exceeded. Any higher value cannot currently be used as
> the percpu counters are long ints and not unsigned integers. This behavior
> also aligns with other tuneables that return -EINVAL when their range is
> exceeded. See e.g. [1], [2] and others.
>
> [1]: fb910c42cceb ("sysctl: check for UINT_MAX before unsigned int min/max")
> [2]: 196851bed522 ("s390/topology: correct topology mode proc handler")
>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Christian Brauner <christian@brauner.io>
> ---
> v2->v1:
> - consistenly fail on overflow
> v0->v1:
> - if max value is < than ULONG_MAX use max as upper bound
> - (Dominik) remove double "the" from commit message
> ---
> kernel/sysctl.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 7d98e02e5d72..0874001e5435 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -127,6 +127,7 @@ static int __maybe_unused one = 1;
> static int __maybe_unused two = 2;
> static int __maybe_unused four = 4;
> static unsigned long one_ul = 1;
> +static unsigned long long_max = LONG_MAX;
> static int one_hundred = 100;
> static int one_thousand = 1000;
> #ifdef CONFIG_PRINTK
> @@ -1696,6 +1697,8 @@ static struct ctl_table fs_table[] = {
> .maxlen = sizeof(files_stat.max_files),
> .mode = 0644,
> .proc_handler = proc_doulongvec_minmax,
> + .extra1 = &zero,
> + .extra2 = &long_max,
> },
> {
> .procname = "nr_open",
> @@ -2797,6 +2800,10 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
> break;
> if (neg)
> continue;
> + if ((max && val > *max) || (min && val < *min)) {
> + err = -EINVAL;
> + break;
> + }
> val = convmul * val / convdiv;
Should the conversion be done before the min/max check?
> if ((min && val < *min) || (max && val > *max))
> continue;
You may be able to drop the above statement.
Cheers,
Longman
^ permalink raw reply
* Re: [RFC PATCH] mm, proc: report PR_SET_THP_DISABLE in proc
From: David Rientjes @ 2018-10-16 21:24 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: <20181016104855.GQ18839@dhcp22.suse.cz>
On Tue, 16 Oct 2018, Michal Hocko wrote:
> > I don't understand the point of extending smaps with yet another line.
>
> Because abusing a vma flag part is just wrong. What are you going to do
> when a next bug report states that the flag is set even though no
> userspace has set it and that leads to some malfunctioning? Can you rule
> that out? Even your abuse of the flag is surprising so why others
> wouldn't be?
>
The flag has taken on the meaning of "thp disabled for this vma", how it
is set is not the scope of the flag. If a thp is explicitly disabled from
being eligible for thp, whether by madvise, prctl, or any future
mechanism, it should use VM_NOHUGEPAGE or show_smap_vma_flags() needs to
be modified.
I agree with you that this could have been done better if an interface was
defined earlier that userspace could have used. PR_SET_THP_DISABLE was
merged long after thp had already been merged so this can be a reminder
that defining clean, robust, and extensible APIs is important, but I'm
afraid we can't go back in time and change how userspace queries
information, especially in cases where there was only one way to do it.
> > The only way for a different process to determine if a single vma from
> > another process is thp disabled is by the "nh" flag, so it is reasonable
> > that userspace reads this. My patch fixes that. If smaps is extended
> > with another line per your patch, it doesn't change the fact that previous
> > binaries are built to check for "nh" so it does not deprecate that.
> > ("THP_Enabled" is also ambiguous since it only refers to prctl and not the
> > default thp setting or madvise.)
>
> As I've said there are two things. Exporting PR_SET_THP_DISABLE to
> userspace so that a 3rd party process can query it. I've already
> explained why that might be useful. If you really insist on having
> a per-vma field then let's do it properly now. Are you going to agree on
> that? If yes, I am willing to spend my time on that but I am not going
> to bother if this will lead to "I want my vma field abuse anyway".
I think what you and I want is largely irrelevant :) What's important is
that there are userspace implementations that query this today so
continuing to support it as the way to determine if a vma has been thp
disabled doesn't seem problematic and guarantees that userspace doesn't
break.
^ permalink raw reply
* Re: [PATCH v2 1/2] sysctl: handle overflow in proc_get_long
From: Kees Cook @ 2018-10-16 21:46 UTC (permalink / raw)
To: Christian Brauner
Cc: LKML, Eric W. Biederman, Luis R. Rodriguez, Andrew Morton,
Joe Lawrence, Waiman Long, Dominik Brodowski, Al Viro,
Alexey Dobriyan, Linux API
In-Reply-To: <20181016195337.2440-2-christian@brauner.io>
On Tue, Oct 16, 2018 at 12:53 PM, Christian Brauner
<christian@brauner.io> wrote:
> proc_get_long() is a funny function. It uses simple_strtoul() and for a
> good reason. proc_get_long() wants to always succeed the parse and return
> the maybe incorrect value and the trailing characters to check against a
> pre-defined list of acceptable trailing values.
> However, simple_strtoul() explicitly ignores overflows which can cause
> funny things like the following to happen:
>
> echo 18446744073709551616 > /proc/sys/fs/file-max
> cat /proc/sys/fs/file-max
> 0
>
> (Which will cause your system to silently die behind your back.)
>
> On the other hand kstrtoul() does do overflow detection but does not return
> the trailing characters, and also fails the parse when anything other than
> '\n' is a trailing character whereas proc_get_long() wants to be more
> lenient.
>
> Now, before adding another kstrtoul() function let's simply add a static
> parse strtoul_lenient() which:
> - fails on overflow with -ERANGE
> - returns the trailing characters to the caller
Can you add this as kerndoc above strtoul_lenient()? (New people
reading this code should be able to tell quickly what it's lenient
about, and how it behaves without have to know the internals of
kstrtox.h.)
> The reason why we should fail on ERANGE is that we already do a partial
> fail on overflow right now. Namely, when the TMPBUFLEN is exceeded. So we
> already reject values such as 184467440737095516160 (21 chars) but accept
> values such as 18446744073709551616 (20 chars) but both are overflows. So
> we should just always reject 64bit overflows and not special-case this
> based on the number of chars.
Yup -- I think this makes a lot of sense.
>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Christian Brauner <christian@brauner.io>
> ---
> v1->v2:
> - s/sysctl_cap_erange/sysctl_lenient/g
> - consistenly fail on overflow
> v0->v1:
> - s/sysctl_strtoul_lenient/strtoul_cap_erange/g
> - (Al) remove bool overflow return argument from strtoul_cap_erange
> - (Al) return ULONG_MAX on ERANGE from strtoul_cap_erange
> - (Dominik) fix spelling in commit message
> ---
> kernel/sysctl.c | 24 +++++++++++++++++++++++-
> 1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index cc02050fd0c4..7d98e02e5d72 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -67,6 +67,7 @@
> #include <linux/bpf.h>
> #include <linux/mount.h>
> #include <linux/pipe_fs_i.h>
> +#include <../lib/kstrtox.h>
Should this be "../lib/kstrtox.h" instead of <>?
>
> #include <linux/uaccess.h>
> #include <asm/processor.h>
> @@ -2065,6 +2066,26 @@ static void proc_skip_char(char **buf, size_t *size, const char v)
> }
> }
>
> +static int strtoul_lenient(const char *cp, char **endp, unsigned int base,
> + unsigned long *res)
> +{
> + unsigned long long result;
> + unsigned int rv;
> +
> + cp = _parse_integer_fixup_radix(cp, &base);
> + rv = _parse_integer(cp, base, &result);
> + if ((rv & KSTRTOX_OVERFLOW) || (result != (unsigned long)result))
> + return -ERANGE;
> +
> + cp += rv;
> +
> + if (endp)
> + *endp = (char *)cp;
> +
> + *res = (unsigned long)result;
> + return 0;
> +}
> +
> #define TMPBUFLEN 22
> /**
> * proc_get_long - reads an ASCII formatted integer from a user buffer
> @@ -2108,7 +2129,8 @@ static int proc_get_long(char **buf, size_t *size,
> if (!isdigit(*p))
> return -EINVAL;
>
> - *val = simple_strtoul(p, &p, 0);
> + if (strtoul_lenient(p, &p, 0, val))
> + return -EINVAL;
>
> len = p - tmp;
>
> --
> 2.17.1
>
With kerndoc added, please consider this acked by me:
Acked-by: Kees Cook <keescook@chromium.org>
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* [PATCH v3 0/2] sysctl: handle overflow for file-max
From: Christian Brauner @ 2018-10-16 22:33 UTC (permalink / raw)
To: keescook, linux-kernel
Cc: ebiederm, mcgrof, akpm, joe.lawrence, longman, linux, viro,
adobriyan, linux-api, Christian Brauner
Hey,
Here is v3 of this patchset. Changelogs are in the individual commits.
Currently, when writing
echo 18446744073709551616 > /proc/sys/fs/file-max
/proc/sys/fs/file-max will overflow and be set to 0. That quickly
crashes the system.
The first version of this patch intended to detect the overflow and cap
at ULONG_MAX. However, we should not do this and rather return EINVAL on
overflow. The reasons are:
- this aligns with other sysctl handlers that simply reject overflows
(cf. [1], [2], and a bunch of others)
- we already do a partial fail on overflow right now
Namely, when the TMPBUFLEN is exceeded. So we already reject values
such as 184467440737095516160 (21 chars) but accept values such as
18446744073709551616 (20 chars) but both are overflows. So we should
just always reject 64bit overflows and not special-case this based on
the number of chars.
(This patchset is in reference to https://lkml.org/lkml/2018/10/11/585.)
Thanks!
Christian
[1]: fb910c42cceb ("sysctl: check for UINT_MAX before unsigned int min/max")
[2]: 196851bed522 ("s390/topology: correct topology mode proc handler")
Christian Brauner (2):
sysctl: handle overflow in proc_get_long
sysctl: handle overflow for file-max
kernel/sysctl.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 46 insertions(+), 1 deletion(-)
--
2.17.1
^ 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