* Re: [RFC PATCH v6 08/11] x86: Insert endbr32/endbr64 to vDSO
From: Andy Lutomirski @ 2018-11-19 22:17 UTC (permalink / raw)
To: Yu-cheng Yu
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,
Eugene Syromiatnikov, Florian Weimer, H. J. Lu, Jann Horn,
Jonathan Corbet, Kees Cook, Mike Kravetz,
Nadav Amit <nadav.am>
In-Reply-To: <20181119214934.6174-9-yu-cheng.yu@intel.com>
On Mon, Nov 19, 2018 at 1:55 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
> From: "H.J. Lu" <hjl.tools@gmail.com>
>
> When Intel indirect branch tracking is enabled, functions in vDSO which
> may be called indirectly must have endbr32 or endbr64 as the first
> instruction. Compiler must support -fcf-protection=branch so that it
> can be used to compile vDSO.
>
> Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
> ---
> arch/x86/entry/vdso/.gitignore | 4 ++++
> arch/x86/entry/vdso/Makefile | 12 +++++++++++-
> arch/x86/entry/vdso/vdso-layout.lds.S | 1 +
> 3 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/entry/vdso/.gitignore b/arch/x86/entry/vdso/.gitignore
> index aae8ffdd5880..552941fdfae0 100644
> --- a/arch/x86/entry/vdso/.gitignore
> +++ b/arch/x86/entry/vdso/.gitignore
> @@ -5,3 +5,7 @@ vdso32-sysenter-syms.lds
> vdso32-int80-syms.lds
> vdso-image-*.c
> vdso2c
> +vclock_gettime.S
> +vgetcpu.S
> +vclock_gettime.asm
> +vgetcpu.asm
What's this hunk about?
^ permalink raw reply
* Re: [RFC PATCH v6 10/11] x86/vsyscall/64: Add ENDBR64 to vsyscall entry points
From: Andy Lutomirski @ 2018-11-19 22:22 UTC (permalink / raw)
To: Yu-cheng Yu
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,
Eugene Syromiatnikov, Florian Weimer, H. J. Lu, Jann Horn,
Jonathan Corbet, Kees Cook, Mike Kravetz,
Nadav Amit <nadav.am>
In-Reply-To: <20181119214934.6174-11-yu-cheng.yu@intel.com>
On Mon, Nov 19, 2018 at 1:55 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
> From: "H.J. Lu" <hjl.tools@gmail.com>
>
> Add ENDBR64 to vsyscall entry points.
>
> Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
Acked-by: Andy Lutomirski <luto@kernel.org>
although the scenarios where this matters will be extremely rare,
given that this code is mapped NX :) Tools like 'pin' may care.
--Andy
^ permalink raw reply
* Re: [RFC PATCH v6 09/11] x86/vsyscall/32: Add ENDBR32 to vsyscall entry point
From: Andy Lutomirski @ 2018-11-19 22:23 UTC (permalink / raw)
To: Yu-cheng Yu
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,
Eugene Syromiatnikov, Florian Weimer, H. J. Lu, Jann Horn,
Jonathan Corbet, Kees Cook, Mike Kravetz,
Nadav Amit <nadav.am>
In-Reply-To: <20181119214934.6174-10-yu-cheng.yu@intel.com>
On Mon, Nov 19, 2018 at 1:55 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
> From: "H.J. Lu" <hjl.tools@gmail.com>
>
> Add ENDBR32 to vsyscall entry point.
$SUBJECT should be "x86/vdso/32: Add ENDBR32 to __kernel_vsyscall entry point".
--Andy
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Tycho Andersen @ 2018-11-19 22:39 UTC (permalink / raw)
To: Christian Brauner
Cc: ebiederm, linux-kernel, serge, jannh, luto, akpm, oleg, cyphar,
viro, linux-fsdevel, linux-api, dancol, timmurray, linux-man,
Kees Cook
In-Reply-To: <20181119103241.5229-3-christian@brauner.io>
On Mon, Nov 19, 2018 at 11:32:39AM +0100, Christian Brauner wrote:
>
> +/**
> + * sys_procfd_signal - send a signal to a process through a process file
> + * descriptor
> + * @fd: the file descriptor of the process
> + * @sig: signal to be sent
> + * @info: the signal info
> + * @flags: future flags to be passed
> + */
> +SYSCALL_DEFINE4(procfd_signal, int, fd, int, sig, siginfo_t __user *, info,
> + int, flags)
> +{
Can I just register an objection here that I think using a syscall
just for this is silly?
My understanding is that the concern is that some code might do:
unknown_fd = recv_fd();
ioctl(unknown_fd, SOME_IOCTL, NULL); // where SOME_IOCTL == PROC_FD_KILL
// whoops, unknown_fd was a procfd and we killed a task!
In my experience when writing fd sending/receiving code, the sender and
receiver are fairly tightly coupled. Has anyone ever actually fixed a
bug where they had an fd that they lost track of what "type" it was
and screwed up like this? It seems completely theoretical to me.
The ioctl() approach has the benefit of being extensible. Adding a
syscall means that everyone has to do all the boilerplate for each new
pid op in the kernel, arches, libc, strace, etc.
Tycho
^ permalink raw reply
* Re: [RFC PATCH v6 09/11] x86/vsyscall/32: Add ENDBR32 to vsyscall entry point
From: Yu-cheng Yu @ 2018-11-19 22:40 UTC (permalink / raw)
To: 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,
Eugene Syromiatnikov, Florian Weimer, H. J. Lu, Jann Horn,
Jonathan Corbet, Kees Cook, Mike Kravetz,
Nadav Amit <nadav.am>
In-Reply-To: <CALCETrUkXMHcwX8u+mGDD9Vj+iQE7CRMHL7jQnfAT6WXV8SBEQ@mail.gmail.com>
On Mon, 2018-11-19 at 14:23 -0800, Andy Lutomirski wrote:
> On Mon, Nov 19, 2018 at 1:55 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> >
> > From: "H.J. Lu" <hjl.tools@gmail.com>
> >
> > Add ENDBR32 to vsyscall entry point.
>
> $SUBJECT should be "x86/vdso/32: Add ENDBR32 to __kernel_vsyscall entry
> point".
I will fix it.
Yu-cheng
^ permalink raw reply
* Re: [RFC PATCH v6 08/11] x86: Insert endbr32/endbr64 to vDSO
From: Yu-cheng Yu @ 2018-11-19 22:43 UTC (permalink / raw)
To: 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,
Eugene Syromiatnikov, Florian Weimer, H. J. Lu, Jann Horn,
Jonathan Corbet, Kees Cook, Mike Kravetz,
Nadav Amit <nadav.am>
In-Reply-To: <CALCETrW9ABDotyM020V147+kuizpTRJUAANn-6kUt6-h0Qn0og@mail.gmail.com>
On Mon, 2018-11-19 at 14:17 -0800, Andy Lutomirski wrote:
> On Mon, Nov 19, 2018 at 1:55 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> >
> > From: "H.J. Lu" <hjl.tools@gmail.com>
> >
> > When Intel indirect branch tracking is enabled, functions in vDSO which
> > may be called indirectly must have endbr32 or endbr64 as the first
> > instruction. Compiler must support -fcf-protection=branch so that it
> > can be used to compile vDSO.
> >
> > Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
> > ---
> > arch/x86/entry/vdso/.gitignore | 4 ++++
> > arch/x86/entry/vdso/Makefile | 12 +++++++++++-
> > arch/x86/entry/vdso/vdso-layout.lds.S | 1 +
> > 3 files changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/entry/vdso/.gitignore b/arch/x86/entry/vdso/.gitignore
> > index aae8ffdd5880..552941fdfae0 100644
> > --- a/arch/x86/entry/vdso/.gitignore
> > +++ b/arch/x86/entry/vdso/.gitignore
> > @@ -5,3 +5,7 @@ vdso32-sysenter-syms.lds
> > vdso32-int80-syms.lds
> > vdso-image-*.c
> > vdso2c
> > +vclock_gettime.S
> > +vgetcpu.S
> > +vclock_gettime.asm
> > +vgetcpu.asm
>
>
> What's this hunk about?
We used to allow using non-CET capable BINUTILS and the Makefile would create
these. I will remove them from the patch.
Yu-cheng
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Daniel Colascione @ 2018-11-19 22:49 UTC (permalink / raw)
To: Tycho Andersen
Cc: Christian Brauner, Eric W. Biederman, linux-kernel,
Serge E. Hallyn, Jann Horn, Andy Lutomirski, Andrew Morton,
Oleg Nesterov, Aleksa Sarai, Al Viro, Linux FS Devel, Linux API,
Tim Murray, linux-man, Kees Cook
In-Reply-To: <20181119223954.GA4992@cisco>
On Mon, Nov 19, 2018 at 2:40 PM Tycho Andersen <tycho@tycho.ws> wrote:
> Can I just register an objection here that I think using a syscall
> just for this is silly?
Yes, you can argue that the bikeshed should be ioctl-colored and not
syscall-colored.
> My understanding is that the concern is that some code might do:
>
> unknown_fd = recv_fd();
> ioctl(unknown_fd, SOME_IOCTL, NULL); // where SOME_IOCTL == PROC_FD_KILL
> // whoops, unknown_fd was a procfd and we killed a task!
>
> In my experience when writing fd sending/receiving code, the sender and
> receiver are fairly tightly coupled. Has anyone ever actually fixed a
> bug where they had an fd that they lost track of what "type" it was
> and screwed up like this? It seems completely theoretical to me.
Yes, I have fixed bugs of this form.
> The ioctl() approach has the benefit of being extensible.
The system call table is also extensible. ioctl is for when a given
piece of functionality *can't* realistically get its own system call
because it's separated from the main kernel somehow. procfs is a core
part of the kernel, so we can and should expose interfaces to it using
system calls.
> Adding a
> syscall means that everyone has to do all the boilerplate for each new
> pid op in the kernel, arches, libc, strace, etc.
These tools also care about ioctls. Adding a system call is a pain,
but the solution is to make adding system calls less of a pain, not to
permanently make the Linux ABI worse.
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Tycho Andersen @ 2018-11-19 23:07 UTC (permalink / raw)
To: Daniel Colascione
Cc: Christian Brauner, Eric W. Biederman, linux-kernel,
Serge E. Hallyn, Jann Horn, Andy Lutomirski, Andrew Morton,
Oleg Nesterov, Aleksa Sarai, Al Viro, Linux FS Devel, Linux API,
Tim Murray, linux-man, Kees Cook
In-Reply-To: <CAKOZuetQDziWeRLydHbDNv1abGM3LyF=WohLFvbzmtdT_+nBdg@mail.gmail.com>
On Mon, Nov 19, 2018 at 02:49:22PM -0800, Daniel Colascione wrote:
> On Mon, Nov 19, 2018 at 2:40 PM Tycho Andersen <tycho@tycho.ws> wrote:
> > Can I just register an objection here that I think using a syscall
> > just for this is silly?
>
> Yes, you can argue that the bikeshed should be ioctl-colored and not
> syscall-colored.
>
> > My understanding is that the concern is that some code might do:
> >
> > unknown_fd = recv_fd();
> > ioctl(unknown_fd, SOME_IOCTL, NULL); // where SOME_IOCTL == PROC_FD_KILL
> > // whoops, unknown_fd was a procfd and we killed a task!
> >
> > In my experience when writing fd sending/receiving code, the sender and
> > receiver are fairly tightly coupled. Has anyone ever actually fixed a
> > bug where they had an fd that they lost track of what "type" it was
> > and screwed up like this? It seems completely theoretical to me.
>
> Yes, I have fixed bugs of this form.
>
> > The ioctl() approach has the benefit of being extensible.
>
> The system call table is also extensible.
But not infinitely so. The x32 ABI starts at 512, and right now I see
334 syscalls on x86_64. So the next 178 people can say "let's just
define a syscall", and after that? I suppose we could move to setting
BIT(10), but how much userspace assumes > 512 => compat syscall and
blocks it via seccomp or whatever?
Contrast that with the ioctl space, which is an unsigned long and
fairly sparse still (Documentation/ioctl/ioctl-number.txt).
> ioctl is for when a given piece of functionality *can't*
> realistically get its own system call because it's separated from
> the main kernel somehow. procfs is a core part of the kernel, so we
> can and should expose interfaces to it using system calls.
I suppose it's obvious, but I disagree.
> > Adding a
> > syscall means that everyone has to do all the boilerplate for each new
> > pid op in the kernel, arches, libc, strace, etc.
>
> These tools also care about ioctls. Adding a system call is a pain,
> but the solution is to make adding system calls less of a pain, not to
> permanently make the Linux ABI worse.
For user-defined values of "worse" :)
Tycho
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: kbuild test robot @ 2018-11-19 23:35 UTC (permalink / raw)
Cc: kbuild-all, ebiederm, linux-kernel, serge, jannh, luto, akpm,
oleg, cyphar, viro, linux-fsdevel, linux-api, dancol, timmurray,
linux-man, Christian Brauner, Kees Cook
In-Reply-To: <20181119103241.5229-3-christian@brauner.io>
[-- Attachment #1: Type: text/plain, Size: 1048 bytes --]
Hi Christian,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v4.20-rc3]
[cannot apply to next-20181119]
[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/Christian-Brauner/proc-allow-signaling-processes-via-file-descriptors/20181120-063836
config: riscv-tinyconfig (attached as .config)
compiler: riscv64-linux-gcc (GCC) 8.1.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=8.1.0 make.cross ARCH=riscv
All warnings (new ones prefixed by >>):
>> <stdin>:1338:2: warning: #warning syscall procfd_signal not implemented [-Wcpp]
---
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: 4416 bytes --]
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: kbuild test robot @ 2018-11-19 23:37 UTC (permalink / raw)
Cc: kbuild-all, ebiederm, linux-kernel, serge, jannh, luto, akpm,
oleg, cyphar, viro, linux-fsdevel, linux-api, dancol, timmurray,
linux-man, Christian Brauner, Kees Cook
In-Reply-To: <20181119103241.5229-3-christian@brauner.io>
[-- Attachment #1: Type: text/plain, Size: 2942 bytes --]
Hi Christian,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.20-rc3]
[cannot apply to next-20181119]
[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/Christian-Brauner/proc-allow-signaling-processes-via-file-descriptors/20181120-063836
config: riscv-tinyconfig (attached as .config)
compiler: riscv64-linux-gcc (GCC) 8.1.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=8.1.0 make.cross ARCH=riscv
All errors (new ones prefixed by >>):
kernel/signal.c: In function '__do_sys_procfd_signal':
>> kernel/signal.c:3341:7: error: implicit declaration of function 'proc_is_procfd'; did you mean 'clockid_to_fd'? [-Werror=implicit-function-declaration]
if (!proc_is_procfd(f.file))
^~~~~~~~~~~~~~
clockid_to_fd
cc1: some warnings being treated as errors
vim +3341 kernel/signal.c
3314
3315 /**
3316 * sys_procfd_signal - send a signal to a process through a process file
3317 * descriptor
3318 * @fd: the file descriptor of the process
3319 * @sig: signal to be sent
3320 * @info: the signal info
3321 * @flags: future flags to be passed
3322 */
3323 SYSCALL_DEFINE4(procfd_signal, int, fd, int, sig, siginfo_t __user *, info,
3324 int, flags)
3325 {
3326 int ret;
3327 struct pid *pid;
3328 kernel_siginfo_t kinfo;
3329 struct fd f;
3330
3331 /* Enforce flags be set to 0 until we add an extension. */
3332 if (flags)
3333 return -EINVAL;
3334
3335 f = fdget_raw(fd);
3336 if (!f.file)
3337 return -EBADF;
3338
3339 ret = -EINVAL;
3340 /* Is this a process file descriptor? */
> 3341 if (!proc_is_procfd(f.file))
3342 goto err;
3343
3344 pid = f.file->private_data;
3345 if (!pid)
3346 goto err;
3347
3348 if (info) {
3349 ret = __copy_siginfo_from_user(sig, &kinfo, info);
3350 if (unlikely(ret))
3351 goto err;
3352 /*
3353 * Not even root can pretend to send signals from the kernel.
3354 * Nor can they impersonate a kill()/tgkill(), which adds
3355 * source info.
3356 */
3357 ret = -EPERM;
3358 if ((kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL) &&
3359 (task_pid(current) != pid))
3360 goto err;
3361 } else {
3362 prepare_kill_siginfo(sig, &kinfo);
3363 }
3364
3365 ret = kill_pid_info(sig, &kinfo, pid);
3366
3367 err:
3368 fdput(f);
3369 return ret;
3370 }
3371
---
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: 4416 bytes --]
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Christian Brauner @ 2018-11-19 23:45 UTC (permalink / raw)
To: kbuild test robot
Cc: kbuild-all, ebiederm, linux-kernel, serge, jannh, luto, akpm,
oleg, cyphar, viro, linux-fsdevel, linux-api, dancol, timmurray,
linux-man, Kees Cook
In-Reply-To: <201811200755.avfmktKQ%fengguang.wu@intel.com>
On Tue, Nov 20, 2018 at 07:37:58AM +0800, kbuild test robot wrote:
> Hi Christian,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on linus/master]
> [also build test ERROR on v4.20-rc3]
> [cannot apply to next-20181119]
> [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/Christian-Brauner/proc-allow-signaling-processes-via-file-descriptors/20181120-063836
> config: riscv-tinyconfig (attached as .config)
> compiler: riscv64-linux-gcc (GCC) 8.1.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=8.1.0 make.cross ARCH=riscv
>
> All errors (new ones prefixed by >>):
>
> kernel/signal.c: In function '__do_sys_procfd_signal':
> >> kernel/signal.c:3341:7: error: implicit declaration of function 'proc_is_procfd'; did you mean 'clockid_to_fd'? [-Werror=implicit-function-declaration]
> if (!proc_is_procfd(f.file))
> ^~~~~~~~~~~~~~
On my radar and fixed. This happens when CONFIG_PROC_FS unset.
> clockid_to_fd
> cc1: some warnings being treated as errors
>
> vim +3341 kernel/signal.c
>
> 3314
> 3315 /**
> 3316 * sys_procfd_signal - send a signal to a process through a process file
> 3317 * descriptor
> 3318 * @fd: the file descriptor of the process
> 3319 * @sig: signal to be sent
> 3320 * @info: the signal info
> 3321 * @flags: future flags to be passed
> 3322 */
> 3323 SYSCALL_DEFINE4(procfd_signal, int, fd, int, sig, siginfo_t __user *, info,
> 3324 int, flags)
> 3325 {
> 3326 int ret;
> 3327 struct pid *pid;
> 3328 kernel_siginfo_t kinfo;
> 3329 struct fd f;
> 3330
> 3331 /* Enforce flags be set to 0 until we add an extension. */
> 3332 if (flags)
> 3333 return -EINVAL;
> 3334
> 3335 f = fdget_raw(fd);
> 3336 if (!f.file)
> 3337 return -EBADF;
> 3338
> 3339 ret = -EINVAL;
> 3340 /* Is this a process file descriptor? */
> > 3341 if (!proc_is_procfd(f.file))
> 3342 goto err;
> 3343
> 3344 pid = f.file->private_data;
> 3345 if (!pid)
> 3346 goto err;
> 3347
> 3348 if (info) {
> 3349 ret = __copy_siginfo_from_user(sig, &kinfo, info);
> 3350 if (unlikely(ret))
> 3351 goto err;
> 3352 /*
> 3353 * Not even root can pretend to send signals from the kernel.
> 3354 * Nor can they impersonate a kill()/tgkill(), which adds
> 3355 * source info.
> 3356 */
> 3357 ret = -EPERM;
> 3358 if ((kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL) &&
> 3359 (task_pid(current) != pid))
> 3360 goto err;
> 3361 } else {
> 3362 prepare_kill_siginfo(sig, &kinfo);
> 3363 }
> 3364
> 3365 ret = kill_pid_info(sig, &kinfo, pid);
> 3366
> 3367 err:
> 3368 fdput(f);
> 3369 return ret;
> 3370 }
> 3371
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply
* Re: [PATCH v3 0/4] bpf: allow zero-initialising hash map seed
From: Daniel Borkmann @ 2018-11-19 23:56 UTC (permalink / raw)
To: Lorenz Bauer, ast; +Cc: netdev, linux-api, songliubraving
In-Reply-To: <20181116114111.31177-1-lmb@cloudflare.com>
On 11/16/2018 12:41 PM, Lorenz Bauer wrote:
> Allow forcing the seed of a hash table to zero, for deterministic
> execution during benchmarking and testing.
>
> Changes from v2:
> * Change ordering of BPF_F_ZERO_SEED in linux/bpf.h
>
> Comments adressed from v1:
> * Add comment to discourage production use to linux/bpf.h
> * Require CAP_SYS_ADMIN
>
> Lorenz Bauer (4):
> bpf: allow zero-initializing hash map seed
> bpf: move BPF_F_QUERY_EFFECTIVE after map flags
> tools: sync linux/bpf.h
> tools: add selftest for BPF_F_ZERO_SEED
>
> include/uapi/linux/bpf.h | 9 ++--
> kernel/bpf/hashtab.c | 13 ++++-
> tools/include/uapi/linux/bpf.h | 13 +++--
> tools/testing/selftests/bpf/test_maps.c | 68 +++++++++++++++++++++----
> 4 files changed, 84 insertions(+), 19 deletions(-)
>
Applied to bpf-next, thanks!
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Andy Lutomirski @ 2018-11-20 0:27 UTC (permalink / raw)
To: Tycho Andersen
Cc: Daniel Colascione, Christian Brauner, Eric W. Biederman, LKML,
Serge E. Hallyn, Jann Horn, Andrew Lutomirski, Andrew Morton,
Oleg Nesterov, Aleksa Sarai, Al Viro, Linux FS Devel, Linux API,
Tim Murray, linux-man, Kees Cook
In-Reply-To: <20181119230709.GB4992@cisco>
On Mon, Nov 19, 2018 at 3:07 PM Tycho Andersen <tycho@tycho.ws> wrote:
> > These tools also care about ioctls. Adding a system call is a pain,
> > but the solution is to make adding system calls less of a pain, not to
> > permanently make the Linux ABI worse.
>
> For user-defined values of "worse" :)
>
I tend to agree with Tycho here. But I'm wondering if it might be
worth considering a better ioctl.
/me dons flame-proof hat
We could do:
long better_ioctl(int fd, u32 nr, const void *inbuf, size_t inlen,
const void *outbuf, size_t outlen);
and have a central table in the kernel listing all possible nr values
along with which driver they belong to. We could have a sane
signature and get rid of the nr collision problem.
The major problem I see is that u32 isn't really enough to have a sane
way to allow out-of-tree drivers to use this, and that we can't
readily use anything bigger than u32 without indirection because we're
out of syscall argument space.
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Christian Brauner @ 2018-11-20 0:32 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Tycho Andersen, Daniel Colascione, Eric W. Biederman, LKML,
Serge E. Hallyn, Jann Horn, Andrew Morton, Oleg Nesterov,
Aleksa Sarai, Al Viro, Linux FS Devel, Linux API, Tim Murray,
linux-man, Kees Cook
In-Reply-To: <CALCETrWyuQvTtksN1J1XbCFPka_rLOaFqa5W==EvGQvoaf9f3Q@mail.gmail.com>
On Mon, Nov 19, 2018 at 04:27:49PM -0800, Andy Lutomirski wrote:
> On Mon, Nov 19, 2018 at 3:07 PM Tycho Andersen <tycho@tycho.ws> wrote:
> > > These tools also care about ioctls. Adding a system call is a pain,
> > > but the solution is to make adding system calls less of a pain, not to
> > > permanently make the Linux ABI worse.
> >
> > For user-defined values of "worse" :)
> >
>
> I tend to agree with Tycho here. But I'm wondering if it might be
> worth considering a better ioctl.
>
> /me dons flame-proof hat
>
> We could do:
>
> long better_ioctl(int fd, u32 nr, const void *inbuf, size_t inlen,
> const void *outbuf, size_t outlen);
I'm the writer of this patch so take this with a grain of salt.
I think it is a bad idea to stop this patch and force us to introduce a
new type of ioctl().
An ioctl() is also not easy to use for this task because we want to add
a siginfo_t argument which I actually think provides value and makes
this interface more useful.
>
> and have a central table in the kernel listing all possible nr values
> along with which driver they belong to. We could have a sane
> signature and get rid of the nr collision problem.
>
> The major problem I see is that u32 isn't really enough to have a sane
> way to allow out-of-tree drivers to use this, and that we can't
> readily use anything bigger than u32 without indirection because we're
> out of syscall argument space.
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Andy Lutomirski @ 2018-11-20 0:34 UTC (permalink / raw)
To: Christian Brauner
Cc: Andrew Lutomirski, Tycho Andersen, Daniel Colascione,
Eric W. Biederman, LKML, Serge E. Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, Aleksa Sarai, Al Viro,
Linux FS Devel, Linux API, Tim Murray, linux-man, Kees Cook
In-Reply-To: <20181120003247.a776bej54baxqoxv@brauner.io>
On Mon, Nov 19, 2018 at 4:33 PM Christian Brauner <christian@brauner.io> wrote:
>
> On Mon, Nov 19, 2018 at 04:27:49PM -0800, Andy Lutomirski wrote:
> > On Mon, Nov 19, 2018 at 3:07 PM Tycho Andersen <tycho@tycho.ws> wrote:
> > > > These tools also care about ioctls. Adding a system call is a pain,
> > > > but the solution is to make adding system calls less of a pain, not to
> > > > permanently make the Linux ABI worse.
> > >
> > > For user-defined values of "worse" :)
> > >
> >
> > I tend to agree with Tycho here. But I'm wondering if it might be
> > worth considering a better ioctl.
> >
> > /me dons flame-proof hat
> >
> > We could do:
> >
> > long better_ioctl(int fd, u32 nr, const void *inbuf, size_t inlen,
> > const void *outbuf, size_t outlen);
>
> I'm the writer of this patch so take this with a grain of salt.
> I think it is a bad idea to stop this patch and force us to introduce a
> new type of ioctl().
I agree completely.
> An ioctl() is also not easy to use for this task because we want to add
> a siginfo_t argument which I actually think provides value and makes
> this interface more useful.
>
You could always have a struct procfd_kill and pass a pointer to
*that*. But sure, it's ugly.
^ permalink raw reply
* Re: [PATCH 0/3] Fix unsafe BPF_PROG_TEST_RUN interface
From: Daniel Borkmann @ 2018-11-20 0:34 UTC (permalink / raw)
To: Lorenz Bauer, ys114321; +Cc: Alexei Starovoitov, netdev, linux-api
In-Reply-To: <CACAyw98fu_=WufBrv_7CwO4iJj3jhOu1n6b3Pr0aZxqSk8++Gw@mail.gmail.com>
On 11/19/2018 03:30 PM, Lorenz Bauer wrote:
> On Sun, 18 Nov 2018 at 06:13, Y Song <ys114321@gmail.com> wrote:
>>
>> There is a slight change of user space behavior for this patch.
>> Without this patch, the value bpf_attr.test.data_size_out is output only.
>> For example,
>> output buffer : out_buf (user allocated size 10)
>> data_size_out is a random value (e.g., 1),
>>
>> The actual data to copy is 5.
>>
>> In today's implementation, the kernel will copy 5 and set data_size_out is 5.
>>
>> With this patch, the kernel will copy 1 and set data_size_out is 5.
>>
>> I am not 100% sure at this time whether we CAN overload data_size_out
>> since it MAY break existing applications.
>
> Yes, that's correct. I think that the likelihood of this is low. It would
> affect code that uses bpf_attr without zeroing it first. I had a look around,
> and I could only find code that would keep working:
Agree, it seems like this would be rather unlikely to break the old behavior
and only if some test app forgot to zero it (given data_size_out is also in
the middle and not at the end). I'd rather prefer this approach here and then
push the patch via stable than adding yet another data_size_out-like member.
I think it also makes sense to return a -ENOSPC as Yonghong suggested in order
to indicate to user space that the buffer is not sufficient. Right now this
would have no such indication to the user so it would not be possible to
distinguish whether truncation or not happened. Was thinking whether it makes
sense to indicate through a new flag member that buffer truncation happened,
but I do like -ENOSPC better.
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Daniel Colascione @ 2018-11-20 0:49 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Tycho Andersen, Christian Brauner, Eric W. Biederman,
linux-kernel, Serge E. Hallyn, Jann Horn, Andrew Morton,
Oleg Nesterov, Aleksa Sarai, Al Viro, Linux FS Devel, Linux API,
Tim Murray, linux-man, Kees Cook
In-Reply-To: <CALCETrWyuQvTtksN1J1XbCFPka_rLOaFqa5W==EvGQvoaf9f3Q@mail.gmail.com>
On Mon, Nov 19, 2018 at 4:28 PM Andy Lutomirski <luto@kernel.org> wrote:
>
> On Mon, Nov 19, 2018 at 3:07 PM Tycho Andersen <tycho@tycho.ws> wrote:
> > > These tools also care about ioctls. Adding a system call is a pain,
> > > but the solution is to make adding system calls less of a pain, not to
> > > permanently make the Linux ABI worse.
> >
> > For user-defined values of "worse" :)
> >
>
> I tend to agree with Tycho here. But I'm wondering if it might be
> worth considering a better ioctl.
>
> /me dons flame-proof hat
>
> We could do:
>
> long better_ioctl(int fd, u32 nr, const void *inbuf, size_t inlen,
> const void *outbuf, size_t outlen);
>
> and have a central table in the kernel listing all possible nr values
> along with which driver they belong to. We could have a sane
> signature and get rid of the nr collision problem.
The essential difference between a regular system call and an ioctl is
that in the former, the invoked kernel-side code depends on the
operation number, and in the latter, the invoked kernel-side code
depends on the operation number and file descriptor type. By creating
a new kind of collision-free ioctl, all you've done is re-invent the
system call, but with a funky calling convention and less operand
space. It makes no sense. Previous system call multiplexers --- e.g.,
socketcall --- are widely regarded as mistakes, and there's no reason
to repeat these mistakes.
System call numbers are not scarce, and your other proposal to clean
up the x86 numbering will make wiring up a new system call less
annoying. The *only* purpose of an ioctl is to solve the system call
numbering coordination problem --- if the invoked kernel-side code
depends on (DRIVER, OPERATION_NUMBER), and DRIVER can vary out-of-tree
with ioctl, ioctl lets out-of-tree code expose interfaces. For in-tree
code, this problem doesn't exist, so there's no reason to use the
awkward ioctl workaround!
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Eric W. Biederman @ 2018-11-20 4:59 UTC (permalink / raw)
To: Daniel Colascione
Cc: Christian Brauner, Aleksa Sarai, linux-kernel, Serge E. Hallyn,
Jann Horn, Andy Lutomirski, Andrew Morton, Oleg Nesterov, Al Viro,
Linux FS Devel, Linux API, Tim Murray, linux-man, Kees Cook
In-Reply-To: <CAKOZueup5L9Fd=tTK5g1415x0muLP_cPkxf4o26-kCPtH_caqw@mail.gmail.com>
Daniel Colascione <dancol@google.com> writes:
> On Mon, Nov 19, 2018 at 1:37 PM Christian Brauner <christian@brauner.io> wrote:
>>
>> On Mon, Nov 19, 2018 at 01:26:22PM -0800, Daniel Colascione wrote:
>> > On Mon, Nov 19, 2018 at 1:21 PM, Christian Brauner <christian@brauner.io> wrote:
>> > > That can be done without a loop by comparing the level counter for the
>> > > two pid namespaces.
>> > >
>> > >>
>> > >> And you can rewrite pidns_get_parent to use it. So you would instead be
>> > >> doing:
>> > >>
>> > >> if (pidns_is_descendant(proc_pid_ns, task_active_pid_ns(current)))
>> > >> return -EPERM;
>> > >>
>> > >> (Or you can just copy the 5-line loop into procfd_signal -- though I
>> > >> imagine we'll need this for all of the procfd_* APIs.)
>> >
>> > Why is any of this even necessary? Why does the child namespace we're
>> > considering even have a file descriptor to its ancestor's procfs? If
>>
>> Because you can send file descriptors between processes and container
>> runtimes tend to do that.
>
> Right. But why *would* a container runtime send one of these procfs
> FDs to a container?
>
>> > it has one of these FDs, it can already *read* all sorts of
>> > information it really shouldn't be able to acquire, so the additional
>> > ability to send a signal (subject to the usual permission checks)
>> > feels like sticking a finger in a dike that's already well-perforated.
>> > IMHO, we shouldn't bother with this check. The patch would be simpler
>> > without it.
>>
>> We will definitely not allow signaling processes in an ancestor pid
>> namespace! That is a security issue! I can imagine container runtimes
>> killing their monitoring process etc. pp. Not happening, unless someone
>> with deep expertise in signals can convince me otherwise.
>
> If parent namespace procfs FDs or mounts really can leak into child
> namespaces as easily as Aleksa says, then I don't mind adding the
> check. I was under the impression that if you find yourself in this
> situation, you already have a big problem.
There is one big reason to have the check, and I have not seen it
mentioned yet in this thread.
When SI_USER is set we report the pid of the sender of the signal in
si_pid. When the signal comes from the kernel si_pid == 0. When signal
is sent from an ancestor pid namespace si_pid also equals 0 (which is
reasonable).
A signal out to a process in a parent pid namespace such as SIGCHLD is
reasonable as we can map the pid. I really don't see the point of
forbidding that. From the perspective of the process in the parent pid
namespace it is just another process in it's pid namespace. So it
should pose no problem from the perspective of the receiving process.
A signal to a process in a pid namespace that is neither a parent nor a
descendent pid namespace would be a problem, as there is no well defined
notion of what si_pid should be set to. So for that case perhaps we
should have something like a noprocess pid that we can set. Perhaps we
could set si_pid to 0xffffffff. That would take a small extension to
pid_nr_ns.
File descriptors are not namespaced. It is completely legitimate to use
file descriptors to get around limitations of namespaces.
Adding limitations to a file descriptor based api because someone else
can't set up their processes in such a way as to get the restrictions
they are looking for seems very sad.
Frankly I think it is one of the better features of namespaces that we
have to carefully handle and define these cases so that when the
inevitable leaks happen you are not immediately in a world of hurt. All
of the other permission checks etc continue to do their job. Plus you
are prepared for the case when someone wants their containers to have an
interesting communication primitive.
Eric
^ permalink raw reply
* [PATCH -next 1/2] mm/memfd: make F_SEAL_FUTURE_WRITE seal more robust
From: Joel Fernandes (Google) @ 2018-11-20 5:21 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Andy Lutomirski, Andrew Morton,
Hugh Dickins, Jann Horn, Khalid Aziz, linux-api, linux-kselftest,
linux-mm, Marc-André Lureau, Matthew Wilcox, Mike Kravetz,
Shuah Khan, Stephen Rothwell
A better way to do F_SEAL_FUTURE_WRITE seal was discussed [1] last week
where we don't need to modify core VFS structures to get the same
behavior of the seal. This solves several side-effects pointed out by
Andy [2].
[1] https://lore.kernel.org/lkml/20181111173650.GA256781@google.com/
[2] https://lore.kernel.org/lkml/69CE06CC-E47C-4992-848A-66EB23EE6C74@amacapital.net/
Suggested-by: Andy Lutomirski <luto@kernel.org>
Fixes: 5e653c2923fd ("mm: Add an F_SEAL_FUTURE_WRITE seal to memfd")
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
fs/hugetlbfs/inode.c | 2 +-
mm/memfd.c | 19 -------------------
mm/shmem.c | 24 +++++++++++++++++++++---
3 files changed, 22 insertions(+), 23 deletions(-)
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 762028994f47..5b54bf893a67 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -558,7 +558,7 @@ static long hugetlbfs_punch_hole(struct inode *inode, loff_t offset, loff_t len)
inode_lock(inode);
/* protected by i_mutex */
- if (info->seals & F_SEAL_WRITE) {
+ if (info->seals & (F_SEAL_WRITE | F_SEAL_FUTURE_WRITE)) {
inode_unlock(inode);
return -EPERM;
}
diff --git a/mm/memfd.c b/mm/memfd.c
index 63fff5e77114..650e65a46b9c 100644
--- a/mm/memfd.c
+++ b/mm/memfd.c
@@ -201,25 +201,6 @@ static int memfd_add_seals(struct file *file, unsigned int seals)
}
}
- if ((seals & F_SEAL_FUTURE_WRITE) &&
- !(*file_seals & F_SEAL_FUTURE_WRITE)) {
- /*
- * The FUTURE_WRITE seal also prevents growing and shrinking
- * so we need them to be already set, or requested now.
- */
- int test_seals = (seals | *file_seals) &
- (F_SEAL_GROW | F_SEAL_SHRINK);
-
- if (test_seals != (F_SEAL_GROW | F_SEAL_SHRINK)) {
- error = -EINVAL;
- goto unlock;
- }
-
- spin_lock(&file->f_lock);
- file->f_mode &= ~(FMODE_WRITE | FMODE_PWRITE);
- spin_unlock(&file->f_lock);
- }
-
*file_seals |= seals;
error = 0;
diff --git a/mm/shmem.c b/mm/shmem.c
index 32eb29bd72c6..cee9878c87f1 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2121,6 +2121,23 @@ int shmem_lock(struct file *file, int lock, struct user_struct *user)
static int shmem_mmap(struct file *file, struct vm_area_struct *vma)
{
+ struct shmem_inode_info *info = SHMEM_I(file_inode(file));
+
+ /*
+ * New PROT_READ and MAP_SHARED mmaps are not allowed when "future
+ * write" seal active.
+ */
+ if ((vma->vm_flags & VM_SHARED) && (vma->vm_flags & VM_WRITE) &&
+ (info->seals & F_SEAL_FUTURE_WRITE))
+ return -EPERM;
+
+ /*
+ * Since the F_SEAL_FUTURE_WRITE seals allow for a MAP_SHARED read-only
+ * mapping, take care to not allow mprotect to revert protections.
+ */
+ if (info->seals & F_SEAL_FUTURE_WRITE)
+ vma->vm_flags &= ~(VM_MAYWRITE);
+
file_accessed(file);
vma->vm_ops = &shmem_vm_ops;
if (IS_ENABLED(CONFIG_TRANSPARENT_HUGE_PAGECACHE) &&
@@ -2346,8 +2363,9 @@ shmem_write_begin(struct file *file, struct address_space *mapping,
pgoff_t index = pos >> PAGE_SHIFT;
/* i_mutex is held by caller */
- if (unlikely(info->seals & (F_SEAL_WRITE | F_SEAL_GROW))) {
- if (info->seals & F_SEAL_WRITE)
+ if (unlikely(info->seals & (F_SEAL_GROW |
+ F_SEAL_WRITE | F_SEAL_FUTURE_WRITE))) {
+ if (info->seals & (F_SEAL_WRITE | F_SEAL_FUTURE_WRITE))
return -EPERM;
if ((info->seals & F_SEAL_GROW) && pos + len > inode->i_size)
return -EPERM;
@@ -2610,7 +2628,7 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,
DECLARE_WAIT_QUEUE_HEAD_ONSTACK(shmem_falloc_waitq);
/* protected by i_mutex */
- if (info->seals & F_SEAL_WRITE) {
+ if (info->seals & (F_SEAL_WRITE | F_SEAL_FUTURE_WRITE)) {
error = -EPERM;
goto out;
}
--
2.19.1.1215.g8438c0b245-goog
^ permalink raw reply related
* [PATCH -next 2/2] selftests/memfd: modify tests for F_SEAL_FUTURE_WRITE seal
From: Joel Fernandes (Google) @ 2018-11-20 5:21 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Jann Horn, Andrew Morton,
Andy Lutomirski, Hugh Dickins, Khalid Aziz, linux-api,
linux-kselftest, linux-mm, Marc-André Lureau, Matthew Wilcox,
Mike Kravetz, Shuah Khan, Stephen Rothwell
In-Reply-To: <20181120052137.74317-1-joel@joelfernandes.org>
Modify the tests for F_SEAL_FUTURE_WRITE based on the changes
introduced in previous patch.
Also add a test to make sure the reopen issue pointed by Jann Horn [1]
is fixed.
[1] https://lore.kernel.org/lkml/CAG48ez1h=v-JYnDw81HaYJzOfrNhwYksxmc2r=cJvdQVgYM+NA@mail.gmail.com/
Cc: Jann Horn <jannh@google.com>
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
tools/testing/selftests/memfd/memfd_test.c | 88 +++++++++++-----------
1 file changed, 44 insertions(+), 44 deletions(-)
diff --git a/tools/testing/selftests/memfd/memfd_test.c b/tools/testing/selftests/memfd/memfd_test.c
index 32b207ca7372..c67d32eeb668 100644
--- a/tools/testing/selftests/memfd/memfd_test.c
+++ b/tools/testing/selftests/memfd/memfd_test.c
@@ -54,6 +54,22 @@ static int mfd_assert_new(const char *name, loff_t sz, unsigned int flags)
return fd;
}
+static int mfd_assert_reopen_fd(int fd_in)
+{
+ int r, fd;
+ char path[100];
+
+ sprintf(path, "/proc/self/fd/%d", fd_in);
+
+ fd = open(path, O_RDWR);
+ if (fd < 0) {
+ printf("re-open of existing fd %d failed\n", fd_in);
+ abort();
+ }
+
+ return fd;
+}
+
static void mfd_fail_new(const char *name, unsigned int flags)
{
int r;
@@ -255,6 +271,25 @@ static void mfd_assert_read(int fd)
munmap(p, mfd_def_size);
}
+/* Test that PROT_READ + MAP_SHARED mappings work. */
+static void mfd_assert_read_shared(int fd)
+{
+ void *p;
+
+ /* verify PROT_READ and MAP_SHARED *is* allowed */
+ p = mmap(NULL,
+ mfd_def_size,
+ PROT_READ,
+ MAP_SHARED,
+ fd,
+ 0);
+ if (p == MAP_FAILED) {
+ printf("mmap() failed: %m\n");
+ abort();
+ }
+ munmap(p, mfd_def_size);
+}
+
static void mfd_assert_write(int fd)
{
ssize_t l;
@@ -698,7 +733,7 @@ static void test_seal_write(void)
*/
static void test_seal_future_write(void)
{
- int fd;
+ int fd, fd2;
void *p;
printf("%s SEAL-FUTURE-WRITE\n", memfd_str);
@@ -710,58 +745,23 @@ static void test_seal_future_write(void)
p = mfd_assert_mmap_shared(fd);
mfd_assert_has_seals(fd, 0);
- /* Not adding grow/shrink seals makes the future write
- * seal fail to get added
- */
- mfd_fail_add_seals(fd, F_SEAL_FUTURE_WRITE);
-
- mfd_assert_add_seals(fd, F_SEAL_GROW);
- mfd_assert_has_seals(fd, F_SEAL_GROW);
-
- /* Should still fail since shrink seal has
- * not yet been added
- */
- mfd_fail_add_seals(fd, F_SEAL_FUTURE_WRITE);
-
- mfd_assert_add_seals(fd, F_SEAL_SHRINK);
- mfd_assert_has_seals(fd, F_SEAL_GROW |
- F_SEAL_SHRINK);
- /* Now should succeed, also verifies that the seal
- * could be added with an existing writable mmap
- */
mfd_assert_add_seals(fd, F_SEAL_FUTURE_WRITE);
- mfd_assert_has_seals(fd, F_SEAL_SHRINK |
- F_SEAL_GROW |
- F_SEAL_FUTURE_WRITE);
+ mfd_assert_has_seals(fd, F_SEAL_FUTURE_WRITE);
/* read should pass, writes should fail */
mfd_assert_read(fd);
+ mfd_assert_read_shared(fd);
mfd_fail_write(fd);
- munmap(p, mfd_def_size);
- close(fd);
-
- /* Test adding all seals (grow, shrink, future write) at once */
- fd = mfd_assert_new("kern_memfd_seal_future_write2",
- mfd_def_size,
- MFD_CLOEXEC | MFD_ALLOW_SEALING);
-
- p = mfd_assert_mmap_shared(fd);
-
- mfd_assert_has_seals(fd, 0);
- mfd_assert_add_seals(fd, F_SEAL_SHRINK |
- F_SEAL_GROW |
- F_SEAL_FUTURE_WRITE);
- mfd_assert_has_seals(fd, F_SEAL_SHRINK |
- F_SEAL_GROW |
- F_SEAL_FUTURE_WRITE);
-
- /* read should pass, writes should fail */
- mfd_assert_read(fd);
- mfd_fail_write(fd);
+ fd2 = mfd_assert_reopen_fd(fd);
+ /* read should pass, writes should still fail */
+ mfd_assert_read(fd2);
+ mfd_assert_read_shared(fd2);
+ mfd_fail_write(fd2);
munmap(p, mfd_def_size);
+ close(fd2);
close(fd);
}
--
2.19.1.1215.g8438c0b245-goog
^ permalink raw reply related
* [PATCH -manpage 1/2] fcntl.2: Update manpage with new memfd F_SEAL_FUTURE_WRITE seal
From: Joel Fernandes (Google) @ 2018-11-20 5:25 UTC (permalink / raw)
To: linux-man
Cc: Joel Fernandes (Google), Andrew Morton, Andy Lutomirski,
Hugh Dickins, Jann Horn, John Stultz, linux-api, linux-kernel,
linux-kselftest, linux-mm, Marc-André Lureau, Matthew Wilcox,
Mike Kravetz, Shuah Khan, Stephen Rothwell
More details of the seal can be found in the LKML patch:
https://lore.kernel.org/lkml/20181120052137.74317-1-joel@joelfernandes.org/T/#t
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
man2/fcntl.2 | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/man2/fcntl.2 b/man2/fcntl.2
index 03533d65b49d..54772f94964c 100644
--- a/man2/fcntl.2
+++ b/man2/fcntl.2
@@ -1525,6 +1525,21 @@ Furthermore, if there are any asynchronous I/O operations
.RB ( io_submit (2))
pending on the file,
all outstanding writes will be discarded.
+.TP
+.BR F_SEAL_FUTURE_WRITE
+If this seal is set, the contents of the file can be modified only from
+existing writeable mappings that were created prior to the seal being set.
+Any attempt to create a new writeable mapping on the memfd via
+.BR mmap (2)
+will fail with
+.BR EPERM.
+Also any attempts to write to the memfd via
+.BR write (2)
+will fail with
+.BR EPERM.
+This is useful in situations where existing writable mapped regions need to be
+kept intact while preventing any future writes. For example, to share a
+read-only memory buffer to other processes that only the sender can write to.
.\"
.SS File read/write hints
Write lifetime hints can be used to inform the kernel about the relative
--
2.19.1.1215.g8438c0b245-goog
^ permalink raw reply related
* [PATCH -manpage 2/2] memfd_create.2: Update manpage with new memfd F_SEAL_FUTURE_WRITE seal
From: Joel Fernandes (Google) @ 2018-11-20 5:25 UTC (permalink / raw)
To: linux-man
Cc: Joel Fernandes (Google), Andrew Morton, Andy Lutomirski,
Hugh Dickins, Jann Horn, John Stultz, linux-api, linux-kernel,
linux-kselftest, linux-mm, Marc-André Lureau, Matthew Wilcox,
Mike Kravetz, Shuah Khan, Stephen Rothwell
In-Reply-To: <20181120052545.76560-1-joel@joelfernandes.org>
More details of the seal can be found in the LKML patch:
https://lore.kernel.org/lkml/20181120052137.74317-1-joel@joelfernandes.org/T/#t
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
man2/memfd_create.2 | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/man2/memfd_create.2 b/man2/memfd_create.2
index 3cd392d1b4d9..fce2bf8d0fff 100644
--- a/man2/memfd_create.2
+++ b/man2/memfd_create.2
@@ -280,7 +280,15 @@ in order to restrict further modifications on the file.
(If placing the seal
.BR F_SEAL_WRITE ,
then it will be necessary to first unmap the shared writable mapping
-created in the previous step.)
+created in the previous step. Otherwise, behavior similar to
+.BR F_SEAL_WRITE
+can be achieved, by using
+.BR F_SEAL_FUTURE_WRITE
+which will prevent future writes via
+.BR mmap (2)
+and
+.BR write (2)
+from succeeding, while keeping existing shared writable mappings).
.IP 4.
A second process obtains a file descriptor for the
.BR tmpfs (5)
@@ -425,6 +433,7 @@ main(int argc, char *argv[])
fprintf(stderr, "\\t\\tg \- F_SEAL_GROW\\n");
fprintf(stderr, "\\t\\ts \- F_SEAL_SHRINK\\n");
fprintf(stderr, "\\t\\tw \- F_SEAL_WRITE\\n");
+ fprintf(stderr, "\\t\\tW \- F_SEAL_FUTURE_WRITE\\n");
fprintf(stderr, "\\t\\tS \- F_SEAL_SEAL\\n");
exit(EXIT_FAILURE);
}
@@ -463,6 +472,8 @@ main(int argc, char *argv[])
seals |= F_SEAL_SHRINK;
if (strchr(seals_arg, \(aqw\(aq) != NULL)
seals |= F_SEAL_WRITE;
+ if (strchr(seals_arg, \(aqW\(aq) != NULL)
+ seals |= F_SEAL_FUTURE_WRITE;
if (strchr(seals_arg, \(aqS\(aq) != NULL)
seals |= F_SEAL_SEAL;
@@ -518,6 +529,8 @@ main(int argc, char *argv[])
printf(" GROW");
if (seals & F_SEAL_WRITE)
printf(" WRITE");
+ if (seals & F_SEAL_FUTURE_WRITE)
+ printf(" FUTURE_WRITE");
if (seals & F_SEAL_SHRINK)
printf(" SHRINK");
printf("\\n");
--
2.19.1.1215.g8438c0b245-goog
^ permalink raw reply related
* Re: [RFC PATCH] mm, proc: report PR_SET_THP_DISABLE in proc
From: Michal Hocko @ 2018-11-20 7: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.1811191404030.150313@chino.kir.corp.google.com>
On Mon 19-11-18 14:05:34, David Rientjes wrote:
> On Thu, 15 Nov 2018, Michal Hocko wrote:
>
> > > The userspace had a single way to determine if thp had been disabled for a
> > > specific vma and that was broken with your commit. We have since fixed
> > > it. Modifying our software stack to start looking for some field
> > > somewhere else will not help anybody else that this has affected or will
> > > affect. I'm interested in not breaking userspace, not trying a wait and
> > > see approach to see if anybody else complains once we start looking for
> > > some other field. The risk outweighs the reward, it already broke us, and
> > > I'd prefer not to even open the possibility of breaking anybody else.
> >
> > I very much agree on "do not break userspace" part but this is kind of
> > gray area. VMA flags are a deep internal implementation detail and
> > nobody should really depend on it for anything important. The original
> > motivation for introducing it was CRIU where it is kind of
> > understandable. I would argue they should find a different way but it is
> > just too late for them.
> >
> > For this particular case there was no other bug report except for yours
> > and if it is possible to fix it on your end then I would really love to
> > make the a sensible user interface to query the status. If we are going
> > to change the semantic of the exported flag again then we risk yet
> > another breakage.
> >
> > Therefore I am asking whether changing your particular usecase to a new
> > interface is possible because that would allow to have a longerm
> > sensible user interface rather than another kludge which still doesn't
> > cover all the usecases (e.g. there is no way to reliably query the
> > madvise status after your patch).
> >
>
> Providing another interface is great, I have no objection other than
> emitting another line for every vma on the system for smaps is probably
> overkill for something as rare as PR_SET_THP_DISABLE.
Let me think about a full patch and see how it looks like.
>
> That said, I think the current handling of the "nh" flag being emitted in
> smaps is logical and ensures no further userspace breakage.
I have already expressed a concern that there is no way to query for
MADV_NOHUGEPAGE if we overload the flag. So this is not a riskfree
option.
> If that is to
> be removed, I consider it an unnecessary risk. That would raised in code
> review.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RFC PATCH v6 01/26] Documentation/x86: Add CET description
From: Ingo Molnar @ 2018-11-20 9:52 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: <20181119214809.6086-2-yu-cheng.yu@intel.com>
* Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> +X86 Documentation
> +=======================
> +
> +Control-flow Enforcement
> +========================
> +
> +.. toctree::
> + :maxdepth: 1
> +
> + intel_cet
> diff --git a/Documentation/x86/intel_cet.rst b/Documentation/x86/intel_cet.rst
> new file mode 100644
> index 000000000000..dac83bbf8a24
> --- /dev/null
> +++ b/Documentation/x86/intel_cet.rst
> @@ -0,0 +1,268 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=========================================
> +Control-flow Enforcement Technology (CET)
> +=========================================
> +
> +[1] Overview
> +============
> +
> +Control-flow Enforcement Technology (CET) provides protection against
> +return/jump-oriented programming (ROP) attacks. It can be setup to
> +protect both the kernel and applications. In the first phase,
> +only the user-mode protection is implemented in 64-bit mode; 32-bit
> +applications are supported in compatibility mode.
> +
> +CET introduces shadow stack (SHSTK) and indirect branch tracking
> +(IBT). SHSTK is a secondary stack allocated from memory and cannot
> +be directly modified by applications. When executing a CALL, the
> +processor pushes a copy of the return address to SHSTK. Upon
> +function return, the processor pops the SHSTK copy and compares it
> +to the one from the program stack. If the two copies differ, the
> +processor raises a control-protection exception. IBT verifies all
> +indirect CALL/JMP targets are intended as marked by the compiler
> +with 'ENDBR' opcodes (see CET instructions below).
> +
> +There are two kernel configuration options:
> +
> + INTEL_X86_SHADOW_STACK_USER, and
> + INTEL_X86_BRANCH_TRACKING_USER.
> +
> +To build a CET-enabled kernel, Binutils v2.31 and GCC v8.1 or later
> +are required. To build a CET-enabled application, GLIBC v2.28 or
> +later is also required.
> +
> +There are two command-line options for disabling CET features:
> +
> + no_cet_shstk - disables SHSTK, and
> + no_cet_ibt - disables IBT.
> +
> +At run time, /proc/cpuinfo shows the availability of SHSTK and IBT.
What is the rough expected performance impact of CET on average function
call frequency user applications and the kernel itself?
Thanks,
Ingo
^ permalink raw reply
* Re: RFC: userspace exception fixups
From: Jarkko Sakkinen @ 2018-11-20 10:11 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Dave Hansen, Christopherson, Sean J, Jethro Beekman,
Florian Weimer, Linux API, Jann Horn, Linus Torvalds, X86 ML,
linux-arch, LKML, Peter Zijlstra, Rich Felker, nhorman,
npmccallum, Ayoun, Serge, shay.katz-zamir, linux-sgx,
Andy Shevchenko, Thomas Gleixner, Ingo Molnar, Borislav Petkov
In-Reply-To: <CALCETrWRVgPjnDd-CknskrXfNbke=rSnr_jezszNNWJUM_YJKQ@mail.gmail.com>
On Mon, Nov 19, 2018 at 09:00:08AM -0800, Andy Lutomirski wrote:
> On Mon, Nov 19, 2018 at 8:02 AM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Mon, Nov 19, 2018 at 07:29:36AM -0800, Andy Lutomirski wrote:
> > > 1. The kernel needs some way to know *when* to apply this fixup.
> > > Decoding the instruction stream and doing it to all exceptions that
> > > hit an ENCLU instruction seems like a poor design.
> >
> > I'm not sure why you would ever need to do any type of fixup as the idea
> > is to just return to AEP i.e. from chosen exceptions (EPCM, #UD) the AEP
> > would work the same way as for exceptions that the kernel can deal with
> > except filling the exception information to registers.
>
> Sure, but how does the kernel know when to do that and when to send a
> signal? I don't really like decoding the instruction stream to figure
> it out.
Hmm... why you have to decode instruction stream to find that out? Would
just depend on exception type (#GP with EPCM, #UD). Or are you saying
that kernel should need to SIGSEGV if there is in fact ENCLU so that
there is no infinite trap loop? Sorry, I'm a bit lost here that where
does this decoding requirement comes from in the first place. I
understand how it is used in Sean's proposal...
Anyway, this option can be probably discarded without further
consideration because apparently single stepping can cause #DB SS fault
if AEP handler is anything else than a single instruction.
For me it seems that by ruling out options, vDSO option is what is
left. I don't like it but at least it works...
/Jarkko
^ 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