* Re: [RFC PATCH] Add /proc/<pid>/numa_vamaps for numa node information
From: Jann Horn @ 2018-09-14 12:49 UTC (permalink / raw)
To: Michal Hocko
Cc: Prakash Sangappa, Dave Hansen, Anshuman Khandual, Andrew Morton,
kernel list, Linux-MM, Linux API, Kirill A . Shutemov,
n-horiguchi, Ulrich Drepper, David Rientjes, Horiguchi Naoya,
steven.sistare
In-Reply-To: <20180914062132.GI20287@dhcp22.suse.cz>
On Fri, Sep 14, 2018 at 8:21 AM Michal Hocko <mhocko@kernel.org> wrote:
> On Fri 14-09-18 03:33:28, Jann Horn wrote:
> > On Wed, Sep 12, 2018 at 10:43 PM prakash.sangappa
> > <prakash.sangappa@oracle.com> wrote:
> > > On 05/09/2018 04:31 PM, Dave Hansen wrote:
> > > > On 05/07/2018 06:16 PM, prakash.sangappa wrote:
> > > >> It will be /proc/<pid>/numa_vamaps. Yes, the behavior will be
> > > >> different with respect to seeking. Output will still be text and
> > > >> the format will be same.
> > > >>
> > > >> I want to get feedback on this approach.
> > > > I think it would be really great if you can write down a list of the
> > > > things you actually want to accomplish. Dare I say: you need a
> > > > requirements list.
> > > >
> > > > The numa_vamaps approach continues down the path of an ever-growing list
> > > > of highly-specialized /proc/<pid> files. I don't think that is
> > > > sustainable, even if it has been our trajectory for many years.
> > > >
> > > > Pagemap wasn't exactly a shining example of us getting new ABIs right,
> > > > but it sounds like something along those is what we need.
> > >
> > > Just sent out a V2 patch. This patch simplifies the file content. It
> > > only provides VA range to numa node id information.
> > >
> > > The requirement is basically observability for performance analysis.
> > >
> > > - Need to be able to determine VA range to numa node id information.
> > > Which also gives an idea of which range has memory allocated.
> > >
> > > - The proc file /proc/<pid>/numa_vamaps is in text so it is easy to
> > > directly view.
> > >
> > > The V2 patch supports seeking to a particular process VA from where
> > > the application could read the VA to numa node id information.
> > >
> > > Also added the 'PTRACE_MODE_READ_REALCREDS' check when opening the
> > > file /proc file as was indicated by Michal Hacko
> >
> > procfs files should use PTRACE_MODE_*_FSCREDS, not PTRACE_MODE_*_REALCREDS.
>
> Out of my curiosity, what is the semantic difference? At least
> kernel_move_pages uses PTRACE_MODE_READ_REALCREDS. Is this a bug?
No, that's fine. REALCREDS basically means "look at the caller's real
UID for the access check", while FSCREDS means "look at the caller's
filesystem UID". The ptrace access check has historically been using
the real UID, which is sorta weird, but normally works fine. Given
that this is documented, I didn't see any reason to change it for most
things that do ptrace access checks, even if the EUID would IMO be
more appropriate. But things that capture caller credentials at points
like open() really shouldn't look at the real UID; instead, they
should use the filesystem UID (which in practice is basically the same
as the EUID).
So in short, it depends on the interface you're coming through: Direct
syscalls use REALCREDS, things that go through the VFS layer use
FSCREDS.
^ permalink raw reply
* [PATCH] [RFC] making uapi/linux/elfcore.h useful again
From: Arnd Bergmann @ 2018-09-14 11:38 UTC (permalink / raw)
To: linux-arch
Cc: linux-api, Arnd Bergmann, Joseph Myers , David Howells,
libc-alpha, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
linux-kernel
After finding a bug in glibc the question came up how linux/elfcore.h
is supposed to be used from user space. As far as I can tell, it's
not possible, as it references data types that are simply unavailable
there.
The #ifndef __KERNEL__ section in that header dates back to when the
file was introduced in linux-1.3.5, and presumably was meant to
provide the structures for the libc sys/procfs.h implementation.
However, this was never portable to architectures other than x86-32,
and has been broken on that architecture at a later point.
These are the steps that I needed to make it possible to include the
header file, e.g. for libc self-testing in order to make sure the
structures are compatible with its own:
- drop the #ifndef __KERNEL__ section that are obviously useless
and get in the way
- change the pid_t references to __kernel_pid_t
- Move required data from the private x86 asm/elf.h file into
a new uapi/asm/elf.h. Some other architectures already do that,
but most of them do not. Before applying the patch, we have
to do this for all architectures
- Change ELF_NGREG to an integer literal constant instead of
a sizeof operation based on a private type.
Cc: Joseph Myers <joseph@codesourcery.com>
Cc: David Howells <dhowells@redhat.com>
Cc: libc-alpha@sourceware.org
Link: https://patchwork.ozlabs.org/patch/969540/
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
arch/x86/include/asm/elf.h | 24 +-----------------------
arch/x86/include/uapi/asm/elf.h | 30 ++++++++++++++++++++++++++++++
arch/x86/include/uapi/asm/signal.h | 2 +-
include/uapi/linux/elf.h | 1 +
include/uapi/linux/elfcore.h | 26 +++++---------------------
5 files changed, 38 insertions(+), 45 deletions(-)
create mode 100644 arch/x86/include/uapi/asm/elf.h
diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
index 0d157d2a1e2a..973bd7b5b164 100644
--- a/arch/x86/include/asm/elf.h
+++ b/arch/x86/include/asm/elf.h
@@ -10,18 +10,10 @@
#include <asm/ptrace.h>
#include <asm/user.h>
#include <asm/auxvec.h>
-
-typedef unsigned long elf_greg_t;
-
-#define ELF_NGREG (sizeof(struct user_regs_struct) / sizeof(elf_greg_t))
-typedef elf_greg_t elf_gregset_t[ELF_NGREG];
-
-typedef struct user_i387_struct elf_fpregset_t;
+#include <uapi/asm/elf.h>
#ifdef __i386__
-typedef struct user_fxsr_struct elf_fpxregset_t;
-
#define R_386_NONE 0
#define R_386_32 1
#define R_386_PC32 2
@@ -35,13 +27,6 @@ typedef struct user_fxsr_struct elf_fpxregset_t;
#define R_386_GOTPC 10
#define R_386_NUM 11
-/*
- * These are used to set parameters in the core dumps.
- */
-#define ELF_CLASS ELFCLASS32
-#define ELF_DATA ELFDATA2LSB
-#define ELF_ARCH EM_386
-
#else
/* x86-64 relocation types */
@@ -65,13 +50,6 @@ typedef struct user_fxsr_struct elf_fpxregset_t;
#define R_X86_64_NUM 16
-/*
- * These are used to set parameters in the core dumps.
- */
-#define ELF_CLASS ELFCLASS64
-#define ELF_DATA ELFDATA2LSB
-#define ELF_ARCH EM_X86_64
-
#endif
#include <asm/vdso.h>
diff --git a/arch/x86/include/uapi/asm/elf.h b/arch/x86/include/uapi/asm/elf.h
new file mode 100644
index 000000000000..a640e1224939
--- /dev/null
+++ b/arch/x86/include/uapi/asm/elf.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_ASM_X86_ELF_H
+#define _UAPI_ASM_X86_ELF_H
+
+#ifdef __i386__
+
+/*
+ * These are used to set parameters in the core dumps.
+ */
+#define ELF_CLASS ELFCLASS32
+#define ELF_DATA ELFDATA2LSB
+#define ELF_ARCH EM_386
+#define ELF_NGREG 17
+
+#else
+
+/*
+ * These are used to set parameters in the core dumps.
+ */
+#define ELF_CLASS ELFCLASS64
+#define ELF_DATA ELFDATA2LSB
+#define ELF_ARCH EM_X86_64
+#define ELF_NGREG 27
+
+#endif /* __i386__ */
+
+typedef unsigned long elf_greg_t;
+typedef elf_greg_t elf_gregset_t[ELF_NGREG];
+
+#endif
diff --git a/arch/x86/include/uapi/asm/signal.h b/arch/x86/include/uapi/asm/signal.h
index e5745d593dc7..00f273eaddf7 100644
--- a/arch/x86/include/uapi/asm/signal.h
+++ b/arch/x86/include/uapi/asm/signal.h
@@ -128,7 +128,7 @@ struct sigaction {
typedef struct sigaltstack {
void __user *ss_sp;
int ss_flags;
- size_t ss_size;
+ __kernel_size_t ss_size;
} stack_t;
#endif /* __ASSEMBLY__ */
diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
index c5358e0ae7c5..e1e4561ed9c2 100644
--- a/include/uapi/linux/elf.h
+++ b/include/uapi/linux/elf.h
@@ -4,6 +4,7 @@
#include <linux/types.h>
#include <linux/elf-em.h>
+#include <asm/elf.h>
/* 32-bit ELF base types. */
typedef __u32 Elf32_Addr;
diff --git a/include/uapi/linux/elfcore.h b/include/uapi/linux/elfcore.h
index baf03562306d..9c0078004275 100644
--- a/include/uapi/linux/elfcore.h
+++ b/include/uapi/linux/elfcore.h
@@ -16,15 +16,6 @@ struct elf_siginfo
int si_errno; /* errno */
};
-
-#ifndef __KERNEL__
-typedef elf_greg_t greg_t;
-typedef elf_gregset_t gregset_t;
-typedef elf_fpregset_t fpregset_t;
-typedef elf_fpxregset_t fpxregset_t;
-#define NGREG ELF_NGREG
-#endif
-
/*
* Definitions to generate Intel SVR4-like core files.
* These mostly have the same names as the SVR4 types with "elf_"
@@ -49,10 +40,10 @@ struct elf_prstatus
struct sigaltstack pr_altstack; /* Alternate stack info */
struct sigaction pr_action; /* Signal action for current sig */
#endif
- pid_t pr_pid;
- pid_t pr_ppid;
- pid_t pr_pgrp;
- pid_t pr_sid;
+ __kernel_pid_t pr_pid;
+ __kernel_pid_t pr_ppid;
+ __kernel_pid_t pr_pgrp;
+ __kernel_pid_t pr_sid;
struct __kernel_old_timeval pr_utime; /* User time */
struct __kernel_old_timeval pr_stime; /* System time */
struct __kernel_old_timeval pr_cutime; /* Cumulative user time */
@@ -85,17 +76,10 @@ struct elf_prpsinfo
unsigned long pr_flag; /* flags */
__kernel_uid_t pr_uid;
__kernel_gid_t pr_gid;
- pid_t pr_pid, pr_ppid, pr_pgrp, pr_sid;
+ __kernel_pid_t pr_pid, pr_ppid, pr_pgrp, pr_sid;
/* Lots missing */
char pr_fname[16]; /* filename of executable */
char pr_psargs[ELF_PRARGSZ]; /* initial part of arg list */
};
-#ifndef __KERNEL__
-typedef struct elf_prstatus prstatus_t;
-typedef struct elf_prpsinfo prpsinfo_t;
-#define PRARGSZ ELF_PRARGSZ
-#endif
-
-
#endif /* _UAPI_LINUX_ELFCORE_H */
--
2.18.0
^ permalink raw reply related
* Re: [PATCH 11/11] UAPI: Check headers build for C++ [ver #2]
From: Arnd Bergmann @ 2018-09-14 9:10 UTC (permalink / raw)
To: David Howells
Cc: Linux API, Linux Kbuild mailing list, Masahiro Yamada,
Michal Marek, Linux Kernel Mailing List, Joseph Myers
In-Reply-To: <153622557736.14298.10350496216090814292.stgit@warthog.procyon.org.uk>
On Thu, Sep 6, 2018 at 11:21 AM David Howells <dhowells@redhat.com> wrote:
> +
> +typedef __s8 int8_t;
> +typedef __s16 int16_t;
> +typedef __s32 int32_t;
> +typedef __s64 int64_t;
> +typedef __u8 uint8_t;
> +typedef __u16 uint16_t;
> +typedef __u32 uint32_t;
> +typedef __u64 uint64_t;
> +typedef long int intptr_t;
> +typedef unsigned long int uintptr_t;
> +typedef unsigned short u_short;
> +typedef unsigned int u_int;
> +typedef unsigned long u_long;
> +typedef char *caddr_t;
> +
> +typedef __kernel_clockid_t clockid_t;
> +typedef __kernel_ino_t ino_t;
> +typedef __kernel_pid_t pid_t;
> +typedef __kernel_sa_family_t sa_family_t;
> +typedef __kernel_size_t size_t;
> +typedef __kernel_uid_t uid_t;
> +
> +typedef unsigned long elf_greg_t;
> +typedef elf_greg_t elf_gregset_t[1];
> +typedef unsigned long long elf_fpregset_t[1];
> +typedef unsigned long long elf_fpxregset_t[1];
> +
> +#define INT_MIN ((int)0x80000000)
> +#define INT_MAX ((int)0x7fffffff)
> +
> +extern size_t strlen(const char *);
> +extern void *memset(void *, int, size_t);
> +extern void *memcpy(void *, const void *, size_t);
> +extern __u16 ntohs(__u16);
> +extern __u16 htons(__u16);
> +extern __u32 ntohl(__u32);
> +extern __u32 htonl(__u32);
> +
> +typedef uint32_t grant_ref_t;
> +typedef uint16_t domid_t;
> +typedef unsigned long xen_pfn_t;
> +
> +#define MSG_FIN 0x200
> +
> +typedef int SVGA3dMSPattern;
> +typedef int SVGA3dMSQualityLevel;
> +
> +struct sockaddr
> +{
> + sa_family_t sa_family;
> + char sa_data[14];
> +};
> +#define sockaddr_storage __kernel_sockaddr_storage
I think we need to reduce that list as much as we can. In
https://patchwork.ozlabs.org/patch/968814/, Joseph Myers pointed
out header file (linux/elfcore.h) that simply cannot be included from
user space at all, because its dependencies cannot be met without
running into conflicting type definitions, and he would like to include
that file in order to automatically check that it's compatible with
the glibc version (I pointed out a couple of architectures on which
it is in fact incompatible right now).
In the list above, I see multiple classes of bugs that could be
addressed:
- references to identifiers that are only present in kernel internal
headers: SVGA3dMSPattern, MSG_FIN, xen_pfn_t, ...
I think these are all simple bugs, and we should either remove
the references, or make sure the respective dependencies are
included in the uapi headers as well, possibly renamed with
a __kernel_ prefix to avoid clashing with user space headers.
- references to user space types that should use the uapi
internal types: sockaddr_storage, clockid_t, uid_t, ...
I think these just need to get the __kernel_prefix
consistently as we did a few years ago. Note that using
the headers otherwise is broken anyway when the types
in libc are different from the ones in the kernel.
- standard types (uint32_t): either include the correct user
space headers ifndef __KERNEL__ or use the kernel types
- standard functions (memcpy(), ntohs(), ...): These should
already be handled in the headers by including the user space.
If we missed any, we should probably do the same thing
there.
Arnd
^ permalink raw reply
* Re: [RFC PATCH] Add /proc/<pid>/numa_vamaps for numa node information
From: Michal Hocko @ 2018-09-14 6:21 UTC (permalink / raw)
To: Jann Horn
Cc: Prakash Sangappa, Dave Hansen, Anshuman Khandual, Andrew Morton,
kernel list, Linux-MM, Linux API, Kirill A . Shutemov,
n-horiguchi, drepper, rientjes, nao.horiguchi, steven.sistare
In-Reply-To: <CAG48ez1YhHKTDHZoH2tEFaLk4LcCSw5G60=+KpGRaMQxvw1qLw@mail.gmail.com>
On Fri 14-09-18 03:33:28, Jann Horn wrote:
> On Wed, Sep 12, 2018 at 10:43 PM prakash.sangappa
> <prakash.sangappa@oracle.com> wrote:
> > On 05/09/2018 04:31 PM, Dave Hansen wrote:
> > > On 05/07/2018 06:16 PM, prakash.sangappa wrote:
> > >> It will be /proc/<pid>/numa_vamaps. Yes, the behavior will be
> > >> different with respect to seeking. Output will still be text and
> > >> the format will be same.
> > >>
> > >> I want to get feedback on this approach.
> > > I think it would be really great if you can write down a list of the
> > > things you actually want to accomplish. Dare I say: you need a
> > > requirements list.
> > >
> > > The numa_vamaps approach continues down the path of an ever-growing list
> > > of highly-specialized /proc/<pid> files. I don't think that is
> > > sustainable, even if it has been our trajectory for many years.
> > >
> > > Pagemap wasn't exactly a shining example of us getting new ABIs right,
> > > but it sounds like something along those is what we need.
> >
> > Just sent out a V2 patch. This patch simplifies the file content. It
> > only provides VA range to numa node id information.
> >
> > The requirement is basically observability for performance analysis.
> >
> > - Need to be able to determine VA range to numa node id information.
> > Which also gives an idea of which range has memory allocated.
> >
> > - The proc file /proc/<pid>/numa_vamaps is in text so it is easy to
> > directly view.
> >
> > The V2 patch supports seeking to a particular process VA from where
> > the application could read the VA to numa node id information.
> >
> > Also added the 'PTRACE_MODE_READ_REALCREDS' check when opening the
> > file /proc file as was indicated by Michal Hacko
>
> procfs files should use PTRACE_MODE_*_FSCREDS, not PTRACE_MODE_*_REALCREDS.
Out of my curiosity, what is the semantic difference? At least
kernel_move_pages uses PTRACE_MODE_READ_REALCREDS. Is this a bug?
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RFC PATCH] Add /proc/<pid>/numa_vamaps for numa node information
From: Jann Horn @ 2018-09-14 1:33 UTC (permalink / raw)
To: Prakash Sangappa
Cc: Dave Hansen, Anshuman Khandual, Andrew Morton, kernel list,
Linux-MM, Linux API, Michal Hocko, Kirill A . Shutemov,
n-horiguchi, drepper, rientjes, nao.horiguchi, steven.sistare
In-Reply-To: <94ee0b6c-4663-0705-d4a8-c50342f6b483@oracle.com>
On Wed, Sep 12, 2018 at 10:43 PM prakash.sangappa
<prakash.sangappa@oracle.com> wrote:
> On 05/09/2018 04:31 PM, Dave Hansen wrote:
> > On 05/07/2018 06:16 PM, prakash.sangappa wrote:
> >> It will be /proc/<pid>/numa_vamaps. Yes, the behavior will be
> >> different with respect to seeking. Output will still be text and
> >> the format will be same.
> >>
> >> I want to get feedback on this approach.
> > I think it would be really great if you can write down a list of the
> > things you actually want to accomplish. Dare I say: you need a
> > requirements list.
> >
> > The numa_vamaps approach continues down the path of an ever-growing list
> > of highly-specialized /proc/<pid> files. I don't think that is
> > sustainable, even if it has been our trajectory for many years.
> >
> > Pagemap wasn't exactly a shining example of us getting new ABIs right,
> > but it sounds like something along those is what we need.
>
> Just sent out a V2 patch. This patch simplifies the file content. It
> only provides VA range to numa node id information.
>
> The requirement is basically observability for performance analysis.
>
> - Need to be able to determine VA range to numa node id information.
> Which also gives an idea of which range has memory allocated.
>
> - The proc file /proc/<pid>/numa_vamaps is in text so it is easy to
> directly view.
>
> The V2 patch supports seeking to a particular process VA from where
> the application could read the VA to numa node id information.
>
> Also added the 'PTRACE_MODE_READ_REALCREDS' check when opening the
> file /proc file as was indicated by Michal Hacko
procfs files should use PTRACE_MODE_*_FSCREDS, not PTRACE_MODE_*_REALCREDS.
^ permalink raw reply
* Re: [PATCH] proc: restrict kernel stack dumps to root
From: Kees Cook @ 2018-09-13 14:39 UTC (permalink / raw)
To: Jann Horn
Cc: Alexey Dobriyan, Ken Chen, kernel list,
linux-fsdevel@vger.kernel.org, Will Deacon, Laura Abbott,
Andy Lutomirski, Security Officers, Catalin Marinas,
Josh Poimboeuf, Thomas Gleixner, Ingo Molnar, H . Peter Anvin,
Linux API
In-Reply-To: <CAG48ez22djpKdtYDTMn0MS=2m_QAq983O0-oXyVGiu3ju2HwXg@mail.gmail.com>
On Thu, Sep 13, 2018 at 4:55 AM, Jann Horn <jannh@google.com> wrote:
> On Thu, Sep 13, 2018 at 12:28 AM Kees Cook <keescook@chromium.org> wrote:
>>
>> On Wed, Sep 12, 2018 at 8:29 AM, Jann Horn <jannh@google.com> wrote:
>> > +linux-api, I guess
>> >
>> > On Tue, Sep 11, 2018 at 8:39 PM Jann Horn <jannh@google.com> wrote:
>> >>
>> >> Restrict the ability to inspect kernel stacks of arbitrary tasks to root
>> >> in order to prevent a local attacker from exploiting racy stack unwinding
>> >> to leak kernel task stack contents.
>> >> See the added comment for a longer rationale.
>> >>
>> >> There don't seem to be any users of this userspace API that can't
>> >> gracefully bail out if reading from the file fails. Therefore, I believe
>> >> that this change is unlikely to break things.
>> >> In the case that this patch does end up needing a revert, the next-best
>> >> solution might be to fake a single-entry stack based on wchan.
>> >>
>> >> Fixes: 2ec220e27f50 ("proc: add /proc/*/stack")
>> >> Cc: stable@vger.kernel.org
>> >> Signed-off-by: Jann Horn <jannh@google.com>
>> >> ---
>> >> fs/proc/base.c | 14 ++++++++++++++
>> >> 1 file changed, 14 insertions(+)
>> >>
>> >> diff --git a/fs/proc/base.c b/fs/proc/base.c
>> >> index ccf86f16d9f0..7e9f07bf260d 100644
>> >> --- a/fs/proc/base.c
>> >> +++ b/fs/proc/base.c
>> >> @@ -407,6 +407,20 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
>> >> unsigned long *entries;
>> >> int err;
>> >>
>> >> + /*
>> >> + * The ability to racily run the kernel stack unwinder on a running task
>> >> + * and then observe the unwinder output is scary; while it is useful for
>> >> + * debugging kernel issues, it can also allow an attacker to leak kernel
>> >> + * stack contents.
>> >> + * Doing this in a manner that is at least safe from races would require
>> >> + * some work to ensure that the remote task can not be scheduled; and
>> >> + * even then, this would still expose the unwinder as local attack
>> >> + * surface.
>> >> + * Therefore, this interface is restricted to root.
>> >> + */
>> >> + if (!file_ns_capable(m->file, &init_user_ns, CAP_SYS_ADMIN))
>> >> + return -EACCES;
>>
>> In the past, we've avoided hard errors like this in favor of just
>> censoring the output. Do we want to be more cautious here? (i.e.
>> return 0 or a fuller seq_printf(m, "[<0>] privileged\n"); return 0;)
>
> In my mind, this is different because it's a place where we don't have
> to selectively censor output while preserving parts of it, and it's a
> place where, as Laura said, it's useful to make lack of privileges
> clearly visible because that informs users that they may have to retry
> with more privileges.
>
> Of course, if you have an example of software that actually breaks due
> to this, I'll change it. But I looked at the three things in Debian
> codesearch that seem to use it, and from what I can tell, they all
> bail out cleanly when the read fails.
I prefer -EACCESS too, but I thought I'd mention the alternative. So, I guess:
Reviewed-by: Kees Cook <keescook@chromium.org>
:)
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: [PATCH] proc: restrict kernel stack dumps to root
From: Jann Horn @ 2018-09-13 11:55 UTC (permalink / raw)
To: Kees Cook
Cc: Alexey Dobriyan, Ken Chen, kernel list, linux-fsdevel,
Will Deacon, Laura Abbott, Andy Lutomirski, security,
Catalin Marinas, Josh Poimboeuf, Thomas Gleixner, Ingo Molnar,
H . Peter Anvin, Linux API
In-Reply-To: <CAGXu5jKBPh+TSCuaitpPdk89hW=ch+QeRsUfE68pLpzLS=z4Jg@mail.gmail.com>
On Thu, Sep 13, 2018 at 12:28 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Sep 12, 2018 at 8:29 AM, Jann Horn <jannh@google.com> wrote:
> > +linux-api, I guess
> >
> > On Tue, Sep 11, 2018 at 8:39 PM Jann Horn <jannh@google.com> wrote:
> >>
> >> Restrict the ability to inspect kernel stacks of arbitrary tasks to root
> >> in order to prevent a local attacker from exploiting racy stack unwinding
> >> to leak kernel task stack contents.
> >> See the added comment for a longer rationale.
> >>
> >> There don't seem to be any users of this userspace API that can't
> >> gracefully bail out if reading from the file fails. Therefore, I believe
> >> that this change is unlikely to break things.
> >> In the case that this patch does end up needing a revert, the next-best
> >> solution might be to fake a single-entry stack based on wchan.
> >>
> >> Fixes: 2ec220e27f50 ("proc: add /proc/*/stack")
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Jann Horn <jannh@google.com>
> >> ---
> >> fs/proc/base.c | 14 ++++++++++++++
> >> 1 file changed, 14 insertions(+)
> >>
> >> diff --git a/fs/proc/base.c b/fs/proc/base.c
> >> index ccf86f16d9f0..7e9f07bf260d 100644
> >> --- a/fs/proc/base.c
> >> +++ b/fs/proc/base.c
> >> @@ -407,6 +407,20 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
> >> unsigned long *entries;
> >> int err;
> >>
> >> + /*
> >> + * The ability to racily run the kernel stack unwinder on a running task
> >> + * and then observe the unwinder output is scary; while it is useful for
> >> + * debugging kernel issues, it can also allow an attacker to leak kernel
> >> + * stack contents.
> >> + * Doing this in a manner that is at least safe from races would require
> >> + * some work to ensure that the remote task can not be scheduled; and
> >> + * even then, this would still expose the unwinder as local attack
> >> + * surface.
> >> + * Therefore, this interface is restricted to root.
> >> + */
> >> + if (!file_ns_capable(m->file, &init_user_ns, CAP_SYS_ADMIN))
> >> + return -EACCES;
>
> In the past, we've avoided hard errors like this in favor of just
> censoring the output. Do we want to be more cautious here? (i.e.
> return 0 or a fuller seq_printf(m, "[<0>] privileged\n"); return 0;)
In my mind, this is different because it's a place where we don't have
to selectively censor output while preserving parts of it, and it's a
place where, as Laura said, it's useful to make lack of privileges
clearly visible because that informs users that they may have to retry
with more privileges.
Of course, if you have an example of software that actually breaks due
to this, I'll change it. But I looked at the three things in Debian
codesearch that seem to use it, and from what I can tell, they all
bail out cleanly when the read fails.
^ permalink raw reply
* Re: [PATCH v6 4/5] seccomp: add support for passing fds via USER_NOTIF
From: Aleksa Sarai @ 2018-09-13 9:42 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Tycho Andersen, Kees Cook, Jann Horn, Linux API, Linux Containers,
Akihiro Suda, Oleg Nesterov, LKML, Eric W . Biederman,
Christian Brauner
In-Reply-To: <CALCETrWZmN4FeCSwemfMeayupBmQ-NqpVWQuqSU34CLvzdx8gw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1712 bytes --]
On 2018-09-12, Andy Lutomirski <luto@amacapital.net> wrote:
> > The idea here is that the userspace handler should be able to pass an fd
> > back to the trapped task, for example so it can be returned from socket().
> >
> > I've proposed one API here, but I'm open to other options. In particular,
> > this only lets you return an fd from a syscall, which may not be enough in
> > all cases. For example, if an fd is written to an output parameter instead
> > of returned, the current API can't handle this. Another case is that
> > netlink takes as input fds sometimes (IFLA_NET_NS_FD, e.g.). If netlink
> > ever decides to install an fd and output it, we wouldn't be able to handle
> > this either.
>
> An alternative could be to have an API (an ioctl on the listener,
> perhaps) that just copies an fd into the tracee. There would be the
> obvious set of options: do we replace an existing fd or allocate a new
> one, and is it CLOEXEC. Then the tracer could add an fd and then
> return it just like it's a regular number.
>
> I feel like this would be more flexible and conceptually simpler, but
> maybe a little slower for the common cases. What do you think?
When we first discussed this I sent a (probably somewhat broken) patch
for "dup4" which would let you inject a file descriptor into a different
process -- I still think that having a method for injecting a file
descriptor without needing ptrace (and SCM_RIGHTS) shenanigans would be
generally useful. (With "dup4" you have a more obvious API for flags and
whether you allocate a new fd or use a specific one.)
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v6 4/5] seccomp: add support for passing fds via USER_NOTIF
From: Tycho Andersen @ 2018-09-13 9:25 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Kees Cook, LKML, Linux Containers, Linux API, Oleg Nesterov,
Eric W . Biederman, Serge E . Hallyn, Christian Brauner,
Tyler Hicks, Akihiro Suda, Jann Horn
In-Reply-To: <CALCETrWZmN4FeCSwemfMeayupBmQ-NqpVWQuqSU34CLvzdx8gw@mail.gmail.com>
On Wed, Sep 12, 2018 at 04:52:38PM -0700, Andy Lutomirski wrote:
> On Thu, Sep 6, 2018 at 8:28 AM, Tycho Andersen <tycho@tycho.ws> wrote:
> > The idea here is that the userspace handler should be able to pass an fd
> > back to the trapped task, for example so it can be returned from socket().
> >
> > I've proposed one API here, but I'm open to other options. In particular,
> > this only lets you return an fd from a syscall, which may not be enough in
> > all cases. For example, if an fd is written to an output parameter instead
> > of returned, the current API can't handle this. Another case is that
> > netlink takes as input fds sometimes (IFLA_NET_NS_FD, e.g.). If netlink
> > ever decides to install an fd and output it, we wouldn't be able to handle
> > this either.
>
> An alternative could be to have an API (an ioctl on the listener,
> perhaps) that just copies an fd into the tracee. There would be the
> obvious set of options: do we replace an existing fd or allocate a new
> one, and is it CLOEXEC. Then the tracer could add an fd and then
> return it just like it's a regular number.
>
> I feel like this would be more flexible and conceptually simpler, but
> maybe a little slower for the common cases. What do you think?
Yes, I like this. It also future (or current-) proofs the API against
instances where we return an FD in a structure and not via the return
code of the syscall.
Tycho
^ permalink raw reply
* Re: [PATCH v6 3/5] seccomp: add a way to get a listener fd from ptrace
From: Tycho Andersen @ 2018-09-13 9:24 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Kees Cook, LKML, Linux Containers, Linux API, Oleg Nesterov,
Eric W . Biederman, Serge E . Hallyn, Christian Brauner,
Tyler Hicks, Akihiro Suda, Jann Horn
In-Reply-To: <CALCETrXGBZyqDb7DEysHmOkxaBrXUp4s2X08HfqZkpjM28s3Hg@mail.gmail.com>
On Wed, Sep 12, 2018 at 05:00:54PM -0700, Andy Lutomirski wrote:
> On Thu, Sep 6, 2018 at 8:28 AM, Tycho Andersen <tycho@tycho.ws> wrote:
> > As an alternative to SECCOMP_FILTER_FLAG_GET_LISTENER, perhaps a ptrace()
> > version which can acquire filters is useful. There are at least two reasons
> > this is preferable, even though it uses ptrace:
> >
> > 1. You can control tasks that aren't cooperating with you
> > 2. You can control tasks whose filters block sendmsg() and socket(); if the
> > task installs a filter which blocks these calls, there's no way with
> > SECCOMP_FILTER_FLAG_GET_LISTENER to get the fd out to the privileged task.
>
> Hmm. I contemplated this a bit and looked at your example a bit, and
> I have a few thoughts:
>
> - What happens if you nest code like your sample? That is, if you
> are already in some container that is seccomped and there's a
> listener, can you even run your sample?
You can attach a filter with SECCOMP_RET_USER_NOTIF, but you can't
attach a listener to any such filter as long as there is another
listener somewhere in the chain (I disallowed this based on some
feedback you sent earlier, it's an artificial restriction).
> - Is there any association between the filter layer that uses the
> USER_NOTIF return and the listener? How would this API express such a
> relationship?
Not sure exactly what you're asking here. There is the struct file*,
but there could be many threads listening to it.
> I realize that my dream of how this should all work requires eBPF and
> BPF_CALL, so it may not be viable right now, but I'd like a better
> understanding of how this all fits together.
>
> Also, I think that it's not strictly true that a filter that blocks
> sendmsg() is problematic. You could clone a thread, call seccomp() in
> that thread, then get a listener, then execve(). Or we could have a
> seccomp() mode that adds a filter but only kicks in after execve().
> The latter could be generally useful.
Yes, in fact some of the test code works this way. However, the case I
was thinking of is the way a typical container manager works: it does
some initial setup, clone()s the init task, does some final setup,
load the seccomp profile, and exec() the container's init binary.
There's no way for this container to get its seccomp fd back out of
the container to the host if sendmsg() is blocked.
Tycho
^ permalink raw reply
* Re: [PATCH v6 3/5] seccomp: add a way to get a listener fd from ptrace
From: Andy Lutomirski @ 2018-09-13 0:00 UTC (permalink / raw)
To: Tycho Andersen
Cc: Kees Cook, LKML, Linux Containers, Linux API, Oleg Nesterov,
Eric W . Biederman, Serge E . Hallyn, Christian Brauner,
Tyler Hicks, Akihiro Suda, Jann Horn
In-Reply-To: <20180906152859.7810-4-tycho@tycho.ws>
On Thu, Sep 6, 2018 at 8:28 AM, Tycho Andersen <tycho@tycho.ws> wrote:
> As an alternative to SECCOMP_FILTER_FLAG_GET_LISTENER, perhaps a ptrace()
> version which can acquire filters is useful. There are at least two reasons
> this is preferable, even though it uses ptrace:
>
> 1. You can control tasks that aren't cooperating with you
> 2. You can control tasks whose filters block sendmsg() and socket(); if the
> task installs a filter which blocks these calls, there's no way with
> SECCOMP_FILTER_FLAG_GET_LISTENER to get the fd out to the privileged task.
Hmm. I contemplated this a bit and looked at your example a bit, and
I have a few thoughts:
- What happens if you nest code like your sample? That is, if you
are already in some container that is seccomped and there's a
listener, can you even run your sample?
- Is there any association between the filter layer that uses the
USER_NOTIF return and the listener? How would this API express such a
relationship?
I realize that my dream of how this should all work requires eBPF and
BPF_CALL, so it may not be viable right now, but I'd like a better
understanding of how this all fits together.
Also, I think that it's not strictly true that a filter that blocks
sendmsg() is problematic. You could clone a thread, call seccomp() in
that thread, then get a listener, then execve(). Or we could have a
seccomp() mode that adds a filter but only kicks in after execve().
The latter could be generally useful.
--Andy
^ permalink raw reply
* Re: [PATCH v6 4/5] seccomp: add support for passing fds via USER_NOTIF
From: Andy Lutomirski @ 2018-09-12 23:52 UTC (permalink / raw)
To: Tycho Andersen
Cc: Kees Cook, LKML, Linux Containers, Linux API, Oleg Nesterov,
Eric W . Biederman, Serge E . Hallyn, Christian Brauner,
Tyler Hicks, Akihiro Suda, Jann Horn
In-Reply-To: <20180906152859.7810-5-tycho@tycho.ws>
On Thu, Sep 6, 2018 at 8:28 AM, Tycho Andersen <tycho@tycho.ws> wrote:
> The idea here is that the userspace handler should be able to pass an fd
> back to the trapped task, for example so it can be returned from socket().
>
> I've proposed one API here, but I'm open to other options. In particular,
> this only lets you return an fd from a syscall, which may not be enough in
> all cases. For example, if an fd is written to an output parameter instead
> of returned, the current API can't handle this. Another case is that
> netlink takes as input fds sometimes (IFLA_NET_NS_FD, e.g.). If netlink
> ever decides to install an fd and output it, we wouldn't be able to handle
> this either.
An alternative could be to have an API (an ioctl on the listener,
perhaps) that just copies an fd into the tracee. There would be the
obvious set of options: do we replace an existing fd or allocate a new
one, and is it CLOEXEC. Then the tracer could add an fd and then
return it just like it's a regular number.
I feel like this would be more flexible and conceptually simpler, but
maybe a little slower for the common cases. What do you think?
^ permalink raw reply
* Re: [PATCH] proc: restrict kernel stack dumps to root
From: Laura Abbott @ 2018-09-12 22:47 UTC (permalink / raw)
To: Kees Cook, Jann Horn
Cc: Alexey Dobriyan, Ken Chen, kernel list,
linux-fsdevel@vger.kernel.org, Will Deacon, Andy Lutomirski,
Security Officers, Catalin Marinas, Josh Poimboeuf,
Thomas Gleixner, Ingo Molnar, H . Peter Anvin, Linux API
In-Reply-To: <CAGXu5jKBPh+TSCuaitpPdk89hW=ch+QeRsUfE68pLpzLS=z4Jg@mail.gmail.com>
On 09/12/2018 03:27 PM, Kees Cook wrote:
> On Wed, Sep 12, 2018 at 8:29 AM, Jann Horn <jannh@google.com> wrote:
>> +linux-api, I guess
>>
>> On Tue, Sep 11, 2018 at 8:39 PM Jann Horn <jannh@google.com> wrote:
>>>
>>> Restrict the ability to inspect kernel stacks of arbitrary tasks to root
>>> in order to prevent a local attacker from exploiting racy stack unwinding
>>> to leak kernel task stack contents.
>>> See the added comment for a longer rationale.
>>>
>>> There don't seem to be any users of this userspace API that can't
>>> gracefully bail out if reading from the file fails. Therefore, I believe
>>> that this change is unlikely to break things.
>>> In the case that this patch does end up needing a revert, the next-best
>>> solution might be to fake a single-entry stack based on wchan.
>>>
>>> Fixes: 2ec220e27f50 ("proc: add /proc/*/stack")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Jann Horn <jannh@google.com>
>>> ---
>>> fs/proc/base.c | 14 ++++++++++++++
>>> 1 file changed, 14 insertions(+)
>>>
>>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>>> index ccf86f16d9f0..7e9f07bf260d 100644
>>> --- a/fs/proc/base.c
>>> +++ b/fs/proc/base.c
>>> @@ -407,6 +407,20 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
>>> unsigned long *entries;
>>> int err;
>>>
>>> + /*
>>> + * The ability to racily run the kernel stack unwinder on a running task
>>> + * and then observe the unwinder output is scary; while it is useful for
>>> + * debugging kernel issues, it can also allow an attacker to leak kernel
>>> + * stack contents.
>>> + * Doing this in a manner that is at least safe from races would require
>>> + * some work to ensure that the remote task can not be scheduled; and
>>> + * even then, this would still expose the unwinder as local attack
>>> + * surface.
>>> + * Therefore, this interface is restricted to root.
>>> + */
>>> + if (!file_ns_capable(m->file, &init_user_ns, CAP_SYS_ADMIN))
>>> + return -EACCES;
>
> In the past, we've avoided hard errors like this in favor of just
> censoring the output. Do we want to be more cautious here? (i.e.
> return 0 or a fuller seq_printf(m, "[<0>] privileged\n"); return 0;)
>
The -EACCES is a strong hint to run with root privileges which is
nice from an end user perspective. If we don't want to return an
actual error, I think the "privileged" message would be okay.
Laura
>>> +
>>> entries = kmalloc_array(MAX_STACK_TRACE_DEPTH, sizeof(*entries),
>>> GFP_KERNEL);
>>> if (!entries)
>>> --
>>> 2.19.0.rc2.392.g5ba43deb5a-goog
>>>
>
> -Kees
>
^ permalink raw reply
* Re: [PATCH] proc: restrict kernel stack dumps to root
From: Kees Cook @ 2018-09-12 22:27 UTC (permalink / raw)
To: Jann Horn
Cc: Alexey Dobriyan, Ken Chen, kernel list,
linux-fsdevel@vger.kernel.org, Will Deacon, Laura Abbott,
Andy Lutomirski, Security Officers, Catalin Marinas,
Josh Poimboeuf, Thomas Gleixner, Ingo Molnar, H . Peter Anvin,
Linux API
In-Reply-To: <CAG48ez03ivLr6w+V911V7pqb1o=kcJw4CLEktcSVVE+=hXXP=w@mail.gmail.com>
On Wed, Sep 12, 2018 at 8:29 AM, Jann Horn <jannh@google.com> wrote:
> +linux-api, I guess
>
> On Tue, Sep 11, 2018 at 8:39 PM Jann Horn <jannh@google.com> wrote:
>>
>> Restrict the ability to inspect kernel stacks of arbitrary tasks to root
>> in order to prevent a local attacker from exploiting racy stack unwinding
>> to leak kernel task stack contents.
>> See the added comment for a longer rationale.
>>
>> There don't seem to be any users of this userspace API that can't
>> gracefully bail out if reading from the file fails. Therefore, I believe
>> that this change is unlikely to break things.
>> In the case that this patch does end up needing a revert, the next-best
>> solution might be to fake a single-entry stack based on wchan.
>>
>> Fixes: 2ec220e27f50 ("proc: add /proc/*/stack")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Jann Horn <jannh@google.com>
>> ---
>> fs/proc/base.c | 14 ++++++++++++++
>> 1 file changed, 14 insertions(+)
>>
>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>> index ccf86f16d9f0..7e9f07bf260d 100644
>> --- a/fs/proc/base.c
>> +++ b/fs/proc/base.c
>> @@ -407,6 +407,20 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
>> unsigned long *entries;
>> int err;
>>
>> + /*
>> + * The ability to racily run the kernel stack unwinder on a running task
>> + * and then observe the unwinder output is scary; while it is useful for
>> + * debugging kernel issues, it can also allow an attacker to leak kernel
>> + * stack contents.
>> + * Doing this in a manner that is at least safe from races would require
>> + * some work to ensure that the remote task can not be scheduled; and
>> + * even then, this would still expose the unwinder as local attack
>> + * surface.
>> + * Therefore, this interface is restricted to root.
>> + */
>> + if (!file_ns_capable(m->file, &init_user_ns, CAP_SYS_ADMIN))
>> + return -EACCES;
In the past, we've avoided hard errors like this in favor of just
censoring the output. Do we want to be more cautious here? (i.e.
return 0 or a fuller seq_printf(m, "[<0>] privileged\n"); return 0;)
>> +
>> entries = kmalloc_array(MAX_STACK_TRACE_DEPTH, sizeof(*entries),
>> GFP_KERNEL);
>> if (!entries)
>> --
>> 2.19.0.rc2.392.g5ba43deb5a-goog
>>
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: [RFC PATCH] Add /proc/<pid>/numa_vamaps for numa node information
From: prakash.sangappa @ 2018-09-12 20:57 UTC (permalink / raw)
To: Dave Hansen, Anshuman Khandual, Andrew Morton
Cc: linux-kernel, linux-mm, linux-api, mhocko, kirill.shutemov,
n-horiguchi, drepper, rientjes, Naoya Horiguchi, Steven Sistare
In-Reply-To: <94ee0b6c-4663-0705-d4a8-c50342f6b483@oracle.com>
On 09/12/2018 01:42 PM, prakash.sangappa wrote:
>
>
> On 05/09/2018 04:31 PM, Dave Hansen wrote:
>> On 05/07/2018 06:16 PM, prakash.sangappa wrote:
>>> It will be /proc/<pid>/numa_vamaps. Yes, the behavior will be
>>> different with respect to seeking. Output will still be text and
>>> the format will be same.
>>>
>>> I want to get feedback on this approach.
>> I think it would be really great if you can write down a list of the
>> things you actually want to accomplish. Dare I say: you need a
>> requirements list.
>>
>> The numa_vamaps approach continues down the path of an ever-growing list
>> of highly-specialized /proc/<pid> files. I don't think that is
>> sustainable, even if it has been our trajectory for many years.
>>
>> Pagemap wasn't exactly a shining example of us getting new ABIs right,
>> but it sounds like something along those is what we need.
>
> Just sent out a V2 patch. This patch simplifies the file content. It
> only provides VA range to numa node id information.
>
> The requirement is basically observability for performance analysis.
>
> - Need to be able to determine VA range to numa node id information.
> Which also gives an idea of which range has memory allocated.
>
> - The proc file /proc/<pid>/numa_vamaps is in text so it is easy to
> directly view.
>
> The V2 patch supports seeking to a particular process VA from where
> the application could read the VA to numa node id information.
>
> Also added the 'PTRACE_MODE_READ_REALCREDS' check when opening the
> file /proc file as was indicated by Michal Hacko
>
I meant Michal Hocko
Sorry misspelled the name.
> The VA range to numa node information from this file can be used by pmap.
>
> Here is a sample from a prototype change to pmap(procps) showing
> numa node information, gathered from the new 'numa_vamaps' file.
>
> $ ./rpmap -L -A 00000000006f8000,00007f5f730fe000 31423|more
> 31423: bash
> 00000000006f8000 16K N1 rw--- bash
> 00000000006fc000 4K N0 rw--- bash
> 00000000006fd000 4K N0 rw--- [ anon ]
> 00000000006fe000 8K N1 rw--- [ anon ]
> 0000000000700000 4K N0 rw--- [ anon ]
> 0000000000701000 4K N1 rw--- [ anon ]
> 0000000000702000 4K N0 rw--- [ anon ]
> 0000000000ce8000 52K N0 rw--- [ anon ]
> 0000000000cf5000 4K N1 rw--- [ anon ]
> 0000000000cf6000 28K N0 rw--- [ anon ]
> 0000000000cfd000 4K N1 rw--- [ anon ]
> 0000000000cfe000 28K N0 rw--- [ anon ]
> 0000000000d05000 504K N1 rw--- [ anon ]
> 0000000000d83000 8K N0 rw--- [ anon ]
> 0000000000d85000 932K N1 rw--- [ anon ]
> 0000000000e6e000 4K - rw--- [ anon ]
> 0000000000e6f000 168K N1 rw--- [ anon ]
> 00007f5f72ef4000 4K N2 r-x-- libnss_files-2.23.so
> 00007f5f72ef5000 40K N0 r-x-- libnss_files-2.23.so
> 00007f5f72eff000 2044K - ----- libnss_files-2.23.so
> 00007f5f730fe000 4K N0 r---- libnss_files-2.23.so
> total 3868K
>
> -Prakash.
^ permalink raw reply
* Re: [RFC PATCH] Add /proc/<pid>/numa_vamaps for numa node information
From: prakash.sangappa @ 2018-09-12 20:42 UTC (permalink / raw)
To: Dave Hansen, Anshuman Khandual, Andrew Morton
Cc: linux-kernel, linux-mm, linux-api, mhocko, kirill.shutemov,
n-horiguchi, drepper, rientjes, Naoya Horiguchi, Steven Sistare
In-Reply-To: <aaca3180-7510-c008-3e12-8bbe92344ef4@intel.com>
On 05/09/2018 04:31 PM, Dave Hansen wrote:
> On 05/07/2018 06:16 PM, prakash.sangappa wrote:
>> It will be /proc/<pid>/numa_vamaps. Yes, the behavior will be
>> different with respect to seeking. Output will still be text and
>> the format will be same.
>>
>> I want to get feedback on this approach.
> I think it would be really great if you can write down a list of the
> things you actually want to accomplish. Dare I say: you need a
> requirements list.
>
> The numa_vamaps approach continues down the path of an ever-growing list
> of highly-specialized /proc/<pid> files. I don't think that is
> sustainable, even if it has been our trajectory for many years.
>
> Pagemap wasn't exactly a shining example of us getting new ABIs right,
> but it sounds like something along those is what we need.
Just sent out a V2 patch. This patch simplifies the file content. It
only provides VA range to numa node id information.
The requirement is basically observability for performance analysis.
- Need to be able to determine VA range to numa node id information.
Which also gives an idea of which range has memory allocated.
- The proc file /proc/<pid>/numa_vamaps is in text so it is easy to
directly view.
The V2 patch supports seeking to a particular process VA from where
the application could read the VA to numa node id information.
Also added the 'PTRACE_MODE_READ_REALCREDS' check when opening the
file /proc file as was indicated by Michal Hacko
The VA range to numa node information from this file can be used by pmap.
Here is a sample from a prototype change to pmap(procps) showing
numa node information, gathered from the new 'numa_vamaps' file.
$ ./rpmap -L -A 00000000006f8000,00007f5f730fe000 31423|more
31423: bash
00000000006f8000 16K N1 rw--- bash
00000000006fc000 4K N0 rw--- bash
00000000006fd000 4K N0 rw--- [ anon ]
00000000006fe000 8K N1 rw--- [ anon ]
0000000000700000 4K N0 rw--- [ anon ]
0000000000701000 4K N1 rw--- [ anon ]
0000000000702000 4K N0 rw--- [ anon ]
0000000000ce8000 52K N0 rw--- [ anon ]
0000000000cf5000 4K N1 rw--- [ anon ]
0000000000cf6000 28K N0 rw--- [ anon ]
0000000000cfd000 4K N1 rw--- [ anon ]
0000000000cfe000 28K N0 rw--- [ anon ]
0000000000d05000 504K N1 rw--- [ anon ]
0000000000d83000 8K N0 rw--- [ anon ]
0000000000d85000 932K N1 rw--- [ anon ]
0000000000e6e000 4K - rw--- [ anon ]
0000000000e6f000 168K N1 rw--- [ anon ]
00007f5f72ef4000 4K N2 r-x-- libnss_files-2.23.so
00007f5f72ef5000 40K N0 r-x-- libnss_files-2.23.so
00007f5f72eff000 2044K - ----- libnss_files-2.23.so
00007f5f730fe000 4K N0 r---- libnss_files-2.23.so
total 3868K
-Prakash.
^ permalink raw reply
* Re: [PATCH] proc: restrict kernel stack dumps to root
From: Jann Horn @ 2018-09-12 15:29 UTC (permalink / raw)
To: Alexey Dobriyan
Cc: Ken Chen, kernel list, linux-fsdevel, Will Deacon, Laura Abbott,
Andy Lutomirski, security, Catalin Marinas, Josh Poimboeuf,
Thomas Gleixner, Ingo Molnar, H . Peter Anvin, Linux API
In-Reply-To: <20180911183909.233413-1-jannh@google.com>
+linux-api, I guess
On Tue, Sep 11, 2018 at 8:39 PM Jann Horn <jannh@google.com> wrote:
>
> Restrict the ability to inspect kernel stacks of arbitrary tasks to root
> in order to prevent a local attacker from exploiting racy stack unwinding
> to leak kernel task stack contents.
> See the added comment for a longer rationale.
>
> There don't seem to be any users of this userspace API that can't
> gracefully bail out if reading from the file fails. Therefore, I believe
> that this change is unlikely to break things.
> In the case that this patch does end up needing a revert, the next-best
> solution might be to fake a single-entry stack based on wchan.
>
> Fixes: 2ec220e27f50 ("proc: add /proc/*/stack")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
> fs/proc/base.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index ccf86f16d9f0..7e9f07bf260d 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -407,6 +407,20 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
> unsigned long *entries;
> int err;
>
> + /*
> + * The ability to racily run the kernel stack unwinder on a running task
> + * and then observe the unwinder output is scary; while it is useful for
> + * debugging kernel issues, it can also allow an attacker to leak kernel
> + * stack contents.
> + * Doing this in a manner that is at least safe from races would require
> + * some work to ensure that the remote task can not be scheduled; and
> + * even then, this would still expose the unwinder as local attack
> + * surface.
> + * Therefore, this interface is restricted to root.
> + */
> + if (!file_ns_capable(m->file, &init_user_ns, CAP_SYS_ADMIN))
> + return -EACCES;
> +
> entries = kmalloc_array(MAX_STACK_TRACE_DEPTH, sizeof(*entries),
> GFP_KERNEL);
> if (!entries)
> --
> 2.19.0.rc2.392.g5ba43deb5a-goog
>
^ permalink raw reply
* Re: [v7] Add udmabuf misc device
From: Yann Droneaud @ 2018-09-11 20:47 UTC (permalink / raw)
To: Gerd Hoffmann, dri-devel
Cc: open list:KERNEL SELFTEST FRAMEWORK, Tomeu Vizoso,
Jonathan Corbet, David Airlie, open list:DOCUMENTATION, open list,
moderated list:DMA BUFFER SHARING FRAMEWORK,
open list:DMA BUFFER SHARING FRAMEWORK, Shuah Khan,
Laurent Pinchart, linux-api
In-Reply-To: <20180827093444.23623-1-kraxel@redhat.com>
Hi,
Le lundi 27 août 2018 à 11:34 +0200, Gerd Hoffmann a écrit :
> A driver to let userspace turn memfd regions into dma-bufs.
>
> Use case: Allows qemu create dmabufs for the vga framebuffer or
> virtio-gpu ressources. Then they can be passed around to display
> those guest things on the host. To spice client for classic full
> framebuffer display, and hopefully some day to wayland server for
> seamless guest window display.
>
> qemu test branch:
> https://git.kraxel.org/cgit/qemu/log/?h=sirius/udmabuf
>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> Documentation/ioctl/ioctl-number.txt | 1 +
> include/uapi/linux/udmabuf.h | 33 +++
> drivers/dma-buf/udmabuf.c | 287
> ++++++++++++++++++++++
> tools/testing/selftests/drivers/dma-buf/udmabuf.c | 96 ++++++++
> MAINTAINERS | 16 ++
> drivers/dma-buf/Kconfig | 8 +
> drivers/dma-buf/Makefile | 1 +
> tools/testing/selftests/drivers/dma-buf/Makefile | 5 +
> 8 files changed, 447 insertions(+)
> create mode 100644 include/uapi/linux/udmabuf.h
> create mode 100644 drivers/dma-buf/udmabuf.c
> create mode 100644 tools/testing/selftests/drivers/dma-buf/udmabuf.c
> create mode 100644 tools/testing/selftests/drivers/dma-buf/Makefile
>
> diff --git a/include/uapi/linux/udmabuf.h
> b/include/uapi/linux/udmabuf.h
> new file mode 100644
> index 0000000000..46b6532ed8
> --- /dev/null
> +++ b/include/uapi/linux/udmabuf.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef _UAPI_LINUX_UDMABUF_H
> +#define _UAPI_LINUX_UDMABUF_H
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +
> +#define UDMABUF_FLAGS_CLOEXEC 0x01
> +
> +struct udmabuf_create {
> + __u32 memfd;
> + __u32 flags;
> + __u64 offset;
> + __u64 size;
> +};
> +
> +struct udmabuf_create_item {
> + __u32 memfd;
> + __u32 __pad;
> + __u64 offset;
> + __u64 size;
> +};
> +
> +struct udmabuf_create_list {
> + __u32 flags;
> + __u32 count;
> + struct udmabuf_create_item list[];
> +};
> +
> +#define UDMABUF_CREATE _IOW('u', 0x42, struct udmabuf_create)
> +#define UDMABUF_CREATE_LIST _IOW('u', 0x43, struct
> udmabuf_create_list)
> +
> +#endif /* _UAPI_LINUX_UDMABUF_H */
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> new file mode 100644
> index 0000000000..8e24204526
> --- /dev/null
> +++ b/drivers/dma-buf/udmabuf.c
> +static long udmabuf_create(struct udmabuf_create_list *head,
> + struct udmabuf_create_item *list)
> +{
> + DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> + struct file *memfd = NULL;
> + struct udmabuf *ubuf;
> + struct dma_buf *buf;
> + pgoff_t pgoff, pgcnt, pgidx, pgbuf;
> + struct page *page;
> + int seals, ret = -EINVAL;
> + u32 i, flags;
> +
> + ubuf = kzalloc(sizeof(struct udmabuf), GFP_KERNEL);
> + if (!ubuf)
> + return -ENOMEM;
> +
> + for (i = 0; i < head->count; i++) {
You need to check .__pad for unsupported value:
if (list[i].__pad) {
ret = -EINVAL;
goto err_free_ubuf;
}
> + if (!IS_ALIGNED(list[i].offset, PAGE_SIZE))
> + goto err_free_ubuf;
> + if (!IS_ALIGNED(list[i].size, PAGE_SIZE))
> + goto err_free_ubuf;
> + ubuf->pagecount += list[i].size >> PAGE_SHIFT;
> + }
> + ubuf->pages = kmalloc_array(ubuf->pagecount, sizeof(struct page
> *),
> + GFP_KERNEL);
> + if (!ubuf->pages) {
> + ret = -ENOMEM;
> + goto err_free_ubuf;
> + }
> +
> + pgbuf = 0;
> + for (i = 0; i < head->count; i++) {
> + memfd = fget(list[i].memfd);
> + if (!memfd)
> + goto err_put_pages;
> + if (!shmem_mapping(file_inode(memfd)->i_mapping))
> + goto err_put_pages;
> + seals = memfd_fcntl(memfd, F_GET_SEALS, 0);
> + if (seals == -EINVAL ||
> + (seals & SEALS_WANTED) != SEALS_WANTED ||
> + (seals & SEALS_DENIED) != 0)
> + goto err_put_pages;
> + pgoff = list[i].offset >> PAGE_SHIFT;
> + pgcnt = list[i].size >> PAGE_SHIFT;
> + for (pgidx = 0; pgidx < pgcnt; pgidx++) {
> + page = shmem_read_mapping_page(
> + file_inode(memfd)->i_mapping, pgoff +
> pgidx);
> + if (IS_ERR(page)) {
> + ret = PTR_ERR(page);
> + goto err_put_pages;
> + }
> + ubuf->pages[pgbuf++] = page;
> + }
> + fput(memfd);
> + }
> + memfd = NULL;
> +
> + exp_info.ops = &udmabuf_ops;
> + exp_info.size = ubuf->pagecount << PAGE_SHIFT;
> + exp_info.priv = ubuf;
> +
> + buf = dma_buf_export(&exp_info);
> + if (IS_ERR(buf)) {
> + ret = PTR_ERR(buf);
> + goto err_put_pages;
> + }
> +
> + flags = 0;
You need to check .flags for unsupported value:
if (head->flags & ~UDMABUF_FLAGS_CLOEXEC)
return -EINVAL;
(at the beginning of the function, of course).
> + if (head->flags & UDMABUF_FLAGS_CLOEXEC)
> + flags |= O_CLOEXEC;
> + return dma_buf_fd(buf, flags);
> +
> +err_put_pages:
> + while (pgbuf > 0)
> + put_page(ubuf->pages[--pgbuf]);
> +err_free_ubuf:
> + fput(memfd);
> + kfree(ubuf->pages);
> + kfree(ubuf);
> + return ret;
> +}
> +
Regards
--
Yann Droneaud
OPTEYA
^ permalink raw reply
* Re: [PATCH v6 4/5] seccomp: add support for passing fds via USER_NOTIF
From: Tycho Andersen @ 2018-09-11 20:29 UTC (permalink / raw)
To: Jann Horn
Cc: Kees Cook, kernel list, containers, Linux API, Andy Lutomirski,
Oleg Nesterov, Eric W. Biederman, Serge E. Hallyn,
Christian Brauner, Tyler Hicks, suda.akihiro
In-Reply-To: <CAG48ez0Dx+35fGcrBzzNRSOsmFhuiQBN+-S5+b3_9m9eWbLeQQ@mail.gmail.com>
On Mon, Sep 10, 2018 at 07:00:43PM +0200, Jann Horn wrote:
> On Thu, Sep 6, 2018 at 8:30 PM Tycho Andersen <tycho@tycho.ws> wrote:
> > On Thu, Sep 06, 2018 at 10:22:46AM -0600, Tycho Andersen wrote:
> > > On Thu, Sep 06, 2018 at 06:15:18PM +0200, Jann Horn wrote:
> > > > On Thu, Sep 6, 2018 at 5:29 PM Tycho Andersen <tycho@tycho.ws> wrote:
> > > > > The idea here is that the userspace handler should be able to pass an fd
> > > > > back to the trapped task, for example so it can be returned from socket().
> > > > [...]
> > > > > diff --git a/Documentation/userspace-api/seccomp_filter.rst b/Documentation/userspace-api/seccomp_filter.rst
> > > > > index d1498885c1c7..1c0aab306426 100644
> > > > > --- a/Documentation/userspace-api/seccomp_filter.rst
> > > > > +++ b/Documentation/userspace-api/seccomp_filter.rst
> > > > > @@ -235,6 +235,9 @@ The interface for a seccomp notification fd consists of two structures:
> > > > > __u64 id;
> > > > > __s32 error;
> > > > > __s64 val;
> > > > > + __u8 return_fd;
> > > > > + __u32 fd;
> > > > > + __u32 fd_flags;
> > > >
> > > > Normally, syscalls that take an optional file descriptor accept a
> > > > signed 32-bit number, with -1 standing for "no file descriptor". Is
> > > > there a reason why this uses a separate variable to signal whether an
> > > > fd was provided?
> > >
> > > No real reason other than I looked at the bpf code and they were using
> > > __u32 for bpf (but I think in their case the fd args are not
> > > optional). I'll switch it to __s32/-1 for the next version.
> >
> > Oh, I think there is a reason actually: since this is an API addition,
> > the "0" value needs to be the previously default behavior if userspace
> > doesn't specify it. Since the previously default behavior was not to
> > return an fd, and we want to allow fd == 0, we need the extra flag to
> > make this work.
> >
> > This is really only a problem because we're introducing this stuff in
> > a second patch (mostly to illustrate how extending the response
> > structure would work). I could fold this into the first patch if we
> > want, or we could keep the return_fd bits if the illustration is
> > useful.
>
> I feel like adding extra struct fields just so that it is possible to
> write programs against the intermediate new API between two kernel
> commits is taking things a bit far.
Yep, I tend to agree with you. I'll fold the whole thing into the
first patch for the next version.
Tycho
^ permalink raw reply
* Re: [PATCH v7] Add udmabuf misc device
From: Gerd Hoffmann @ 2018-09-11 12:03 UTC (permalink / raw)
To: Laurent Pinchart
Cc: dri-devel, David Airlie, Tomeu Vizoso, Daniel Vetter,
Jonathan Corbet, Sumit Semwal, Shuah Khan,
open list:DOCUMENTATION, open list,
open list:DMA BUFFER SHARING FRAMEWORK,
moderated list:DMA BUFFER SHARING FRAMEWORK,
open list:KERNEL SELFTEST FRAMEWORK, linux-api
In-Reply-To: <18750721.r4B5nx0M26@avalon>
> > >> + if (WARN_ON(vmf->pgoff >= ubuf->pagecount))
> > >> + return VM_FAULT_SIGBUS;
> > >
> > > Just curious, when do you expect this to happen ?
> >
> > It should not. If it actually happens it would be a bug somewhere,
> > thats why the WARN_ON.
>
> But you seem to consider that this condition that should never happen still
> has a high enough chance of happening that it's worth a WARN_ON(). I was
> wondering why this one in particular, and not other conditions that also can't
> happen and are not checked through the code.
Added it while writing the code, to get any coding mistake I make
flagged right away instead of things exploding later on.
I can drop it.
> > >> + ubuf = kzalloc(sizeof(struct udmabuf), GFP_KERNEL);
> > >
> > > sizeof(*ubuf)
> >
> > Why? Should not make a difference ...
>
> Because the day we replace
>
> struct udmabuf *ubuf;
>
> with
>
> struct udmabuf_ext *ubuf;
>
> and forget to change the next line, we'll introduce a bug. That's why
> sizeof(variable) is preferred over sizeof(type). Another reason is that I can
> easily see that
>
> ubuf = kzalloc(sizeof(*ubuf), GFP_KERNEL);
>
> is correct, while using sizeof(type) requires me to go and look up the
> declaration of the variable.
So it simplifies review, ok, will change it.
BTW: Maybe the kernel should pick up a neat trick from glib:
g_new0() is a macro which takes the type instead of the size as first
argument, and it casts the return value to that type. So the compiler
will throw warnings in case of a mismatch. That'll work better than
depending purely on the coder being careful and review catching the
remaining issues.
cheers,
Gerd
^ permalink raw reply
* Re: [PATCH v6 2/5] seccomp: make get_nth_filter available outside of CHECKPOINT_RESTORE
From: kbuild test robot @ 2018-09-11 10:25 UTC (permalink / raw)
Cc: kbuild-all, Kees Cook, linux-kernel, containers, linux-api,
Andy Lutomirski, Oleg Nesterov, Eric W . Biederman,
Serge E . Hallyn, Christian Brauner, Tyler Hicks, Akihiro Suda,
Jann Horn, Tycho Andersen
In-Reply-To: <20180906152859.7810-3-tycho@tycho.ws>
[-- Attachment #1: Type: text/plain, Size: 11798 bytes --]
Hi Tycho,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.19-rc3]
[cannot apply to next-20180910]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Tycho-Andersen/seccomp-trap-to-userspace/20180907-130604
config: sparc64-allyesconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=sparc64
All errors (new ones prefixed by >>):
kernel/seccomp.c: In function 'get_nth_filter':
>> kernel/seccomp.c:1211:2: error: implicit declaration of function '__get_seccomp_filter'; did you mean 'get_seccomp_filter'? [-Werror=implicit-function-declaration]
__get_seccomp_filter(orig);
^~~~~~~~~~~~~~~~~~~~
get_seccomp_filter
>> kernel/seccomp.c:1215:45: error: dereferencing pointer to incomplete type 'struct seccomp_filter'
for (filter = orig; filter; filter = filter->prev)
^~
>> kernel/seccomp.c:1235:2: error: implicit declaration of function '__put_seccomp_filter'; did you mean 'put_seccomp_filter'? [-Werror=implicit-function-declaration]
__put_seccomp_filter(orig);
^~~~~~~~~~~~~~~~~~~~
put_seccomp_filter
kernel/seccomp.c: At top level:
>> kernel/seccomp.c:1240:6: error: redefinition of 'seccomp_get_filter'
long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
^~~~~~~~~~~~~~~~~~
In file included from include/linux/sched.h:21:0,
from include/linux/audit.h:26,
from kernel/seccomp.c:18:
include/linux/seccomp.h:103:20: note: previous definition of 'seccomp_get_filter' was here
static inline long seccomp_get_filter(struct task_struct *task,
^~~~~~~~~~~~~~~~~~
kernel/seccomp.c: In function 'seccomp_get_filter':
>> kernel/seccomp.c:1266:13: error: dereferencing pointer to incomplete type 'struct sock_fprog_kern'
ret = fprog->len;
^~
>> kernel/seccomp.c:1270:40: error: implicit declaration of function 'bpf_classic_proglen' [-Werror=implicit-function-declaration]
if (copy_to_user(data, fprog->filter, bpf_classic_proglen(fprog)))
^~~~~~~~~~~~~~~~~~~
kernel/seccomp.c: At top level:
>> kernel/seccomp.c:1278:6: error: redefinition of 'seccomp_get_metadata'
long seccomp_get_metadata(struct task_struct *task,
^~~~~~~~~~~~~~~~~~~~
In file included from include/linux/sched.h:21:0,
from include/linux/audit.h:26,
from kernel/seccomp.c:18:
include/linux/seccomp.h:108:20: note: previous definition of 'seccomp_get_metadata' was here
static inline long seccomp_get_metadata(struct task_struct *task,
^~~~~~~~~~~~~~~~~~~~
kernel/seccomp.c:1791:21: warning: 'init_listener' defined but not used [-Wunused-function]
static struct file *init_listener(struct task_struct *task,
^~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +1211 kernel/seccomp.c
f8e529ed Tycho Andersen 2015-10-27 1190
4f037b54 Tycho Andersen 2018-09-06 1191 #if defined(CONFIG_CHECKPOINT_RESTORE) || \
4f037b54 Tycho Andersen 2018-09-06 1192 defined(CONFIG_SECCOMP_USER_NOTIFICATION)
f06eae83 Tycho Andersen 2017-10-11 1193 static struct seccomp_filter *get_nth_filter(struct task_struct *task,
f06eae83 Tycho Andersen 2017-10-11 1194 unsigned long filter_off)
f8e529ed Tycho Andersen 2015-10-27 1195 {
f06eae83 Tycho Andersen 2017-10-11 1196 struct seccomp_filter *orig, *filter;
f06eae83 Tycho Andersen 2017-10-11 1197 unsigned long count;
f8e529ed Tycho Andersen 2015-10-27 1198
f06eae83 Tycho Andersen 2017-10-11 1199 /*
f06eae83 Tycho Andersen 2017-10-11 1200 * Note: this is only correct because the caller should be the (ptrace)
f06eae83 Tycho Andersen 2017-10-11 1201 * tracer of the task, otherwise lock_task_sighand is needed.
f06eae83 Tycho Andersen 2017-10-11 1202 */
f8e529ed Tycho Andersen 2015-10-27 1203 spin_lock_irq(&task->sighand->siglock);
f06eae83 Tycho Andersen 2017-10-11 1204
f8e529ed Tycho Andersen 2015-10-27 1205 if (task->seccomp.mode != SECCOMP_MODE_FILTER) {
f06eae83 Tycho Andersen 2017-10-11 1206 spin_unlock_irq(&task->sighand->siglock);
f06eae83 Tycho Andersen 2017-10-11 1207 return ERR_PTR(-EINVAL);
f8e529ed Tycho Andersen 2015-10-27 1208 }
f8e529ed Tycho Andersen 2015-10-27 1209
f06eae83 Tycho Andersen 2017-10-11 1210 orig = task->seccomp.filter;
f06eae83 Tycho Andersen 2017-10-11 @1211 __get_seccomp_filter(orig);
f06eae83 Tycho Andersen 2017-10-11 1212 spin_unlock_irq(&task->sighand->siglock);
f06eae83 Tycho Andersen 2017-10-11 1213
f06eae83 Tycho Andersen 2017-10-11 1214 count = 0;
f06eae83 Tycho Andersen 2017-10-11 @1215 for (filter = orig; filter; filter = filter->prev)
f8e529ed Tycho Andersen 2015-10-27 1216 count++;
f8e529ed Tycho Andersen 2015-10-27 1217
f8e529ed Tycho Andersen 2015-10-27 1218 if (filter_off >= count) {
f06eae83 Tycho Andersen 2017-10-11 1219 filter = ERR_PTR(-ENOENT);
f8e529ed Tycho Andersen 2015-10-27 1220 goto out;
f8e529ed Tycho Andersen 2015-10-27 1221 }
f8e529ed Tycho Andersen 2015-10-27 1222
f06eae83 Tycho Andersen 2017-10-11 1223 count -= filter_off;
f06eae83 Tycho Andersen 2017-10-11 1224 for (filter = orig; filter && count > 1; filter = filter->prev)
f8e529ed Tycho Andersen 2015-10-27 1225 count--;
f8e529ed Tycho Andersen 2015-10-27 1226
f8e529ed Tycho Andersen 2015-10-27 1227 if (WARN_ON(count != 1 || !filter)) {
f06eae83 Tycho Andersen 2017-10-11 1228 filter = ERR_PTR(-ENOENT);
f8e529ed Tycho Andersen 2015-10-27 1229 goto out;
f8e529ed Tycho Andersen 2015-10-27 1230 }
f8e529ed Tycho Andersen 2015-10-27 1231
f06eae83 Tycho Andersen 2017-10-11 1232 __get_seccomp_filter(filter);
f06eae83 Tycho Andersen 2017-10-11 1233
f06eae83 Tycho Andersen 2017-10-11 1234 out:
f06eae83 Tycho Andersen 2017-10-11 @1235 __put_seccomp_filter(orig);
f06eae83 Tycho Andersen 2017-10-11 1236 return filter;
f06eae83 Tycho Andersen 2017-10-11 1237 }
f06eae83 Tycho Andersen 2017-10-11 1238
4f037b54 Tycho Andersen 2018-09-06 1239 #if defined(CONFIG_CHECKPOINT_RESTORE)
f06eae83 Tycho Andersen 2017-10-11 @1240 long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
f06eae83 Tycho Andersen 2017-10-11 1241 void __user *data)
f06eae83 Tycho Andersen 2017-10-11 1242 {
f06eae83 Tycho Andersen 2017-10-11 1243 struct seccomp_filter *filter;
f06eae83 Tycho Andersen 2017-10-11 1244 struct sock_fprog_kern *fprog;
f06eae83 Tycho Andersen 2017-10-11 1245 long ret;
f06eae83 Tycho Andersen 2017-10-11 1246
f06eae83 Tycho Andersen 2017-10-11 1247 if (!capable(CAP_SYS_ADMIN) ||
f06eae83 Tycho Andersen 2017-10-11 1248 current->seccomp.mode != SECCOMP_MODE_DISABLED) {
f06eae83 Tycho Andersen 2017-10-11 1249 return -EACCES;
f06eae83 Tycho Andersen 2017-10-11 1250 }
f06eae83 Tycho Andersen 2017-10-11 1251
f06eae83 Tycho Andersen 2017-10-11 1252 filter = get_nth_filter(task, filter_off);
f06eae83 Tycho Andersen 2017-10-11 1253 if (IS_ERR(filter))
f06eae83 Tycho Andersen 2017-10-11 1254 return PTR_ERR(filter);
f06eae83 Tycho Andersen 2017-10-11 1255
f8e529ed Tycho Andersen 2015-10-27 1256 fprog = filter->prog->orig_prog;
f8e529ed Tycho Andersen 2015-10-27 1257 if (!fprog) {
470bf1f2 Mickaël Salaün 2016-03-24 1258 /* This must be a new non-cBPF filter, since we save
f8e529ed Tycho Andersen 2015-10-27 1259 * every cBPF filter's orig_prog above when
f8e529ed Tycho Andersen 2015-10-27 1260 * CONFIG_CHECKPOINT_RESTORE is enabled.
f8e529ed Tycho Andersen 2015-10-27 1261 */
f8e529ed Tycho Andersen 2015-10-27 1262 ret = -EMEDIUMTYPE;
f8e529ed Tycho Andersen 2015-10-27 1263 goto out;
f8e529ed Tycho Andersen 2015-10-27 1264 }
f8e529ed Tycho Andersen 2015-10-27 1265
f8e529ed Tycho Andersen 2015-10-27 @1266 ret = fprog->len;
f8e529ed Tycho Andersen 2015-10-27 1267 if (!data)
f8e529ed Tycho Andersen 2015-10-27 1268 goto out;
f8e529ed Tycho Andersen 2015-10-27 1269
f8e529ed Tycho Andersen 2015-10-27 @1270 if (copy_to_user(data, fprog->filter, bpf_classic_proglen(fprog)))
f8e529ed Tycho Andersen 2015-10-27 1271 ret = -EFAULT;
f8e529ed Tycho Andersen 2015-10-27 1272
f8e529ed Tycho Andersen 2015-10-27 1273 out:
66a733ea Oleg Nesterov 2017-09-27 1274 __put_seccomp_filter(filter);
f8e529ed Tycho Andersen 2015-10-27 1275 return ret;
f8e529ed Tycho Andersen 2015-10-27 1276 }
f8e529ed Tycho Andersen 2015-10-27 1277
26500475 Tycho Andersen 2017-10-11 @1278 long seccomp_get_metadata(struct task_struct *task,
26500475 Tycho Andersen 2017-10-11 1279 unsigned long size, void __user *data)
26500475 Tycho Andersen 2017-10-11 1280 {
26500475 Tycho Andersen 2017-10-11 1281 long ret;
26500475 Tycho Andersen 2017-10-11 1282 struct seccomp_filter *filter;
26500475 Tycho Andersen 2017-10-11 1283 struct seccomp_metadata kmd = {};
26500475 Tycho Andersen 2017-10-11 1284
26500475 Tycho Andersen 2017-10-11 1285 if (!capable(CAP_SYS_ADMIN) ||
26500475 Tycho Andersen 2017-10-11 1286 current->seccomp.mode != SECCOMP_MODE_DISABLED) {
26500475 Tycho Andersen 2017-10-11 1287 return -EACCES;
26500475 Tycho Andersen 2017-10-11 1288 }
26500475 Tycho Andersen 2017-10-11 1289
26500475 Tycho Andersen 2017-10-11 1290 size = min_t(unsigned long, size, sizeof(kmd));
26500475 Tycho Andersen 2017-10-11 1291
63bb0045 Tycho Andersen 2018-02-20 1292 if (size < sizeof(kmd.filter_off))
63bb0045 Tycho Andersen 2018-02-20 1293 return -EINVAL;
63bb0045 Tycho Andersen 2018-02-20 1294
63bb0045 Tycho Andersen 2018-02-20 1295 if (copy_from_user(&kmd.filter_off, data, sizeof(kmd.filter_off)))
26500475 Tycho Andersen 2017-10-11 1296 return -EFAULT;
26500475 Tycho Andersen 2017-10-11 1297
26500475 Tycho Andersen 2017-10-11 1298 filter = get_nth_filter(task, kmd.filter_off);
26500475 Tycho Andersen 2017-10-11 1299 if (IS_ERR(filter))
26500475 Tycho Andersen 2017-10-11 1300 return PTR_ERR(filter);
26500475 Tycho Andersen 2017-10-11 1301
26500475 Tycho Andersen 2017-10-11 1302 if (filter->log)
26500475 Tycho Andersen 2017-10-11 1303 kmd.flags |= SECCOMP_FILTER_FLAG_LOG;
26500475 Tycho Andersen 2017-10-11 1304
26500475 Tycho Andersen 2017-10-11 1305 ret = size;
26500475 Tycho Andersen 2017-10-11 1306 if (copy_to_user(data, &kmd, size))
26500475 Tycho Andersen 2017-10-11 1307 ret = -EFAULT;
26500475 Tycho Andersen 2017-10-11 1308
26500475 Tycho Andersen 2017-10-11 1309 __put_seccomp_filter(filter);
f8e529ed Tycho Andersen 2015-10-27 1310 return ret;
f8e529ed Tycho Andersen 2015-10-27 1311 }
4f037b54 Tycho Andersen 2018-09-06 1312 #endif /* CONFIG_CHECKPOINT_RESTORE */
4f037b54 Tycho Andersen 2018-09-06 1313 #endif /* CONFIG_SECCOMP_FILTER */
8e5f1ad1 Tyler Hicks 2017-08-11 1314
:::::: The code at line 1211 was first introduced by commit
:::::: f06eae831f0c1fc5b982ea200daf552810e1dd55 seccomp: hoist out filter resolving logic
:::::: TO: Tycho Andersen <tycho@docker.com>
:::::: CC: Kees Cook <keescook@chromium.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 55342 bytes --]
^ permalink raw reply
* Re: [PATCH v7] Add udmabuf misc device
From: Daniel Vetter @ 2018-09-11 10:05 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Gerd Hoffmann, dri-devel, David Airlie, Tomeu Vizoso,
Jonathan Corbet, Sumit Semwal, Shuah Khan,
open list:DOCUMENTATION, open list,
open list:DMA BUFFER SHARING FRAMEWORK,
moderated list:DMA BUFFER SHARING FRAMEWORK,
open list:KERNEL SELFTEST FRAMEWORK, open list:ABI/API
In-Reply-To: <18750721.r4B5nx0M26@avalon>
On Tue, Sep 11, 2018 at 11:50 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Gerd,
>
> On Tuesday, 11 September 2018 09:50:14 EEST Gerd Hoffmann wrote:
>> Hi,
>>
>> >> +#define UDMABUF_CREATE _IOW('u', 0x42, struct udmabuf_create)
>> >
>> > Why do you start at 0x42 if you reserve the 0x40-0x4f range ?
>>
>> No particular strong reason, just that using 42 was less boring than
>> starting with 0x40.
>>
>> >> +#define UDMABUF_CREATE_LIST _IOW('u', 0x43, struct
>> >> udmabuf_create_list)
>> >
>> > Where's the documentation ? :-)
>>
>> Isn't it simple enough?
>
> No kernel UAPI is simple enough to get away without documenting it.
Simplest option would be to throw a bit of kerneldoc into the uapi
header, add Documentation/driver-api/dma-buf.rst.
-Daniel
>
>> But, well, yes, I guess I can add some kerneldoc comments.
>>
>> >> +static int udmabuf_vm_fault(struct vm_fault *vmf)
>> >> +{
>> >> + struct vm_area_struct *vma = vmf->vma;
>> >> + struct udmabuf *ubuf = vma->vm_private_data;
>> >> +
>> >> + if (WARN_ON(vmf->pgoff >= ubuf->pagecount))
>> >> + return VM_FAULT_SIGBUS;
>> >
>> > Just curious, when do you expect this to happen ?
>>
>> It should not. If it actually happens it would be a bug somewhere,
>> thats why the WARN_ON.
>
> But you seem to consider that this condition that should never happen still
> has a high enough chance of happening that it's worth a WARN_ON(). I was
> wondering why this one in particular, and not other conditions that also can't
> happen and are not checked through the code.
>
>> >> + struct udmabuf *ubuf;
>> >>
>> >> + ubuf = kzalloc(sizeof(struct udmabuf), GFP_KERNEL);
>> >
>> > sizeof(*ubuf)
>>
>> Why? Should not make a difference ...
>
> Because the day we replace
>
> struct udmabuf *ubuf;
>
> with
>
> struct udmabuf_ext *ubuf;
>
> and forget to change the next line, we'll introduce a bug. That's why
> sizeof(variable) is preferred over sizeof(type). Another reason is that I can
> easily see that
>
> ubuf = kzalloc(sizeof(*ubuf), GFP_KERNEL);
>
> is correct, while using sizeof(type) requires me to go and look up the
> declaration of the variable.
>
>> >> + memfd = fget(list[i].memfd);
>> >> + if (!memfd)
>> >> + goto err_put_pages;
>> >> + if (!shmem_mapping(file_inode(memfd)->i_mapping))
>> >> + goto err_put_pages;
>> >> + seals = memfd_fcntl(memfd, F_GET_SEALS, 0);
>> >> + if (seals == -EINVAL ||
>> >> + (seals & SEALS_WANTED) != SEALS_WANTED ||
>> >> + (seals & SEALS_DENIED) != 0)
>> >> + goto err_put_pages;
>> >
>> > All these conditions will return -EINVAL. I'm not familiar with the memfd
>> > API, should some error conditions return a different error code to make
>> > them distinguishable by userspace ?
>>
>> Hmm, I guess EBADFD would be reasonable in case the file handle isn't a
>> memfd. Other suggestions?
>
> I'll let others comment on this as I don't feel qualified to pick proper error
> codes, not being familiar with the memfd API.
>
>> I'll prepare a fixup patch series addressing most of the other
>> review comments.
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply
* Re: [PATCH v7] Add udmabuf misc device
From: Laurent Pinchart @ 2018-09-11 9:50 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: open list:KERNEL SELFTEST FRAMEWORK, Tomeu Vizoso,
Jonathan Corbet, David Airlie, linux-api, open list:DOCUMENTATION,
open list, dri-devel, moderated list:DMA BUFFER SHARING FRAMEWORK,
Shuah Khan, open list:DMA BUFFER SHARING FRAMEWORK
In-Reply-To: <20180911065014.vo6qp6hkb7cjftdc@sirius.home.kraxel.org>
Hi Gerd,
On Tuesday, 11 September 2018 09:50:14 EEST Gerd Hoffmann wrote:
> Hi,
>
> >> +#define UDMABUF_CREATE _IOW('u', 0x42, struct udmabuf_create)
> >
> > Why do you start at 0x42 if you reserve the 0x40-0x4f range ?
>
> No particular strong reason, just that using 42 was less boring than
> starting with 0x40.
>
> >> +#define UDMABUF_CREATE_LIST _IOW('u', 0x43, struct
> >> udmabuf_create_list)
> >
> > Where's the documentation ? :-)
>
> Isn't it simple enough?
No kernel UAPI is simple enough to get away without documenting it.
> But, well, yes, I guess I can add some kerneldoc comments.
>
> >> +static int udmabuf_vm_fault(struct vm_fault *vmf)
> >> +{
> >> + struct vm_area_struct *vma = vmf->vma;
> >> + struct udmabuf *ubuf = vma->vm_private_data;
> >> +
> >> + if (WARN_ON(vmf->pgoff >= ubuf->pagecount))
> >> + return VM_FAULT_SIGBUS;
> >
> > Just curious, when do you expect this to happen ?
>
> It should not. If it actually happens it would be a bug somewhere,
> thats why the WARN_ON.
But you seem to consider that this condition that should never happen still
has a high enough chance of happening that it's worth a WARN_ON(). I was
wondering why this one in particular, and not other conditions that also can't
happen and are not checked through the code.
> >> + struct udmabuf *ubuf;
> >>
> >> + ubuf = kzalloc(sizeof(struct udmabuf), GFP_KERNEL);
> >
> > sizeof(*ubuf)
>
> Why? Should not make a difference ...
Because the day we replace
struct udmabuf *ubuf;
with
struct udmabuf_ext *ubuf;
and forget to change the next line, we'll introduce a bug. That's why
sizeof(variable) is preferred over sizeof(type). Another reason is that I can
easily see that
ubuf = kzalloc(sizeof(*ubuf), GFP_KERNEL);
is correct, while using sizeof(type) requires me to go and look up the
declaration of the variable.
> >> + memfd = fget(list[i].memfd);
> >> + if (!memfd)
> >> + goto err_put_pages;
> >> + if (!shmem_mapping(file_inode(memfd)->i_mapping))
> >> + goto err_put_pages;
> >> + seals = memfd_fcntl(memfd, F_GET_SEALS, 0);
> >> + if (seals == -EINVAL ||
> >> + (seals & SEALS_WANTED) != SEALS_WANTED ||
> >> + (seals & SEALS_DENIED) != 0)
> >> + goto err_put_pages;
> >
> > All these conditions will return -EINVAL. I'm not familiar with the memfd
> > API, should some error conditions return a different error code to make
> > them distinguishable by userspace ?
>
> Hmm, I guess EBADFD would be reasonable in case the file handle isn't a
> memfd. Other suggestions?
I'll let others comment on this as I don't feel qualified to pick proper error
codes, not being familiar with the memfd API.
> I'll prepare a fixup patch series addressing most of the other
> review comments.
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v7] Add udmabuf misc device
From: Gerd Hoffmann @ 2018-09-11 6:50 UTC (permalink / raw)
To: Laurent Pinchart
Cc: dri-devel, David Airlie, Tomeu Vizoso, Daniel Vetter,
Jonathan Corbet, Sumit Semwal, Shuah Khan,
open list:DOCUMENTATION, open list,
open list:DMA BUFFER SHARING FRAMEWORK,
moderated list:DMA BUFFER SHARING FRAMEWORK,
open list:KERNEL SELFTEST FRAMEWORK, linux-api
In-Reply-To: <21053714.0Xa7F2u2PE@avalon>
Hi,
> > +#define UDMABUF_CREATE _IOW('u', 0x42, struct udmabuf_create)
>
> Why do you start at 0x42 if you reserve the 0x40-0x4f range ?
No particular strong reason, just that using 42 was less boring than
starting with 0x40.
> > +#define UDMABUF_CREATE_LIST _IOW('u', 0x43, struct udmabuf_create_list)
>
> Where's the documentation ? :-)
Isn't it simple enough?
But, well, yes, I guess I can add some kerneldoc comments.
> > +static int udmabuf_vm_fault(struct vm_fault *vmf)
> > +{
> > + struct vm_area_struct *vma = vmf->vma;
> > + struct udmabuf *ubuf = vma->vm_private_data;
> > +
> > + if (WARN_ON(vmf->pgoff >= ubuf->pagecount))
> > + return VM_FAULT_SIGBUS;
>
> Just curious, when do you expect this to happen ?
It should not. If it actually happens it would be a bug somewhere,
thats why the WARN_ON.
> > + struct udmabuf *ubuf;
> > + ubuf = kzalloc(sizeof(struct udmabuf), GFP_KERNEL);
>
> sizeof(*ubuf)
Why? Should not make a difference ...
> > + memfd = fget(list[i].memfd);
> > + if (!memfd)
> > + goto err_put_pages;
> > + if (!shmem_mapping(file_inode(memfd)->i_mapping))
> > + goto err_put_pages;
> > + seals = memfd_fcntl(memfd, F_GET_SEALS, 0);
> > + if (seals == -EINVAL ||
> > + (seals & SEALS_WANTED) != SEALS_WANTED ||
> > + (seals & SEALS_DENIED) != 0)
> > + goto err_put_pages;
>
> All these conditions will return -EINVAL. I'm not familiar with the memfd API,
> should some error conditions return a different error code to make them
> distinguishable by userspace ?
Hmm, I guess EBADFD would be reasonable in case the file handle isn't a
memfd. Other suggestions?
I'll prepare a fixup patch series addressing most of the other
review comments.
cheers,
Gerd
^ permalink raw reply
* [PATCH 2/2] vfs: dedupe should return EPERM if permission is not granted
From: Mark Fasheh @ 2018-09-10 23:21 UTC (permalink / raw)
To: Andrew Morton
Cc: Al Viro, linux-fsdevel, linux-api, Michael Kerrisk, linux-btrfs,
linux-xfs, Darrick J . Wong, Adam Borowski, David Sterba,
Mark Fasheh
In-Reply-To: <20180910232118.14424-1-mfasheh@suse.de>
Right now we return EINVAL if a process does not have permission to dedupe a
file. This was an oversight on my part. EPERM gives a true description of
the nature of our error, and EINVAL is already used for the case that the
filesystem does not support dedupe.
Signed-off-by: Mark Fasheh <mfasheh@suse.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Acked-by: David Sterba <dsterba@suse.com>
---
fs/read_write.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/read_write.c b/fs/read_write.c
index be0e8723a049..c734bc2880a5 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1991,7 +1991,7 @@ int vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
if (ret < 0)
goto out_drop_write;
- ret = -EINVAL;
+ ret = -EPERM;
if (!allow_file_dedupe(dst_file))
goto out_drop_write;
--
2.15.1
^ permalink raw reply related
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