* Re: [RESEND PATCH v3 0/2] sysctl: handle overflow for file-max
From: Kees Cook @ 2019-01-07 23:16 UTC (permalink / raw)
To: Christian Brauner
Cc: Andrew Morton, LKML, Eric W. Biederman, Luis R. Rodriguez,
Joe Lawrence, Waiman Long, Dominik Brodowski, Al Viro,
Alexey Dobriyan, Linux API
In-Reply-To: <20190107222700.15954-1-christian@brauner.io>
On Mon, Jan 7, 2019 at 2:27 PM Christian Brauner <christian@brauner.io> wrote:
>
> Hey,
>
> Here is v3 of this patchset. Changelogs are in the individual commits.
Andrew, can you take these? I've already Acked them.
-Kees
>
> Currently, when writing
>
> echo 18446744073709551616 > /proc/sys/fs/file-max
>
> /proc/sys/fs/file-max will overflow and be set to 0. That quickly
> crashes the system.
>
> The first version of this patch intended to detect the overflow and cap
> at ULONG_MAX. However, we should not do this and rather return EINVAL on
> overflow. The reasons are:
> - this aligns with other sysctl handlers that simply reject overflows
> (cf. [1], [2], and a bunch of others)
> - we already do a partial fail on overflow right now
> Namely, when the TMPBUFLEN is exceeded. So we already reject values
> such as 184467440737095516160 (21 chars) but accept values such as
> 18446744073709551616 (20 chars) but both are overflows. So we should
> just always reject 64bit overflows and not special-case this based on
> the number of chars.
>
> (This patchset is in reference to https://lkml.org/lkml/2018/10/11/585.)
>
> Thanks!
> Christian
>
> [1]: fb910c42cceb ("sysctl: check for UINT_MAX before unsigned int min/max")
> [2]: 196851bed522 ("s390/topology: correct topology mode proc handler")
>
> Christian Brauner (2):
> sysctl: handle overflow in proc_get_long
> sysctl: handle overflow for file-max
>
> kernel/sysctl.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 46 insertions(+), 1 deletion(-)
>
> --
> 2.19.1
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Dave Chinner @ 2019-01-08 4:43 UTC (permalink / raw)
To: Linus Torvalds
Cc: Matthew Wilcox, Jann Horn, Jiri Kosina, Andrew Morton, Greg KH,
Peter Zijlstra, Michal Hocko, Linux-MM, kernel list, Linux API
In-Reply-To: <CAHk-=wiqbKEC5jUXr3ax+oUuiRrp=QMv_ZnUfO-SPv=UNJ-OTw@mail.gmail.com>
On Sun, Jan 06, 2019 at 01:46:37PM -0800, Linus Torvalds wrote:
> On Sat, Jan 5, 2019 at 5:50 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Slightly updated patch in case somebody wants to try things out.
>
> I decided to just apply that patch. It is *not* marked for stable,
> very intentionally, because I expect that we will need to wait and see
> if there are issues with it, and whether we might have to do something
> entirely different (more like the traditional behavior with some extra
> "only for owner" logic).
So, I read the paper and before I was half way through it I figured
there are a bunch of other similar page cache invalidation attacks
we can perform without needing mincore. i.e. Focussing on mmap() and
mincore() misses the wider issues we have with global shared caches.
My first thought:
fd = open(some_file, O_RDONLY);
iov[0].iov_base = buf;
iov[0].iov_len = 1;
ret = preadv2(fd, iov, 1, off, RWF_NOWAIT);
switch (ret) {
case -EAGAIN:
/* page not resident in page cache */
break;
case 1:
/* page resident in page cache */
break;
default:
/* beyond EOF or some other error */
break;
}
This is "as documented" in the man page for preadv2:
RWF_NOWAIT (since Linux 4.14)
Do not wait for data which is not immediately available.
If this flag is specified, the preadv2() system call will
return instantly if it would have to read data from the
backing storage or wait for a lock. If some data was
successfully read, it will return the number of bytes read.
If no bytes were read, it will return -1 and set errno to
EAGAIN. Currently, this flag is meaningful only for
preadv2().
IOWs, we've explicitly designed interfaces to communicate whether
data is "immediately accessible" or not to the application so they
can make sensible decisions about IO scheduling. i.e. IO won't
block the main application processing loop and so can be scheduled in
the background by the app and the data processed when that IO
returns. That just so happens to be exactly the same information
about the page cache that mincore is making available to userspace.
If we "remove" this information from the interfaces like it has been
done for mincore(), it doesn't mean userspace can't get it in other
ways. e.g. it now just has to time the read(2) syscall duration and
infer whether the data came from the page cache or disk from the
timing information.
IMO, there's nothing new or novel about this page cache information
leak - it was just a matter of time before some researcher put 2 and
2 together and realised that sharing the page cache across a
security boundary is little different from sharing deduplicated
pages across those same security boundaries. i.e. As long as we
shared caches across security boundaries and userspace can control
both cache invalidation and instantiation, we cannot prevent
userspace from constructing these invalidate+read information
exfiltration scenarios.
And then there is overlayfs. Overlay is really just a way to
efficiently share the page cache of the same underlying read-only
directory tree across all containers on a host. i.e. we have been
specifically designing our container hosting systems to share the
underlying read-only page cache across all security boundaries on
the host. If overlay is vulnerable to these shared page cache
attacks (seems extremely likely) then we've got much bigger problems
than mincore to think about....
> But doing a test patch during the merge window (which is about to
> close) sounds like the right thing to do.
IMO it seems like the wrong thing to do. It's just a hacky band-aid
over a specific extraction method and does nothing to reduce the
actual scope of the information leak. Focussing on the band-aid
means you've missed all the other avenues that the same information
is exposed and all the infrastructure we've build on the core
concept of sharing kernel side pages across security boundaries.
And that's even without considering whether the change breaks
userspace. Which it does. e.g. vmtouch is fairly widely used to
manage page cache instantiation for rapid bring-up and migration of
guest VMs and containers. They save the hot page cache information
from a running container and then using that to instantiate the page
cache in new instances running the same workload so they run at full
speed right from the start. This use case calls mincore() to pull
the page cache information from the running container.
If anyone else proposed merging a syscall implementation change that
was extremely likely to break userspace you'd be shouting at them
that "we don't break userspace"....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply
* Re: [RESEND PATCH v3 2/2] sysctl: handle overflow for file-max
From: Dominik Brodowski @ 2019-01-08 7:01 UTC (permalink / raw)
To: Christian Brauner
Cc: akpm, keescook, linux-kernel, ebiederm, mcgrof, joe.lawrence,
longman, viro, adobriyan, linux-api
In-Reply-To: <20190107222700.15954-3-christian@brauner.io>
On Mon, Jan 07, 2019 at 11:27:00PM +0100, Christian Brauner wrote:
> @@ -2833,6 +2836,10 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
> break;
> if (neg)
> continue;
> + if ((max && val > *max) || (min && val < *min)) {
> + err = -EINVAL;
> + break;
> + }
> val = convmul * val / convdiv;
> if ((min && val < *min) || (max && val > *max))
> continue;
This is a generic change which affects all users of
do_proc_doulongvec_minmax() that have extra1 or extra2 set. In sysctl.c, I
do not see another user of proc_doulongvec_minmax() that has extra1 or
extra2 set. However, have you verified whether your patch changes the
behaviour for other files that make use of proc_doulongvec_minmax() or
proc_doulongvec_ms_jiffies_minmax(), and not only of the file-max sysctl?
Thanks,
Dominik
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Kevin Easton @ 2019-01-08 8:50 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jiri Kosina, Masatake YAMATO, Andrew Morton, Greg KH,
Peter Zijlstra, Michal Hocko, linux-mm, Linux List Kernel Mailing,
linux-api
In-Reply-To: <CAHk-=wgrSKyN23yp-npq6+J-4pGqbzxb3mJ183PryjHw7PWDyA@mail.gmail.com>
On Sat, Jan 05, 2019 at 01:54:03PM -0800, Linus Torvalds wrote:
> On Sat, Jan 5, 2019 at 12:43 PM Jiri Kosina <jikos@kernel.org> wrote:
> >
> > > Who actually _uses_ mincore()? That's probably the best guide to what
> > > we should do. Maybe they open the file read-only even if they are the
> > > owner, and we really should look at file ownership instead.
> >
> > Yeah, well
> >
> > https://codesearch.debian.net/search?q=mincore
> >
> > is a bit too much mess to get some idea quickly I am afraid.
>
> Yeah, heh.
>
> And the first hit is 'fincore', which probably nobody cares about
> anyway, but it does
>
> fd = open (name, O_RDONLY)
> ..
> mmap(window, len, PROT_NONE, MAP_PRIVATE, ..
>
> so if we want to keep that working, we'd really need to actually check
> file ownership rather than just looking at f_mode.
>
> But I don't know if anybody *uses* and cares about fincore, and it's
> particularly questionable for non-root users.
>
...
> I didn't find anything that seems to really care, but I gave up after
> a few pages of really boring stuff.
I've gone through everything in the Debian code search, and this is the
stuff that seems like it would be affected at all by the current patch:
util-linux
Contains 'fincore' as already noted above.
e2fsprogs
e4defrag tries to drop pages that it caused to be loaded into the
page cache, but it's not clear that this ever worked as designed
anyway (it calls mincore() before ioctl(fd, EXT4_IOC_MOVE_EXT ..)
but then after the sync_file_range it drops the pages that *were*
in the page cache at the time of mincore()).
pgfincore
postgresql extension used to try to dump/restore page cache status
of database backing files across reboots. It uses a fresh mapping
with mincore() to try to determine the current page cache status of
a file.
nocache
LD_PRELOAD library that tries to drop any pages that the victim
program has caused to be loaded into the page cache, uses mincore
on a fresh mapping to see what was resident beforehand. Also
includes 'cachestats' command that's basically another 'fincore'.
xfsprogs
xfs_io has a 'mincore' sub-command that is roughly equivalent to
'fincore'.
vmtouch
vmtouch is "Portable file system cache diagnostics and control",
among other things it implements 'fincore' type functionality, and
one of its touted use-cases is "Preserving virtual memory profile
when failing over servers".
qemu
qemu uses mincore() with a fresh PROT_NONE, MAP_PRIVATE mapping to
implement the "x-check-cache-dropped" option.
( https://patchwork.kernel.org/patch/10395865/ )
(Everything else I could see was either looking at anonymous VMAs, its
own existing mapping that it's been using for actual IO, or was just
using mincore() to see if an address was part of any mapping at all).
- Kevin
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Bernd Petrovitsch @ 2019-01-08 9:14 UTC (permalink / raw)
To: Vlastimil Babka, Jiri Kosina
Cc: Linus Torvalds, Andrew Morton, Greg KH, Peter Zijlstra,
Michal Hocko, linux-mm, linux-kernel, linux-api
In-Reply-To: <fb0414ea-953b-0252-b1d1-12028b190949@suse.cz>
On 05/01/2019 20:38, Vlastimil Babka wrote:
[...]
> I was thinking about "return true" here, assuming that userspace generally wants
> to ensure itself there won't be page faults when it starts doing something
> critical, and if it sees a "false" it will try to do some kind of prefaulting,
> possibly in a loop. There might be somebody trying to make sure something is out
Isn't that racy by design as the pages may get flushed out after the check?
Shouldn't the application use e.g. mlock()/.... to guarantee no page
faults in the first place?
MfG,
Bernd
--
Bernd Petrovitsch Email : bernd@petrovitsch.priv.at
LUGA : http://www.luga.at
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Jiri Kosina @ 2019-01-08 11:37 UTC (permalink / raw)
To: Bernd Petrovitsch
Cc: Vlastimil Babka, Linus Torvalds, Andrew Morton, Greg KH,
Peter Zijlstra, Michal Hocko, linux-mm, linux-kernel, linux-api
In-Reply-To: <047f0582-a4d3-490d-7284-48dfdf9e9471@petrovitsch.priv.at>
On Tue, 8 Jan 2019, Bernd Petrovitsch wrote:
> Shouldn't the application use e.g. mlock()/.... to guarantee no page
> faults in the first place?
Calling mincore() on pages you've just mlock()ed is sort of pointless
though.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [RFC PATCH v1 3/5] Yama: Enforces noexec mounts or file executability through O_MAYEXEC
From: Mickaël Salaün @ 2019-01-08 13:29 UTC (permalink / raw)
To: Jann Horn
Cc: Mickaël Salaün, kernel list, Al Viro, James Morris,
Jonathan Corbet, Kees Cook, Matthew Garrett,
Michael Kerrisk-manpages, zohar, philippe.trebuchet, shuah,
thibaut.sautereau, vincent.strubel, yves-alexis.perez,
Kernel Hardening, Linux API, linux-security-module, linux-fsdevel,
Andy Lutomirski
In-Reply-To: <CAG48ez00CpXOOJPLgpQd_e0gG2KgYb1p8bwssA8FDOugHTxRAA@mail.gmail.com>
On 03/01/2019 12:17, Jann Horn wrote:
> On Thu, Dec 13, 2018 at 3:49 PM Mickaël Salaün
> <mickael.salaun@ssi.gouv.fr> wrote:
>> On 12/12/2018 18:09, Jann Horn wrote:
>>> On Wed, Dec 12, 2018 at 9:18 AM Mickaël Salaün <mic@digikod.net> wrote:
>>>> Enable to either propagate the mount options from the underlying VFS
>>>> mount to prevent execution, or to propagate the file execute permission.
>>>> This may allow a script interpreter to check execution permissions
>>>> before reading commands from a file.
>>>>
>>>> The main goal is to be able to protect the kernel by restricting
>>>> arbitrary syscalls that an attacker could perform with a crafted binary
>>>> or certain script languages. It also improves multilevel isolation
>>>> by reducing the ability of an attacker to use side channels with
>>>> specific code. These restrictions can natively be enforced for ELF
>>>> binaries (with the noexec mount option) but require this kernel
>>>> extension to properly handle scripts (e.g., Python, Perl).
>>>>
>>>> Add a new sysctl kernel.yama.open_mayexec_enforce to control this
>>>> behavior. A following patch adds documentation.
> [...]
>>>> +{
>>>> + if (!(mask & MAY_OPENEXEC))
>>>> + return 0;
>>>> + /*
>>>> + * Match regular files and directories to make it easier to
>>>> + * modify script interpreters.
>>>> + */
>>>> + if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
>>>> + return 0;
>>>
>>> So files are subject to checks, but loading code from things like
>>> sockets is always fine?
>>
>> As I said in a previous email, these checks do not handle fifo either.
>> This is relevant in a threat model targeting persistent attacks (and
>> with additional protections/restrictions). We may want to only whitelist
>> fifo, but I don't get how a socket is relevant here. Can you please clarify?
>
> I don't think that there's a security problem here. I just think it's
> weird to have the extra check when it seems to me like it isn't really
> necessary - nobody is going to want to execute a socket or fifo
> anyway, right?
Right, the fifo whitelisting should answer your concern then.
>
>>>
>>>> + if ((open_mayexec_enforce & YAMA_OMAYEXEC_ENFORCE_MOUNT) &&
>>>> + !(mask & MAY_EXECMOUNT))
>>>> + return -EACCES;
>>>> +
>>>> + /*
>>>> + * May prefer acl_permission_check() instead of generic_permission(),
>>>> + * to not be bypassable with CAP_DAC_READ_SEARCH.
>>>> + */
>>>> + if (open_mayexec_enforce & YAMA_OMAYEXEC_ENFORCE_FILE)
>>>> + return generic_permission(inode, MAY_EXEC);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> static struct security_hook_list yama_hooks[] __lsm_ro_after_init = {
>>>> + LSM_HOOK_INIT(inode_permission, yama_inode_permission),
>>>> LSM_HOOK_INIT(ptrace_access_check, yama_ptrace_access_check),
>>>> LSM_HOOK_INIT(ptrace_traceme, yama_ptrace_traceme),
>>>> LSM_HOOK_INIT(task_prctl, yama_task_prctl),
>>>> @@ -447,6 +489,37 @@ static int yama_dointvec_minmax(struct ctl_table *table, int write,
>>>> return proc_dointvec_minmax(&table_copy, write, buffer, lenp, ppos);
>>>> }
>>>>
>>>> +static int yama_dointvec_bitmask_macadmin(struct ctl_table *table, int write,
>>>> + void __user *buffer, size_t *lenp,
>>>> + loff_t *ppos)
>>>> +{
>>>> + int error;
>>>> +
>>>> + if (write) {
>>>> + struct ctl_table table_copy;
>>>> + int tmp_mayexec_enforce;
>>>> +
>>>> + if (!capable(CAP_MAC_ADMIN))
>>>> + return -EPERM;
>>>
>>> Don't put capable() checks in sysctls, it doesn't work.
>>>
>>
>> I tested it and the root user can indeed open the file even if the
>> process doesn't have CAP_MAC_ADMIN, however writing in the sysctl file
>> is denied. Btw there is a similar check in the previous function
>> (yama_dointvec_minmax).
>
> It's still wrong. If an attacker without CAP_MAC_ADMIN opens the
> sysctl file, then passes the file descriptor to a setcap binary that
> has CAP_MAC_ADMIN as stdout/stderr, and the setcap binary writes to
> it, then the capable() check is bypassed. (But of course, to open the
> sysctl file in the first place, you'd need to be root (uid 0), so the
> check doesn't really matter.)
I agree with you that a confused deputy attack may uses file
descriptors, but I don't see how the current sysctl API may be used to
check the process capability at open time. Anyway, on a properly
configured system, especially one leveraging Linux capabilities (e.g.
CLIP OS), root processes may not have CAP_SYS_ADMIN. Moreover, SUID or
fcap binaries may not be available to an attacker (e.g. in a container).
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Bernd Petrovitsch @ 2019-01-08 13:53 UTC (permalink / raw)
To: Jiri Kosina
Cc: Vlastimil Babka, Linus Torvalds, Andrew Morton, Greg KH,
Peter Zijlstra, Michal Hocko, linux-mm, linux-kernel, linux-api
In-Reply-To: <nycvar.YFH.7.76.1901081235380.16954@cbobk.fhfr.pm>
On 08/01/2019 12:37, Jiri Kosina wrote:
> On Tue, 8 Jan 2019, Bernd Petrovitsch wrote:
>
>> Shouldn't the application use e.g. mlock()/.... to guarantee no page
>> faults in the first place?
>
> Calling mincore() on pages you've just mlock()ed is sort of pointless
> though.
Obviously;-)
Sorry for being unclear above: If I want my application to
avoid suffering from page faults, I use simply mlock()
(and/or friends) to nail the relevant pages into physical
RAM and not "look if they are out, if yes, get them in" which
has also the risk that these important pages are too soon
evicted again.
But perhaps I'm missing some very special use cases ....
MfG,
Brend
--
Bernd Petrovitsch Email : bernd@petrovitsch.priv.at
LUGA : http://www.luga.at
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Kirill A. Shutemov @ 2019-01-08 14:08 UTC (permalink / raw)
To: Bernd Petrovitsch
Cc: Jiri Kosina, Vlastimil Babka, Linus Torvalds, Andrew Morton,
Greg KH, Peter Zijlstra, Michal Hocko, linux-mm, linux-kernel,
linux-api
In-Reply-To: <8c9feac8-fecb-a56a-afaf-c1352a666991@petrovitsch.priv.at>
On Tue, Jan 08, 2019 at 02:53:00PM +0100, Bernd Petrovitsch wrote:
> On 08/01/2019 12:37, Jiri Kosina wrote:
> > On Tue, 8 Jan 2019, Bernd Petrovitsch wrote:
> >
> >> Shouldn't the application use e.g. mlock()/.... to guarantee no page
> >> faults in the first place?
> >
> > Calling mincore() on pages you've just mlock()ed is sort of pointless
> > though.
>
> Obviously;-)
>
> Sorry for being unclear above: If I want my application to
> avoid suffering from page faults, I use simply mlock()
> (and/or friends) to nail the relevant pages into physical
> RAM and not "look if they are out, if yes, get them in" which
> has also the risk that these important pages are too soon
> evicted again.
Note, that mlock() doesn't prevent minor page faults. Mlocked memory is
still subject to mechanisms that makes the page temporary unmapped. For
instance migration (including NUMA balancing), compaction, khugepaged...
--
Kirill A. Shutemov
^ permalink raw reply
* Re: [PATCH v7 2/2] selftests: add tests for pidfd_send_signal()
From: Tycho Andersen @ 2019-01-08 17:53 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-kernel, linux-api, luto, arnd, serge, keescook, akpm, jannh,
oleg, cyphar, viro, linux-fsdevel, dancol, timmurray, fweimer,
tglx, x86, ebiederm
In-Reply-To: <20190102161654.9093-2-christian@brauner.io>
On Wed, Jan 02, 2019 at 05:16:54PM +0100, Christian Brauner wrote:
> + /*
> + * Stop the child so we can inspect whether we have
> + * recycled pid PID_RECYCLE.
> + */
> + close(pipe_fds[0]);
> + ret = kill(recycled_pid, SIGSTOP);
> + close(pipe_fds[1]);
> + if (ret) {
> + (void)wait_for_pid(recycled_pid);
> + _exit(PIDFD_ERROR);
> + }
Sorry for being late to the party, but I wonder if this whole thing
couldn't be simplified with /proc/sys/kenrel/ns_last_pid?
Tycho
^ permalink raw reply
* Re: [PATCH v7 2/2] selftests: add tests for pidfd_send_signal()
From: Serge E. Hallyn @ 2019-01-08 17:54 UTC (permalink / raw)
To: Tycho Andersen
Cc: Christian Brauner, linux-kernel, linux-api, luto, arnd, serge,
keescook, akpm, jannh, oleg, cyphar, viro, linux-fsdevel, dancol,
timmurray, fweimer, tglx, x86, ebiederm
In-Reply-To: <20190108175306.GF29009@cisco>
On Tue, Jan 08, 2019 at 10:53:06AM -0700, Tycho Andersen wrote:
> On Wed, Jan 02, 2019 at 05:16:54PM +0100, Christian Brauner wrote:
> > + /*
> > + * Stop the child so we can inspect whether we have
> > + * recycled pid PID_RECYCLE.
> > + */
> > + close(pipe_fds[0]);
> > + ret = kill(recycled_pid, SIGSTOP);
> > + close(pipe_fds[1]);
> > + if (ret) {
> > + (void)wait_for_pid(recycled_pid);
> > + _exit(PIDFD_ERROR);
> > + }
>
> Sorry for being late to the party, but I wonder if this whole thing
> couldn't be simplified with /proc/sys/kenrel/ns_last_pid?
no, bc it's not namespaced :)
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Linus Torvalds @ 2019-01-08 17:57 UTC (permalink / raw)
To: Dave Chinner
Cc: Matthew Wilcox, Jann Horn, Jiri Kosina, Andrew Morton, Greg KH,
Peter Zijlstra, Michal Hocko, Linux-MM, kernel list, Linux API
In-Reply-To: <20190108044336.GB27534@dastard>
On Mon, Jan 7, 2019 at 8:43 PM Dave Chinner <david@fromorbit.com> wrote:
>
> So, I read the paper and before I was half way through it I figured
> there are a bunch of other similar page cache invalidation attacks
> we can perform without needing mincore. i.e. Focussing on mmap() and
> mincore() misses the wider issues we have with global shared caches.
Oh, agreed, and that was discussed in the original report too.
The thing is, you can also depend on our pre-faulting of pages in the
page fault handler, and use that to get the cached status of nearby
pages. So do something like "fault one page, then do mincore() to see
how many pages near it were mapped". See our "do_fault_around()"
logic.
But mincore is certainly the easiest interface, and the one that
doesn't require much effort or setup. It's also the one where our old
behavior was actually arguably simply stupid and actively wrong (ie
"in caches" isn't even strictly speaking a valid question, since the
caches in question may be invalid). So let's try to see if giving
mincore() slightly more well-defined semantics actually causes any
pain.
I do think that the RWF_NOWAIT case might also be interesting to look at.
Linus
^ permalink raw reply
* Re: [PATCH v7 2/2] selftests: add tests for pidfd_send_signal()
From: Tycho Andersen @ 2019-01-08 17:58 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: Christian Brauner, linux-kernel, linux-api, luto, arnd, keescook,
akpm, jannh, oleg, cyphar, viro, linux-fsdevel, dancol, timmurray,
fweimer, tglx, x86, ebiederm
In-Reply-To: <20190108175415.GA22353@mail.hallyn.com>
On Tue, Jan 08, 2019 at 11:54:15AM -0600, Serge E. Hallyn wrote:
> On Tue, Jan 08, 2019 at 10:53:06AM -0700, Tycho Andersen wrote:
> > On Wed, Jan 02, 2019 at 05:16:54PM +0100, Christian Brauner wrote:
> > > + /*
> > > + * Stop the child so we can inspect whether we have
> > > + * recycled pid PID_RECYCLE.
> > > + */
> > > + close(pipe_fds[0]);
> > > + ret = kill(recycled_pid, SIGSTOP);
> > > + close(pipe_fds[1]);
> > > + if (ret) {
> > > + (void)wait_for_pid(recycled_pid);
> > > + _exit(PIDFD_ERROR);
> > > + }
> >
> > Sorry for being late to the party, but I wonder if this whole thing
> > couldn't be simplified with /proc/sys/kenrel/ns_last_pid?
>
> no, bc it's not namespaced :)
Huh? It looks like it is...
static int pid_ns_ctl_handler(struct ctl_table *table, int write,
void __user *buffer, size_t *lenp, loff_t *ppos)
{
struct pid_namespace *pid_ns = task_active_pid_ns(current);
struct ctl_table tmp = *table;
int ret, next;
if (write && !ns_capable(pid_ns->user_ns, CAP_SYS_ADMIN))
return -EPERM;
...
Tycho
^ permalink raw reply
* Re: [PATCH v7 2/2] selftests: add tests for pidfd_send_signal()
From: Serge E. Hallyn @ 2019-01-08 18:17 UTC (permalink / raw)
To: Tycho Andersen
Cc: Serge E. Hallyn, Christian Brauner, linux-kernel, linux-api, luto,
arnd, keescook, akpm, jannh, oleg, cyphar, viro, linux-fsdevel,
dancol, timmurray, fweimer, tglx, x86, ebiederm
In-Reply-To: <20190108175843.GA26016@cisco>
On Tue, Jan 08, 2019 at 10:58:43AM -0700, Tycho Andersen wrote:
> On Tue, Jan 08, 2019 at 11:54:15AM -0600, Serge E. Hallyn wrote:
> > On Tue, Jan 08, 2019 at 10:53:06AM -0700, Tycho Andersen wrote:
> > > On Wed, Jan 02, 2019 at 05:16:54PM +0100, Christian Brauner wrote:
> > > > + /*
> > > > + * Stop the child so we can inspect whether we have
> > > > + * recycled pid PID_RECYCLE.
> > > > + */
> > > > + close(pipe_fds[0]);
> > > > + ret = kill(recycled_pid, SIGSTOP);
> > > > + close(pipe_fds[1]);
> > > > + if (ret) {
> > > > + (void)wait_for_pid(recycled_pid);
> > > > + _exit(PIDFD_ERROR);
> > > > + }
> > >
> > > Sorry for being late to the party, but I wonder if this whole thing
> > > couldn't be simplified with /proc/sys/kenrel/ns_last_pid?
> >
> > no, bc it's not namespaced :)
>
> Huh? It looks like it is...
>
> static int pid_ns_ctl_handler(struct ctl_table *table, int write,
> void __user *buffer, size_t *lenp, loff_t *ppos)
> {
> struct pid_namespace *pid_ns = task_active_pid_ns(current);
> struct ctl_table tmp = *table;
> int ret, next;
>
> if (write && !ns_capable(pid_ns->user_ns, CAP_SYS_ADMIN))
> return -EPERM;
>
> ...
Oh - hah, but that's ns_last_pid. You'd want pid_max. And that one
is not namespaced.
^ permalink raw reply
* Re: [PATCH v7 2/2] selftests: add tests for pidfd_send_signal()
From: Tycho Andersen @ 2019-01-08 18:20 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: Christian Brauner, linux-kernel, linux-api, luto, arnd, keescook,
akpm, jannh, oleg, cyphar, viro, linux-fsdevel, dancol, timmurray,
fweimer, tglx, x86, ebiederm
In-Reply-To: <20190108181742.GA22614@mail.hallyn.com>
On Tue, Jan 08, 2019 at 12:17:42PM -0600, Serge E. Hallyn wrote:
> On Tue, Jan 08, 2019 at 10:58:43AM -0700, Tycho Andersen wrote:
> > On Tue, Jan 08, 2019 at 11:54:15AM -0600, Serge E. Hallyn wrote:
> > > On Tue, Jan 08, 2019 at 10:53:06AM -0700, Tycho Andersen wrote:
> > > > On Wed, Jan 02, 2019 at 05:16:54PM +0100, Christian Brauner wrote:
> > > > > + /*
> > > > > + * Stop the child so we can inspect whether we have
> > > > > + * recycled pid PID_RECYCLE.
> > > > > + */
> > > > > + close(pipe_fds[0]);
> > > > > + ret = kill(recycled_pid, SIGSTOP);
> > > > > + close(pipe_fds[1]);
> > > > > + if (ret) {
> > > > > + (void)wait_for_pid(recycled_pid);
> > > > > + _exit(PIDFD_ERROR);
> > > > > + }
> > > >
> > > > Sorry for being late to the party, but I wonder if this whole thing
> > > > couldn't be simplified with /proc/sys/kenrel/ns_last_pid?
> > >
> > > no, bc it's not namespaced :)
> >
> > Huh? It looks like it is...
> >
> > static int pid_ns_ctl_handler(struct ctl_table *table, int write,
> > void __user *buffer, size_t *lenp, loff_t *ppos)
> > {
> > struct pid_namespace *pid_ns = task_active_pid_ns(current);
> > struct ctl_table tmp = *table;
> > int ret, next;
> >
> > if (write && !ns_capable(pid_ns->user_ns, CAP_SYS_ADMIN))
> > return -EPERM;
> >
> > ...
>
> Oh - hah, but that's ns_last_pid. You'd want pid_max. And that one
> is not namespaced.
Perhaps I'm misunderstanding, but isn't the point of all this code to
get the same pid again? So can't we just fork(), kill(), then set
ns_last_pid to pid-1, and fork() again to re-use?
Tycho
^ permalink raw reply
* Re: [PATCH v7 2/2] selftests: add tests for pidfd_send_signal()
From: Serge E. Hallyn @ 2019-01-08 18:21 UTC (permalink / raw)
To: Tycho Andersen
Cc: Serge E. Hallyn, Christian Brauner, linux-kernel, linux-api, luto,
arnd, keescook, akpm, jannh, oleg, cyphar, viro, linux-fsdevel,
dancol, timmurray, fweimer, tglx, x86, ebiederm
In-Reply-To: <20190108182023.GA28330@cisco>
On Tue, Jan 08, 2019 at 11:20:23AM -0700, Tycho Andersen wrote:
> On Tue, Jan 08, 2019 at 12:17:42PM -0600, Serge E. Hallyn wrote:
> > On Tue, Jan 08, 2019 at 10:58:43AM -0700, Tycho Andersen wrote:
> > > On Tue, Jan 08, 2019 at 11:54:15AM -0600, Serge E. Hallyn wrote:
> > > > On Tue, Jan 08, 2019 at 10:53:06AM -0700, Tycho Andersen wrote:
> > > > > On Wed, Jan 02, 2019 at 05:16:54PM +0100, Christian Brauner wrote:
> > > > > > + /*
> > > > > > + * Stop the child so we can inspect whether we have
> > > > > > + * recycled pid PID_RECYCLE.
> > > > > > + */
> > > > > > + close(pipe_fds[0]);
> > > > > > + ret = kill(recycled_pid, SIGSTOP);
> > > > > > + close(pipe_fds[1]);
> > > > > > + if (ret) {
> > > > > > + (void)wait_for_pid(recycled_pid);
> > > > > > + _exit(PIDFD_ERROR);
> > > > > > + }
> > > > >
> > > > > Sorry for being late to the party, but I wonder if this whole thing
> > > > > couldn't be simplified with /proc/sys/kenrel/ns_last_pid?
> > > >
> > > > no, bc it's not namespaced :)
> > >
> > > Huh? It looks like it is...
> > >
> > > static int pid_ns_ctl_handler(struct ctl_table *table, int write,
> > > void __user *buffer, size_t *lenp, loff_t *ppos)
> > > {
> > > struct pid_namespace *pid_ns = task_active_pid_ns(current);
> > > struct ctl_table tmp = *table;
> > > int ret, next;
> > >
> > > if (write && !ns_capable(pid_ns->user_ns, CAP_SYS_ADMIN))
> > > return -EPERM;
> > >
> > > ...
> >
> > Oh - hah, but that's ns_last_pid. You'd want pid_max. And that one
> > is not namespaced.
>
> Perhaps I'm misunderstanding, but isn't the point of all this code to
> get the same pid again? So can't we just fork(), kill(), then set
> ns_last_pid to pid-1, and fork() again to re-use?
Oh yeah that would work :)
I was stuck on the idea of just limiting the range of pids.
^ permalink raw reply
* Re: [PATCH v7 2/2] selftests: add tests for pidfd_send_signal()
From: Christian Brauner @ 2019-01-08 18:24 UTC (permalink / raw)
To: Tycho Andersen
Cc: Serge E. Hallyn, linux-kernel, linux-api, luto, arnd, keescook,
akpm, jannh, oleg, cyphar, viro, linux-fsdevel, dancol, timmurray,
fweimer, tglx, x86, ebiederm
In-Reply-To: <20190108182023.GA28330@cisco>
On Tue, Jan 08, 2019 at 11:20:23AM -0700, Tycho Andersen wrote:
> On Tue, Jan 08, 2019 at 12:17:42PM -0600, Serge E. Hallyn wrote:
> > On Tue, Jan 08, 2019 at 10:58:43AM -0700, Tycho Andersen wrote:
> > > On Tue, Jan 08, 2019 at 11:54:15AM -0600, Serge E. Hallyn wrote:
> > > > On Tue, Jan 08, 2019 at 10:53:06AM -0700, Tycho Andersen wrote:
> > > > > On Wed, Jan 02, 2019 at 05:16:54PM +0100, Christian Brauner wrote:
> > > > > > + /*
> > > > > > + * Stop the child so we can inspect whether we have
> > > > > > + * recycled pid PID_RECYCLE.
> > > > > > + */
> > > > > > + close(pipe_fds[0]);
> > > > > > + ret = kill(recycled_pid, SIGSTOP);
> > > > > > + close(pipe_fds[1]);
> > > > > > + if (ret) {
> > > > > > + (void)wait_for_pid(recycled_pid);
> > > > > > + _exit(PIDFD_ERROR);
> > > > > > + }
> > > > >
> > > > > Sorry for being late to the party, but I wonder if this whole thing
> > > > > couldn't be simplified with /proc/sys/kenrel/ns_last_pid?
> > > >
> > > > no, bc it's not namespaced :)
> > >
> > > Huh? It looks like it is...
> > >
> > > static int pid_ns_ctl_handler(struct ctl_table *table, int write,
> > > void __user *buffer, size_t *lenp, loff_t *ppos)
> > > {
> > > struct pid_namespace *pid_ns = task_active_pid_ns(current);
> > > struct ctl_table tmp = *table;
> > > int ret, next;
> > >
> > > if (write && !ns_capable(pid_ns->user_ns, CAP_SYS_ADMIN))
> > > return -EPERM;
> > >
> > > ...
> >
> > Oh - hah, but that's ns_last_pid. You'd want pid_max. And that one
> > is not namespaced.
>
> Perhaps I'm misunderstanding, but isn't the point of all this code to
> get the same pid again? So can't we just fork(), kill(), then set
> ns_last_pid to pid-1, and fork() again to re-use?
Maybe. It's just a selftest that works reliably as it is so unless
there's a technical issue with the patch I'm not going to do another
version just because of that unless people feel super strongly about
this.
Another advantage is that the code we have right now works even when
CONFIG_CHECKPOINT_RESTORE is not selected.
^ permalink raw reply
* Re: [PATCH v7 2/2] selftests: add tests for pidfd_send_signal()
From: Tycho Andersen @ 2019-01-08 18:25 UTC (permalink / raw)
To: Christian Brauner
Cc: Serge E. Hallyn, linux-kernel, linux-api, luto, arnd, keescook,
akpm, jannh, oleg, cyphar, viro, linux-fsdevel, dancol, timmurray,
fweimer, tglx, x86, ebiederm
In-Reply-To: <20190108182444.e6kq2gqftde23jou@brauner.io>
On Tue, Jan 08, 2019 at 07:24:46PM +0100, Christian Brauner wrote:
> On Tue, Jan 08, 2019 at 11:20:23AM -0700, Tycho Andersen wrote:
> > On Tue, Jan 08, 2019 at 12:17:42PM -0600, Serge E. Hallyn wrote:
> > > On Tue, Jan 08, 2019 at 10:58:43AM -0700, Tycho Andersen wrote:
> > > > On Tue, Jan 08, 2019 at 11:54:15AM -0600, Serge E. Hallyn wrote:
> > > > > On Tue, Jan 08, 2019 at 10:53:06AM -0700, Tycho Andersen wrote:
> > > > > > On Wed, Jan 02, 2019 at 05:16:54PM +0100, Christian Brauner wrote:
> > > > > > > + /*
> > > > > > > + * Stop the child so we can inspect whether we have
> > > > > > > + * recycled pid PID_RECYCLE.
> > > > > > > + */
> > > > > > > + close(pipe_fds[0]);
> > > > > > > + ret = kill(recycled_pid, SIGSTOP);
> > > > > > > + close(pipe_fds[1]);
> > > > > > > + if (ret) {
> > > > > > > + (void)wait_for_pid(recycled_pid);
> > > > > > > + _exit(PIDFD_ERROR);
> > > > > > > + }
> > > > > >
> > > > > > Sorry for being late to the party, but I wonder if this whole thing
> > > > > > couldn't be simplified with /proc/sys/kenrel/ns_last_pid?
> > > > >
> > > > > no, bc it's not namespaced :)
> > > >
> > > > Huh? It looks like it is...
> > > >
> > > > static int pid_ns_ctl_handler(struct ctl_table *table, int write,
> > > > void __user *buffer, size_t *lenp, loff_t *ppos)
> > > > {
> > > > struct pid_namespace *pid_ns = task_active_pid_ns(current);
> > > > struct ctl_table tmp = *table;
> > > > int ret, next;
> > > >
> > > > if (write && !ns_capable(pid_ns->user_ns, CAP_SYS_ADMIN))
> > > > return -EPERM;
> > > >
> > > > ...
> > >
> > > Oh - hah, but that's ns_last_pid. You'd want pid_max. And that one
> > > is not namespaced.
> >
> > Perhaps I'm misunderstanding, but isn't the point of all this code to
> > get the same pid again? So can't we just fork(), kill(), then set
> > ns_last_pid to pid-1, and fork() again to re-use?
>
> Maybe. It's just a selftest that works reliably as it is so unless
> there's a technical issue with the patch I'm not going to do another
> version just because of that unless people feel super strongly about
> this.
> Another advantage is that the code we have right now works even when
> CONFIG_CHECKPOINT_RESTORE is not selected.
No, it's fine as is. Just a lot less code if we do it the other way.
Cheers,
Tycho
^ permalink raw reply
* [PATCH] arm64: introduce AUDIT_ARCH_AARCH64ILP32 for ilp32
From: Yuri Norov @ 2019-01-08 21:18 UTC (permalink / raw)
To: Catalin Marinas, Arnd Bergmann,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
linux-arch@vger.kernel.org, linux-api@vger.kernel.org,
Adam Borowski, Alexander Graf, Alexey Klimov, Andreas Schwab,
Andrew Pinski, Bamvor Zhangjian, Chris Metcalf,
Christoph Muellner, Dave Martin, David S . Miller, Florian Weimer,
Geert Uytterhoeven, Heiko
Cc: Andy Lutomirski
In-Reply-To: <20190107155028.GA25178@yury-thinkpad>
Make syscall_get_arch() distinguish arm64 and arm64/ilp32 by adding
AUDIT_ARCH_AARCH64ILP32.
Sugested-by: Andy Lutomirski <luto@amacapital.net>
Signed-off-by: Yury Norov <ynorov@marvell.com>
---
arch/arm64/include/asm/syscall.h | 3 +++
include/uapi/linux/audit.h | 1 +
2 files changed, 4 insertions(+)
diff --git a/arch/arm64/include/asm/syscall.h b/arch/arm64/include/asm/syscall.h
index 73fbe08763b0..77578d703cb1 100644
--- a/arch/arm64/include/asm/syscall.h
+++ b/arch/arm64/include/asm/syscall.h
@@ -126,6 +126,9 @@ static inline int syscall_get_arch(void)
if (is_a32_compat_task())
return AUDIT_ARCH_ARM;
+ if (is_ilp32_compat_task())
+ return AUDIT_ARCH_AARCH64ILP32;
+
return AUDIT_ARCH_AARCH64;
}
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 818ae690ab79..624127147404 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -374,6 +374,7 @@ enum {
#define __AUDIT_ARCH_LE 0x40000000
#define AUDIT_ARCH_AARCH64 (EM_AARCH64|__AUDIT_ARCH_64BIT|__AUDIT_ARCH_LE)
+#define AUDIT_ARCH_AARCH64ILP32 (EM_AARCH64|__AUDIT_ARCH_LE)
#define AUDIT_ARCH_ALPHA (EM_ALPHA|__AUDIT_ARCH_64BIT|__AUDIT_ARCH_LE)
#define AUDIT_ARCH_ARM (EM_ARM|__AUDIT_ARCH_LE)
#define AUDIT_ARCH_ARMEB (EM_ARM)
--
2.17.1
^ permalink raw reply related
* Re: [RFC PATCH v1 3/5] Yama: Enforces noexec mounts or file executability through O_MAYEXEC
From: Kees Cook @ 2019-01-08 23:30 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Jann Horn, Mickaël Salaün, kernel list, Al Viro,
James Morris, Jonathan Corbet, Matthew Garrett,
Michael Kerrisk-manpages, Mimi Zohar, philippe.trebuchet,
Shuah Khan, thibaut.sautereau, vincent.strubel, Perez Yves-Alexis,
Kernel Hardening, Linux API, linux-security-module,
linux-fsdevel@vger.kernel.org
In-Reply-To: <0f7d39f8-035b-8566-94c9-ea836b280e24@ssi.gouv.fr>
On Tue, Jan 8, 2019 at 5:29 AM Mickaël Salaün
<mickael.salaun@ssi.gouv.fr> wrote:
>
>
> On 03/01/2019 12:17, Jann Horn wrote:
> > On Thu, Dec 13, 2018 at 3:49 PM Mickaël Salaün
> > <mickael.salaun@ssi.gouv.fr> wrote:
> >> On 12/12/2018 18:09, Jann Horn wrote:
> >>> On Wed, Dec 12, 2018 at 9:18 AM Mickaël Salaün <mic@digikod.net> wrote:
> >>>> Enable to either propagate the mount options from the underlying VFS
> >>>> mount to prevent execution, or to propagate the file execute permission.
> >>>> This may allow a script interpreter to check execution permissions
> >>>> before reading commands from a file.
> >>>>
> >>>> The main goal is to be able to protect the kernel by restricting
> >>>> arbitrary syscalls that an attacker could perform with a crafted binary
> >>>> or certain script languages. It also improves multilevel isolation
> >>>> by reducing the ability of an attacker to use side channels with
> >>>> specific code. These restrictions can natively be enforced for ELF
> >>>> binaries (with the noexec mount option) but require this kernel
> >>>> extension to properly handle scripts (e.g., Python, Perl).
I like this idea, but I think it shouldn't live in Yama (since it is
currently intended to be a ptrace-policy-only LSM). It was
_originally_ designed to do various DAC improvements, but the
agreement was that those should live directly in the VFS instead (i.e.
the symlink, hardlink and now fifo and regular file defenses).
This should likely go in similarly. (But if not, it could also be its own LSM.)
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v7 1/2] signal: add pidfd_send_signal() syscall
From: Christian Brauner @ 2019-01-08 23:47 UTC (permalink / raw)
To: linux-kernel, linux-api, luto, arnd, serge, keescook, akpm
Cc: jannh, oleg, cyphar, viro, linux-fsdevel, dancol, timmurray,
fweimer, tglx, x86, ebiederm
In-Reply-To: <20190102161654.9093-1-christian@brauner.io>
On Wed, Jan 02, 2019 at 05:16:53PM +0100, Christian Brauner wrote:
> The kill() syscall operates on process identifiers (pid). After a process
> has exited its pid can be reused by another process. If a caller sends a
> signal to a reused pid it will end up signaling the wrong process. This
> issue has often surfaced and there has been a push to address this problem [1].
>
> This patch uses file descriptors (fd) from proc/<pid> as stable handles on
> struct pid. Even if a pid is recycled the handle will not change. The fd
> can be used to send signals to the process it refers to.
> Thus, the new syscall pidfd_send_signal() is introduced to solve this
> problem. Instead of pids it operates on process fds (pidfd).
>
> /* prototype and argument /*
> long pidfd_send_signal(int pidfd, int sig, siginfo_t *info, unsigned int flags);
>
> In addition to the pidfd and signal argument it takes an additional
> siginfo_t and flags argument. If the siginfo_t argument is NULL then
> pidfd_send_signal() is equivalent to kill(<positive-pid>, <signal>). If it
> is not NULL pidfd_send_signal() is equivalent to rt_sigqueueinfo().
> The flags argument is added to allow for future extensions of this syscall.
> It currently needs to be passed as 0. Failing to do so will cause EINVAL.
>
> /* pidfd_send_signal() replaces multiple pid-based syscalls */
> The pidfd_send_signal() syscall currently takes on the job of
> rt_sigqueueinfo(2) and parts of the functionality of kill(2), Namely, when a
> positive pid is passed to kill(2). It will however be possible to also
> replace tgkill(2) and rt_tgsigqueueinfo(2) if this syscall is extended.
>
> /* sending signals to threads (tid) and process groups (pgid) */
> Specifically, the pidfd_send_signal() syscall does currently not operate on
> process groups or threads. This is left for future extensions.
> In order to extend the syscall to allow sending signal to threads and
> process groups appropriately named flags (e.g. PIDFD_TYPE_PGID, and
> PIDFD_TYPE_TID) should be added. This implies that the flags argument will
> determine what is signaled and not the file descriptor itself. Put in other
> words, grouping in this api is a property of the flags argument not a
> property of the file descriptor (cf. [13]). Clarification for this has been
> requested by Eric (cf. [19]).
> When appropriate extensions through the flags argument are added then
> pidfd_send_signal() can additionally replace the part of kill(2) which
> operates on process groups as well as the tgkill(2) and
> rt_tgsigqueueinfo(2) syscalls.
> How such an extension could be implemented has been very roughly sketched
> in [14], [15], and [16]. However, this should not be taken as a commitment
> to a particular implementation. There might be better ways to do it.
> Right now this is intentionally left out to keep this patchset as simple as
> possible (cf. [4]). For example, if a pidfd for a tid from
> /proc/<pid>/task/<tid> is passed EOPNOTSUPP will be returned to give
> userspace a way to detect when I add support for signaling to threads (cf. [10]).
>
> /* naming */
> The syscall had various names throughout iterations of this patchset:
> - procfd_signal()
> - procfd_send_signal()
> - taskfd_send_signal()
> In the last round of reviews it was pointed out that given that if the
> flags argument decides the scope of the signal instead of different types
> of fds it might make sense to either settle for "procfd_" or "pidfd_" as
> prefix. The community was willing to accept either (cf. [17] and [18]).
> Given that one developer expressed strong preference for the "pidfd_"
> prefix (cf. [13] and with other developers less opinionated about the name
> we should settle for "pidfd_" to avoid further bikeshedding.
>
> The "_send_signal" suffix was chosen to reflect the fact that the syscall
> takes on the job of multiple syscalls. It is therefore intentional that the
> name is not reminiscent of neither kill(2) nor rt_sigqueueinfo(2). Not the
> fomer because it might imply that pidfd_send_signal() is a replacement for
> kill(2), and not the latter because it is a hassle to remember the correct
> spelling - especially for non-native speakers - and because it is not
> descriptive enough of what the syscall actually does. The name
> "pidfd_send_signal" makes it very clear that its job is to send signals.
>
> /* zombies */
> Zombies can be signaled just as any other process. No special error will be
> reported since a zombie state is an unreliable state (cf. [3]). However,
> this can be added as an extension through the @flags argument if the need
> ever arises.
>
> /* cross-namespace signals */
> The patch currently enforces that the signaler and signalee either are in
> the same pid namespace or that the signaler's pid namespace is an ancestor
> of the signalee's pid namespace. This is done for the sake of simplicity
> and because it is unclear to what values certain members of struct
> siginfo_t would need to be set to (cf. [5], [6]).
>
> /* compat syscalls */
> It became clear that we would like to avoid adding compat syscalls
> (cf. [7]). The compat syscall handling is now done in kernel/signal.c
> itself by adding __copy_siginfo_from_user_generic() which lets us avoid
> compat syscalls (cf. [8]). It should be noted that the addition of
> __copy_siginfo_from_user_any() is caused by a bug in the original
> implementation of rt_sigqueueinfo(2) (cf. 12).
> With upcoming rework for syscall handling things might improve
> significantly (cf. [11]) and __copy_siginfo_from_user_any() will not gain
> any additional callers.
>
> /* testing */
> This patch was tested on x64 and x86.
>
> /* userspace usage */
> An asciinema recording for the basic functionality can be found under [9].
> With this patch a process can be killed via:
>
> #define _GNU_SOURCE
> #include <errno.h>
> #include <fcntl.h>
> #include <signal.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <sys/stat.h>
> #include <sys/syscall.h>
> #include <sys/types.h>
> #include <unistd.h>
>
> static inline int do_pidfd_send_signal(int pidfd, int sig, siginfo_t *info,
> unsigned int flags)
> {
> #ifdef __NR_pidfd_send_signal
> return syscall(__NR_pidfd_send_signal, pidfd, sig, info, flags);
> #else
> return -ENOSYS;
> #endif
> }
>
> int main(int argc, char *argv[])
> {
> int fd, ret, saved_errno, sig;
>
> if (argc < 3)
> exit(EXIT_FAILURE);
>
> fd = open(argv[1], O_DIRECTORY | O_CLOEXEC);
> if (fd < 0) {
> printf("%s - Failed to open \"%s\"\n", strerror(errno), argv[1]);
> exit(EXIT_FAILURE);
> }
>
> sig = atoi(argv[2]);
>
> printf("Sending signal %d to process %s\n", sig, argv[1]);
> ret = do_pidfd_send_signal(fd, sig, NULL, 0);
>
> saved_errno = errno;
> close(fd);
> errno = saved_errno;
>
> if (ret < 0) {
> printf("%s - Failed to send signal %d to process %s\n",
> strerror(errno), sig, argv[1]);
> exit(EXIT_FAILURE);
> }
>
> exit(EXIT_SUCCESS);
> }
>
> /* Q&A
> * Given that it seems the same questions get asked again by people who are
> * late to the party it makes sense to add a Q&A section to the commit
> * message so it's hopefully easier to avoid duplicate threads.
> *
> * For the sake of progress please consider these arguments settled unless
> * there is a new point that desperately needs to be addressed. Please make
> * sure to check the links to the threads in this commit message whether
> * this has not already been covered.
> */
> Q-01: (Florian Weimer [20], Andrew Morton [21])
> What happens when the target process has exited?
> A-01: Sending the signal will fail with ESRCH (cf. [22]).
>
> Q-02: (Andrew Morton [21])
> Is the task_struct pinned by the fd?
> A-02: No. A reference to struct pid is kept. struct pid - as far as I
> understand - was created exactly for the reason to not require to
> pin struct task_struct (cf. [22]).
>
> Q-03: (Andrew Morton [21])
> Does the entire procfs directory remain visible? Just one entry
> within it?
> A-03: The same thing that happens right now when you hold a file descriptor
> to /proc/<pid> open (cf. [22]).
>
> Q-04: (Andrew Morton [21])
> Does the pid remain reserved?
> A-04: No. This patchset guarantees a stable handle not that pids are not
> recycled (cf. [22]).
>
> Q-05: (Andrew Morton [21])
> Do attempts to signal that fd return errors?
> A-05: See {Q,A}-01.
>
> Q-06: (Andrew Morton [22])
> Is there a cleaner way of obtaining the fd? Another syscall perhaps.
> A-06: Userspace can already trivially retrieve file descriptors from procfs
> so this is something that we will need to support anyway. Hence,
> there's no immediate need to add another syscalls just to make
> pidfd_send_signal() not dependent on the presence of procfs. However,
> adding a syscalls to get such file descriptors is planned for a
> future patchset (cf. [22]).
>
> Q-07: (Andrew Morton [21] and others)
> This fd-for-a-process sounds like a handy thing and people may well
> think up other uses for it in the future, probably unrelated to
> signals. Are the code and the interface designed to permit such
> future applications?
> A-07: Yes (cf. [22]).
>
> Q-08: (Andrew Morton [21] and others)
> Now I think about it, why a new syscall? This thing is looking
> rather like an ioctl?
> A-08: This has been extensively discussed. It was agreed that a syscall is
> preferred for a variety or reasons. Here are just a few taken from
> prior threads. Syscalls are safer than ioctl()s especially when
> signaling to fds. Processes are a core kernel concept so a syscall
> seems more appropriate. The layout of the syscall with its four
> arguments would require the addition of a custom struct for the
> ioctl() thereby causing at least the same amount or even more
> complexity for userspace than a simple syscall. The new syscall will
> replace multiple other pid-based syscalls (see description above).
> The file-descriptors-for-processes concept introduced with this
> syscall will be extended with other syscalls in the future. See also
> [22], [23] and various other threads already linked in here.
>
> Q-09: (Florian Weimer [24])
> What happens if you use the new interface with an O_PATH descriptor?
> A-09:
> pidfds opened as O_PATH fds cannot be used to send signals to a
> process (cf. [2]). Signaling processes through pidfds is the
> equivalent of writing to a file. Thus, this is not an operation that
> operates "purely at the file descriptor level" as required by the
> open(2) manpage. See also [4].
>
> /* References */
> [1]: https://lore.kernel.org/lkml/20181029221037.87724-1-dancol@google.com/
> [2]: https://lore.kernel.org/lkml/874lbtjvtd.fsf@oldenburg2.str.redhat.com/
> [3]: https://lore.kernel.org/lkml/20181204132604.aspfupwjgjx6fhva@brauner.io/
> [4]: https://lore.kernel.org/lkml/20181203180224.fkvw4kajtbvru2ku@brauner.io/
> [5]: https://lore.kernel.org/lkml/20181121213946.GA10795@mail.hallyn.com/
> [6]: https://lore.kernel.org/lkml/20181120103111.etlqp7zop34v6nv4@brauner.io/
> [7]: https://lore.kernel.org/lkml/36323361-90BD-41AF-AB5B-EE0D7BA02C21@amacapital.net/
> [8]: https://lore.kernel.org/lkml/87tvjxp8pc.fsf@xmission.com/
> [9]: https://asciinema.org/a/IQjuCHew6bnq1cr78yuMv16cy
> [10]: https://lore.kernel.org/lkml/20181203180224.fkvw4kajtbvru2ku@brauner.io/
> [11]: https://lore.kernel.org/lkml/F53D6D38-3521-4C20-9034-5AF447DF62FF@amacapital.net/
> [12]: https://lore.kernel.org/lkml/87zhtjn8ck.fsf@xmission.com/
> [13]: https://lore.kernel.org/lkml/871s6u9z6u.fsf@xmission.com/
> [14]: https://lore.kernel.org/lkml/20181206231742.xxi4ghn24z4h2qki@brauner.io/
> [15]: https://lore.kernel.org/lkml/20181207003124.GA11160@mail.hallyn.com/
> [16]: https://lore.kernel.org/lkml/20181207015423.4miorx43l3qhppfz@brauner.io/
> [17]: https://lore.kernel.org/lkml/CAGXu5jL8PciZAXvOvCeCU3wKUEB_dU-O3q0tDw4uB_ojMvDEew@mail.gmail.com/
> [18]: https://lore.kernel.org/lkml/20181206222746.GB9224@mail.hallyn.com/
> [19]: https://lore.kernel.org/lkml/20181208054059.19813-1-christian@brauner.io/
> [20]: https://lore.kernel.org/lkml/8736rebl9s.fsf@oldenburg.str.redhat.com/
> [21]: https://lore.kernel.org/lkml/20181228152012.dbf0508c2508138efc5f2bbe@linux-foundation.org/
> [22]: https://lore.kernel.org/lkml/20181228233725.722tdfgijxcssg76@brauner.io/
> [23]: https://lwn.net/Articles/773459/
> [24]: https://lore.kernel.org/lkml/8736rebl9s.fsf@oldenburg.str.redhat.com/
>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Jann Horn <jannh@google.com>
> Cc: Andy Lutomirsky <luto@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Florian Weimer <fweimer@redhat.com>
> Signed-off-by: Christian Brauner <christian@brauner.io>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> Acked-by: Serge Hallyn <serge@hallyn.com>
> Acked-by: Aleksa Sarai <cyphar@cyphar.com>
We now have a separate repo on kernel.org for future work related to
pidfds [1]. This should be the target tree from which we can send prs
for new syscalls etc. The target branch is named "pidfd".
Patches for a new merge window will be placed in the "for-next" branch.
The "for-next" branch is already tracked by Stephen in linux-next.
[1]: https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/
Thanks!
Christian
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Dave Chinner @ 2019-01-09 2:24 UTC (permalink / raw)
To: Linus Torvalds
Cc: Matthew Wilcox, Jann Horn, Jiri Kosina, Andrew Morton, Greg KH,
Peter Zijlstra, Michal Hocko, Linux-MM, kernel list, Linux API
In-Reply-To: <CAHk-=wjvzEFQcTGJFh9cyV_MPQftNrjOLon8YMMxaX0G1TLqkg@mail.gmail.com>
On Tue, Jan 08, 2019 at 09:57:49AM -0800, Linus Torvalds wrote:
> On Mon, Jan 7, 2019 at 8:43 PM Dave Chinner <david@fromorbit.com> wrote:
> >
> > So, I read the paper and before I was half way through it I figured
> > there are a bunch of other similar page cache invalidation attacks
> > we can perform without needing mincore. i.e. Focussing on mmap() and
> > mincore() misses the wider issues we have with global shared caches.
>
> Oh, agreed, and that was discussed in the original report too.
>
> The thing is, you can also depend on our pre-faulting of pages in the
> page fault handler, and use that to get the cached status of nearby
> pages. So do something like "fault one page, then do mincore() to see
> how many pages near it were mapped". See our "do_fault_around()"
> logic.
Observing fault-around could help you detect what code an application is
running, but it's not necessary (and can be turned off). Also, such
an it observation is not dependent on using mincore. neither
fault-around nor mincore are required functionality to exploit the
information leaks.
And, FWIW, fault-around actually destroys the information in the
exfiltration channel described in the paper because it perturbs the
carefully constructed page cache residency pattern that encodes the
message.
> But mincore is certainly the easiest interface, and the one that
> doesn't require much effort or setup.
Off the top of my head, here's a few vectors for reading the page
cache residency state without perturbing the page cache residency
pattern:
- mincore
- preadv2(RWF_NOWAIT)
- fadvise(POSIX_FADV_RANDOM); timed read(2) syscalls
- madvise(MADV_RANDOM); timed read of first byte in each page
i.e. mincore is a messenger, but it's not the only trivial
observation technique available. The only difference between mincore
and the others will be the observation latency and hence channel
bandwidth.
IOWs, the question we need to focus on now is not "does breaking
mincore affect anyone", it is "how the hell do we mitigate and
isolate an information leak exposed by fundamental OS functionality
that *everything* depends on for performance"?
> It's also the one where our old
> behavior was actually arguably simply stupid and actively wrong (ie
> "in caches" isn't even strictly speaking a valid question, since the
> caches in question may be invalid).
This is irrelevant to the problem reported. Sure, mincore may be
an awful interface, but it's semantics are not the cause of the
information leak. You're just shooting the messenger...
> I do think that the RWF_NOWAIT case might also be interesting to look at.
As are all the other mechanisms you can use to observer page cache
residency without perturbing it's state.
Keep in mind that the researchers documented a remote observation
technique that leaked the information across the network to a remote
host, so this leak has much, much wider scope than changing mincore
can address...
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Jiri Kosina @ 2019-01-09 2:31 UTC (permalink / raw)
To: Dave Chinner
Cc: Linus Torvalds, Matthew Wilcox, Jann Horn, Andrew Morton, Greg KH,
Peter Zijlstra, Michal Hocko, Linux-MM, kernel list, Linux API
In-Reply-To: <20190109022430.GE27534@dastard>
On Wed, 9 Jan 2019, Dave Chinner wrote:
> > But mincore is certainly the easiest interface, and the one that
> > doesn't require much effort or setup.
>
> Off the top of my head, here's a few vectors for reading the page
> cache residency state without perturbing the page cache residency
> pattern:
> - mincore
> - preadv2(RWF_NOWAIT)
> - fadvise(POSIX_FADV_RANDOM); timed read(2) syscalls
> - madvise(MADV_RANDOM); timed read of first byte in each page
While I obviously agree that all those are creating pagecache sidechannel
in principle, I think we really should mostly focus on the first two (with
mincore() already having been covered).
Rationale has been provided by Daniel Gruss in this thread -- if the
attacker is left with cache timing as the only available vector, he's
going to be much more successful with mounting hardware cache timing
attack anyway.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Dave Chinner @ 2019-01-09 4:39 UTC (permalink / raw)
To: Jiri Kosina
Cc: Linus Torvalds, Matthew Wilcox, Jann Horn, Andrew Morton, Greg KH,
Peter Zijlstra, Michal Hocko, Linux-MM, kernel list, Linux API
In-Reply-To: <nycvar.YFH.7.76.1901090326460.16954@cbobk.fhfr.pm>
On Wed, Jan 09, 2019 at 03:31:35AM +0100, Jiri Kosina wrote:
> On Wed, 9 Jan 2019, Dave Chinner wrote:
>
> > > But mincore is certainly the easiest interface, and the one that
> > > doesn't require much effort or setup.
> >
> > Off the top of my head, here's a few vectors for reading the page
> > cache residency state without perturbing the page cache residency
> > pattern:
> > - mincore
> > - preadv2(RWF_NOWAIT)
> > - fadvise(POSIX_FADV_RANDOM); timed read(2) syscalls
> > - madvise(MADV_RANDOM); timed read of first byte in each page
>
> While I obviously agree that all those are creating pagecache sidechannel
> in principle, I think we really should mostly focus on the first two (with
> mincore() already having been covered).
FWIW, I just realised that the easiest, most reliable way to
invalidate the page cache over a file range is simply to do a
O_DIRECT read on it. IOWs, all three requirements of this
information leak - highly specific, reliable cache invalidation
control, controlled cache instantiation and 3rd-party detection of
cache residency can all be performed with just the read(2)
syscall...
> Rationale has been provided by Daniel Gruss in this thread -- if the
> attacker is left with cache timing as the only available vector, he's
> going to be much more successful with mounting hardware cache timing
> attack anyway.
No, he said:
"Restricting mincore() is sufficient to fix the hardware-agnostic
part."
That's not correct - preadv2(RWF_NOWAIT) is also hardware agnostic
and provides exactly the same information about the page cache as
mincore. Timed read/mmap access loops for cache observation are
also hardware agnostic, and on fast SSD based storage will only be
marginally slower bandwidth than preadv2(RWF_NOWAIT).
Attackers will pick whatever leak vector we don't fix, so we either
fix them all (which I think is probably impossible without removing
caching altogether) or we start thinking about how we need to
isolate the page cache so that information isn't shared across
important security boundaries (e.g. page cache contents are
per-mount namespace).
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Jiri Kosina @ 2019-01-09 10:08 UTC (permalink / raw)
To: Dave Chinner
Cc: Linus Torvalds, Matthew Wilcox, Jann Horn, Andrew Morton, Greg KH,
Peter Zijlstra, Michal Hocko, Linux-MM, kernel list, Linux API
In-Reply-To: <20190109043906.GF27534@dastard>
On Wed, 9 Jan 2019, Dave Chinner wrote:
> FWIW, I just realised that the easiest, most reliable way to invalidate
> the page cache over a file range is simply to do a O_DIRECT read on it.
Neat, good catch indeed. Still, it's only the invalidation part, but the
residency check is the crucial one.
> > Rationale has been provided by Daniel Gruss in this thread -- if the
> > attacker is left with cache timing as the only available vector, he's
> > going to be much more successful with mounting hardware cache timing
> > attack anyway.
>
> No, he said:
>
> "Restricting mincore() is sufficient to fix the hardware-agnostic
> part."
>
> That's not correct - preadv2(RWF_NOWAIT) is also hardware agnostic and
> provides exactly the same information about the page cache as mincore.
Yeah, preadv2(RWF_NOWAIT) is in the same teritory as mincore(), it has
"just" been overlooked. I can't speak for Daniel, but I believe he might
be ok with rephrasing the above as "Restricting mincore() and RWF_NOWAIT
is sufficient ...".
> Timed read/mmap access loops for cache observation are also hardware
> agnostic, and on fast SSD based storage will only be marginally slower
> bandwidth than preadv2(RWF_NOWAIT).
>
> Attackers will pick whatever leak vector we don't fix, so we either fix
> them all (which I think is probably impossible without removing caching
> altogether)
We can't really fix the fact that it's possible to do the timing on the HW
caches though.
> or we start thinking about how we need to isolate the page cache so that
> information isn't shared across important security boundaries (e.g. page
> cache contents are per-mount namespace).
Umm, sorry for being dense, but how would that help that particular attack
scenario on a system that doesn't really employ any namespacing? (which I
still believe is a majority of the systems out there, but I might have
just missed the containers train long time ago :) ).
--
Jiri Kosina
SUSE Labs
^ 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