* Re: [PATCH v5 04/27] x86/fpu/xstate: Add XSAVES system states for shadow stack
From: Andy Lutomirski @ 2018-11-11 14:59 UTC (permalink / raw)
To: Pavel Machek
Cc: Borislav Petkov, Yu-cheng Yu, x86, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, linux-kernel, linux-doc, 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
In-Reply-To: <20181111113103.GG27666@amd>
> On Nov 11, 2018, at 3:31 AM, Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
>>> +/*
>>> + * State component 12 is Control flow Enforcement kernel states
>>> + */
>>> +struct cet_kernel_state {
>>> + u64 kernel_ssp; /* kernel shadow stack */
>>> + u64 pl1_ssp; /* ring-1 shadow stack */
>>> + u64 pl2_ssp; /* ring-2 shadow stack */
>>
>> Just write "privilege level" everywhere - not "ring".
>
> Please just use word "ring". It is well estabilished terminology.
>
> Which ring is priviledge level 1, given that we have SMM and
> virtualization support?
To the contrary: CPL, DPL, and RPL are very well defined terms in the architecture manuals. “PL” is privilege level. PL 1 is very well defined.
SMM is SMM, full stop (unless dual mode or whatever it’s called is on, but AFAIK no one uses it). VMX non-root CPL 1 is *still* privilege level 1.
In contrast, the security community likes to call SMM “ring -1”, which is cute, but wrong from a systems programmer view. For example, SMM’s CPL can still range from 0-3.
>
> Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Daniel Colascione @ 2018-11-11 14:22 UTC (permalink / raw)
To: Florian Weimer
Cc: Michael Kerrisk (man-pages), linux-kernel, Joel Fernandes,
Linux API, Willy Tarreau, Vlastimil Babka, Carlos O'Donell,
libc-alpha@sourceware.org
In-Reply-To: <877ehjx447.fsf@oldenburg.str.redhat.com>
On Sun, Nov 11, 2018 at 3:09 AM, Florian Weimer <fweimer@redhat.com> wrote:
> We had a patch for the membarrier system call, but the kernel developers
> could not tell us what the system call does in therms of the C/C++
> memory model
[snip]
> A lot of the new system calls lack clear specifications or are just
> somewhat misdesigned. For example, pkey_alloc
[snip]
> getrandom still causes boot delays
[snip]
> For copy_file_range, we still have debates whether the system call (and
> the glibc emulation) should preserve holes or not,
[snip]
These objections illustrate my point. glibc development is not the
proper forum for raising post-hoc objections to system call design.
Withholding wrappers will not un-ship these system calls. Applications
are already using them, via syscall(2). Developers and users would be
better served by providing access to the system as it is, with
appropriate documentation caveats, than by holding out for some
alternate and more ideal set of system calls that may or may not
appear in the future. This resistance to exposing the capabilities of
the system as they are, even in flawed and warty form, is what I meant
by "misplaced idealism" in my previous message. If the kernel provides
a system call, libc should provide a C wrapper for it, even if in the
opinion of the libc maintainers, that system call is flawed.
I agree with the proposals mentioned above to split system interface
responsibility, having glibc handle higher-level concerns like stdio
while punting system call wrappers and other low-level facilities to a
kernel-provided userspace library that can move faster and more
explicitly conform to the Linux kernel's userspace ABI.
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Willy Tarreau @ 2018-11-11 12:09 UTC (permalink / raw)
To: Florian Weimer
Cc: Michael Kerrisk (man-pages), Daniel Colascione, linux-kernel,
Joel Fernandes, Linux API, Vlastimil Babka, Carlos O'Donell,
libc-alpha@sourceware.org
In-Reply-To: <87zhufvntw.fsf@oldenburg.str.redhat.com>
On Sun, Nov 11, 2018 at 12:46:35PM +0100, Florian Weimer wrote:
> > In practice we all know (believe?) that both statements above are
> > true but in practice 99% of the time there's a 1:1 relation between
> > these two components.
>
> Eh. Most Linux systems do not run glibc at all (and use cryptography
> and other tricks to prevent users from installing it).
Good point on this one. I could even have thought that most syscalls
are added with glibc in mind but your counter-example above could
remain valid.
> > What we'd really need would be to have the libc
> > interface as part of the operating system itself. I'm perfectly fine
> > with glibc providing all the "high-level" stuff like strcpy(), FILE*
> > operations etc, and all this probably is mostly system-independent.
>
> That's a bit messy, unfortunately.
>
> The kernel does not know about TCB layout, so a lot of low-level
> threading aspects are defined by userspace.
>
> The kernel does not know about POSIX cancellation. Directly calling
> system calls breaks support for that.
>
> A lot of multi-threaded applications assume that most high-level
> functionality remains usable even after fork in a multi-threaded
> process. (This is increasingly a problem today with all those direct
> calls to clone.) Unfortunately, this introduces rather tricky
> low-level/high-level cross-subsystem issues, too.
But don't you think that moving a bit of this into the kernel
repository could improve the situation ? The corner cases could then
be detected when the feature is developed and be addressed either by
adapting the kernel side of the syscall or even by changing the design
before it's committed. Maybe a few extra syscalls are missing to
retrieve some critial info that would make things more reliable or
easier between userland and kernel, and that would become more obvious
with all the relevant parts at the same place ?
> > But the system interface could possibly be handled easier in the
> > system itself, which would also provide a smoother adoption of new
> > syscalls and API updates. It would also limit the hassle required to
> > provide new syscalls, as if you start to have to contribute to two
> > projects at once for a single syscall, it becomes really painful.
>
> Sure, the duplication is unfortunate.
>
> Several glibc contributors deeply care about standards compliance for
> header files.
For having suffered a lot from the libc-4 to libc-5 then libc-5 to glibc,
I certainly can understand their concerns about standards compliance.
> The kernel developers care not, and the result is that we
> copy definitions and declarations from the kernel header files, creating
> additional problems.
Probably that these standard compatibility issues should be addressed at
their root in the kernel header definitions in fact. Working around issues
always leads to a stall at some point, and it encourages the process not
to change.
> We also want to use old kernel headers to compile glibc and still
> implement features which are only defined by newer (upstream) kernels,
> so that leads to more duplication.
This one could possibly be got rid of. When I build glibc, I specify the
oldest supported kernel, which usually is older than or equal to the
headers used to build, but I don't expect that newer features will
magically work at all. Thus I normally build with the most recent
headers covering my needs.
Thanks,
Willy
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Florian Weimer @ 2018-11-11 12:07 UTC (permalink / raw)
To: Willy Tarreau
Cc: Michael Kerrisk (man-pages), Daniel Colascione, linux-kernel,
Joel Fernandes, Linux API, Vlastimil Babka, Carlos O'Donell,
libc-alpha@sourceware.org
In-Reply-To: <20181111110236.GA4189@1wt.eu>
* Willy Tarreau:
> On Sun, Nov 11, 2018 at 11:30:25AM +0100, Florian Weimer wrote:
>> * Willy Tarreau:
>>
>> > On Sun, Nov 11, 2018 at 07:55:30AM +0100, Michael Kerrisk (man-pages) wrote:
>> >> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=6399 is a
>> >> longstanding example.
>> >
>> > This one was a sad read and shows that applications will continue to
>> > suffer from glibc's prehistorical view on operating systems and will
>> > continue to have to define their own syscall wrappers to exploit the
>> > full potential of the modern operating systems they execute on.
>>
>> What's modern about a 15-bit thread identifier?
>
> It's 15-bit on 32-bit systems, and 22 on 64-bit, hence you can have
> 4 million threads and/or processes on a single system image provided
> you have the resources for that of course.
I believe the default for pid_max is still 32768.
>> I understand that using this interface is required in some cases (which
>> includes some system calls for which glibc does provide wrappers), but I
>> assumed that it was at least understood that these reusable IDs for
>> tasks were an extremely poor interface. Aren't the resulting bugs
>> common knowledge?
>
> Sure, just as are the bugs created by people trying to implement their
> own syscall wrappers. It's not by denying access to some native system
> interfaces that you will prevent users from accessing them, you'll just
> force them to work around the restriction and make things even worse.
Well, once we have the fixed interface, it becomes easier to use if we
only expose that, and not the confusing interface which is described in
countless Stackoverflow answers. More choice isn't always good.
>> > This reminds me when one had to write their own spinlocks and atomics
>> > many years ago. Seeing comments suggesting an application should open
>> > /proc/$PID makes me really wonder if people actually want to use slow
>> > and insecure applications designed this way.
>>
>> I don't understand. If you want a non-reusable identifier, you have to
>> go through the /proc interface anyway. I think the recommendation is to
>> use the PID/start time combination to get a unique process identifier or
>> something like that.
>
> It depends what you want to achieve. If you just need the tid, the one
> you'll pass to sched_setaffinity(), gettid() is fine.
You can use pthread_setaffinity_np to control the affinity mask of a
thread without knowing its TID, and you can call sched_setaffinity on
the current thread without knowing its TID anyway.
And for sched_setattr, you need to call syscall anyway because there is
no wrapper, so calling gettid via syscall isn't that bad. (We can't add
wrappers for sched_setattr because it's not entirely clear how the
userspace ABI will evolve in the future.)
> There are two issues
> with abusing /proc to emulate syscalls :
> - it's sometimes much slower than the equivalent syscall and can
> encourage users to cache the resulting values when they should not
> - either it's done upon process startup and it may not get valid value
> or may not work if /proc is not mounted yet (think init, mount etc),
> or it's done upon first use and can break daemons which chroot()
> themselves.
Sure, but many kernel developers prefer /proc and file-based interfaces.
See getumask for a particularly illuminating example.
> Syscalls don't have such limitations and are much safer to use. For other
> things it's quite possible that you cannot rely on this syscall at all,
> it's not a solution to everything, but it's a nice solution to all cases
> where you need to access the system-wide identifier to pin a thread to a
> given CPU set or renice it.
Again, you don't need gettid for that at all. glibc has covered this
fully.
Surely there is a better justification for using gettid?
I suspect quite a few calls to the gettid system calls could actually be
getpid, and the programmer used __NR_gettid instead of __NR_getpid to
bypass the glibc PID cache. But the cache isn't used by the syscall
code path anyway, so it really does not matter.
>> I wanted to add gettid to glibc this cycle, but your comments suggest to
>> me that if we did this, we'd likely never get a proper non-reusable
>> thread identifier from the kernel. So I'm not sure what do anymore.
>
> "Look people, I was about to do what we all refused to do for 10 years
> now and Willy's comment made me change my mind, I'm sorry". The *real*
> argument that most users could understand is "guys, we're sorry, but we
> are running out of time and we won't work on this low priority stuff,
> so someone else will have to take care of it".
I can assure you that in the past, a glibc patch for gettid would have
been rejected even if it were perfectly fine as far as the contribution
guidelines go (that is, copyright assignment, coding style, manual
update, ABI list update etc.). It's not a matter of resources or lack
thereof.
> In my opinion what matters is not whether or not people will use it
> appropriately, but that its validity, side effects and wrong assumptions
> are properly documented so that users don't shoot themselves in the foot.
Well, there I disagree. I think adding bad interfaces that confuse
developers is not a good idea, particularly if there is no compelling
use case. On the other hand, a userspace interface that is different
from what the kernel provides is confusing as well and leads to bugs
(see clone).
Thanks,
Florian
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Florian Weimer @ 2018-11-11 11:46 UTC (permalink / raw)
To: Willy Tarreau
Cc: Michael Kerrisk (man-pages), Daniel Colascione, linux-kernel,
Joel Fernandes, Linux API, Vlastimil Babka, Carlos O'Donell,
libc-alpha@sourceware.org
In-Reply-To: <20181111111143.GB4189@1wt.eu>
* Willy Tarreau:
> I think the issue is a bit more complex :
> - linux doesn't support a single libc
> - glibc doesn't support a single OS
>
> In practice we all know (believe?) that both statements above are
> true but in practice 99% of the time there's a 1:1 relation between
> these two components.
Eh. Most Linux systems do not run glibc at all (and use cryptography
and other tricks to prevent users from installing it).
> What we'd really need would be to have the libc
> interface as part of the operating system itself. I'm perfectly fine
> with glibc providing all the "high-level" stuff like strcpy(), FILE*
> operations etc, and all this probably is mostly system-independent.
That's a bit messy, unfortunately.
The kernel does not know about TCB layout, so a lot of low-level
threading aspects are defined by userspace.
The kernel does not know about POSIX cancellation. Directly calling
system calls breaks support for that.
A lot of multi-threaded applications assume that most high-level
functionality remains usable even after fork in a multi-threaded
process. (This is increasingly a problem today with all those direct
calls to clone.) Unfortunately, this introduces rather tricky
low-level/high-level cross-subsystem issues, too.
> But the system interface could possibly be handled easier in the
> system itself, which would also provide a smoother adoption of new
> syscalls and API updates. It would also limit the hassle required to
> provide new syscalls, as if you start to have to contribute to two
> projects at once for a single syscall, it becomes really painful.
Sure, the duplication is unfortunate.
Several glibc contributors deeply care about standards compliance for
header files. The kernel developers care not, and the result is that we
copy definitions and declarations from the kernel header files, creating
additional problems.
We also want to use old kernel headers to compile glibc and still
implement features which are only defined by newer (upstream) kernels,
so that leads to more duplication.
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH v5 04/27] x86/fpu/xstate: Add XSAVES system states for shadow stack
From: Pavel Machek @ 2018-11-11 11:31 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Yu-cheng Yu, 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 <mike.krav>
In-Reply-To: <CALCETrUKzXYzRrWRdi8Z7AdAF0uZW5Gs7J4s=55dszoyzc29rw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1219 bytes --]
> > > > +/*
> > > > + * State component 12 is Control flow Enforcement kernel states
> > > > + */
> > > > +struct cet_kernel_state {
> > > > + u64 kernel_ssp; /* kernel shadow stack */
> > > > + u64 pl1_ssp; /* ring-1 shadow stack */
> > > > + u64 pl2_ssp; /* ring-2 shadow stack */
> > > > +} __packed;
> > > > +
> > >
> > > Why are these __packed? It seems like it'll generate bad code for no
> > > obvious purpose.
> >
> > That prevents any possibility that the compiler will insert padding, although in
> > 64-bit kernel this should not happen to either struct. Also all xstate
> > components here are packed.
> >
>
> They both seem like bugs, perhaps. As I understand it, __packed
> removes padding, but it also forces the compiler to expect the fields
> to be unaligned even if they are actually aligned.
This structure is shared with hardware, right? __packed seems like
right thing to do semantically.
As x86 handles unaligned accesses automatically, there should not be
much difference either way.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v5 04/27] x86/fpu/xstate: Add XSAVES system states for shadow stack
From: Pavel Machek @ 2018-11-11 11:31 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: Borislav Petkov, 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
In-Reply-To: <bb049aa9578bae7cfc6bd7c05b540f033f6685cc.camel@intel.com>
[-- Attachment #1: Type: text/plain, Size: 378 bytes --]
> > Just write "privilege level" everywhere - not "ring".
> >
> > Btw, do you see how the type and the name of all those other fields in
> > that file are tabulated? Except yours...
>
> I will fix it.
Don't. It is not broken.
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v5 04/27] x86/fpu/xstate: Add XSAVES system states for shadow stack
From: Pavel Machek @ 2018-11-11 11:31 UTC (permalink / raw)
To: Borislav Petkov
Cc: 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,
Dave Hansen, Eugene Syromiatnikov, Florian Weimer, H.J. Lu,
Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit
In-Reply-To: <20181108184038.GJ7543@zn.tnic>
[-- Attachment #1: Type: text/plain, Size: 645 bytes --]
Hi!
> > +/*
> > + * State component 12 is Control flow Enforcement kernel states
> > + */
> > +struct cet_kernel_state {
> > + u64 kernel_ssp; /* kernel shadow stack */
> > + u64 pl1_ssp; /* ring-1 shadow stack */
> > + u64 pl2_ssp; /* ring-2 shadow stack */
>
> Just write "privilege level" everywhere - not "ring".
Please just use word "ring". It is well estabilished terminology.
Which ring is priviledge level 1, given that we have SMM and
virtualization support?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Willy Tarreau @ 2018-11-11 11:11 UTC (permalink / raw)
To: Michael Kerrisk (man-pages)
Cc: Daniel Colascione, linux-kernel, Joel Fernandes, Linux API,
Vlastimil Babka, Florian Weimer, Carlos O'Donell,
libc-alpha@sourceware.org
In-Reply-To: <3664a508-ca74-4ff0-39a6-34543194a24e@gmail.com>
On Sun, Nov 11, 2018 at 11:53:54AM +0100, Michael Kerrisk (man-pages) wrote:
> I'm not sure I'd view the glibc position quite so harshly (although
> it is disappointing to me that bug 6399 remains open). I think they
> are simply short of people to work on this task.
I think so as well and really have great respect for this limitation,
which differs from the technical arguments on the bugzilla trying to
find every single good reason why using this syscall was wrong.
(...)
> A converse question that one could ask is: why did a culture
> evolve whereby kernel developers don't take responsibility for
> working with the major libc to ensure that wrappers are added as
> part of the job of adding each new system call? Yes, I know, there
> are some historical reasons (and even today, IMO, they do
> themselves no favors by requiring a CLA), but glibc really is
> a different place today, compared to where it was a few years
> ago.
I think the issue is a bit more complex :
- linux doesn't support a single libc
- glibc doesn't support a single OS
In practice we all know (believe?) that both statements above are
true but in practice 99% of the time there's a 1:1 relation between
these two components. What we'd really need would be to have the libc
interface as part of the operating system itself. I'm perfectly fine
with glibc providing all the "high-level" stuff like strcpy(), FILE*
operations etc, and all this probably is mostly system-independent.
But the system interface could possibly be handled easier in the
system itself, which would also provide a smoother adoption of new
syscalls and API updates. It would also limit the hassle required to
provide new syscalls, as if you start to have to contribute to two
projects at once for a single syscall, it becomes really painful.
But I don't know what changes that would require and it could really
turn out that in the end I'm totally wrong about the expected benefits.
Cheers,
Willy
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Florian Weimer @ 2018-11-11 11:09 UTC (permalink / raw)
To: Michael Kerrisk (man-pages)
Cc: Daniel Colascione, linux-kernel, Joel Fernandes, Linux API,
Willy Tarreau, Vlastimil Babka, Carlos O'Donell,
libc-alpha@sourceware.org
In-Reply-To: <bbc12da5-830e-99a7-95e3-d9da42947dc9@gmail.com>
* Michael Kerrisk:
> [adding in glibc folk for comment]
>
> On 11/10/18 7:52 PM, Daniel Colascione wrote:
>> Now that glibc is basically not adding any new system call wrappers,
>> how about publishing an "official" system call glue library as part of
>> the kernel distribution, along with the uapi headers? I don't think
>> it's reasonable to expect people to keep using syscall(__NR_XXX) for
>> all new functionality, especially as the system grows increasingly
>> sophisticated capabilities (like the new mount API, and hopefully the
>> new process API) outside the strictures of the POSIX process.
>
> As a quick glance at the glibc NEWS file shows, the above is not
> quite true:
>
> [[
> Version 2.28
> * The renameat2 function has been added...
> * The statx function has been added...
>
> Version 2.27
> * Support for memory protection keys was added. The <sys/mman.h> header now
> declares the functions pkey_alloc, pkey_free, pkey_mprotect...
> * The copy_file_range function was added.
>
> Version 2.26
> * New wrappers for the Linux-specific system calls preadv2 and pwritev2.
>
> Version 2.25
> * The getrandom [function] have been added.
> ]]
>
> I make that 11 system call wrappers added in the last 2 years.
And you missed mlock2 and memfd_create.
In some cases, we used system calls before the kernel had them (because
the kernel does not add system calls consistently across architectures).
On the other hand, this is only half of the story because distributions
do not backport system call wrappers, even those that backport kernel
implementations (or just rebase the kernel). This is something that
could be fixed eventually, but it is realted to another problem:
We had a patch for the membarrier system call, but the kernel developers
could not tell us what the system call does in therms of the C/C++
memory model, and the kernel developers and our concurrency expert could
not agree on documentation.
A lot of the new system calls lack clear specifications or are just
somewhat misdesigned. For example, pkey_alloc uses PKEY_DISABLE_WRITE
and PKEY_DISABLE_ACCESS flags (where the latter implies disabling both
read and write access), not something that matches the PROT_READ and
PROT_WRITE flags used by mmap/mprotect. This caused problems when POWER
support for pkey_alloc was added, and we are still working on resolving
that.
getrandom still causes boot delays because the kernel somehow fails to
seed its internal pool before starting PID 1 even on mainstream hardware
which has plenty of (true) randomness sources available, leading to
indefinite blocking of getrandom. It seems to me that people have
largely given up on fixing this in the upstream kernel.
For copy_file_range, we still have debates whether the system call (and
the glibc emulation) should preserve holes or not, and there a plans to
lift the cross-device restriction.
For renameat2, we already had a function in gnulib with the same name,
but which did not provide the atomic RENAME_NOREPLACE behavior for which
renameat2 was introduced.
These problems are relevant to the backporting question. One relatively
low-cost way do backport straight wrappers would be to put them as
hidden functions into libc_nonshared.a. But with these uncertainties,
this would be rather risky because fixing bugs of the wrappers would
then require relinking.
Thanks,
Florian
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Willy Tarreau @ 2018-11-11 11:02 UTC (permalink / raw)
To: Florian Weimer
Cc: Michael Kerrisk (man-pages), Daniel Colascione, linux-kernel,
Joel Fernandes, Linux API, Vlastimil Babka, Carlos O'Donell,
libc-alpha@sourceware.org
In-Reply-To: <87y39zx5xa.fsf@oldenburg.str.redhat.com>
On Sun, Nov 11, 2018 at 11:30:25AM +0100, Florian Weimer wrote:
> * Willy Tarreau:
>
> > On Sun, Nov 11, 2018 at 07:55:30AM +0100, Michael Kerrisk (man-pages) wrote:
> >> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=6399 is a
> >> longstanding example.
> >
> > This one was a sad read and shows that applications will continue to
> > suffer from glibc's prehistorical view on operating systems and will
> > continue to have to define their own syscall wrappers to exploit the
> > full potential of the modern operating systems they execute on.
>
> What's modern about a 15-bit thread identifier?
It's 15-bit on 32-bit systems, and 22 on 64-bit, hence you can have
4 million threads and/or processes on a single system image provided
you have the resources for that of course.
> I understand that using this interface is required in some cases (which
> includes some system calls for which glibc does provide wrappers), but I
> assumed that it was at least understood that these reusable IDs for
> tasks were an extremely poor interface. Aren't the resulting bugs
> common knowledge?
Sure, just as are the bugs created by people trying to implement their
own syscall wrappers. It's not by denying access to some native system
interfaces that you will prevent users from accessing them, you'll just
force them to work around the restriction and make things even worse.
> > This reminds me when one had to write their own spinlocks and atomics
> > many years ago. Seeing comments suggesting an application should open
> > /proc/$PID makes me really wonder if people actually want to use slow
> > and insecure applications designed this way.
>
> I don't understand. If you want a non-reusable identifier, you have to
> go through the /proc interface anyway. I think the recommendation is to
> use the PID/start time combination to get a unique process identifier or
> something like that.
It depends what you want to achieve. If you just need the tid, the one
you'll pass to sched_setaffinity(), gettid() is fine. There are two issues
with abusing /proc to emulate syscalls :
- it's sometimes much slower than the equivalent syscall and can
encourage users to cache the resulting values when they should not
- either it's done upon process startup and it may not get valid value
or may not work if /proc is not mounted yet (think init, mount etc),
or it's done upon first use and can break daemons which chroot()
themselves.
Syscalls don't have such limitations and are much safer to use. For other
things it's quite possible that you cannot rely on this syscall at all,
it's not a solution to everything, but it's a nice solution to all cases
where you need to access the system-wide identifier to pin a thread to a
given CPU set or renice it.
> I wanted to add gettid to glibc this cycle, but your comments suggest to
> me that if we did this, we'd likely never get a proper non-reusable
> thread identifier from the kernel. So I'm not sure what do anymore.
"Look people, I was about to do what we all refused to do for 10 years
now and Willy's comment made me change my mind, I'm sorry". The *real*
argument that most users could understand is "guys, we're sorry, but we
are running out of time and we won't work on this low priority stuff,
so someone else will have to take care of it".
In my opinion what matters is not whether or not people will use it
appropriately, but that its validity, side effects and wrong assumptions
are properly documented so that users don't shoot themselves in the foot.
But I guess that most of those defining it by themselves already figured
this out and are happy to use this available syscall when their application
wants to make use of certain feature that are offered by their operating
system.
Thanks,
Willy
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Florian Weimer @ 2018-11-11 11:02 UTC (permalink / raw)
To: Michael Kerrisk (man-pages)
Cc: Willy Tarreau, Daniel Colascione, linux-kernel, Joel Fernandes,
Linux API, Vlastimil Babka, Carlos O'Donell,
libc-alpha@sourceware.org
In-Reply-To: <3664a508-ca74-4ff0-39a6-34543194a24e@gmail.com>
* Michael Kerrisk:
> I'm not sure I'd view the glibc position quite so harshly (although
> it is disappointing to me that bug 6399 remains open). I think they
> are simply short of people to work on this task. There was a lengthy
> period where no syscall wrappers were being added (pretty much from
> 2.16 to 2.24, as far as I can tell), but that has changed.
People may have disappeared from glibc development who have objected to
gettid. I thought this was the case with strlcpy/strlcat, but it was
not.
At present, it takes one semi-active glibc contributor to block addition
of a system call. The process to override a sustained objection has
never been used successfully, and it is a lot of work to get it even
started.
Thanks,
Florian
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Michael Kerrisk (man-pages) @ 2018-11-11 10:53 UTC (permalink / raw)
To: Willy Tarreau
Cc: mtk.manpages, Daniel Colascione, linux-kernel, Joel Fernandes,
Linux API, Vlastimil Babka, Florian Weimer, Carlos O'Donell,
libc-alpha@sourceware.org
In-Reply-To: <20181111081725.GA30248@1wt.eu>
On 11/11/18 9:17 AM, Willy Tarreau wrote:
> On Sun, Nov 11, 2018 at 07:55:30AM +0100, Michael Kerrisk (man-pages) wrote:
>> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=6399 is a
>> longstanding example.
>
> This one was a sad read and shows that applications will continue to
> suffer from glibc's prehistorical view on operating systems and will
> continue to have to define their own syscall wrappers to exploit the
> full potential of the modern operating systems they execute on. This
> reminds me when one had to write their own spinlocks and atomics many
> years ago. Seeing comments suggesting an application should open
> /proc/$PID makes me really wonder if people actually want to use slow
> and insecure applications designed this way. Bah, after all, this
> wipes quite a bit of the shame I feel every time I do something to
> bypass it :-/
>
> The sad thing is that the energy wasted arguing in the bug above could
> have been better spent designing and implementing a generic solution
> to expose syscalls without depending on glibc's politics anymore.
I'm not sure I'd view the glibc position quite so harshly (although
it is disappointing to me that bug 6399 remains open). I think they
are simply short of people to work on this task. There was a lengthy
period where no syscall wrappers were being added (pretty much from
2.16 to 2.24, as far as I can tell), but that has changed.
And there is an expectation in some cases from the kernel side
that glibc will provide wrappers that build on (rather than just
wrap) some syscalls. And sometimes those wrappers are non-trivial.
A converse question that one could ask is: why did a culture
evolve whereby kernel developers don't take responsibility for
working with the major libc to ensure that wrappers are added as
part of the job of adding each new system call? Yes, I know, there
are some historical reasons (and even today, IMO, they do
themselves no favors by requiring a CLA), but glibc really is
a different place today, compared to where it was a few years
ago.
Cheers,
Micahel
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Florian Weimer @ 2018-11-11 10:40 UTC (permalink / raw)
To: Daniel Colascione
Cc: Willy Tarreau, Michael Kerrisk (man-pages), linux-kernel,
Joel Fernandes, Linux API, Vlastimil Babka, Carlos O'Donell,
libc-alpha@sourceware.org
In-Reply-To: <CAKOZuesi79-4J8bXU2bTY4876DOtL0K4A_Sr=6XsDD8dESPQbw@mail.gmail.com>
* Daniel Colascione:
> On Sun, Nov 11, 2018 at 12:17 AM, Willy Tarreau <w@1wt.eu> wrote:
>>
>> On Sun, Nov 11, 2018 at 07:55:30AM +0100, Michael Kerrisk (man-pages) wrote:
>> > [1] https://sourceware.org/
>>
>>
>> Bah, after all, this
>>
>> wipes quite a bit of the shame I feel every time I do something to
>>
>> bypass it :-/
>>
>>
>> The sad thing is that the energy wasted arguing in the bug above could
>>
>> have been better spent designing and implementing a generic solution
>>
>> to expose syscalls without depending on glibc's politics anymore.
>>
>>
>> Willy
>>
>> bugzilla/show_bug.cgi?id=6399 is a
>> > longstanding example.
>>
>> This one was a sad read and shows that applications will continue to
>> suffer from glibc's prehistorical view on operating systems
>
> Yes. I'm really not sure what glibc's current policies are meant to
> accomplish. They don't serve any useful purpose. There seems to be
> this weird subtext that glibc has leverage to change OS design, and it
> really doesn't. It's a misplaced idealism and ends up just hurting
> everyone.
I'm not sure what this comment tries to accomplish.
glibc tries to serve many masters: Current and past Linux kernel
interfaces, current Hurd kernel interfaces, different versions of POSIX
and C (and even C++), current C/C++ programming practice, historic C
programming practice, current and historic Linux userspace programming,
various platform ABIs, just to name a few.
These requirements are often in conflict.
>> Seeing comments suggesting an application should open
>> /proc/$PID makes me really wonder if people actually want to use slow
>> and insecure applications designed this way.
>
> That's a separate point. Yes, gettid should have a wrapper, but *also*
> we should have an FD-based interface to processes, because outside
> specialized contexts (e.g., parent-child waiting), the traditional
> Unix process API really is impossible to use safely. But that's a
> separate ongoing discussion.
A descriptor-based API would not help glibc that much because there is
an expectation encoded into many C programs that the C library does not
keep permanently open descriptors for its own internal use.
Thanks,
Florian
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Florian Weimer @ 2018-11-11 10:30 UTC (permalink / raw)
To: Willy Tarreau
Cc: Michael Kerrisk (man-pages), Daniel Colascione, linux-kernel,
Joel Fernandes, Linux API, Vlastimil Babka, Carlos O'Donell,
libc-alpha@sourceware.org
In-Reply-To: <20181111081725.GA30248@1wt.eu>
* Willy Tarreau:
> On Sun, Nov 11, 2018 at 07:55:30AM +0100, Michael Kerrisk (man-pages) wrote:
>> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=6399 is a
>> longstanding example.
>
> This one was a sad read and shows that applications will continue to
> suffer from glibc's prehistorical view on operating systems and will
> continue to have to define their own syscall wrappers to exploit the
> full potential of the modern operating systems they execute on.
What's modern about a 15-bit thread identifier?
I understand that using this interface is required in some cases (which
includes some system calls for which glibc does provide wrappers), but I
assumed that it was at least understood that these reusable IDs for
tasks were an extremely poor interface. Aren't the resulting bugs
common knowledge?
> This reminds me when one had to write their own spinlocks and atomics
> many years ago. Seeing comments suggesting an application should open
> /proc/$PID makes me really wonder if people actually want to use slow
> and insecure applications designed this way.
I don't understand. If you want a non-reusable identifier, you have to
go through the /proc interface anyway. I think the recommendation is to
use the PID/start time combination to get a unique process identifier or
something like that.
I wanted to add gettid to glibc this cycle, but your comments suggest to
me that if we did this, we'd likely never get a proper non-reusable
thread identifier from the kernel. So I'm not sure what do anymore.
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Daniel Colascione @ 2018-11-11 8:30 UTC (permalink / raw)
To: Joel Fernandes
Cc: Andy Lutomirski, Jann Horn, kernel list, John Reck, John Stultz,
Todd Kjos, Greg Kroah-Hartman, Christoph Hellwig, Al Viro,
Andrew Morton, Bruce Fields, Jeff Layton, Khalid Aziz, Lei.Yang,
linux-fsdevel, linux-kselftest, Linux-MM, marcandre.lureau,
Mike Kravetz, Minchan Kim, Shuah Khan, Valdis
In-Reply-To: <20181111080945.GA78191@google.com>
On Sun, Nov 11, 2018 at 12:09 AM, Joel Fernandes <joel@joelfernandes.org> wrote:
> On Sat, Nov 10, 2018 at 07:40:10PM -0800, Andy Lutomirski wrote:
> [...]
>> >>>>>>> I see two reasonable solutions:
>> >>>>>>>
>> >>>>>>> 1. Don’t fiddle with the struct file at all. Instead make the inode flag
>> >>>>>>> work by itself.
>> >>>>>>
>> >>>>>> Currently, the various VFS paths check only the struct file's f_mode to deny
>> >>>>>> writes of already opened files. This would mean more checking in all those
>> >>>>>> paths (and modification of all those paths).
>> >>>>>>
>> >>>>>> Anyway going with that idea, we could
>> >>>>>> 1. call deny_write_access(file) from the memfd's seal path which decrements
>> >>>>>> the inode::i_writecount.
>> >>>>>> 2. call get_write_access(inode) in the various VFS paths in addition to
>> >>>>>> checking for FMODE_*WRITE and deny the write (incase i_writecount is negative)
>> >>>>>>
>> >>>>>> That will prevent both reopens, and writes from succeeding. However I worry a
>> >>>>>> bit about 2 not being too familiar with VFS internals, about what the
>> >>>>>> consequences of doing that may be.
>> >>>>>
>> >>>>> IMHO, modifying both the inode and the struct file separately is fine,
>> >>>>> since they mean different things. In regular filesystems, it's fine to
>> >>>>> have a read-write open file description for a file whose inode grants
>> >>>>> write permission to nobody. Speaking of which: is fchmod enough to
>> >>>>> prevent this attack?
>> >>>>
>> >>>> Well, yes and no. fchmod does prevent reopening the file RW, but
>> >>>> anyone with permissions (owner, CAP_FOWNER) can just fchmod it back. A
>> >>>> seal is supposed to be irrevocable, so fchmod-as-inode-seal probably
>> >>>> isn't sufficient by itself. While it might be good enough for Android
>> >>>> (in the sense that it'll prevent RW-reopens from other security
>> >>>> contexts to which we send an open memfd file), it's still conceptually
>> >>>> ugly, IMHO. Let's go with the original approach of just tweaking the
>> >>>> inode so that open-for-write is permanently blocked.
>> >>>
>> >>> Agreed with the idea of modifying both file and inode flags. I was thinking
>> >>> modifying i_mode may do the trick but as you pointed it probably could be
>> >>> reverted by chmod or some other attribute setting calls.
>> >>>
>> >>> OTOH, I don't think deny_write_access(file) can be reverted from any
>> >>> user-facing path so we could do that from the seal to prevent the future
>> >>> opens in write mode. I'll double check and test that out tomorrow.
>> >>>
>> >>>
>> >>
>> >> This seems considerably more complicated and more fragile than needed. Just
>> >> add a new F_SEAL_WRITE_FUTURE. Grep for F_SEAL_WRITE and make the _FUTURE
>> >> variant work exactly like it with two exceptions:
>> >>
>> >> - shmem_mmap and maybe its hugetlbfs equivalent should check for it and act
>> >> accordingly.
>> >
>> > There's more to it than that, we also need to block future writes through
>> > write syscall, so we have to hook into the write path too once the seal is
>> > set, not just the mmap. That means we have to add code in mm/shmem.c to do
>> > that in all those handlers, to check for the seal (and hope we didn't miss a
>> > file_operations handler). Is that what you are proposing?
>>
>> The existing code already does this. That’s why I suggested grepping :)
>>
>> >
>> > Also, it means we have to keep CONFIG_TMPFS enabled so that the
>> > shmem_file_operations write handlers like write_iter are hooked up. Currently
>> > memfd works even with !CONFIG_TMPFS.
>>
>> If so, that sounds like it may already be a bug.
Why shouldn't memfd work independently of CONFIG_TMPFS? In particular,
write(2) on tmpfs FDs shouldn't work differently. If it does, that's a
kernel implementation detail leaking out into userspace.
>> >> - add_seals won’t need the wait_for_pins and mapping_deny_write logic.
>> >>
>> >> That really should be all that’s needed.
>> >
>> > It seems a fair idea what you're saying. But I don't see how its less
>> > complex.. IMO its far more simple to have VFS do the denial of the operations
>> > based on the flags of its datastructures.. and if it works (which I will test
>> > to be sure it will), then we should be good.
>>
>> I agree it’s complicated, but the code is already written. You should just
>> need to adjust some masks.
>>
>
> Its actually not that bad and a great idea, I did something like the
> following and it works pretty well. I would say its cleaner than the old
> approach for sure (and I also added a /proc/pid/fd/N reopen test to the
> selftest and made sure that issue goes away).
>
> Side note: One subtelty I discovered from the existing selftests is once the
> F_SEAL_WRITE are active, an mmap of PROT_READ and MAP_SHARED region is
> expected to fail. This is also evident from this code in mmap_region:
> if (vm_flags & VM_SHARED) {
> error = mapping_map_writable(file->f_mapping);
> if (error)
> goto allow_write_and_free_vma;
> }
>
This behavior seems like a bug. Why should MAP_SHARED writes be denied
here? There's no semantic incompatibility between shared mappings and
the seal. And I think this change would represent an ABI break using
memfd seals for ashmem, since ashmem currently allows MAP_SHARED
mappings after changing prot_mask.
> ---8<-----------------------
>
> From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> Subject: [PATCH] mm/memfd: implement future write seal using shmem ops
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
> fs/hugetlbfs/inode.c | 2 +-
> mm/memfd.c | 19 -------------------
> mm/shmem.c | 13 ++++++++++---
> 3 files changed, 11 insertions(+), 23 deletions(-)
>
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index 32920a10100e..1978581abfdf 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -530,7 +530,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;
> }
Maybe we can always set F_SEAL_FUTURE_WRITE when F_SEAL_WRITE so we
can just test one bit except where the F_SEAL_WRITE behavior differs
from F_SEAL_FUTURE_WRITE.
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Daniel Colascione @ 2018-11-11 8:25 UTC (permalink / raw)
To: Willy Tarreau
Cc: Michael Kerrisk (man-pages), linux-kernel, Joel Fernandes,
Linux API, Vlastimil Babka, Florian Weimer, Carlos O'Donell,
libc-alpha@sourceware.org
In-Reply-To: <20181111081725.GA30248@1wt.eu>
On Sun, Nov 11, 2018 at 12:17 AM, Willy Tarreau <w@1wt.eu> wrote:
>
> On Sun, Nov 11, 2018 at 07:55:30AM +0100, Michael Kerrisk (man-pages) wrote:
> > [1] https://sourceware.org/
>
>
> Bah, after all, this
>
> wipes quite a bit of the shame I feel every time I do something to
>
> bypass it :-/
>
>
> The sad thing is that the energy wasted arguing in the bug above could
>
> have been better spent designing and implementing a generic solution
>
> to expose syscalls without depending on glibc's politics anymore.
>
>
> Willy
>
> bugzilla/show_bug.cgi?id=6399 is a
> > longstanding example.
>
> This one was a sad read and shows that applications will continue to
> suffer from glibc's prehistorical view on operating systems
Yes. I'm really not sure what glibc's current policies are meant to
accomplish. They don't serve any useful purpose. There seems to be
this weird subtext that glibc has leverage to change OS design, and it
really doesn't. It's a misplaced idealism and ends up just hurting
everyone.
>
> Seeing comments suggesting an application should open
> /proc/$PID makes me really wonder if people actually want to use slow
> and insecure applications designed this way.
That's a separate point. Yes, gettid should have a wrapper, but *also*
we should have an FD-based interface to processes, because outside
specialized contexts (e.g., parent-child waiting), the traditional
Unix process API really is impossible to use safely. But that's a
separate ongoing discussion.
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Willy Tarreau @ 2018-11-11 8:17 UTC (permalink / raw)
To: Michael Kerrisk (man-pages)
Cc: Daniel Colascione, linux-kernel, Joel Fernandes, Linux API,
Vlastimil Babka, Florian Weimer, Carlos O'Donell,
libc-alpha@sourceware.org
In-Reply-To: <bbc12da5-830e-99a7-95e3-d9da42947dc9@gmail.com>
On Sun, Nov 11, 2018 at 07:55:30AM +0100, Michael Kerrisk (man-pages) wrote:
> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=6399 is a
> longstanding example.
This one was a sad read and shows that applications will continue to
suffer from glibc's prehistorical view on operating systems and will
continue to have to define their own syscall wrappers to exploit the
full potential of the modern operating systems they execute on. This
reminds me when one had to write their own spinlocks and atomics many
years ago. Seeing comments suggesting an application should open
/proc/$PID makes me really wonder if people actually want to use slow
and insecure applications designed this way. Bah, after all, this
wipes quite a bit of the shame I feel every time I do something to
bypass it :-/
The sad thing is that the energy wasted arguing in the bug above could
have been better spent designing and implementing a generic solution
to expose syscalls without depending on glibc's politics anymore.
Willy
^ permalink raw reply
* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Joel Fernandes @ 2018-11-11 8:09 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Daniel Colascione, Jann Horn, kernel list, John Reck, John Stultz,
Todd Kjos, Greg Kroah-Hartman, Christoph Hellwig, Al Viro,
Andrew Morton, Bruce Fields, Jeff Layton, Khalid Aziz, Lei.Yang,
linux-fsdevel, linux-kselftest, Linux-MM, marcandre.lureau,
Mike Kravetz, Minchan Kim, Shuah Khan, Valdis
In-Reply-To: <543A5181-3A16-438E-B372-97BEC48A74F8@amacapital.net>
On Sat, Nov 10, 2018 at 07:40:10PM -0800, Andy Lutomirski wrote:
[...]
> >>>>>>> I see two reasonable solutions:
> >>>>>>>
> >>>>>>> 1. Don’t fiddle with the struct file at all. Instead make the inode flag
> >>>>>>> work by itself.
> >>>>>>
> >>>>>> Currently, the various VFS paths check only the struct file's f_mode to deny
> >>>>>> writes of already opened files. This would mean more checking in all those
> >>>>>> paths (and modification of all those paths).
> >>>>>>
> >>>>>> Anyway going with that idea, we could
> >>>>>> 1. call deny_write_access(file) from the memfd's seal path which decrements
> >>>>>> the inode::i_writecount.
> >>>>>> 2. call get_write_access(inode) in the various VFS paths in addition to
> >>>>>> checking for FMODE_*WRITE and deny the write (incase i_writecount is negative)
> >>>>>>
> >>>>>> That will prevent both reopens, and writes from succeeding. However I worry a
> >>>>>> bit about 2 not being too familiar with VFS internals, about what the
> >>>>>> consequences of doing that may be.
> >>>>>
> >>>>> IMHO, modifying both the inode and the struct file separately is fine,
> >>>>> since they mean different things. In regular filesystems, it's fine to
> >>>>> have a read-write open file description for a file whose inode grants
> >>>>> write permission to nobody. Speaking of which: is fchmod enough to
> >>>>> prevent this attack?
> >>>>
> >>>> Well, yes and no. fchmod does prevent reopening the file RW, but
> >>>> anyone with permissions (owner, CAP_FOWNER) can just fchmod it back. A
> >>>> seal is supposed to be irrevocable, so fchmod-as-inode-seal probably
> >>>> isn't sufficient by itself. While it might be good enough for Android
> >>>> (in the sense that it'll prevent RW-reopens from other security
> >>>> contexts to which we send an open memfd file), it's still conceptually
> >>>> ugly, IMHO. Let's go with the original approach of just tweaking the
> >>>> inode so that open-for-write is permanently blocked.
> >>>
> >>> Agreed with the idea of modifying both file and inode flags. I was thinking
> >>> modifying i_mode may do the trick but as you pointed it probably could be
> >>> reverted by chmod or some other attribute setting calls.
> >>>
> >>> OTOH, I don't think deny_write_access(file) can be reverted from any
> >>> user-facing path so we could do that from the seal to prevent the future
> >>> opens in write mode. I'll double check and test that out tomorrow.
> >>>
> >>>
> >>
> >> This seems considerably more complicated and more fragile than needed. Just
> >> add a new F_SEAL_WRITE_FUTURE. Grep for F_SEAL_WRITE and make the _FUTURE
> >> variant work exactly like it with two exceptions:
> >>
> >> - shmem_mmap and maybe its hugetlbfs equivalent should check for it and act
> >> accordingly.
> >
> > There's more to it than that, we also need to block future writes through
> > write syscall, so we have to hook into the write path too once the seal is
> > set, not just the mmap. That means we have to add code in mm/shmem.c to do
> > that in all those handlers, to check for the seal (and hope we didn't miss a
> > file_operations handler). Is that what you are proposing?
>
> The existing code already does this. That’s why I suggested grepping :)
>
> >
> > Also, it means we have to keep CONFIG_TMPFS enabled so that the
> > shmem_file_operations write handlers like write_iter are hooked up. Currently
> > memfd works even with !CONFIG_TMPFS.
>
> If so, that sounds like it may already be a bug.
>
> >
> >> - add_seals won’t need the wait_for_pins and mapping_deny_write logic.
> >>
> >> That really should be all that’s needed.
> >
> > It seems a fair idea what you're saying. But I don't see how its less
> > complex.. IMO its far more simple to have VFS do the denial of the operations
> > based on the flags of its datastructures.. and if it works (which I will test
> > to be sure it will), then we should be good.
>
> I agree it’s complicated, but the code is already written. You should just
> need to adjust some masks.
>
Its actually not that bad and a great idea, I did something like the
following and it works pretty well. I would say its cleaner than the old
approach for sure (and I also added a /proc/pid/fd/N reopen test to the
selftest and made sure that issue goes away).
Side note: One subtelty I discovered from the existing selftests is once the
F_SEAL_WRITE are active, an mmap of PROT_READ and MAP_SHARED region is
expected to fail. This is also evident from this code in mmap_region:
if (vm_flags & VM_SHARED) {
error = mapping_map_writable(file->f_mapping);
if (error)
goto allow_write_and_free_vma;
}
---8<-----------------------
From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
Subject: [PATCH] mm/memfd: implement future write seal using shmem ops
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
fs/hugetlbfs/inode.c | 2 +-
mm/memfd.c | 19 -------------------
mm/shmem.c | 13 ++++++++++---
3 files changed, 11 insertions(+), 23 deletions(-)
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 32920a10100e..1978581abfdf 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -530,7 +530,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 5ba9804e9515..a9ece5fab439 100644
--- a/mm/memfd.c
+++ b/mm/memfd.c
@@ -220,25 +220,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 446942677cd4..7dad7efd8b99 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2163,6 +2163,12 @@ 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 shared mmaps are not allowed when "future write" seal active. */
+ if ((vma->vm_flags & VM_SHARED) && (info->seals & F_SEAL_FUTURE_WRITE))
+ return -EPERM;
+
file_accessed(file);
vma->vm_ops = &shmem_vm_ops;
if (IS_ENABLED(CONFIG_TRANSPARENT_HUGE_PAGECACHE) &&
@@ -2391,8 +2397,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;
@@ -2657,7 +2664,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 || info->seals & F_SEAL_FUTURE_WRITE) {
error = -EPERM;
goto out;
}
--
2.19.1.930.g4563a0d9d0-goog
^ permalink raw reply related
* Re: Official Linux system wrapper library?
From: Michael Kerrisk (man-pages) @ 2018-11-11 6:55 UTC (permalink / raw)
To: Daniel Colascione, linux-kernel
Cc: mtk.manpages, Joel Fernandes, Linux API, Willy Tarreau,
Vlastimil Babka, Florian Weimer, Carlos O'Donell,
libc-alpha@sourceware.org
In-Reply-To: <CAKOZuesB4R=dCz4merWQN0FSCGrXmOgUUr4ienSbStBJguNv8g@mail.gmail.com>
[adding in glibc folk for comment]
On 11/10/18 7:52 PM, Daniel Colascione wrote:
> Now that glibc is basically not adding any new system call wrappers,
> how about publishing an "official" system call glue library as part of
> the kernel distribution, along with the uapi headers? I don't think
> it's reasonable to expect people to keep using syscall(__NR_XXX) for
> all new functionality, especially as the system grows increasingly
> sophisticated capabilities (like the new mount API, and hopefully the
> new process API) outside the strictures of the POSIX process.
As a quick glance at the glibc NEWS file shows, the above is not
quite true:
[[
Version 2.28
* The renameat2 function has been added...
* The statx function has been added...
Version 2.27
* Support for memory protection keys was added. The <sys/mman.h> header now
declares the functions pkey_alloc, pkey_free, pkey_mprotect...
* The copy_file_range function was added.
Version 2.26
* New wrappers for the Linux-specific system calls preadv2 and pwritev2.
Version 2.25
* The getrandom [function] have been added.
]]
I make that 11 system call wrappers added in the last 2 years.
That said, of course, there are many system calls that lack wrappers [1],
and the use of syscall() is undesirable.
The glibc folk do have their reasons for being conservative around
adding system calls (https://lwn.net/Articles/655028/). However, at
this point, I think one of the limiting factors is developer time
on the glibc project. Quite possibly, they just need some help to
add more (properly designed) wrappers faster.
Cheers,
Michael
[1] https://sourceware.org/bugzilla/show_bug.cgi?id=6399 is a
longstanding example.
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Joel Fernandes @ 2018-11-11 4:01 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Daniel Colascione, Jann Horn, kernel list, John Reck, John Stultz,
Todd Kjos, Greg Kroah-Hartman, Christoph Hellwig, Al Viro,
Andrew Morton, Bruce Fields, Jeff Layton, Khalid Aziz, Lei.Yang,
linux-fsdevel, linux-kselftest, Linux-MM, marcandre.lureau,
Mike Kravetz, Minchan Kim, Shuah Khan, Valdis
In-Reply-To: <543A5181-3A16-438E-B372-97BEC48A74F8@amacapital.net>
On Sat, Nov 10, 2018 at 07:40:10PM -0800, Andy Lutomirski wrote:
>
>
> > On Nov 10, 2018, at 6:38 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
> >
> >> On Sat, Nov 10, 2018 at 02:18:23PM -0800, Andy Lutomirski wrote:
> >>
> >>>> On Nov 10, 2018, at 2:09 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
> >>>>
> >>>>> On Sat, Nov 10, 2018 at 11:11:27AM -0800, Daniel Colascione wrote:
> >>>>>> On Sat, Nov 10, 2018 at 10:45 AM, Daniel Colascione <dancol@google.com> wrote:
> >>>>>> On Sat, Nov 10, 2018 at 10:24 AM, Joel Fernandes <joel@joelfernandes.org> wrote:
> >>>>>> Thanks Andy for your thoughts, my comments below:
> >>>> [snip]
> >>>>>> I don't see it as warty, different seals will work differently. It works
> >>>>>> quite well for our usecase, and since Linux is all about solving real
> >>>>>> problems in the real work, it would be useful to have it.
> >>>>>>
> >>>>>>> - causes a probably-observable effect in the file mode in F_GETFL.
> >>>>>>
> >>>>>> Wouldn't that be the right thing to observe anyway?
> >>>>>>
> >>>>>>> - causes reopen to fail.
> >>>>>>
> >>>>>> So this concern isn't true anymore if we make reopen fail only for WRITE
> >>>>>> opens as Daniel suggested. I will make this change so that the security fix
> >>>>>> is a clean one.
> >>>>>>
> >>>>>>> - does *not* affect other struct files that may already exist on the same inode.
> >>>>>>
> >>>>>> TBH if you really want to block all writes to the file, then you want
> >>>>>> F_SEAL_WRITE, not this seal. The usecase we have is the fd is sent over IPC
> >>>>>> to another process and we want to prevent any new writes in the receiver
> >>>>>> side. There is no way this other receiving process can have an existing fd
> >>>>>> unless it was already sent one without the seal applied. The proposed seal
> >>>>>> could be renamed to F_SEAL_FD_WRITE if that is preferred.
> >>>>>>
> >>>>>>> - mysteriously malfunctions if you try to set it again on another struct
> >>>>>>> file that already exists
> >>>>>>>
> >>>>>>
> >>>>>> I didn't follow this, could you explain more?
> >>>>>>
> >>>>>>> - probably is insecure when used on hugetlbfs.
> >>>>>>
> >>>>>> The usecase is not expected to prevent all writes, indeed the usecase
> >>>>>> requires existing mmaps to continue to be able to write into the memory map.
> >>>>>> So would you call that a security issue too? The use of the seal wants to
> >>>>>> allow existing mmap regions to be continue to be written into (I mentioned
> >>>>>> more details in the cover letter).
> >>>>>>
> >>>>>>> I see two reasonable solutions:
> >>>>>>>
> >>>>>>> 1. Don’t fiddle with the struct file at all. Instead make the inode flag
> >>>>>>> work by itself.
> >>>>>>
> >>>>>> Currently, the various VFS paths check only the struct file's f_mode to deny
> >>>>>> writes of already opened files. This would mean more checking in all those
> >>>>>> paths (and modification of all those paths).
> >>>>>>
> >>>>>> Anyway going with that idea, we could
> >>>>>> 1. call deny_write_access(file) from the memfd's seal path which decrements
> >>>>>> the inode::i_writecount.
> >>>>>> 2. call get_write_access(inode) in the various VFS paths in addition to
> >>>>>> checking for FMODE_*WRITE and deny the write (incase i_writecount is negative)
> >>>>>>
> >>>>>> That will prevent both reopens, and writes from succeeding. However I worry a
> >>>>>> bit about 2 not being too familiar with VFS internals, about what the
> >>>>>> consequences of doing that may be.
> >>>>>
> >>>>> IMHO, modifying both the inode and the struct file separately is fine,
> >>>>> since they mean different things. In regular filesystems, it's fine to
> >>>>> have a read-write open file description for a file whose inode grants
> >>>>> write permission to nobody. Speaking of which: is fchmod enough to
> >>>>> prevent this attack?
> >>>>
> >>>> Well, yes and no. fchmod does prevent reopening the file RW, but
> >>>> anyone with permissions (owner, CAP_FOWNER) can just fchmod it back. A
> >>>> seal is supposed to be irrevocable, so fchmod-as-inode-seal probably
> >>>> isn't sufficient by itself. While it might be good enough for Android
> >>>> (in the sense that it'll prevent RW-reopens from other security
> >>>> contexts to which we send an open memfd file), it's still conceptually
> >>>> ugly, IMHO. Let's go with the original approach of just tweaking the
> >>>> inode so that open-for-write is permanently blocked.
> >>>
> >>> Agreed with the idea of modifying both file and inode flags. I was thinking
> >>> modifying i_mode may do the trick but as you pointed it probably could be
> >>> reverted by chmod or some other attribute setting calls.
> >>>
> >>> OTOH, I don't think deny_write_access(file) can be reverted from any
> >>> user-facing path so we could do that from the seal to prevent the future
> >>> opens in write mode. I'll double check and test that out tomorrow.
> >>>
> >>>
> >>
> >> This seems considerably more complicated and more fragile than needed. Just
> >> add a new F_SEAL_WRITE_FUTURE. Grep for F_SEAL_WRITE and make the _FUTURE
> >> variant work exactly like it with two exceptions:
> >>
> >> - shmem_mmap and maybe its hugetlbfs equivalent should check for it and act
> >> accordingly.
> >
> > There's more to it than that, we also need to block future writes through
> > write syscall, so we have to hook into the write path too once the seal is
> > set, not just the mmap. That means we have to add code in mm/shmem.c to do
> > that in all those handlers, to check for the seal (and hope we didn't miss a
> > file_operations handler). Is that what you are proposing?
>
> The existing code already does this. That’s why I suggested grepping :)
Ahh sorry I see your point now. Ok let me try this approach. Thank you!
Probably we can make this work this way and it is sufficient.
> >
> > Also, it means we have to keep CONFIG_TMPFS enabled so that the
> > shmem_file_operations write handlers like write_iter are hooked up. Currently
> > memfd works even with !CONFIG_TMPFS.
>
> If so, that sounds like it may already be a bug.
Actually, its not a bug. If CONFIG_TMPFS is disabled, then IIRC write syscall
will be prevented anyway and then the mmap is the only way. I'll double check
that once I work on this idea.
> >
> >> - add_seals won’t need the wait_for_pins and mapping_deny_write logic.
> >>
> >> That really should be all that’s needed.
> >
> > It seems a fair idea what you're saying. But I don't see how its less
> > complex.. IMO its far more simple to have VFS do the denial of the operations
> > based on the flags of its datastructures.. and if it works (which I will test
> > to be sure it will), then we should be good.
>
> I agree it’s complicated, but the code is already written. You should just
> need to adjust some masks.
>
Right.
> >
> > Btw by any chance, are you also coming by LPC conference next week?
> >
>
> No. I’d like to, but I can’t make the trip this year.
Ok, no worries.
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Andy Lutomirski @ 2018-11-11 3:40 UTC (permalink / raw)
To: Joel Fernandes
Cc: Daniel Colascione, Jann Horn, kernel list, John Reck, John Stultz,
Todd Kjos, Greg Kroah-Hartman, Christoph Hellwig, Al Viro,
Andrew Morton, Bruce Fields, Jeff Layton, Khalid Aziz, Lei.Yang,
linux-fsdevel, linux-kselftest, Linux-MM, marcandre.lureau,
Mike Kravetz, Minchan Kim, Shuah Khan, Valdis
In-Reply-To: <20181111023808.GA174670@google.com>
> On Nov 10, 2018, at 6:38 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
>
>> On Sat, Nov 10, 2018 at 02:18:23PM -0800, Andy Lutomirski wrote:
>>
>>>> On Nov 10, 2018, at 2:09 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
>>>>
>>>>> On Sat, Nov 10, 2018 at 11:11:27AM -0800, Daniel Colascione wrote:
>>>>>> On Sat, Nov 10, 2018 at 10:45 AM, Daniel Colascione <dancol@google.com> wrote:
>>>>>> On Sat, Nov 10, 2018 at 10:24 AM, Joel Fernandes <joel@joelfernandes.org> wrote:
>>>>>> Thanks Andy for your thoughts, my comments below:
>>>> [snip]
>>>>>> I don't see it as warty, different seals will work differently. It works
>>>>>> quite well for our usecase, and since Linux is all about solving real
>>>>>> problems in the real work, it would be useful to have it.
>>>>>>
>>>>>>> - causes a probably-observable effect in the file mode in F_GETFL.
>>>>>>
>>>>>> Wouldn't that be the right thing to observe anyway?
>>>>>>
>>>>>>> - causes reopen to fail.
>>>>>>
>>>>>> So this concern isn't true anymore if we make reopen fail only for WRITE
>>>>>> opens as Daniel suggested. I will make this change so that the security fix
>>>>>> is a clean one.
>>>>>>
>>>>>>> - does *not* affect other struct files that may already exist on the same inode.
>>>>>>
>>>>>> TBH if you really want to block all writes to the file, then you want
>>>>>> F_SEAL_WRITE, not this seal. The usecase we have is the fd is sent over IPC
>>>>>> to another process and we want to prevent any new writes in the receiver
>>>>>> side. There is no way this other receiving process can have an existing fd
>>>>>> unless it was already sent one without the seal applied. The proposed seal
>>>>>> could be renamed to F_SEAL_FD_WRITE if that is preferred.
>>>>>>
>>>>>>> - mysteriously malfunctions if you try to set it again on another struct
>>>>>>> file that already exists
>>>>>>>
>>>>>>
>>>>>> I didn't follow this, could you explain more?
>>>>>>
>>>>>>> - probably is insecure when used on hugetlbfs.
>>>>>>
>>>>>> The usecase is not expected to prevent all writes, indeed the usecase
>>>>>> requires existing mmaps to continue to be able to write into the memory map.
>>>>>> So would you call that a security issue too? The use of the seal wants to
>>>>>> allow existing mmap regions to be continue to be written into (I mentioned
>>>>>> more details in the cover letter).
>>>>>>
>>>>>>> I see two reasonable solutions:
>>>>>>>
>>>>>>> 1. Don’t fiddle with the struct file at all. Instead make the inode flag
>>>>>>> work by itself.
>>>>>>
>>>>>> Currently, the various VFS paths check only the struct file's f_mode to deny
>>>>>> writes of already opened files. This would mean more checking in all those
>>>>>> paths (and modification of all those paths).
>>>>>>
>>>>>> Anyway going with that idea, we could
>>>>>> 1. call deny_write_access(file) from the memfd's seal path which decrements
>>>>>> the inode::i_writecount.
>>>>>> 2. call get_write_access(inode) in the various VFS paths in addition to
>>>>>> checking for FMODE_*WRITE and deny the write (incase i_writecount is negative)
>>>>>>
>>>>>> That will prevent both reopens, and writes from succeeding. However I worry a
>>>>>> bit about 2 not being too familiar with VFS internals, about what the
>>>>>> consequences of doing that may be.
>>>>>
>>>>> IMHO, modifying both the inode and the struct file separately is fine,
>>>>> since they mean different things. In regular filesystems, it's fine to
>>>>> have a read-write open file description for a file whose inode grants
>>>>> write permission to nobody. Speaking of which: is fchmod enough to
>>>>> prevent this attack?
>>>>
>>>> Well, yes and no. fchmod does prevent reopening the file RW, but
>>>> anyone with permissions (owner, CAP_FOWNER) can just fchmod it back. A
>>>> seal is supposed to be irrevocable, so fchmod-as-inode-seal probably
>>>> isn't sufficient by itself. While it might be good enough for Android
>>>> (in the sense that it'll prevent RW-reopens from other security
>>>> contexts to which we send an open memfd file), it's still conceptually
>>>> ugly, IMHO. Let's go with the original approach of just tweaking the
>>>> inode so that open-for-write is permanently blocked.
>>>
>>> Agreed with the idea of modifying both file and inode flags. I was thinking
>>> modifying i_mode may do the trick but as you pointed it probably could be
>>> reverted by chmod or some other attribute setting calls.
>>>
>>> OTOH, I don't think deny_write_access(file) can be reverted from any
>>> user-facing path so we could do that from the seal to prevent the future
>>> opens in write mode. I'll double check and test that out tomorrow.
>>>
>>>
>>
>> This seems considerably more complicated and more fragile than needed. Just
>> add a new F_SEAL_WRITE_FUTURE. Grep for F_SEAL_WRITE and make the _FUTURE
>> variant work exactly like it with two exceptions:
>>
>> - shmem_mmap and maybe its hugetlbfs equivalent should check for it and act
>> accordingly.
>
> There's more to it than that, we also need to block future writes through
> write syscall, so we have to hook into the write path too once the seal is
> set, not just the mmap. That means we have to add code in mm/shmem.c to do
> that in all those handlers, to check for the seal (and hope we didn't miss a
> file_operations handler). Is that what you are proposing?
The existing code already does this. That’s why I suggested grepping :)
>
> Also, it means we have to keep CONFIG_TMPFS enabled so that the
> shmem_file_operations write handlers like write_iter are hooked up. Currently
> memfd works even with !CONFIG_TMPFS.
If so, that sounds like it may already be a bug.
>
>> - add_seals won’t need the wait_for_pins and mapping_deny_write logic.
>>
>> That really should be all that’s needed.
>
> It seems a fair idea what you're saying. But I don't see how its less
> complex.. IMO its far more simple to have VFS do the denial of the operations
> based on the flags of its datastructures.. and if it works (which I will test
> to be sure it will), then we should be good.
I agree it’s complicated, but the code is already written. You should just need to adjust some masks.
>
> Btw by any chance, are you also coming by LPC conference next week?
>
No. I’d like to, but I can’t make the trip this year.
> thanks!
>
> - Joel
>
^ permalink raw reply
* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Joel Fernandes @ 2018-11-11 2:38 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Daniel Colascione, Jann Horn, kernel list, John Reck, John Stultz,
Todd Kjos, Greg Kroah-Hartman, Christoph Hellwig, Al Viro,
Andrew Morton, Bruce Fields, Jeff Layton, Khalid Aziz, Lei.Yang,
linux-fsdevel, linux-kselftest, Linux-MM, marcandre.lureau,
Mike Kravetz, Minchan Kim, Shuah Khan, Valdis
In-Reply-To: <907D942E-E321-4BD7-BED7-ACD1D96A3643@amacapital.net>
On Sat, Nov 10, 2018 at 02:18:23PM -0800, Andy Lutomirski wrote:
>
> > On Nov 10, 2018, at 2:09 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
> >
> >> On Sat, Nov 10, 2018 at 11:11:27AM -0800, Daniel Colascione wrote:
> >>> On Sat, Nov 10, 2018 at 10:45 AM, Daniel Colascione <dancol@google.com> wrote:
> >>>> On Sat, Nov 10, 2018 at 10:24 AM, Joel Fernandes <joel@joelfernandes.org> wrote:
> >>>> Thanks Andy for your thoughts, my comments below:
> >> [snip]
> >>>> I don't see it as warty, different seals will work differently. It works
> >>>> quite well for our usecase, and since Linux is all about solving real
> >>>> problems in the real work, it would be useful to have it.
> >>>>
> >>>>> - causes a probably-observable effect in the file mode in F_GETFL.
> >>>>
> >>>> Wouldn't that be the right thing to observe anyway?
> >>>>
> >>>>> - causes reopen to fail.
> >>>>
> >>>> So this concern isn't true anymore if we make reopen fail only for WRITE
> >>>> opens as Daniel suggested. I will make this change so that the security fix
> >>>> is a clean one.
> >>>>
> >>>>> - does *not* affect other struct files that may already exist on the same inode.
> >>>>
> >>>> TBH if you really want to block all writes to the file, then you want
> >>>> F_SEAL_WRITE, not this seal. The usecase we have is the fd is sent over IPC
> >>>> to another process and we want to prevent any new writes in the receiver
> >>>> side. There is no way this other receiving process can have an existing fd
> >>>> unless it was already sent one without the seal applied. The proposed seal
> >>>> could be renamed to F_SEAL_FD_WRITE if that is preferred.
> >>>>
> >>>>> - mysteriously malfunctions if you try to set it again on another struct
> >>>>> file that already exists
> >>>>>
> >>>>
> >>>> I didn't follow this, could you explain more?
> >>>>
> >>>>> - probably is insecure when used on hugetlbfs.
> >>>>
> >>>> The usecase is not expected to prevent all writes, indeed the usecase
> >>>> requires existing mmaps to continue to be able to write into the memory map.
> >>>> So would you call that a security issue too? The use of the seal wants to
> >>>> allow existing mmap regions to be continue to be written into (I mentioned
> >>>> more details in the cover letter).
> >>>>
> >>>>> I see two reasonable solutions:
> >>>>>
> >>>>> 1. Don’t fiddle with the struct file at all. Instead make the inode flag
> >>>>> work by itself.
> >>>>
> >>>> Currently, the various VFS paths check only the struct file's f_mode to deny
> >>>> writes of already opened files. This would mean more checking in all those
> >>>> paths (and modification of all those paths).
> >>>>
> >>>> Anyway going with that idea, we could
> >>>> 1. call deny_write_access(file) from the memfd's seal path which decrements
> >>>> the inode::i_writecount.
> >>>> 2. call get_write_access(inode) in the various VFS paths in addition to
> >>>> checking for FMODE_*WRITE and deny the write (incase i_writecount is negative)
> >>>>
> >>>> That will prevent both reopens, and writes from succeeding. However I worry a
> >>>> bit about 2 not being too familiar with VFS internals, about what the
> >>>> consequences of doing that may be.
> >>>
> >>> IMHO, modifying both the inode and the struct file separately is fine,
> >>> since they mean different things. In regular filesystems, it's fine to
> >>> have a read-write open file description for a file whose inode grants
> >>> write permission to nobody. Speaking of which: is fchmod enough to
> >>> prevent this attack?
> >>
> >> Well, yes and no. fchmod does prevent reopening the file RW, but
> >> anyone with permissions (owner, CAP_FOWNER) can just fchmod it back. A
> >> seal is supposed to be irrevocable, so fchmod-as-inode-seal probably
> >> isn't sufficient by itself. While it might be good enough for Android
> >> (in the sense that it'll prevent RW-reopens from other security
> >> contexts to which we send an open memfd file), it's still conceptually
> >> ugly, IMHO. Let's go with the original approach of just tweaking the
> >> inode so that open-for-write is permanently blocked.
> >
> > Agreed with the idea of modifying both file and inode flags. I was thinking
> > modifying i_mode may do the trick but as you pointed it probably could be
> > reverted by chmod or some other attribute setting calls.
> >
> > OTOH, I don't think deny_write_access(file) can be reverted from any
> > user-facing path so we could do that from the seal to prevent the future
> > opens in write mode. I'll double check and test that out tomorrow.
> >
> >
>
> This seems considerably more complicated and more fragile than needed. Just
> add a new F_SEAL_WRITE_FUTURE. Grep for F_SEAL_WRITE and make the _FUTURE
> variant work exactly like it with two exceptions:
>
> - shmem_mmap and maybe its hugetlbfs equivalent should check for it and act
> accordingly.
There's more to it than that, we also need to block future writes through
write syscall, so we have to hook into the write path too once the seal is
set, not just the mmap. That means we have to add code in mm/shmem.c to do
that in all those handlers, to check for the seal (and hope we didn't miss a
file_operations handler). Is that what you are proposing?
Also, it means we have to keep CONFIG_TMPFS enabled so that the
shmem_file_operations write handlers like write_iter are hooked up. Currently
memfd works even with !CONFIG_TMPFS.
> - add_seals won’t need the wait_for_pins and mapping_deny_write logic.
>
> That really should be all that’s needed.
It seems a fair idea what you're saying. But I don't see how its less
complex.. IMO its far more simple to have VFS do the denial of the operations
based on the flags of its datastructures.. and if it works (which I will test
to be sure it will), then we should be good.
Btw by any chance, are you also coming by LPC conference next week?
thanks!
- Joel
^ permalink raw reply
* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Andy Lutomirski @ 2018-11-10 22:18 UTC (permalink / raw)
To: Joel Fernandes
Cc: Daniel Colascione, Jann Horn, kernel list, John Reck, John Stultz,
Todd Kjos, Greg Kroah-Hartman, Christoph Hellwig, Al Viro,
Andrew Morton, Bruce Fields, Jeff Layton, Khalid Aziz, Lei.Yang,
linux-fsdevel, linux-kselftest, Linux-MM, marcandre.lureau,
Mike Kravetz, Minchan Kim, Shuah Khan, Valdis
In-Reply-To: <20181110220933.GB96924@google.com>
> On Nov 10, 2018, at 2:09 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
>
>> On Sat, Nov 10, 2018 at 11:11:27AM -0800, Daniel Colascione wrote:
>>> On Sat, Nov 10, 2018 at 10:45 AM, Daniel Colascione <dancol@google.com> wrote:
>>>> On Sat, Nov 10, 2018 at 10:24 AM, Joel Fernandes <joel@joelfernandes.org> wrote:
>>>> Thanks Andy for your thoughts, my comments below:
>> [snip]
>>>> I don't see it as warty, different seals will work differently. It works
>>>> quite well for our usecase, and since Linux is all about solving real
>>>> problems in the real work, it would be useful to have it.
>>>>
>>>>> - causes a probably-observable effect in the file mode in F_GETFL.
>>>>
>>>> Wouldn't that be the right thing to observe anyway?
>>>>
>>>>> - causes reopen to fail.
>>>>
>>>> So this concern isn't true anymore if we make reopen fail only for WRITE
>>>> opens as Daniel suggested. I will make this change so that the security fix
>>>> is a clean one.
>>>>
>>>>> - does *not* affect other struct files that may already exist on the same inode.
>>>>
>>>> TBH if you really want to block all writes to the file, then you want
>>>> F_SEAL_WRITE, not this seal. The usecase we have is the fd is sent over IPC
>>>> to another process and we want to prevent any new writes in the receiver
>>>> side. There is no way this other receiving process can have an existing fd
>>>> unless it was already sent one without the seal applied. The proposed seal
>>>> could be renamed to F_SEAL_FD_WRITE if that is preferred.
>>>>
>>>>> - mysteriously malfunctions if you try to set it again on another struct
>>>>> file that already exists
>>>>>
>>>>
>>>> I didn't follow this, could you explain more?
>>>>
>>>>> - probably is insecure when used on hugetlbfs.
>>>>
>>>> The usecase is not expected to prevent all writes, indeed the usecase
>>>> requires existing mmaps to continue to be able to write into the memory map.
>>>> So would you call that a security issue too? The use of the seal wants to
>>>> allow existing mmap regions to be continue to be written into (I mentioned
>>>> more details in the cover letter).
>>>>
>>>>> I see two reasonable solutions:
>>>>>
>>>>> 1. Don’t fiddle with the struct file at all. Instead make the inode flag
>>>>> work by itself.
>>>>
>>>> Currently, the various VFS paths check only the struct file's f_mode to deny
>>>> writes of already opened files. This would mean more checking in all those
>>>> paths (and modification of all those paths).
>>>>
>>>> Anyway going with that idea, we could
>>>> 1. call deny_write_access(file) from the memfd's seal path which decrements
>>>> the inode::i_writecount.
>>>> 2. call get_write_access(inode) in the various VFS paths in addition to
>>>> checking for FMODE_*WRITE and deny the write (incase i_writecount is negative)
>>>>
>>>> That will prevent both reopens, and writes from succeeding. However I worry a
>>>> bit about 2 not being too familiar with VFS internals, about what the
>>>> consequences of doing that may be.
>>>
>>> IMHO, modifying both the inode and the struct file separately is fine,
>>> since they mean different things. In regular filesystems, it's fine to
>>> have a read-write open file description for a file whose inode grants
>>> write permission to nobody. Speaking of which: is fchmod enough to
>>> prevent this attack?
>>
>> Well, yes and no. fchmod does prevent reopening the file RW, but
>> anyone with permissions (owner, CAP_FOWNER) can just fchmod it back. A
>> seal is supposed to be irrevocable, so fchmod-as-inode-seal probably
>> isn't sufficient by itself. While it might be good enough for Android
>> (in the sense that it'll prevent RW-reopens from other security
>> contexts to which we send an open memfd file), it's still conceptually
>> ugly, IMHO. Let's go with the original approach of just tweaking the
>> inode so that open-for-write is permanently blocked.
>
> Agreed with the idea of modifying both file and inode flags. I was thinking
> modifying i_mode may do the trick but as you pointed it probably could be
> reverted by chmod or some other attribute setting calls.
>
> OTOH, I don't think deny_write_access(file) can be reverted from any
> user-facing path so we could do that from the seal to prevent the future
> opens in write mode. I'll double check and test that out tomorrow.
>
>
This seems considerably more complicated and more fragile than needed. Just add a new F_SEAL_WRITE_FUTURE. Grep for F_SEAL_WRITE and make the _FUTURE variant work exactly like it with two exceptions:
- shmem_mmap and maybe its hugetlbfs equivalent should check for it and act accordingly.
- add_seals won’t need the wait_for_pins and mapping_deny_write logic.
That really should be all that’s needed.
^ permalink raw reply
* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Joel Fernandes @ 2018-11-10 22:09 UTC (permalink / raw)
To: Daniel Colascione
Cc: Andy Lutomirski, Jann Horn, kernel list, John Reck, John Stultz,
Todd Kjos, Greg Kroah-Hartman, Christoph Hellwig, Al Viro,
Andrew Morton, Bruce Fields, Jeff Layton, Khalid Aziz, Lei.Yang,
linux-fsdevel, linux-kselftest, Linux-MM, marcandre.lureau,
Mike Kravetz, Minchan Kim, Shuah Khan, Valdis
In-Reply-To: <CAKOZueum8MtNvJ5P=W7_pRw62TdQdCgyjCwwbG1wezNboC1cxQ@mail.gmail.com>
On Sat, Nov 10, 2018 at 11:11:27AM -0800, Daniel Colascione wrote:
> On Sat, Nov 10, 2018 at 10:45 AM, Daniel Colascione <dancol@google.com> wrote:
> > On Sat, Nov 10, 2018 at 10:24 AM, Joel Fernandes <joel@joelfernandes.org> wrote:
> >> Thanks Andy for your thoughts, my comments below:
> [snip]
> >> I don't see it as warty, different seals will work differently. It works
> >> quite well for our usecase, and since Linux is all about solving real
> >> problems in the real work, it would be useful to have it.
> >>
> >>> - causes a probably-observable effect in the file mode in F_GETFL.
> >>
> >> Wouldn't that be the right thing to observe anyway?
> >>
> >>> - causes reopen to fail.
> >>
> >> So this concern isn't true anymore if we make reopen fail only for WRITE
> >> opens as Daniel suggested. I will make this change so that the security fix
> >> is a clean one.
> >>
> >>> - does *not* affect other struct files that may already exist on the same inode.
> >>
> >> TBH if you really want to block all writes to the file, then you want
> >> F_SEAL_WRITE, not this seal. The usecase we have is the fd is sent over IPC
> >> to another process and we want to prevent any new writes in the receiver
> >> side. There is no way this other receiving process can have an existing fd
> >> unless it was already sent one without the seal applied. The proposed seal
> >> could be renamed to F_SEAL_FD_WRITE if that is preferred.
> >>
> >>> - mysteriously malfunctions if you try to set it again on another struct
> >>> file that already exists
> >>>
> >>
> >> I didn't follow this, could you explain more?
> >>
> >>> - probably is insecure when used on hugetlbfs.
> >>
> >> The usecase is not expected to prevent all writes, indeed the usecase
> >> requires existing mmaps to continue to be able to write into the memory map.
> >> So would you call that a security issue too? The use of the seal wants to
> >> allow existing mmap regions to be continue to be written into (I mentioned
> >> more details in the cover letter).
> >>
> >>> I see two reasonable solutions:
> >>>
> >>> 1. Don’t fiddle with the struct file at all. Instead make the inode flag
> >>> work by itself.
> >>
> >> Currently, the various VFS paths check only the struct file's f_mode to deny
> >> writes of already opened files. This would mean more checking in all those
> >> paths (and modification of all those paths).
> >>
> >> Anyway going with that idea, we could
> >> 1. call deny_write_access(file) from the memfd's seal path which decrements
> >> the inode::i_writecount.
> >> 2. call get_write_access(inode) in the various VFS paths in addition to
> >> checking for FMODE_*WRITE and deny the write (incase i_writecount is negative)
> >>
> >> That will prevent both reopens, and writes from succeeding. However I worry a
> >> bit about 2 not being too familiar with VFS internals, about what the
> >> consequences of doing that may be.
> >
> > IMHO, modifying both the inode and the struct file separately is fine,
> > since they mean different things. In regular filesystems, it's fine to
> > have a read-write open file description for a file whose inode grants
> > write permission to nobody. Speaking of which: is fchmod enough to
> > prevent this attack?
>
> Well, yes and no. fchmod does prevent reopening the file RW, but
> anyone with permissions (owner, CAP_FOWNER) can just fchmod it back. A
> seal is supposed to be irrevocable, so fchmod-as-inode-seal probably
> isn't sufficient by itself. While it might be good enough for Android
> (in the sense that it'll prevent RW-reopens from other security
> contexts to which we send an open memfd file), it's still conceptually
> ugly, IMHO. Let's go with the original approach of just tweaking the
> inode so that open-for-write is permanently blocked.
Agreed with the idea of modifying both file and inode flags. I was thinking
modifying i_mode may do the trick but as you pointed it probably could be
reverted by chmod or some other attribute setting calls.
OTOH, I don't think deny_write_access(file) can be reverted from any
user-facing path so we could do that from the seal to prevent the future
opens in write mode. I'll double check and test that out tomorrow.
thanks,
- Joel
^ 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