* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: Andy Lutomirski @ 2018-07-13 0:24 UTC (permalink / raw)
To: David Howells
Cc: Theodore Y. Ts'o, Linus Torvalds, Andrew Lutomirski, Al Viro,
Linux API, linux-fsdevel, Linux Kernel Mailing List, Jann Horn
In-Reply-To: <26064.1531440190@warthog.procyon.org.uk>
> On Jul 12, 2018, at 5:03 PM, David Howells <dhowells@redhat.com> wrote:
>
> Andy Lutomirski <luto@amacapital.net> wrote:
>
>>>> I tend to think that this *should* fail using the new API. The semantics
>>>> of the second mount request are bizarre at best.
>>>
>>> You still have to support existing behaviour lest you break userspace.
>>>
>>
>> I assume the existing behavior is that a bind mount is created? If so, the
>> new mount(8) tool could do it in user code.
>
> You have a race there.
>
> Also you can't currently directly create a bind mount from userspace as you
> can only bind from another path point - which you may not be able to access
> (either by permission failure or because it's not in your mount namespace).
>
Are you trying to preserve the magic bind semantics with the new API? If you are, I think it should be by explicit opt in only. Otherwise you risk having your shiny new way to specify fs options get ignored when the magic bind mount happens.
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: David Howells @ 2018-07-13 0:03 UTC (permalink / raw)
To: Andy Lutomirski
Cc: dhowells, Theodore Y. Ts'o, Linus Torvalds, Andrew Lutomirski,
Al Viro, Linux API, linux-fsdevel, Linux Kernel Mailing List,
Jann Horn
In-Reply-To: <3236C75A-5D74-4BB4-A1EC-06F6E22D810C@amacapital.net>
Andy Lutomirski <luto@amacapital.net> wrote:
> >> I tend to think that this *should* fail using the new API. The semantics
> >> of the second mount request are bizarre at best.
> >
> > You still have to support existing behaviour lest you break userspace.
> >
>
> I assume the existing behavior is that a bind mount is created? If so, the
> new mount(8) tool could do it in user code.
You have a race there.
Also you can't currently directly create a bind mount from userspace as you
can only bind from another path point - which you may not be able to access
(either by permission failure or because it's not in your mount namespace).
David
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: Andy Lutomirski @ 2018-07-12 23:50 UTC (permalink / raw)
To: David Howells
Cc: Theodore Y. Ts'o, Linus Torvalds, Andrew Lutomirski, Al Viro,
Linux API, linux-fsdevel, Linux Kernel Mailing List, Jann Horn
In-Reply-To: <23894.1531438559@warthog.procyon.org.uk>
> On Jul 12, 2018, at 4:35 PM, David Howells <dhowells@redhat.com> wrote:
>
> Andy Lutomirski <luto@amacapital.net> wrote:
>
>> I tend to think that this *should* fail using the new API. The semantics of
>> the second mount request are bizarre at best.
>
> You still have to support existing behaviour lest you break userspace.
>
I assume the existing behavior is that a bind mount is created? If so, the new mount(8) tool could do it in user code.
^ permalink raw reply
* Re: [RFC PATCH v2 18/27] x86/cet/shstk: Introduce WRUSS instruction
From: Dave Hansen @ 2018-07-12 23:49 UTC (permalink / raw)
To: Yu-cheng Yu, 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,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek,
Peter Zijlstra <pet>
In-Reply-To: <1531436398.2965.18.camel@intel.com>
On 07/12/2018 03:59 PM, Yu-cheng Yu wrote:
> On Tue, 2018-07-10 at 16:48 -0700, Dave Hansen wrote:
>>>
>>> +/*
>>> + * WRUSS is a kernel instrcution and but writes to user
>>> + * shadow stack memory. When a fault occurs, both
>>> + * X86_PF_USER and X86_PF_SHSTK are set.
>>> + */
>>> +static int is_wruss(struct pt_regs *regs, unsigned long error_code)
>>> +{
>>> + return (((error_code & (X86_PF_USER | X86_PF_SHSTK)) ==
>>> + (X86_PF_USER | X86_PF_SHSTK)) && !user_mode(regs));
>>> +}
>> I thought X86_PF_USER was set based on the mode in which the fault
>> occurred. Does this mean that the architecture of this bit is different
>> now?
>
> Yes.
>
>> That seems like something we need to call out if so. It also means we
>> need to update the SDM because some of the text is wrong.
>
> It needs to mention the WRUSS case.
Ugh. The documentation for this is not pretty. But, I guess this is
not fundamentally different from access to U=1 pages when SMAP is in
place and we've set EFLAGS.AC=1.
But, sheesh, we need to call this out really explicitly and make it
crystal clear what is going on.
We need to go through the page fault code very carefully and audit all
the X86_PF_USER spots and make sure there's no impact to those. SMAP
should mean that we already dealt with these, but we still need an audit.
The docs[1] are clear as mud on this though: "Page entry has user
privilege (U=1) for a supervisor-level shadow-stack-load,
shadow-stack-store-intent or shadow-stack-store access except those that
originate from the WRUSS instruction."
Or, in short:
"Page has U=1 ... except those that originate from the WRUSS
instruction."
Which is backwards from what you said. I really wish those docs had
reused the established SDM language instead of reinventing their own way
of saying things.
1.
https://software.intel.com/sites/default/files/managed/4d/2a/control-flow-enforcement-technology-preview.pdf
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: David Howells @ 2018-07-12 23:35 UTC (permalink / raw)
To: Andy Lutomirski
Cc: dhowells, Theodore Y. Ts'o, Linus Torvalds, Andrew Lutomirski,
Al Viro, Linux API, linux-fsdevel, Linux Kernel Mailing List,
Jann Horn
In-Reply-To: <611054C7-D6E8-4C89-958E-3128C9305E1E@amacapital.net>
Andy Lutomirski <luto@amacapital.net> wrote:
> I tend to think that this *should* fail using the new API. The semantics of
> the second mount request are bizarre at best.
You still have to support existing behaviour lest you break userspace.
David
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: Jann Horn @ 2018-07-12 23:33 UTC (permalink / raw)
To: David Howells
Cc: Theodore Y. Ts'o, Linus Torvalds, Andy Lutomirski, Al Viro,
Linux API, linux-fsdevel, kernel list
In-Reply-To: <CAG48ez1OF0OfdmnRsV5Ne2mOt-j3Pgf7QRsn3q3Hzjpus9LHGA@mail.gmail.com>
On Thu, Jul 12, 2018 at 4:23 PM Jann Horn <jannh@google.com> wrote:
>
> On Thu, Jul 12, 2018 at 3:54 PM David Howells <dhowells@redhat.com> wrote:
> >
> > Theodore Y. Ts'o <tytso@mit.edu> wrote:
> >
> > > So maybe the answer is that you open /dev/sda1 and /dev/sda2 and then
> > > pass the file descriptors to the fsopen object? We can require that
> > > the fd's be opened with O_RDWR and O_EXCL, which has the benefit where
> > > if you have multiple block devices, you know *which* block device had
> > > a problem with being grabbed for an exclusive open.
> >
> > Would that mean then that doing:
> >
> > mount /dev/sda3 /a
> > mount /dev/sda3 /b
> >
> > would then fail on the second command because /dev/sda3 is already open
> > exclusively?
>
> Not exactly. mount_bdev() uses FMODE_EXCL, which locks out parallel
> usage *with a different filesystem type*. This is the effect:
>
> # strace -e trace=mount mount -t vfat /dev/loop0 mount
> mount("/dev/loop0", "/home/jannh/tmp/x/mount", "vfat", MS_MGC_VAL, NULL) = 0
> +++ exited with 0 +++
> # strace -e trace=mount mount -t ext4 /dev/loop0 mount
> mount("/dev/loop0", "/home/jannh/tmp/x/mount", "ext4", MS_MGC_VAL,
> NULL) = -1 EBUSY (Device or resource busy)
> mount: /home/jannh/tmp/x/mount: /dev/loop0 already mounted on
> /home/jannh/tmp/x/mount.
> +++ exited with 32 +++
>
> I don't really understand why it's not more strict though...
Er, sorry, of course that's the current behavior, not the behavior of
the suggested API.
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: Jann Horn @ 2018-07-12 23:23 UTC (permalink / raw)
To: David Howells
Cc: Theodore Y. Ts'o, Linus Torvalds, Andy Lutomirski, Al Viro,
Linux API, linux-fsdevel, kernel list
In-Reply-To: <22105.1531436081@warthog.procyon.org.uk>
On Thu, Jul 12, 2018 at 3:54 PM David Howells <dhowells@redhat.com> wrote:
>
> Theodore Y. Ts'o <tytso@mit.edu> wrote:
>
> > So maybe the answer is that you open /dev/sda1 and /dev/sda2 and then
> > pass the file descriptors to the fsopen object? We can require that
> > the fd's be opened with O_RDWR and O_EXCL, which has the benefit where
> > if you have multiple block devices, you know *which* block device had
> > a problem with being grabbed for an exclusive open.
>
> Would that mean then that doing:
>
> mount /dev/sda3 /a
> mount /dev/sda3 /b
>
> would then fail on the second command because /dev/sda3 is already open
> exclusively?
Not exactly. mount_bdev() uses FMODE_EXCL, which locks out parallel
usage *with a different filesystem type*. This is the effect:
# strace -e trace=mount mount -t vfat /dev/loop0 mount
mount("/dev/loop0", "/home/jannh/tmp/x/mount", "vfat", MS_MGC_VAL, NULL) = 0
+++ exited with 0 +++
# strace -e trace=mount mount -t ext4 /dev/loop0 mount
mount("/dev/loop0", "/home/jannh/tmp/x/mount", "ext4", MS_MGC_VAL,
NULL) = -1 EBUSY (Device or resource busy)
mount: /home/jannh/tmp/x/mount: /dev/loop0 already mounted on
/home/jannh/tmp/x/mount.
+++ exited with 32 +++
I don't really understand why it's not more strict though...
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: Andy Lutomirski @ 2018-07-12 23:21 UTC (permalink / raw)
To: David Howells
Cc: Theodore Y. Ts'o, Linus Torvalds, Andrew Lutomirski, Al Viro,
Linux API, linux-fsdevel, Linux Kernel Mailing List, Jann Horn
In-Reply-To: <22105.1531436081@warthog.procyon.org.uk>
> On Jul 12, 2018, at 3:54 PM, David Howells <dhowells@redhat.com> wrote:
>
> Theodore Y. Ts'o <tytso@mit.edu> wrote:
>
>> So maybe the answer is that you open /dev/sda1 and /dev/sda2 and then
>> pass the file descriptors to the fsopen object? We can require that
>> the fd's be opened with O_RDWR and O_EXCL, which has the benefit where
>> if you have multiple block devices, you know *which* block device had
>> a problem with being grabbed for an exclusive open.
>
> Would that mean then that doing:
>
> mount /dev/sda3 /a
> mount /dev/sda3 /b
>
> would then fail on the second command because /dev/sda3 is already open
> exclusively?
>
I tend to think that this *should* fail using the new API. The semantics of the second mount request are bizarre at best.
^ permalink raw reply
* Re: [RFC PATCH v2 25/27] x86/cet: Add PTRACE interface for CET
From: Thomas Gleixner @ 2018-07-12 23:08 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: Ingo Molnar, x86, H. Peter Anvin, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek,
Peter Zijlstra
In-Reply-To: <1531435034.2965.15.camel@intel.com>
[-- Attachment #1: Type: text/plain, Size: 1452 bytes --]
On Thu, 12 Jul 2018, Yu-cheng Yu wrote:
> On Thu, 2018-07-12 at 16:03 +0200, Ingo Molnar wrote:
> > * Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> > > > > diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
> > > > > index e2ee403865eb..ac2bc3a18427 100644
> > > > > --- a/arch/x86/kernel/ptrace.c
> > > > > +++ b/arch/x86/kernel/ptrace.c
> > > > > @@ -49,7 +49,9 @@ enum x86_regset {
> > > > > REGSET_IOPERM64 = REGSET_XFP,
> > > > > REGSET_XSTATE,
> > > > > REGSET_TLS,
> > > > > + REGSET_CET64 = REGSET_TLS,
> > > > > REGSET_IOPERM32,
> > > > > + REGSET_CET32,
> > > > > };
> > > > Why does REGSET_CET64 alias on REGSET_TLS?
> > > In x86_64_regsets[], there is no [REGSET_TLS]. The core dump code
> > > cannot handle holes in the array.
> > Is there a fundamental (ABI) reason for that?
>
> What I did was, ran Linux with 'slub_debug', and forced a core dump
> (kill -abrt <pid>), then there was a red zone warning in the dmesg.
> My feeling is there could be issues in the core dump code. These
Kernel development is not about feelings.
Either you can track down the root cause or you cannot. There is no place
for feelings and no place in between. And if you cannot track down the root
cause and explain it proper then the resulting patch is just papering over
the symptoms and will come back to hunt you (or others) sooner than later.
No if, no could, no feelings. Facts is what matters. Really.
Thanks,
tglx
^ permalink raw reply
* Re: [RFC PATCH v2 18/27] x86/cet/shstk: Introduce WRUSS instruction
From: Yu-cheng Yu @ 2018-07-12 22:59 UTC (permalink / raw)
To: Dave Hansen, 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,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <bbb487cc-ac1c-f734-eee3-2463a0ba7efc@linux.intel.com>
On Tue, 2018-07-10 at 16:48 -0700, Dave Hansen wrote:
> >
> > +/*
> > + * WRUSS is a kernel instrcution and but writes to user
> > + * shadow stack memory. When a fault occurs, both
> > + * X86_PF_USER and X86_PF_SHSTK are set.
> > + */
> > +static int is_wruss(struct pt_regs *regs, unsigned long error_code)
> > +{
> > + return (((error_code & (X86_PF_USER | X86_PF_SHSTK)) ==
> > + (X86_PF_USER | X86_PF_SHSTK)) && !user_mode(regs));
> > +}
> I thought X86_PF_USER was set based on the mode in which the fault
> occurred. Does this mean that the architecture of this bit is different
> now?
Yes.
> That seems like something we need to call out if so. It also means we
> need to update the SDM because some of the text is wrong.
It needs to mention the WRUSS case.
>
> >
> > static void
> > show_fault_oops(struct pt_regs *regs, unsigned long error_code,
> > unsigned long address)
> > @@ -848,7 +859,7 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
> > struct task_struct *tsk = current;
> >
> > /* User mode accesses just cause a SIGSEGV */
> > - if (error_code & X86_PF_USER) {
> > + if ((error_code & X86_PF_USER) && !is_wruss(regs, error_code)) {
> > /*
> > * It's possible to have interrupts off here:
> > */
> This needs commenting about why is_wruss() is special.
Ok.
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: David Howells @ 2018-07-12 22:54 UTC (permalink / raw)
To: Theodore Y. Ts'o
Cc: dhowells, Linus Torvalds, Andrew Lutomirski, Al Viro, Linux API,
linux-fsdevel, Linux Kernel Mailing List, Jann Horn
In-Reply-To: <20180712223223.GA28610@thunk.org>
Theodore Y. Ts'o <tytso@mit.edu> wrote:
> So maybe the answer is that you open /dev/sda1 and /dev/sda2 and then
> pass the file descriptors to the fsopen object? We can require that
> the fd's be opened with O_RDWR and O_EXCL, which has the benefit where
> if you have multiple block devices, you know *which* block device had
> a problem with being grabbed for an exclusive open.
Would that mean then that doing:
mount /dev/sda3 /a
mount /dev/sda3 /b
would then fail on the second command because /dev/sda3 is already open
exclusively?
David
^ permalink raw reply
* Re: [RFC PATCH v2 25/27] x86/cet: Add PTRACE interface for CET
From: Yu-cheng Yu @ 2018-07-12 22:37 UTC (permalink / raw)
To: Ingo Molnar
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <20180712140327.GA7810@gmail.com>
On Thu, 2018-07-12 at 16:03 +0200, Ingo Molnar wrote:
> * Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
> >
> > >
> > > >
> > > > diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
> > > > index e2ee403865eb..ac2bc3a18427 100644
> > > > --- a/arch/x86/kernel/ptrace.c
> > > > +++ b/arch/x86/kernel/ptrace.c
> > > > @@ -49,7 +49,9 @@ enum x86_regset {
> > > > REGSET_IOPERM64 = REGSET_XFP,
> > > > REGSET_XSTATE,
> > > > REGSET_TLS,
> > > > + REGSET_CET64 = REGSET_TLS,
> > > > REGSET_IOPERM32,
> > > > + REGSET_CET32,
> > > > };
> > > Why does REGSET_CET64 alias on REGSET_TLS?
> > In x86_64_regsets[], there is no [REGSET_TLS]. The core dump code
> > cannot handle holes in the array.
> Is there a fundamental (ABI) reason for that?
What I did was, ran Linux with 'slub_debug', and forced a core dump
(kill -abrt <pid>), then there was a red zone warning in the dmesg.
My feeling is there could be issues in the core dump code. These
enum's are only local to arch/x86/kernel/ptrace.c and not exported.
I am not aware this is in the ABI.
>
> >
> > >
> > > to "CET" (which is a well-known acronym for "Central European Time"),
> > > not to CFE?
> > >
> > I don't know if I can change that, will find out.
> So what I'd suggest is something pretty simple: to use CFT/cft in kernel internal
> names, except for the Intel feature bit and any MSR enumeration which can be CET
> if Intel named it that way, and a short comment explaining the acronym difference.
>
> Or something like that.
Ok, I will make changes in the next version and probably revise
from that if still not optimal.
Yu-cheng
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: Theodore Y. Ts'o @ 2018-07-12 22:32 UTC (permalink / raw)
To: David Howells
Cc: Linus Torvalds, Andrew Lutomirski, Al Viro, Linux API,
linux-fsdevel, Linux Kernel Mailing List, Jann Horn
In-Reply-To: <18233.1531430797@warthog.procyon.org.uk>
On Thu, Jul 12, 2018 at 10:26:37PM +0100, David Howells wrote:
> The problem is that there's more than one actual "open" involved.
>
> fd = fsopen("ext4"); <--- #1
> whatever_interface(fd, "s /dev/sda1");
> whatever_interface(fd, "o journal_path=/dev/sda2");
> do_the_create_thing(fd); <--- #2 and #3
>
> The initial check to see whether you can mount or not is done at #1.
>
> But later there are two nested file opens. Internally, deep down inside the
> block layer, /dev/sda1 and /dev/sda2 are opened and further permissions checks
> are done, whether you like it or not. But these have no access to the creds
> attached to fd as things currently stand.
So maybe the answer is that you open /dev/sda1 and /dev/sda2 and then
pass the file descriptors to the fsopen object? We can require that
the fd's be opened with O_RDWR and O_EXCL, which has the benefit where
if you have multiple block devices, you know *which* block device had
a problem with being grabbed for an exclusive open.
Just a thought.
- Ted
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: Linus Torvalds @ 2018-07-12 21:40 UTC (permalink / raw)
To: David Howells
Cc: Andrew Lutomirski, Al Viro, Linux API, linux-fsdevel,
Linux Kernel Mailing List, Jann Horn
In-Reply-To: <18233.1531430797@warthog.procyon.org.uk>
On Thu, Jul 12, 2018 at 2:26 PM David Howells <dhowells@redhat.com> wrote:
>
> The problem is that there's more than one actual "open" involved.
No. The problem is "write()".
This is not about open, about fsopen, or about anything at all.
This is about the fact that "write()" by definition can happen in a
different - and unexpected - context. Whether that be due to suid or
due to splice, or due to any other random issue is entirely
immaterial.
(The same is true of "read()" too, but very few people try to make
"read()" have side effects, so it's less of an issue. It does happen,
though).
But once you have another interface than "read/write()", the issues go
away. Those other interfaces are synchronous, and now you can decide
"ok, I'll just use current creds".
> (1) Pass the creds from ->get_tree() all the way down into pathwalk and make
> sure *every* check that pathwalk does uses it.
No. See above.
If your write() does anything but buffering data, it's not getting merged.
> (2) When do_the_create_thing() is invoked, it wraps the call to ->get_tree()
> with override_creds(file->f_cred).
No.
We do not wrap creds in any case. It's just asking for *another* kind
of security issue, where you fool some higher-security thing into
giving you access because it wrapped the higher-security case instead.
> (3) Forget using an fd to refer to the context. fsopen() takes absolutely
> everything, perhaps as a kv array and spits out an O_PATH fd.
That works.
Or you know - do what I told you to do ALL THE TIME, which was to not
use write(), or to only buffer things with write().
But yes, any option that simply avoids read and write is fine.
You can even have a file descriptor. We already have file descriptors
that cannot be read from or written to. It's quite common for special
devices, the whole "open /dev/floppy with O_NONBLOCK only to be able
to do control operations with it" goes back to pretty much day #1.
More recently, we have the whole "FMODE_PATH" kind of file descriptor,
which works as a directory entry, but not for read and write.
So file descriptors can have very useful properties.
But no. We do not use "write()" to implement actions. If you think you
need to check permissions and think you need a "cred", then you're not
using write(). It really is that simple.
Not using write just avouds *all* the problems. If you can fool a suid
application to do arbitrary system calls for you, then it's not the
system call that is the security problem.
Linus
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: Linus Torvalds @ 2018-07-12 21:29 UTC (permalink / raw)
To: David Howells
Cc: Andy Lutomirski, Andrew Lutomirski, Al Viro, Linux API,
linux-fsdevel, Linux Kernel Mailing List, Jann Horn,
Tycho Andersen
In-Reply-To: <17749.1531429218@warthog.procyon.org.uk>
On Thu, Jul 12, 2018 at 2:00 PM David Howells <dhowells@redhat.com> wrote:
>
>
> for example:
>
> fd = fsopen("ext4", FSOPEN_CLOEXEC);
> fsconfig(fd, fsconfig_blockdev, "dev.data", "/dev/sda1", ...);
> fsconfig(fd, fsconfig_blockdev, "dev.journal", "/dev/sda2", ...);
Ok, that looks good to me. It also avoids the parsing issue with using
an interface like "write()", where the expectation is that you can
append things etc.
Linus
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: David Howells @ 2018-07-12 21:26 UTC (permalink / raw)
To: Linus Torvalds
Cc: dhowells, Andrew Lutomirski, Al Viro, Linux API, linux-fsdevel,
Linux Kernel Mailing List, Jann Horn
In-Reply-To: <CA+55aFw1WyKY+-2dg62M-QT_F0-waCTCrba1DW4dStYz-f_fjA@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> wrote:
> The unix semantics are that credentials are checked at open time.
Sigh.
The problem is that there's more than one actual "open" involved.
fd = fsopen("ext4"); <--- #1
whatever_interface(fd, "s /dev/sda1");
whatever_interface(fd, "o journal_path=/dev/sda2");
do_the_create_thing(fd); <--- #2 and #3
The initial check to see whether you can mount or not is done at #1.
But later there are two nested file opens. Internally, deep down inside the
block layer, /dev/sda1 and /dev/sda2 are opened and further permissions checks
are done, whether you like it or not. But these have no access to the creds
attached to fd as things currently stand.
So we have three choices:
(1) Pass the creds from ->get_tree() all the way down into pathwalk and make
sure *every* check that pathwalk does uses it.
(2) When do_the_create_thing() is invoked, it wraps the call to ->get_tree()
with override_creds(file->f_cred).
(3) Forget using an fd to refer to the context. fsopen() takes absolutely
everything, perhaps as a kv array and spits out an O_PATH fd. You don't
get improved error reporting, you don't get a chance for interaction -
say with the server, to construct an ID mapping table - and you don't get
the chance to query the superblock before creating a mount.
So, something like:
struct fsopen_param {
unsigned int type,
const char *key;
const void *val;
unsigned int val_len;
};
mfd = fsopen(const char *fs_type,
unsigned int flags, /* CLOEXEC */
const struct fsopen_param *params,
unsigned int param_count,
unsigned int ms_flags /* eg. MNT_NOEXEC */);
For example:
struct fsopen_param params[] = {
{ fsopen_source, "dev.fs", "/dev/sda1" }
{ fsopen_source, "dev.journal", "/dev/sda2" }
{ fsopen_option, "user_xattr" }
{ fsopen_option, "data", "journal" }
{ fsopen_option, "jqfmt", "vfsv1" }
{ fsopen_security, "selinux.context", "unconfined_u..." }
};
mfd = fsopen("ext4", FSOPEN_CLOEXEC, params, ARRAY_SIZE(params),
MNT_NOEXEC);
There would need to be an fsreconfig() also in a similar vein.
David
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: David Howells @ 2018-07-12 21:00 UTC (permalink / raw)
To: Andy Lutomirski
Cc: dhowells, Andy Lutomirski, Al Viro, Linux API, Linux FS Devel,
Linus Torvalds, LKML, Jann Horn, tycho
In-Reply-To: <338BC3C4-F3E7-48F0-A82E-2C7295B6640E@amacapital.net>
Andy Lutomirski <luto@amacapital.net> wrote:
> fsconfigure(contextfd, ADD_BLOCKDEV, dfd, path, flags);
>
> fsconfigure(contextfd, ADD_OPTION, 0, “foo=bar”, flags);
That seems okayish. I'm not sure we need the flags, but I do want to allow
for binary data in an option. So perhaps something like:
int fsconfig(int fd, unsigned int type,
const char *key, const void *val, size_t val_len);
for example:
fd = fsopen("ext4", FSOPEN_CLOEXEC);
fsconfig(fd, fsconfig_blockdev, "dev.data", "/dev/sda1", ...);
fsconfig(fd, fsconfig_blockdev, "dev.journal", "/dev/sda2", ...);
fsconfig(fd, fsconfig_option, "user_xattr", NULL, ...);
fsconfig(fd, fsconfig_option, "errors", "continue", ...);
fsconfig(fd, fsconfig_option, "data", "journal", ...);
fsconfig(fd, fsconfig_security, "selinux.context", "unconfined_u:...");
fsconfig(fd, fsconfig_create, NULL, NULL, 0);
mfd = fsmount(fd, FSMOUNT_CLOEXEC, MS_NOEXEC);
or:
fd = fsopen("nfs", FSOPEN_CLOEXEC);
fsconfig(fd, fsconfig_namespace, "user", "<usernsfd>", ...);
fsconfig(fd, fsconfig_namespace, "net", "<netnsfd>", ...);
fsconfig(fd, fsconfig_option, "server", "foo.com", ...);
fsconfig(fd, fsconfig_option, "root", "/bar", ...);
fsconfig(fd, fsconfig_option, "soft", NULL, ...);
fsconfig(fd, fsconfig_option, "retry", "3", ...);
fsconfig(fd, fsconfig_option, "wsize", "4096", ...);
fsconfig(fd, fsconfig_uidmap, "dhowells", "1234", ...);
fsconfig(fd, fsconfig_security, "selinux.context", "unconfined_u:...");
fsconfig(fd, fsconfig_create, NULL, NULL, 0);
mfd = fsmount(fd, FSMOUNT_CLOEXEC, MS_NOEXEC);
This does mean that userspace has to work harder, though, but it would
simplify the LSM interface internally.
Al Viro <viro@ftp.linux.org.uk>
> First of all, block device *IS* a fucking option.
Whilst that is true, I still need to be able to separate it out for
unconverted filesystems.
David
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: Linus Torvalds @ 2018-07-12 20:36 UTC (permalink / raw)
To: David Howells
Cc: Andrew Lutomirski, Al Viro, Linux API, linux-fsdevel,
Linux Kernel Mailing List, Jann Horn
In-Reply-To: <CA+55aFw1WyKY+-2dg62M-QT_F0-waCTCrba1DW4dStYz-f_fjA@mail.gmail.com>
On Thu, Jul 12, 2018 at 1:34 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> This is the whole "write() is only for data". If you ever have
> credentials mattering at write time, you're doing something wrong.
>
> Really really.
>
> Don't do it.
.. and I'd like to repeat: we *have* done things wrong. But that's
simply not an excuse. We've done it wrong in SCSI, we've done it wrong
in various /proc files, we've done it wrong in many places.
But let's not do it wrong AGAIN.
Linus
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: Linus Torvalds @ 2018-07-12 20:34 UTC (permalink / raw)
To: David Howells
Cc: Andrew Lutomirski, Al Viro, Linux API, linux-fsdevel,
Linux Kernel Mailing List, Jann Horn
In-Reply-To: <16699.1531426991@warthog.procyon.org.uk>
On Thu, Jul 12, 2018 at 1:23 PM David Howells <dhowells@redhat.com> wrote:
>
> It's all very well to say "use file->f_creds". The problem is this has to be
> handed down all the way through the filesystem and down into the block layer
> as appropriate to anywhere there's an LSM call, a CAP_* check or a pathwalk -
> but there's not currently any way to do that.
.. and the reason is simple: you damn well shouldn't do that.
The unix semantics are that credentials are checked at open time.
If your interface involves checking credentials at write() time, your
interface is garbage shit.
Really.
This is the whole "write() is only for data". If you ever have
credentials mattering at write time, you're doing something wrong.
Really really.
Don't do it.
Linus
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: Andy Lutomirski @ 2018-07-12 20:25 UTC (permalink / raw)
To: David Howells
Cc: Linus Torvalds, Andrew Lutomirski, Al Viro, Linux API,
linux-fsdevel, Linux Kernel Mailing List, Jann Horn
In-Reply-To: <16699.1531426991@warthog.procyon.org.uk>
> On Jul 12, 2018, at 1:23 PM, David Howells <dhowells@redhat.com> wrote:
>
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
>> Don't play games with override_creds. It's wrong.
>>
>> You have to use file->f_creds - no games, no garbage.
>
> You missed the point.
>
>
> My suggestion was to use override_creds() to impose the appropriate creds at
> the top, be that file->f_creds or fs_context->creds (they would be the same in
> any case).
I think it should be a new syscall and use current’s creds. No override needed.
> Btw, do we protect sysfs, debugfs, tracefs, procfs, etc. writes against
> splice? Some of the things in debugfs are really icky, allowing you to muck
> directly with hardware.
>
We try. It has been a perennial source of severe bugs.
This is part of why I’d like to see splice() be an opt in. Also, it’s a major step toward getting rid of set_fs().
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: David Howells @ 2018-07-12 20:23 UTC (permalink / raw)
To: Linus Torvalds
Cc: dhowells, Andrew Lutomirski, Al Viro, Linux API, linux-fsdevel,
Linux Kernel Mailing List, Jann Horn
In-Reply-To: <CA+55aFx+xm333ZZ7d2Z39QuhDu=0XM6nTMChbHvWQOXNk32yxw@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> wrote:
> Don't play games with override_creds. It's wrong.
>
> You have to use file->f_creds - no games, no garbage.
You missed the point.
It's all very well to say "use file->f_creds". The problem is this has to be
handed down all the way through the filesystem and down into the block layer
as appropriate to anywhere there's an LSM call, a CAP_* check or a pathwalk -
but there's not currently any way to do that.
mount_bdev() and blkdev_get_by_path() are examples of this. At the moment
there is no cred parameter there. We'd also have to pass the creds down into
path_init() to store in struct nameidata and make sure that every permissions
call that might be invoked during pathwalk in every filesystem uses that, not
current_cred().
I made an attempt to do this a while ago and the patch got rather large before
I gave up. In many ways, it's what we *should* do, but so many things need an
extra parameter... If you really want, I can try that again. It's possible I
can automate it with some perl scripting to parse the error messages from the
compiler.
My suggestion was to use override_creds() to impose the appropriate creds at
the top, be that file->f_creds or fs_context->creds (they would be the same in
any case).
If we want to go down the pass-the-creds-down route, then we can temporarily
do override_creds() until we've made the changes and then remove it later.
> But "write()" simply is *NOT* a good "command" interface. If you want
> to send a command, use an ioctl or a system call.
Okay.
> Because it's not just about credentials. It's not just about fooling a
> suid app into writing an error message to a descriptor you wrote. It's
> also about things like "splice()", which can write to your target
> using a kernel buffer, and thus trick you into doing a command while
> we have the context set to kernel addresses.
>
> Are we trying to get away from that issue? Yes. But it's just another
> example of why "write()" IS NOT TO BE USED FOR COMMANDS.
Btw, do we protect sysfs, debugfs, tracefs, procfs, etc. writes against
splice? Some of the things in debugfs are really icky, allowing you to muck
directly with hardware.
David
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: Greg KH @ 2018-07-12 19:08 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Al Viro, David Howells, linux-api, linux-fsdevel, torvalds,
linux-kernel
In-Reply-To: <76211EBC-05ED-4FBA-A136-F5EEBFDD9FDF@amacapital.net>
On Thu, Jul 12, 2018 at 11:30:32AM -0700, Andy Lutomirski wrote:
>
> > On Jul 12, 2018, at 11:03 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> >> On Thu, Jul 12, 2018 at 06:20:24PM +0100, Al Viro wrote:
> >>> On Thu, Jul 12, 2018 at 07:15:05PM +0200, Greg KH wrote:
> >>>> On Tue, Jul 10, 2018 at 11:44:09PM +0100, David Howells wrote:
> >>>> Provide an fsopen() system call that starts the process of preparing to
> >>>> create a superblock that will then be mountable, using an fd as a context
> >>>> handle. fsopen() is given the name of the filesystem that will be used:
> >>>>
> >>>> int mfd = fsopen(const char *fsname, unsigned int flags);
> >>>>
> >>>> where flags can be 0 or FSOPEN_CLOEXEC.
> >>>>
> >>>> For example:
> >>>>
> >>>> sfd = fsopen("ext4", FSOPEN_CLOEXEC);
> >>>> write(sfd, "s /dev/sdb1"); // note I'm ignoring write's length arg
> >>>> write(sfd, "o noatime");
> >>>> write(sfd, "o acl");
> >>>> write(sfd, "o user_attr");
> >>>> write(sfd, "o iversion");
> >>>> write(sfd, "o ");
> >>>> write(sfd, "r /my/container"); // root inside the fs
> >>>> write(sfd, "x create"); // create the superblock
> >>>
> >>> Ugh, creating configfs again in a syscall form? I know people love
> >>> file descriptors, but can't you do this with a configfs entry instead if
> >>> you really want to do this type of thing from userspace in this type of
> >>> "style"?
> >>>
> >>> Why reinvent the wheel again?
> >>
> >> The damn thing REALLY, REALLY depends upon the fs type. How would
> >> you map it on configfs?
> >
> > /sys/kernel/config/fs/ext4/ would work, right? Each fs "type" would be
> > listed there.
> >
> > Anyway, the whole "write a bunch of options and then do a 'create'" is
> > exactly the way configfs works. Why not use that?
> >
> >
>
> How do you mount configfs in the first place? And how do you use this
> in a mount namespace without a private configfs instance or where you
> don’t want configfs mounted?--
Ok, fair enough, I missed the part where this is going to replace
mount(2). Although you could just use mount(2) to mount configfs on a
mount point in the initramfs image and then go from there at boot time :)
/me runs away...
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: Al Viro @ 2018-07-12 18:35 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Greg KH, David Howells, linux-api, linux-fsdevel, torvalds,
linux-kernel
In-Reply-To: <20180712183426.GC30522@ZenIV.linux.org.uk>
On Thu, Jul 12, 2018 at 07:34:26PM +0100, Al Viro wrote:
> On Thu, Jul 12, 2018 at 11:30:32AM -0700, Andy Lutomirski wrote:
>
> Andi,
Apologies for misspelling - finger macros strike ;-/
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: Al Viro @ 2018-07-12 18:34 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Greg KH, David Howells, linux-api, linux-fsdevel, torvalds,
linux-kernel
In-Reply-To: <76211EBC-05ED-4FBA-A136-F5EEBFDD9FDF@amacapital.net>
On Thu, Jul 12, 2018 at 11:30:32AM -0700, Andy Lutomirski wrote:
Andi, Greg - alt.tasteless is over -> that way.
And for fsck sake, fix your MUA. Lines are obscenely long...
> How do you mount configfs in the first place? And how do you use this in a mount namespace without a private configfs instance or where you don’t want configfs mounted?
^ permalink raw reply
* Re: [PATCH 24/32] vfs: syscall: Add fsopen() to prepare for superblock creation [ver #9]
From: Andy Lutomirski @ 2018-07-12 18:30 UTC (permalink / raw)
To: Greg KH
Cc: Al Viro, David Howells, linux-api, linux-fsdevel, torvalds,
linux-kernel
In-Reply-To: <20180712180304.GA27758@kroah.com>
> On Jul 12, 2018, at 11:03 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>
>> On Thu, Jul 12, 2018 at 06:20:24PM +0100, Al Viro wrote:
>>> On Thu, Jul 12, 2018 at 07:15:05PM +0200, Greg KH wrote:
>>>> On Tue, Jul 10, 2018 at 11:44:09PM +0100, David Howells wrote:
>>>> Provide an fsopen() system call that starts the process of preparing to
>>>> create a superblock that will then be mountable, using an fd as a context
>>>> handle. fsopen() is given the name of the filesystem that will be used:
>>>>
>>>> int mfd = fsopen(const char *fsname, unsigned int flags);
>>>>
>>>> where flags can be 0 or FSOPEN_CLOEXEC.
>>>>
>>>> For example:
>>>>
>>>> sfd = fsopen("ext4", FSOPEN_CLOEXEC);
>>>> write(sfd, "s /dev/sdb1"); // note I'm ignoring write's length arg
>>>> write(sfd, "o noatime");
>>>> write(sfd, "o acl");
>>>> write(sfd, "o user_attr");
>>>> write(sfd, "o iversion");
>>>> write(sfd, "o ");
>>>> write(sfd, "r /my/container"); // root inside the fs
>>>> write(sfd, "x create"); // create the superblock
>>>
>>> Ugh, creating configfs again in a syscall form? I know people love
>>> file descriptors, but can't you do this with a configfs entry instead if
>>> you really want to do this type of thing from userspace in this type of
>>> "style"?
>>>
>>> Why reinvent the wheel again?
>>
>> The damn thing REALLY, REALLY depends upon the fs type. How would
>> you map it on configfs?
>
> /sys/kernel/config/fs/ext4/ would work, right? Each fs "type" would be
> listed there.
>
> Anyway, the whole "write a bunch of options and then do a 'create'" is
> exactly the way configfs works. Why not use that?
>
>
How do you mount configfs in the first place? And how do you use this in a mount namespace without a private configfs instance or where you don’t want configfs mounted?
^ 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