* Re: [PATCH 1/3] mm: introduce NR_INDIRECTLY_RECLAIMABLE_BYTES
From: Michal Hocko @ 2018-04-12 14:46 UTC (permalink / raw)
To: Roman Gushchin
Cc: Vlastimil Babka, linux-mm, Andrew Morton, Alexander Viro,
Johannes Weiner, linux-fsdevel, linux-kernel, kernel-team,
Linux API
In-Reply-To: <20180412143826.GA30714@castle.DHCP.thefacebook.com>
On Thu 12-04-18 15:38:33, Roman Gushchin wrote:
> On Thu, Apr 12, 2018 at 01:52:17PM +0200, Michal Hocko wrote:
> > On Thu 12-04-18 08:52:52, Vlastimil Babka wrote:
> > > On 04/11/2018 03:56 PM, Roman Gushchin wrote:
> > > > On Wed, Apr 11, 2018 at 03:16:08PM +0200, Vlastimil Babka wrote:
> > [...]
> > > >> With that in mind, can we at least for now put the (manually maintained)
> > > >> byte counter in a variable that's not directly exposed via /proc/vmstat,
> > > >> and then when printing nr_slab_reclaimable, simply add the value
> > > >> (divided by PAGE_SIZE), and when printing nr_slab_unreclaimable,
> > > >> subtract the same value. This way we would be simply making the existing
> > > >> counters more precise, in line with their semantics.
> > > >
> > > > Idk, I don't like the idea of adding a counter outside of the vm counters
> > > > infrastructure, and I definitely wouldn't touch the exposed
> > > > nr_slab_reclaimable and nr_slab_unreclaimable fields.
> >
> > Why?
>
> Both nr_slab_reclaimable and nr_slab_unreclaimable have a very simple
> meaning: they are numbers of pages used by corresponding slab caches.
Right, but if names are reclaimable then they should end up in the
reclaimable slabs and to be accounted as such. Objects themselves are
not sufficient to reclaim the accounted memory.
> In the answer to the very first version of this patchset
> Andrew suggested to generalize the idea to allow further
> accounting of non-kmalloc() allocations.
> I like the idea, even if don't have a good example right now.
Well, I have to disagree here. It sounds completely ad-hoc without
a reasoable semantic. Or how does it help users when they do not know
what is the indirect dependency and how to trigger it.
> The problem with external names existed for many years before
> we've accidentally hit it, so if we don't have other examples
> right now, it doesn't mean that we wouldn't have them in the future.
>
> >
> > > We would be just making the reported values more precise wrt reality.
> >
> > I was suggesting something similar in an earlier discussion. I am not
> > really happy about the new exposed counter either. It is just arbitrary
> > by name yet very specific for this particular usecase.
> >
> > What is a poor user supposed to do with the new counter? Can this be
> > used for any calculations?
>
> For me the most important part is to fix the overcommit logic, because it's
> a real security and production issue.
Sure, the problem is ugly. Not the first one when the unaccounted kernel
allocation can eat a lot of memory. We have many other such. The usual
answer was to use kmemcg accounting.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH 1/3] mm: introduce NR_INDIRECTLY_RECLAIMABLE_BYTES
From: Roman Gushchin @ 2018-04-12 14:38 UTC (permalink / raw)
To: Michal Hocko
Cc: Vlastimil Babka, linux-mm, Andrew Morton, Alexander Viro,
Johannes Weiner, linux-fsdevel, linux-kernel, kernel-team,
Linux API
In-Reply-To: <20180412115217.GC23400@dhcp22.suse.cz>
On Thu, Apr 12, 2018 at 01:52:17PM +0200, Michal Hocko wrote:
> On Thu 12-04-18 08:52:52, Vlastimil Babka wrote:
> > On 04/11/2018 03:56 PM, Roman Gushchin wrote:
> > > On Wed, Apr 11, 2018 at 03:16:08PM +0200, Vlastimil Babka wrote:
> [...]
> > >> With that in mind, can we at least for now put the (manually maintained)
> > >> byte counter in a variable that's not directly exposed via /proc/vmstat,
> > >> and then when printing nr_slab_reclaimable, simply add the value
> > >> (divided by PAGE_SIZE), and when printing nr_slab_unreclaimable,
> > >> subtract the same value. This way we would be simply making the existing
> > >> counters more precise, in line with their semantics.
> > >
> > > Idk, I don't like the idea of adding a counter outside of the vm counters
> > > infrastructure, and I definitely wouldn't touch the exposed
> > > nr_slab_reclaimable and nr_slab_unreclaimable fields.
>
> Why?
Both nr_slab_reclaimable and nr_slab_unreclaimable have a very simple
meaning: they are numbers of pages used by corresponding slab caches.
In the answer to the very first version of this patchset
Andrew suggested to generalize the idea to allow further
accounting of non-kmalloc() allocations.
I like the idea, even if don't have a good example right now.
The problem with external names existed for many years before
we've accidentally hit it, so if we don't have other examples
right now, it doesn't mean that we wouldn't have them in the future.
>
> > We would be just making the reported values more precise wrt reality.
>
> I was suggesting something similar in an earlier discussion. I am not
> really happy about the new exposed counter either. It is just arbitrary
> by name yet very specific for this particular usecase.
>
> What is a poor user supposed to do with the new counter? Can this be
> used for any calculations?
For me the most important part is to fix the overcommit logic, because it's
a real security and production issue. Adjusting MemAvailable is important too.
I really open here for any concrete suggestions on how to do it without exporting
of a new value, and without adding too much complexity to the code
(e.g. skipping this particular mm counter on printing will be quite messy).
Thanks!
^ permalink raw reply
* Re: [PATCH] mmap.2: Add description of MAP_SHARED_VALIDATE and MAP_SYNC
From: Jan Kara @ 2018-04-12 14:22 UTC (permalink / raw)
To: Michael Kerrisk (man-pages)
Cc: xfs, Jan Kara, linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Linux API,
Darrick J . Wong, Christoph Hellwig, Linux-MM,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Ext4 Developers List
In-Reply-To: <CAKgNAkhsFrcdkXNA2cw3o0gJV0uLRtBg9ybaCe5xy1QBC2PgqA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hello Michael!
On Thu 12-04-18 15:00:49, Michael Kerrisk (man-pages) wrote:
> Hello Jan,
>
> I have applied your patch, and tweaked the text a little, and pushed
> the result to the git repo.
Thanks!
> > +.B MAP_SHARED
> > +type will silently ignore this flag.
> > +This flag is supported only for files supporting DAX (direct mapping of persistent
> > +memory). For other files, creating mapping with this flag results in
> > +.B EOPNOTSUPP
> > +error. Shared file mappings with this flag provide the guarantee that while
> > +some memory is writeably mapped in the address space of the process, it will
> > +be visible in the same file at the same offset even after the system crashes or
> > +is rebooted. This allows users of such mappings to make data modifications
> > +persistent in a more efficient way using appropriate CPU instructions.
>
> It feels like there's a word missing/unclear wording in the previous
> line, before "using". Without that word, the sentence feels a bit
> ambiguous.
>
> Should it be:
>
> persistent in a more efficient way *through the use of* appropriate
> CPU instructions.
>
> or:
>
> persistent in a more efficient way *than using* appropriate CPU instructions.
>
> ?
>
> Is suspect the first is correct, but need to check.
Yes, the first is correct.
Honza
--
Jan Kara <jack-IBi9RG/b67k@public.gmane.org>
SUSE Labs, CR
^ permalink raw reply
* [PATCH v2 00/13] y2038: convert IPC syscalls
From: Arnd Bergmann @ 2018-04-12 14:20 UTC (permalink / raw)
To: y2038, linux-kernel
Cc: Arnd Bergmann, linux-api, linux-arch, libc-alpha, tglx,
deepa.kernel, viro, ebiederm, albert.aribaud, linux-s390,
schwidefsky, x86, catalin.marinas, will.deacon, linux-mips,
jhogan, ralf, linuxppc-dev, sparclinux
This is an update of a series I posted a long time ago [1], updating
the IPC subsystem to pass down 64-bit time stamps to user space.
In particular, for sys_msgctl, sys_semctl and sys_shmctl, I do not
introduce a completely new set of replacement system calls, but instead
extend the existing ones to return data in the reserved fields of the
normal data structure.
This should be completely transparent to any existing user space, and
only after the 32-bit time_t wraps, it will make a difference in the
returned data.
libc implementations will consequently have to provide their own data
structures when they move to 64-bit time_t, and convert the structures
in user space from the ones returned by the kernel.
There are three cases here:
- little-endian architectures (except powerpc and mips) can use
the normal layout and just cast the data structure to the user space
type that contains 64-bit numbers.
- parisc and sparc can do the same thing with big-endian user space
- little-endian powerpc and most big-endian architectures have
to flip the upper and lower 32-bit halves of the time_t value in memory,
but can otherwise keep using the normal layout
- mips and big-endian xtensa need to be more careful because
they are not consistent in their definitions, and they have to provide
custom libc implementations for the system calls to use 64-bit time_t.
Changes to the previous version include
- Rebased to the latest kernel (4.17-rc)
- Dropped changes for removed architectures
- Simplified the IPC code changes, based on prior work from
both Deepa and Eric
- Fixed a few bugs that I found during rebasing, in parcular the
sparc version was incorrect.
If everyone agrees with the series, I'd like to have it merged through
the tip tree once Deepa's earlier syscall series in there (I have both
in my y2038 tree [2]).
Arnd
[1] https://lkml.org/lkml/2015/5/20/605
[2] git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git y2038-next
Arnd Bergmann (13):
y2038: asm-generic: extend sysvipc data structures
y2038: alpha: remove unneeded ipc uapi header files
y2038: ia64: remove unneeded ipc uapi header files
y2038: s390: remove unneeded ipc uapi header files
y2038: arm64: extend sysvipc compat data structures
y2038: mips: extend sysvipc data structures
y2038: x86: extend sysvipc data structures
y2038: parisc: extend sysvipc data structures
y2038: sparc: extend sysvipc data structures
y2038: powerpc: extend sysvipc data structures
y2038: xtensa: extend sysvipc data structures
y2038: ipc: use ktime_get_real_seconds consistently
y2038: ipc: report long times to user space
Cc: linux-api@vger.kernel.org
Cc: linux-arch@vger.kernel.org
Cc: libc-alpha@sourceware.org
Cc: tglx@linutronix.de
Cc: deepa.kernel@gmail.com
Cc: viro@zeniv.linux.org.uk
Cc: ebiederm@xmission.com
Cc: albert.aribaud@3adev.fr
Cc: linux-s390@vger.kernel.org
Cc: schwidefsky@de.ibm.com
Cc: x86@kernel.org
Cc: catalin.marinas@arm.com
Cc: will.deacon@arm.com
Cc: linux-mips@linux-mips.org
Cc: jhogan@kernel.org
Cc: ralf@linux-mips.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: sparclinux@vger.kernel.org
arch/alpha/include/asm/Kbuild | 4 +++
arch/alpha/include/uapi/asm/ipcbuf.h | 2 --
arch/alpha/include/uapi/asm/msgbuf.h | 28 -----------------
arch/alpha/include/uapi/asm/sembuf.h | 23 --------------
arch/alpha/include/uapi/asm/shmbuf.h | 39 -----------------------
arch/arm64/include/asm/compat.h | 32 +++++++++----------
arch/ia64/include/asm/Kbuild | 4 +++
arch/ia64/include/uapi/asm/ipcbuf.h | 2 --
arch/ia64/include/uapi/asm/msgbuf.h | 28 -----------------
arch/ia64/include/uapi/asm/sembuf.h | 23 --------------
arch/ia64/include/uapi/asm/shmbuf.h | 39 -----------------------
arch/mips/include/asm/compat.h | 38 ++++++++++++-----------
arch/mips/include/uapi/asm/msgbuf.h | 57 ++++++++++++++++++++++------------
arch/mips/include/uapi/asm/sembuf.h | 15 +++++++--
arch/mips/include/uapi/asm/shmbuf.h | 23 ++++++++++++--
arch/parisc/include/asm/compat.h | 32 +++++++++----------
arch/parisc/include/uapi/asm/msgbuf.h | 33 ++++++++++----------
arch/parisc/include/uapi/asm/sembuf.h | 16 +++++-----
arch/parisc/include/uapi/asm/shmbuf.h | 19 +++++-------
arch/powerpc/include/asm/compat.h | 32 +++++++++----------
arch/powerpc/include/uapi/asm/msgbuf.h | 18 +++++------
arch/powerpc/include/uapi/asm/sembuf.h | 14 ++++-----
arch/powerpc/include/uapi/asm/shmbuf.h | 19 +++++-------
arch/s390/include/asm/Kbuild | 3 ++
arch/s390/include/asm/compat.h | 32 +++++++++----------
arch/s390/include/uapi/asm/msgbuf.h | 38 -----------------------
arch/s390/include/uapi/asm/sembuf.h | 30 ------------------
arch/s390/include/uapi/asm/shmbuf.h | 49 -----------------------------
arch/sparc/include/asm/compat.h | 32 +++++++++----------
arch/sparc/include/uapi/asm/msgbuf.h | 22 ++++++-------
arch/sparc/include/uapi/asm/sembuf.h | 16 +++++-----
arch/sparc/include/uapi/asm/shmbuf.h | 21 ++++++-------
arch/x86/include/asm/compat.h | 32 +++++++++----------
arch/x86/include/uapi/asm/Kbuild | 5 ++-
arch/x86/include/uapi/asm/msgbuf.h | 1 -
arch/x86/include/uapi/asm/sembuf.h | 11 ++++++-
arch/x86/include/uapi/asm/shmbuf.h | 1 -
arch/xtensa/include/uapi/asm/msgbuf.h | 25 +++++++--------
arch/xtensa/include/uapi/asm/sembuf.h | 17 +++++-----
arch/xtensa/include/uapi/asm/shmbuf.h | 37 +++++-----------------
include/uapi/asm-generic/msgbuf.h | 17 +++++-----
include/uapi/asm-generic/sembuf.h | 26 ++++++++++------
include/uapi/asm-generic/shmbuf.h | 17 +++++-----
ipc/msg.c | 20 ++++++++----
ipc/sem.c | 20 ++++++++----
ipc/shm.c | 14 +++++++--
46 files changed, 393 insertions(+), 633 deletions(-)
delete mode 100644 arch/alpha/include/uapi/asm/ipcbuf.h
delete mode 100644 arch/alpha/include/uapi/asm/msgbuf.h
delete mode 100644 arch/alpha/include/uapi/asm/sembuf.h
delete mode 100644 arch/alpha/include/uapi/asm/shmbuf.h
delete mode 100644 arch/ia64/include/uapi/asm/ipcbuf.h
delete mode 100644 arch/ia64/include/uapi/asm/msgbuf.h
delete mode 100644 arch/ia64/include/uapi/asm/sembuf.h
delete mode 100644 arch/ia64/include/uapi/asm/shmbuf.h
delete mode 100644 arch/s390/include/uapi/asm/msgbuf.h
delete mode 100644 arch/s390/include/uapi/asm/sembuf.h
delete mode 100644 arch/s390/include/uapi/asm/shmbuf.h
delete mode 100644 arch/x86/include/uapi/asm/msgbuf.h
delete mode 100644 arch/x86/include/uapi/asm/shmbuf.h
--
2.9.0
^ permalink raw reply
* Re: [PATCH 24/24] debugfs: Restrict debugfs when the kernel is locked down
From: Andy Lutomirski @ 2018-04-12 14:19 UTC (permalink / raw)
To: Greg KH
Cc: Andy Lutomirski, David Howells, Linus Torvalds, linux-man,
Linux API, James Morris, LKML, LSM List
In-Reply-To: <20180412082313.GA6054@kroah.com>
On Thu, Apr 12, 2018 at 1:23 AM, Greg KH <greg@kroah.com> wrote:
> On Wed, Apr 11, 2018 at 07:54:12PM -0700, Andy Lutomirski wrote:
>> On Wed, Apr 11, 2018 at 1:33 PM, Greg KH <greg@kroah.com> wrote:
>> > On Wed, Apr 11, 2018 at 09:09:16PM +0100, David Howells wrote:
>> >> Greg KH <greg@kroah.com> wrote:
>> >>
>> >> > Why not just disable debugfs entirely? This half-hearted way to sorta
>> >> > lock it down is odd, it is meant to not be there at all, nothing in your
>> >> > normal system should ever depend on it.
>> >> >
>> >> > So again just don't allow it to be mounted at all, much simpler and more
>> >> > obvious as to what is going on.
>> >>
>> >> Yeah, I agree - and then I got complaints because it seems that it's been
>> >> abused to allow drivers and userspace components to communicate.
>> >
>> > With in-kernel code? Please let me know and I'll go fix it up to not
>> > allow that, as that is not ok.
>> >
>> > I do know of some bad examples of out-of-tree code abusing debugfs to do
>> > crazy things (battery level monitoring?), but that's their own fault...
>> >
>> > debugfs is for DEBUGGING! For anything you all feel should be "secure",
>> > then just disable it entirely.
>> >
>>
>> Debugfs is very, very useful for, ahem, debugging. I really think
>> this is an example of why we should split lockdown into the read and
>> write varieties and allow mounting and reading debugfs when only write
>> is locked down.
>
> Ok, but be sure that there are no "secrets" in those debugging files if
> you really buy into the whole "lock down" mess...
>
> Really, it's easier to just disable the whole thing.
>
I mostly agree with your sentiment. I'm saying that, for most uses, I
*don't* buy into the idea that a normal secure-boot-supporting distro
should block debugfs. I sometimes like to ask people who report
problems to send me the contents of such-and-such file in debugfs, and
I think it should keep working. Blocking write access to debugfs is
mostly sensible for a lockdown system, but blocking read only makes
sense if you're worried about straight-up bugs or if you think that
debugfs contains protection-worthy secrets.
What I want to see is:
lockdown=protect_integrity: debugfs is read-only, bpf and perf are
unrestricted, iopl and ioperm are disabled, etc.
lockdown=protect_integrity_and_secrecy: debugfs is gone, bpf and perf
are restricted, plus all the restrictions from
lockdown=protect_integrity
Distros should strongly prefer lockdown=protect_integrity (or
lockdown=off) by default. lockdown=protect_integrity_and_secrecy is
for custom setups, embedded applications, etc.
--Andy
^ permalink raw reply
* Re: [PATCH] mmap.2: Add description of MAP_SHARED_VALIDATE and MAP_SYNC
From: Ross Zwisler @ 2018-04-12 14:00 UTC (permalink / raw)
To: Michael Kerrisk (man-pages)
Cc: xfs, Jan Kara, linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Linux API,
Darrick J . Wong, Christoph Hellwig, Linux-MM,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Ext4 Developers List
In-Reply-To: <CAKgNAkhsFrcdkXNA2cw3o0gJV0uLRtBg9ybaCe5xy1QBC2PgqA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Thu, Apr 12, 2018 at 03:00:49PM +0200, Michael Kerrisk (man-pages) wrote:
> Hello Jan,
>
> I have applied your patch, and tweaked the text a little, and pushed
> the result to the git repo.
>
> On 1 November 2017 at 16:36, Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> wrote:
> > Reviewed-by: Ross Zwisler <ross.zwisler-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > Signed-off-by: Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org>
>
> I have a question below.
>
> > ---
> > man2/mmap.2 | 35 ++++++++++++++++++++++++++++++++++-
> > 1 file changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git a/man2/mmap.2 b/man2/mmap.2
> > index 47c3148653be..b38ee6809327 100644
> > --- a/man2/mmap.2
> > +++ b/man2/mmap.2
> > @@ -125,6 +125,21 @@ are carried through to the underlying file.
> > to the underlying file requires the use of
> > .BR msync (2).)
> > .TP
> > +.BR MAP_SHARED_VALIDATE " (since Linux 4.15)"
> > +The same as
> > +.B MAP_SHARED
> > +except that
> > +.B MAP_SHARED
> > +mappings ignore unknown flags in
> > +.IR flags .
> > +In contrast when creating mapping of
> > +.B MAP_SHARED_VALIDATE
> > +mapping type, the kernel verifies all passed flags are known and fails the
> > +mapping with
> > +.BR EOPNOTSUPP
> > +otherwise. This mapping type is also required to be able to use some mapping
> > +flags.
> > +.TP
> > .B MAP_PRIVATE
> > Create a private copy-on-write mapping.
> > Updates to the mapping are not visible to other processes
> > @@ -134,7 +149,10 @@ It is unspecified whether changes made to the file after the
> > .BR mmap ()
> > call are visible in the mapped region.
> > .PP
> > -Both of these flags are described in POSIX.1-2001 and POSIX.1-2008.
> > +.B MAP_SHARED
> > +and
> > +.B MAP_PRIVATE
> > +are described in POSIX.1-2001 and POSIX.1-2008.
> > .PP
> > In addition, zero or more of the following values can be ORed in
> > .IR flags :
> > @@ -352,6 +370,21 @@ option.
> > Because of the security implications,
> > that option is normally enabled only on embedded devices
> > (i.e., devices where one has complete control of the contents of user memory).
> > +.TP
> > +.BR MAP_SYNC " (since Linux 4.15)"
> > +This flags is available only with
> > +.B MAP_SHARED_VALIDATE
> > +mapping type. Mappings of
> > +.B MAP_SHARED
> > +type will silently ignore this flag.
> > +This flag is supported only for files supporting DAX (direct mapping of persistent
> > +memory). For other files, creating mapping with this flag results in
> > +.B EOPNOTSUPP
> > +error. Shared file mappings with this flag provide the guarantee that while
> > +some memory is writeably mapped in the address space of the process, it will
> > +be visible in the same file at the same offset even after the system crashes or
> > +is rebooted. This allows users of such mappings to make data modifications
> > +persistent in a more efficient way using appropriate CPU instructions.
>
> It feels like there's a word missing/unclear wording in the previous
> line, before "using". Without that word, the sentence feels a bit
> ambiguous.
>
> Should it be:
>
> persistent in a more efficient way *through the use of* appropriate
> CPU instructions.
>
> or:
>
> persistent in a more efficient way *than using* appropriate CPU instructions.
>
> ?
>
> Is suspect the first is correct, but need to check.
You're right, the first one is correct.
^ permalink raw reply
* Re: [PATCH 01/24] Add the ability to lock down access to the running kernel image
From: Justin Forbes @ 2018-04-12 13:09 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jordan Glover, David Howells, linux-man, Linux API, James Morris,
Linux Kernel Mailing List, LSM List
In-Reply-To: <CA+55aFzvnf=4OeBy5vQQ6HQoCsgCkAJw1LhSBOTkDWm3ck1pZA@mail.gmail.com>
On Wed, Apr 11, 2018, 5:38 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Wed, Apr 11, 2018 at 2:05 PM, Jordan Glover
> <Golden_Miller83@protonmail.ch> wrote:
> >>
> >> If that /dev/mem access prevention was just instead done as an even
> >> stricter mode of the existing CONFIG_STRICT_DEVMEM, it could just be
> >> enabled unconditionally.
> >
> > CONFIG_DEVMEM=n
>
> It's actually CONFIG_DEVMEM, CONFIG_DEVKMEM and CONFIG_DEVPORT, it's
> just not obvious from the patch.
>
> But the important part is this part:
>
> >> So I would seriously ask that the distros that have been using these
> >> patches look at which parts of lockdown they could make unconditional
> >> (because it doesn't break machines), and which ones need that escape
> >> clause.
>
> .. because I get the feeling that not a lot of people have actually
> been testing this, because "turn off secure boot" is such a universal
> thing when people boot Linux.
>
> So it's really the whole claim that distributions have been running
> for this for the last five years that I wonder about, and how often
> people end up being told: "just disable secure boot":.
Very rarely in my experience. And the one time that we sent a kernel
to updates-testing that was signed with the test key instead of the
real key, we had a surprisingly high number of reports from users that
it was broken before the update even got synched to mirrors. So we
don't have actual numbers of users running active secure boot with
Fedora, but we do know it is more than we expected. The majority of
people who do run into issues are those running out of tree modules,
who haven't imported any sort of key for local signing. This isn't
like SELinux was at launch where it was so invasive that a large
number of users instinctively turned it off with every installation, I
would guess even people who turned it off in the past, don't even
think about it when they get a new machine and leave it on.
> But if people really don't need DEVMEM/DEVKMEM/DEVPORT, maybe we
> should just disable them in the default configs, and consider them
> legacy.
>
> I'm just surprised. I suspect a lot of people end up actually using
> devmem as a fallback for dmidecode etc. Maybe those people don't boot
> with EFI secure mode, but if so that just shows that this whole
> "hardening" is just security theater.
>
> Linus
^ permalink raw reply
* Re: [PATCH] mmap.2: Add description of MAP_SHARED_VALIDATE and MAP_SYNC
From: Michael Kerrisk (man-pages) @ 2018-04-12 13:00 UTC (permalink / raw)
To: Jan Kara
Cc: xfs, linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Linux API,
Darrick J . Wong, Christoph Hellwig, Linux-MM,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Ext4 Developers List
In-Reply-To: <20171101153648.30166-20-jack-AlSwsSmVLrQ@public.gmane.org>
Hello Jan,
I have applied your patch, and tweaked the text a little, and pushed
the result to the git repo.
On 1 November 2017 at 16:36, Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org> wrote:
> Reviewed-by: Ross Zwisler <ross.zwisler-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Signed-off-by: Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org>
I have a question below.
> ---
> man2/mmap.2 | 35 ++++++++++++++++++++++++++++++++++-
> 1 file changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/man2/mmap.2 b/man2/mmap.2
> index 47c3148653be..b38ee6809327 100644
> --- a/man2/mmap.2
> +++ b/man2/mmap.2
> @@ -125,6 +125,21 @@ are carried through to the underlying file.
> to the underlying file requires the use of
> .BR msync (2).)
> .TP
> +.BR MAP_SHARED_VALIDATE " (since Linux 4.15)"
> +The same as
> +.B MAP_SHARED
> +except that
> +.B MAP_SHARED
> +mappings ignore unknown flags in
> +.IR flags .
> +In contrast when creating mapping of
> +.B MAP_SHARED_VALIDATE
> +mapping type, the kernel verifies all passed flags are known and fails the
> +mapping with
> +.BR EOPNOTSUPP
> +otherwise. This mapping type is also required to be able to use some mapping
> +flags.
> +.TP
> .B MAP_PRIVATE
> Create a private copy-on-write mapping.
> Updates to the mapping are not visible to other processes
> @@ -134,7 +149,10 @@ It is unspecified whether changes made to the file after the
> .BR mmap ()
> call are visible in the mapped region.
> .PP
> -Both of these flags are described in POSIX.1-2001 and POSIX.1-2008.
> +.B MAP_SHARED
> +and
> +.B MAP_PRIVATE
> +are described in POSIX.1-2001 and POSIX.1-2008.
> .PP
> In addition, zero or more of the following values can be ORed in
> .IR flags :
> @@ -352,6 +370,21 @@ option.
> Because of the security implications,
> that option is normally enabled only on embedded devices
> (i.e., devices where one has complete control of the contents of user memory).
> +.TP
> +.BR MAP_SYNC " (since Linux 4.15)"
> +This flags is available only with
> +.B MAP_SHARED_VALIDATE
> +mapping type. Mappings of
> +.B MAP_SHARED
> +type will silently ignore this flag.
> +This flag is supported only for files supporting DAX (direct mapping of persistent
> +memory). For other files, creating mapping with this flag results in
> +.B EOPNOTSUPP
> +error. Shared file mappings with this flag provide the guarantee that while
> +some memory is writeably mapped in the address space of the process, it will
> +be visible in the same file at the same offset even after the system crashes or
> +is rebooted. This allows users of such mappings to make data modifications
> +persistent in a more efficient way using appropriate CPU instructions.
It feels like there's a word missing/unclear wording in the previous
line, before "using". Without that word, the sentence feels a bit
ambiguous.
Should it be:
persistent in a more efficient way *through the use of* appropriate
CPU instructions.
or:
persistent in a more efficient way *than using* appropriate CPU instructions.
?
Is suspect the first is correct, but need to check.
Cheers,
Michael
--
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 manpages] membarrier.2: New membarrier commands introduced in 4.16
From: Mathieu Desnoyers @ 2018-04-12 12:20 UTC (permalink / raw)
To: Michael Kerrisk
Cc: linux-kernel, linux-api, Ingo Molnar, Peter Zijlstra,
Thomas Gleixner, Andy Lutomirski, Paul E. McKenney, Boqun Feng,
Andrew Hunter, maged michael, Avi Kivity, Benjamin Herrenschmidt,
Paul Mackerras, Michael Ellerman, Dave Watson, H. Peter Anvin,
Andrea Parri, Russell King, ARM Linux, Greg Hackmann, Will
In-Reply-To: <CAKgNAkiGXL6Th4DGS_OD0MuwCtM6f7_nAehOVcbpU=u5L9atKA@mail.gmail.com>
----- On Apr 12, 2018, at 7:42 AM, Michael Kerrisk mtk.manpages@gmail.com wrote:
Hi Michael,
[...]
>
> I have applied the above patch, and done quite a bit of tweaking, and
> pushed the results to the git repo.
>
> I would be grateful if you would read the entire manual page as it
> currently stands, to see if anything needs improving. I isolated some
> of the more significant changes into a simple patch, shown below, and
> especially I'd like your confirmation that all of those changes are
> okay.
Thanks for applying my membarrier man pages updates. I've reviewed the
result and it is all good.
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH] mm/page_alloc: don't reserve ZONE_HIGHMEM for ZONE_MOVABLE request
From: Michal Hocko @ 2018-04-12 12:01 UTC (permalink / raw)
To: Joonsoo Kim
Cc: Andrew Morton, Vlastimil Babka, Mel Gorman, Johannes Weiner,
Aneesh Kumar K . V, Minchan Kim, Linux Memory Management List,
LKML, Joonsoo Kim, linux-api
In-Reply-To: <CAAmzW4NGv7RyCYyokPoj4aR3ySKub4jaBZ3k=pt_YReFbByvsw@mail.gmail.com>
On Wed 04-04-18 09:24:06, Joonsoo Kim wrote:
> 2017-09-14 22:24 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> > [Sorry for a later reply]
> >
> > On Wed 06-09-17 13:35:25, Joonsoo Kim wrote:
> >> From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> >>
> >> Freepage on ZONE_HIGHMEM doesn't work for kernel memory so it's not that
> >> important to reserve.
> >
> > I am still not convinced this is a good idea. I do agree that reserving
> > memory in both HIGHMEM and MOVABLE is just wasting memory but removing
> > the reserve from the highmem as well will result that an oom victim will
> > allocate from lower zones and that might have unexpected side effects.
>
> Looks like you are confused.
>
> This patch only affects the situation that ZONE_HIGHMEM and ZONE_MOVABLE is
> used at the same time. In that case, before this patch, ZONE_HIGHMEM has
> reserve for GFP_HIGHMEM | GFP_MOVABLE request, but, with this patch, no reserve
> in ZONE_HIGHMEM for GFP_HIGHMEM | GFP_MOVABLE request. This perfectly
> matchs with your hope. :)
I have forgot all the details but my vague recollection is that the
concern was that GFP_HIGHUSER_MOVABLE etc. wouldn't keep any reserve in
the highmem zone and so emergency allocations - e.g. those during OOM
will have to fallback to kernel zones and might lead to hard to predict
results. Am I still confused and this will not happen after the patch?
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH 1/3] mm: introduce NR_INDIRECTLY_RECLAIMABLE_BYTES
From: Michal Hocko @ 2018-04-12 11:52 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Roman Gushchin, linux-mm, Andrew Morton, Alexander Viro,
Johannes Weiner, linux-fsdevel, linux-kernel, kernel-team,
Linux API
In-Reply-To: <46dbe2a5-e65f-8b72-f835-0210bc445e52@suse.cz>
On Thu 12-04-18 08:52:52, Vlastimil Babka wrote:
> On 04/11/2018 03:56 PM, Roman Gushchin wrote:
> > On Wed, Apr 11, 2018 at 03:16:08PM +0200, Vlastimil Babka wrote:
[...]
> >> With that in mind, can we at least for now put the (manually maintained)
> >> byte counter in a variable that's not directly exposed via /proc/vmstat,
> >> and then when printing nr_slab_reclaimable, simply add the value
> >> (divided by PAGE_SIZE), and when printing nr_slab_unreclaimable,
> >> subtract the same value. This way we would be simply making the existing
> >> counters more precise, in line with their semantics.
> >
> > Idk, I don't like the idea of adding a counter outside of the vm counters
> > infrastructure, and I definitely wouldn't touch the exposed
> > nr_slab_reclaimable and nr_slab_unreclaimable fields.
Why?
> We would be just making the reported values more precise wrt reality.
I was suggesting something similar in an earlier discussion. I am not
really happy about the new exposed counter either. It is just arbitrary
by name yet very specific for this particular usecase.
What is a poor user supposed to do with the new counter? Can this be
used for any calculations?
--
Michal Hocko
SUSE Lab
^ permalink raw reply
* Re: [PATCH manpages] membarrier.2: New membarrier commands introduced in 4.16
From: Michael Kerrisk (man-pages) @ 2018-04-12 11:42 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: lkml, Linux API, Ingo Molnar, Peter Zijlstra, Thomas Gleixner,
Andy Lutomirski, Paul E . McKenney, Boqun Feng, Andrew Hunter,
Maged Michael, Avi Kivity, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Dave Watson, H . Peter Anvin, Andrea Parri,
Russell King, Greg Hackmann, Will Deacon
In-Reply-To: <20180212195549.11485-1-mathieu.desnoyers@efficios.com>
Hello Mathieu,
On 12 February 2018 at 20:55, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
> Document the following membarrier commands introduced in 4.16:
> - MEMBARRIER_CMD_GLOBAL_EXPEDITED (the old enum label
> MEMBARRIER_CMD_SHARED is now an alias to preserve header backward
> compatibility),
> - MEMBARRIER_CMD_GLOBAL_EXPEDITED,
> - MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED,
> - MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE,
> - MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE.
>
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> CC: Michael Kerrisk <mtk.manpages@gmail.com>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: Peter Zijlstra <peterz@infradead.org>
> CC: Thomas Gleixner <tglx@linutronix.de>
> ---
> man2/membarrier.2 | 73 ++++++++++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 62 insertions(+), 11 deletions(-)
>
> diff --git a/man2/membarrier.2 b/man2/membarrier.2
> index c47bc875a..e878301ca 100644
> --- a/man2/membarrier.2
> +++ b/man2/membarrier.2
> @@ -80,7 +80,7 @@ This command is always supported (on kernels where
> .BR membarrier ()
> is provided).
> .TP
> -.B MEMBARRIER_CMD_SHARED
> +.B MEMBARRIER_CMD_GLOBAL " (since Linux 4.16)"
> Ensure that all threads from all processes on the system pass through a
> state where all memory accesses to user-space addresses match program
> order between entry to and return from the
> @@ -88,7 +88,30 @@ order between entry to and return from the
> system call.
> All threads on the system are targeted by this command.
> .TP
> -.BR MEMBARRIER_CMD_PRIVATE_EXPEDITED " (since Linux 4.14)"
> +.B MEMBARRIER_CMD_GLOBAL_EXPEDITED " (since Linux 4.16)"
> +Execute a memory barrier on all running threads of all processes which
> +previously registered with
> +.BR MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED .
> +Upon return from system call, the caller thread is ensured that all
> +running threads have passed through a state where all memory accesses to
> +user-space addresses match program order between entry to and return
> +from the system call (non-running threads are de facto in such a state).
> +This only covers threads from processes which registered with
> +.BR MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED .
> +Given that registration is about the intent to receive the barriers, it
> +is valid to invoke
> +.BR MEMBARRIER_CMD_GLOBAL_EXPEDITED
> +from a non-registered process.
> +.IP
> +The "expedited" commands complete faster than the non-expedited ones;
> +they never block, but have the downside of causing extra overhead.
> +.TP
> +.B MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED " (since Linux 4.16)"
> +Register the process intent to receive
> +.BR MEMBARRIER_CMD_GLOBAL_EXPEDITED
> +memory barriers.
> +.TP
> +.B MEMBARRIER_CMD_PRIVATE_EXPEDITED " (since Linux 4.14)"
> Execute a memory barrier on each running thread belonging to the same
> process as the current thread.
> Upon return from system call, the calling
> @@ -103,9 +126,29 @@ they never block, but have the downside of causing extra overhead.
> A process needs to register its intent to use the private
> expedited command prior to using it.
> .TP
> -.BR MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED " (since Linux 4.14)"
> +.B MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED " (since Linux 4.14)"
> Register the process's intent to use
> -.BR MEMBARRIER_CMD_PRIVATE_EXPEDITED .
> +.B MEMBARRIER_CMD_PRIVATE_EXPEDITED .
> +.TP
> +.B MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE " (since Linux 4.16)"
> +In addition to provide memory ordering guarantees described in
> +.BR MEMBARRIER_CMD_PRIVATE_EXPEDITED ,
> +ensure the caller thread, upon return from system call, that all its
> +running threads siblings have executed a core serializing instruction.
> +This only covers threads from the same process as the caller thread.
> +The "expedited" commands complete faster than the non-expedited ones,
> +they never block, but have the downside of causing extra overhead. A
> +process needs to register its intent to use the private expedited sync
> +core command prior to using it.
> +.TP
> +.B MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE " (since Linux 4.16)"
> +Register the process intent to use
> +.BR MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE .
> +.TP
> +.B MEMBARRIER_CMD_SHARED
> + Alias to
> +.BR MEMBARRIER_CMD_GLOBAL .
> +Provided for header backward compatibility.
> .PP
> The
> .I flags
> @@ -137,10 +180,14 @@ The pair ordering is detailed as (O: ordered, X: not ordered):
> On success, the
> .B MEMBARRIER_CMD_QUERY
> operation returns a bit mask of supported commands, and the
> -.B MEMBARRIER_CMD_SHARED ,
> +.B MEMBARRIER_CMD_GLOBAL ,
> +.B MEMBARRIER_CMD_GLOBAL_EXPEDITED ,
> +.B MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED ,
> .B MEMBARRIER_CMD_PRIVATE_EXPEDITED ,
> -and
> .B MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED ,
> +.B MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE ,
> +and
> +.B MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE
> operations return zero.
> On error, \-1 is returned,
> and
> @@ -163,10 +210,14 @@ set to 0, error handling is required only for the first call to
> is invalid, or
> .I flags
> is nonzero, or the
> -.BR MEMBARRIER_CMD_SHARED
> +.BR MEMBARRIER_CMD_GLOBAL
> command is disabled because the
> .I nohz_full
> -CPU parameter has been set.
> +CPU parameter has been set, or the
> +.BR MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE
> +and
> +.BR MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE
> +commands are not implemented by the architecture.
> .TP
> .B ENOSYS
> The
> @@ -294,9 +345,9 @@ init_membarrier(void)
> return \-1;
> }
>
> - if (!(ret & MEMBARRIER_CMD_SHARED)) {
> + if (!(ret & MEMBARRIER_CMD_GLOBAL)) {
> fprintf(stderr,
> - "membarrier does not support MEMBARRIER_CMD_SHARED\\n");
> + "membarrier does not support MEMBARRIER_CMD_GLOBAL\\n");
> return \-1;
> }
>
> @@ -315,7 +366,7 @@ static void
> slow_path(int *read_a)
> {
> b = 1;
> - membarrier(MEMBARRIER_CMD_SHARED, 0);
> + membarrier(MEMBARRIER_CMD_GLOBAL, 0);
> *read_a = a;
> }
I have applied the above patch, and done quite a bit of tweaking, and
pushed the results to the git repo.
I would be grateful if you would read the entire manual page as it
currently stands, to see if anything needs improving. I isolated some
of the more significant changes into a simple patch, shown below, and
especially I'd like your confirmation that all of those changes are
okay.
Cheers,
Michael
diff --git a/man2/membarrier.2 b/man2/membarrier.2
index b3a94f95f..81d573dd5 100644
--- a/man2/membarrier.2
+++ b/man2/membarrier.2
@@ -92,16 +92,18 @@ All threads on the system are targeted by this command.
Execute a memory barrier on all running threads of all processes that
previously registered with
.BR MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED .
-Upon return from the system call, the calling thread is ensured that all
+Upon return from the system call, the calling thread has a guarantee that all
running threads have passed through a state where all memory accesses to
user-space addresses match program order between entry to and return
from the system call (non-running threads are de facto in such a state).
-This covers only threads from processes which registered with
+This guarantee is provided only for the threads of processes that
+previously registered with
.BR MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED .
Given that registration is about the intent to receive the barriers, it
is valid to invoke
.BR MEMBARRIER_CMD_GLOBAL_EXPEDITED
-from a non-registered process.
+from a process that has not employed
+.BR MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED .
.IP
The "expedited" commands complete faster than the non-expedited ones;
they never block, but have the downside of causing extra overhead.
@@ -113,17 +115,18 @@ memory barriers.
.TP
.BR MEMBARRIER_CMD_PRIVATE_EXPEDITED " (since Linux 4.14)"
Execute a memory barrier on each running thread belonging to the same
-process as the current thread.
-Upon return from system call, the calling
-thread is assured that all its running threads siblings have passed
+process as the calling thread.
+Upon return from the system call, the calling
+thread has a guarantee that all its running thread siblings have passed
through a state where all memory accesses to user-space addresses match
program order between entry to and return from the system call
(non-running threads are de facto in such a state).
-This covers only threads from the same process as the calling thread.
+This guarantee is provided only for threads in
+the same process as the calling thread.
.IP
The "expedited" commands complete faster than the non-expedited ones;
they never block, but have the downside of causing extra overhead.
-A process needs to register its intent to use the private
+A process must register its intent to use the private
expedited command prior to using it.
.TP
.BR MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED " (since Linux 4.14)"
@@ -133,12 +136,13 @@ Register the process's intent to use
.BR MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE " (since Linux 4.16)"
In addition to providing the memory ordering guarantees described in
.BR MEMBARRIER_CMD_PRIVATE_EXPEDITED ,
-ensure the calling thread, upon return from system call, that all its
-running threads siblings have executed a core serializing instruction.
-This only covers threads from the same process as the calling thread.
+upon return from system call the calling thread has a guarantee that all its
+running thread siblings have executed a core serializing instruction.
+This guarantee is provided only for threads in
+the same process as the calling thread.
The "expedited" commands complete faster than the non-expedited ones,
they never block, but have the downside of causing extra overhead.
-A process needs to register its intent to use the private expedited sync
+A process must register its intent to use the private expedited sync
core command prior to using it.
.TP
.BR MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE " (since Linux 4.16)"
--
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 related
* Re: [PATCH 06/24] kexec_load: Disable at runtime if the kernel is locked down
From: Mimi Zohar @ 2018-04-12 11:38 UTC (permalink / raw)
To: Eric W. Biederman, David Howells
Cc: torvalds, linux-man, linux-api, jmorris, linux-kernel,
linux-security-module
In-Reply-To: <1523477375.5268.78.camel@linux.vnet.ibm.com>
On Wed, 2018-04-11 at 16:09 -0400, Mimi Zohar wrote:
> On Wed, 2018-04-11 at 14:00 -0500, Eric W. Biederman wrote:
> > David Howells <dhowells@redhat.com> writes:
> >
> > > From: Matthew Garrett <mjg59@srcf.ucam.org>
> > >
> > > The kexec_load() syscall permits the loading and execution of arbitrary
> > > code in ring 0, which is something that lock-down is meant to prevent. It
> > > makes sense to disable kexec_load() in this situation.
> > >
> > > This does not affect kexec_file_load() syscall which can check for a
> > > signature on the image to be booted.
> >
> > Maybing I am missing it but I am not seeing anything that would require
> > kexec_file_load be configured such that it checks the loaded kernel.
> >
> > Without that I don't see the point of disabling kexec_load.
> >
> > Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
> The IMA "secure boot" policy requires the kexec image to be signed.
> This call to kernel_is_locked_down() could be replaced with a call
> to security_kernel_read_file(NULL, READING_KEXEC_IMAGE).
>
> It would be similar to the existing init_module syscall calling
> security_kernel_read_file().
David, enabling the IMA-appraisal "secure boot" policy should probably
not be dependent on lockdown either.
Mimi
^ permalink raw reply
* Re: [PATCH 24/24] debugfs: Restrict debugfs when the kernel is locked down
From: Greg KH @ 2018-04-12 8:23 UTC (permalink / raw)
To: Andy Lutomirski
Cc: David Howells, Linus Torvalds, linux-man, Linux API, James Morris,
LKML, LSM List
In-Reply-To: <CALCETrWynogzZcaKMLWKce1rMvAC_sViReBDyiEWL-rm21HvAg@mail.gmail.com>
On Wed, Apr 11, 2018 at 07:54:12PM -0700, Andy Lutomirski wrote:
> On Wed, Apr 11, 2018 at 1:33 PM, Greg KH <greg@kroah.com> wrote:
> > On Wed, Apr 11, 2018 at 09:09:16PM +0100, David Howells wrote:
> >> Greg KH <greg@kroah.com> wrote:
> >>
> >> > Why not just disable debugfs entirely? This half-hearted way to sorta
> >> > lock it down is odd, it is meant to not be there at all, nothing in your
> >> > normal system should ever depend on it.
> >> >
> >> > So again just don't allow it to be mounted at all, much simpler and more
> >> > obvious as to what is going on.
> >>
> >> Yeah, I agree - and then I got complaints because it seems that it's been
> >> abused to allow drivers and userspace components to communicate.
> >
> > With in-kernel code? Please let me know and I'll go fix it up to not
> > allow that, as that is not ok.
> >
> > I do know of some bad examples of out-of-tree code abusing debugfs to do
> > crazy things (battery level monitoring?), but that's their own fault...
> >
> > debugfs is for DEBUGGING! For anything you all feel should be "secure",
> > then just disable it entirely.
> >
>
> Debugfs is very, very useful for, ahem, debugging. I really think
> this is an example of why we should split lockdown into the read and
> write varieties and allow mounting and reading debugfs when only write
> is locked down.
Ok, but be sure that there are no "secrets" in those debugging files if
you really buy into the whole "lock down" mess...
Really, it's easier to just disable the whole thing.
greg k-h
^ permalink raw reply
* Re: [PATCH] mmap.2: document new MAP_FIXED_NOREPLACE flag
From: Michal Hocko @ 2018-04-12 8:21 UTC (permalink / raw)
To: Michael Kerrisk (man-pages)
Cc: John Hubbard, Andrew Morton, linux-mm, LKML, linux-api
In-Reply-To: <97504dda-4252-a150-e7b5-43fe587aa055@gmail.com>
On Thu 12-04-18 10:04:06, Michael Kerrisk wrote:
> Hello Michal,
>
> On 04/11/2018 02:04 PM, mhocko@kernel.org wrote:
> > From: Michal Hocko <mhocko@suse.com>
> >
> > 4.17+ kernels offer a new MAP_FIXED_NOREPLACE flag which allows the caller to
> > atomicaly probe for a given address range.
> >
> > [wording heavily updated by John Hubbard <jhubbard@nvidia.com>]
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
>
> Thanks! I've applied your patch, and done a little tweaking. The results
> have already been pushed.
Thanks!
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH] mmap.2: document new MAP_FIXED_NOREPLACE flag
From: Michael Kerrisk (man-pages) @ 2018-04-12 8:10 UTC (permalink / raw)
To: Jann Horn, Michal Hocko
Cc: mtk.manpages, John Hubbard, Andrew Morton, Linux-MM, LKML,
Linux API
In-Reply-To: <CAG48ez2wYqxJEHgZCz5g6ZYBY4_qDcYWSGAErC8pUzmrW62rug@mail.gmail.com>
Hi Jann,
On 04/11/2018 06:40 PM, Jann Horn wrote:
> On Wed, Apr 11, 2018 at 6:36 PM, Michal Hocko <mhocko@kernel.org> wrote:
>> On Wed 11-04-18 17:37:46, Jann Horn wrote:
>>> On Wed, Apr 11, 2018 at 2:04 PM, <mhocko@kernel.org> wrote:
>>>> From: Michal Hocko <mhocko@suse.com>
>>>>
>>>> 4.17+ kernels offer a new MAP_FIXED_NOREPLACE flag which allows the caller to
>>>> atomicaly probe for a given address range.
>>>>
>>>> [wording heavily updated by John Hubbard <jhubbard@nvidia.com>]
>>>> Signed-off-by: Michal Hocko <mhocko@suse.com>
>>>> ---
>>>> Hi,
>>>> Andrew's sent the MAP_FIXED_NOREPLACE to Linus for the upcoming merge
>>>> window. So here we go with the man page update.
>>>>
>>>> man2/mmap.2 | 27 +++++++++++++++++++++++++++
>>>> 1 file changed, 27 insertions(+)
>>>>
>>>> diff --git a/man2/mmap.2 b/man2/mmap.2
>>>> index ea64eb8f0dcc..f702f3e4eba2 100644
>>>> --- a/man2/mmap.2
>>>> +++ b/man2/mmap.2
>>>> @@ -261,6 +261,27 @@ Examples include
>>>> and the PAM libraries
>>>> .UR http://www.linux-pam.org
>>>> .UE .
>>>> +Newer kernels
>>>> +(Linux 4.17 and later) have a
>>>> +.B MAP_FIXED_NOREPLACE
>>>> +option that avoids the corruption problem; if available, MAP_FIXED_NOREPLACE
>>>> +should be preferred over MAP_FIXED.
>>>
>>> This still looks wrong to me. There are legitimate uses for MAP_FIXED,
>>> and for most users of MAP_FIXED that I'm aware of, MAP_FIXED_NOREPLACE
>>> wouldn't work while MAP_FIXED works perfectly well.
>>>
>>> MAP_FIXED is for when you have already reserved the targeted memory
>>> area using another VMA; MAP_FIXED_NOREPLACE is for when you haven't.
>>> Please don't make it sound as if MAP_FIXED is always wrong.
>>
>> Well, this was suggested by John. I think, nobody is objecting that
>> MAP_FIXED has legitimate usecases. The above text just follows up on
>> the previous section which emphasises the potential memory corruption
>> problems and it suggests that a new flag is safe with that regards.
>>
>> If you have specific wording that would be better I am open for changes.
>
> I guess I'd probably also want to change the previous text; so I
> should probably send a followup patch once this one has landed.
Okay -- I'm ready to take that piece now. Please send me a patch!
Cheers,
Michael
--
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] mmap.2: document new MAP_FIXED_NOREPLACE flag
From: Michael Kerrisk (man-pages) @ 2018-04-12 8:09 UTC (permalink / raw)
To: Jann Horn, Michal Hocko
Cc: mtk.manpages, John Hubbard, Andrew Morton, Linux-MM, LKML,
Linux API
In-Reply-To: <CAG48ez2wYqxJEHgZCz5g6ZYBY4_qDcYWSGAErC8pUzmrW62rug@mail.gmail.com>
On 04/11/2018 06:40 PM, Jann Horn wrote:
> On Wed, Apr 11, 2018 at 6:36 PM, Michal Hocko <mhocko@kernel.org> wrote:
>> On Wed 11-04-18 17:37:46, Jann Horn wrote:
>>> On Wed, Apr 11, 2018 at 2:04 PM, <mhocko@kernel.org> wrote:
>>>> From: Michal Hocko <mhocko@suse.com>
>>>>
>>>> 4.17+ kernels offer a new MAP_FIXED_NOREPLACE flag which allows the caller to
>>>> atomicaly probe for a given address range.
>>>>
>>>> [wording heavily updated by John Hubbard <jhubbard@nvidia.com>]
>>>> Signed-off-by: Michal Hocko <mhocko@suse.com>
>>>> ---
>>>> Hi,
>>>> Andrew's sent the MAP_FIXED_NOREPLACE to Linus for the upcoming merge
>>>> window. So here we go with the man page update.
>>>>
>>>> man2/mmap.2 | 27 +++++++++++++++++++++++++++
>>>> 1 file changed, 27 insertions(+)
>>>>
>>>> diff --git a/man2/mmap.2 b/man2/mmap.2
>>>> index ea64eb8f0dcc..f702f3e4eba2 100644
>>>> --- a/man2/mmap.2
>>>> +++ b/man2/mmap.2
>>>> @@ -261,6 +261,27 @@ Examples include
>>>> and the PAM libraries
>>>> .UR http://www.linux-pam.org
>>>> .UE .
>>>> +Newer kernels
>>>> +(Linux 4.17 and later) have a
>>>> +.B MAP_FIXED_NOREPLACE
>>>> +option that avoids the corruption problem; if available, MAP_FIXED_NOREPLACE
>>>> +should be preferred over MAP_FIXED.
>>>
>>> This still looks wrong to me. There are legitimate uses for MAP_FIXED,
>>> and for most users of MAP_FIXED that I'm aware of, MAP_FIXED_NOREPLACE
>>> wouldn't work while MAP_FIXED works perfectly well.
>>>
>>> MAP_FIXED is for when you have already reserved the targeted memory
>>> area using another VMA; MAP_FIXED_NOREPLACE is for when you haven't.
>>> Please don't make it sound as if MAP_FIXED is always wrong.
>>
>> Well, this was suggested by John. I think, nobody is objecting that
>> MAP_FIXED has legitimate usecases. The above text just follows up on
>> the previous section which emphasises the potential memory corruption
>> problems and it suggests that a new flag is safe with that regards.
>>
>> If you have specific wording that would be better I am open for changes.
>
> I guess I'd probably also want to change the previous text; so I
> should probably send a followup patch once this one has landed.
>
>>>> +.TP
>>>> +.BR MAP_FIXED_NOREPLACE " (since Linux 4.17)"
>>>> +Similar to MAP_FIXED with respect to the
>>>> +.I
>>>> +addr
>>>> +enforcement, but different in that MAP_FIXED_NOREPLACE never clobbers a pre-existing
>>>> +mapped range. If the requested range would collide with an existing
>>>> +mapping, then this call fails with
>>>> +.B EEXIST.
>>>> +This flag can therefore be used as a way to atomically (with respect to other
>>>> +threads) attempt to map an address range: one thread will succeed; all others
>>>> +will report failure. Please note that older kernels which do not recognize this
>>>> +flag will typically (upon detecting a collision with a pre-existing mapping)
>>>> +fall back to a "non-MAP_FIXED" type of behavior: they will return an address that
>>>> +is different than the requested one. Therefore, backward-compatible software
>>>> +should check the returned address against the requested address.
>>>> .TP
>>>> .B MAP_GROWSDOWN
>>>> This flag is used for stacks.
>>>> @@ -487,6 +508,12 @@ is not a valid file descriptor (and
>>>> .B MAP_ANONYMOUS
>>>> was not set).
>>>> .TP
>>>> +.B EEXIST
>>>> +range covered by
>>>> +.IR addr ,
>>>> +.IR length
>>>> +is clashing with an existing mapping.
>>>
>>> Maybe add something like ", and MAP_FIXED_NOREPLACE was specified"? I
>>> think most manpages explicitly document which error conditions can be
>>> triggered by which flags.
>>
>> sure, no objection from me.
I've added the suggested piece from Jann to the EEXIST error description.
Cheers,
Michael
--
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] mmap.2: document new MAP_FIXED_NOREPLACE flag
From: Michael Kerrisk (man-pages) @ 2018-04-12 8:04 UTC (permalink / raw)
To: mhocko
Cc: mtk.manpages, John Hubbard, Andrew Morton, linux-mm, LKML,
linux-api, Michal Hocko
In-Reply-To: <20180411120452.1736-1-mhocko@kernel.org>
Hello Michal,
On 04/11/2018 02:04 PM, mhocko@kernel.org wrote:
> From: Michal Hocko <mhocko@suse.com>
>
> 4.17+ kernels offer a new MAP_FIXED_NOREPLACE flag which allows the caller to
> atomicaly probe for a given address range.
>
> [wording heavily updated by John Hubbard <jhubbard@nvidia.com>]
> Signed-off-by: Michal Hocko <mhocko@suse.com>
Thanks! I've applied your patch, and done a little tweaking. The results
have already been pushed.
Cheers
Michael
> ---
> Hi,
> Andrew's sent the MAP_FIXED_NOREPLACE to Linus for the upcoming merge
> window. So here we go with the man page update.
>
> man2/mmap.2 | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/man2/mmap.2 b/man2/mmap.2
> index ea64eb8f0dcc..f702f3e4eba2 100644
> --- a/man2/mmap.2
> +++ b/man2/mmap.2
> @@ -261,6 +261,27 @@ Examples include
> and the PAM libraries
> .UR http://www.linux-pam.org
> .UE .
> +Newer kernels
> +(Linux 4.17 and later) have a
> +.B MAP_FIXED_NOREPLACE
> +option that avoids the corruption problem; if available, MAP_FIXED_NOREPLACE
> +should be preferred over MAP_FIXED.
> +.TP
> +.BR MAP_FIXED_NOREPLACE " (since Linux 4.17)"
> +Similar to MAP_FIXED with respect to the
> +.I
> +addr
> +enforcement, but different in that MAP_FIXED_NOREPLACE never clobbers a pre-existing
> +mapped range. If the requested range would collide with an existing
> +mapping, then this call fails with
> +.B EEXIST.
> +This flag can therefore be used as a way to atomically (with respect to other
> +threads) attempt to map an address range: one thread will succeed; all others
> +will report failure. Please note that older kernels which do not recognize this
> +flag will typically (upon detecting a collision with a pre-existing mapping)
> +fall back to a "non-MAP_FIXED" type of behavior: they will return an address that
> +is different than the requested one. Therefore, backward-compatible software
> +should check the returned address against the requested address.
> .TP
> .B MAP_GROWSDOWN
> This flag is used for stacks.
> @@ -487,6 +508,12 @@ is not a valid file descriptor (and
> .B MAP_ANONYMOUS
> was not set).
> .TP
> +.B EEXIST
> +range covered by
> +.IR addr ,
> +.IR length
> +is clashing with an existing mapping.
> +.TP
> .B EINVAL
> We don't like
> .IR addr ,
>
--
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 1/3] mm: introduce NR_INDIRECTLY_RECLAIMABLE_BYTES
From: Vlastimil Babka @ 2018-04-12 6:52 UTC (permalink / raw)
To: Roman Gushchin
Cc: linux-mm, Andrew Morton, Alexander Viro, Michal Hocko,
Johannes Weiner, linux-fsdevel, linux-kernel, kernel-team,
Linux API
In-Reply-To: <20180411135624.GA24260@castle.DHCP.thefacebook.com>
On 04/11/2018 03:56 PM, Roman Gushchin wrote:
> On Wed, Apr 11, 2018 at 03:16:08PM +0200, Vlastimil Babka wrote:
>> [+CC linux-api]
>>
>> On 03/05/2018 02:37 PM, Roman Gushchin wrote:
>>> This patch introduces a concept of indirectly reclaimable memory
>>> and adds the corresponding memory counter and /proc/vmstat item.
>>>
>>> Indirectly reclaimable memory is any sort of memory, used by
>>> the kernel (except of reclaimable slabs), which is actually
>>> reclaimable, i.e. will be released under memory pressure.
>>>
>>> The counter is in bytes, as it's not always possible to
>>> count such objects in pages. The name contains BYTES
>>> by analogy to NR_KERNEL_STACK_KB.
>>>
>>> Signed-off-by: Roman Gushchin <guro@fb.com>
>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
>>> Cc: Michal Hocko <mhocko@suse.com>
>>> Cc: Johannes Weiner <hannes@cmpxchg.org>
>>> Cc: linux-fsdevel@vger.kernel.org
>>> Cc: linux-kernel@vger.kernel.org
>>> Cc: linux-mm@kvack.org
>>> Cc: kernel-team@fb.com
>>
>> Hmm, looks like I'm late and this user-visible API change was just
>> merged. But it's for rc1, so we can still change it, hopefully?
>>
>> One problem I see with the counter is that it's in bytes, but among
>> counters that use pages, and the name doesn't indicate it.
>
> Here I just followed "nr_kernel_stack" path, which is measured in kB,
> but this is not mentioned in the field name.
Oh, didn't know. Bad example to follow :P
>> Then, I don't
>> see why users should care about the "indirectly" part, as that's just an
>> implementation detail. It is reclaimable and that's what matters, right?
>> (I also wanted to complain about lack of Documentation/... update, but
>> looks like there's no general file about vmstat, ugh)
>
> I agree, that it's a bit weird, and it's probably better to not expose
> it at all; but this is how all vm counters work. We do expose them all
> in /proc/vmstat. A good number of them is useless until you are not a
> mm developer, so it's arguable more "debug info" rather than "api".
Yeah the problem is that once tools start rely on them, they fall under
the "do not break userspace" rule, however we call them. So being
cautious and conservative can't hurt.
> It's definitely not a reason to make them messy.
> Does "nr_indirectly_reclaimable_bytes" look better to you?
It still has has the "indirecly" part and feels arbitrary :/
>>
>> I also kind of liked the idea from v1 rfc posting that there would be a
>> separate set of reclaimable kmalloc-X caches for these kind of
>> allocations. Besides accounting, it should also help reduce memory
>> fragmentation. The right variant of cache would be detected via
>> __GFP_RECLAIMABLE.
>
> Well, the downside is that we have to introduce X new caches
> just for this particular problem. I'm not strictly against the idea,
> but not convinced that it's much better.
Maybe we can find more cases that would benefit from it. Heck, even slab
itself allocates some management structures from the generic kmalloc
caches, and if they are used for reclaimable caches, they could be
tracked as reclaimable as well.
>>
>> With that in mind, can we at least for now put the (manually maintained)
>> byte counter in a variable that's not directly exposed via /proc/vmstat,
>> and then when printing nr_slab_reclaimable, simply add the value
>> (divided by PAGE_SIZE), and when printing nr_slab_unreclaimable,
>> subtract the same value. This way we would be simply making the existing
>> counters more precise, in line with their semantics.
>
> Idk, I don't like the idea of adding a counter outside of the vm counters
> infrastructure, and I definitely wouldn't touch the exposed
> nr_slab_reclaimable and nr_slab_unreclaimable fields.
We would be just making the reported values more precise wrt reality.
> We do have some stats in /proc/slabinfo, /proc/meminfo and /sys/kernel/slab
> and I think that we should keep it consistent.
Right, meminfo would be adjusted the same. slabinfo doesn't indicate
which caches are reclaimable, so there will be no change.
/sys/kernel/slab/cache/reclaim_account does, but I doubt anything will
break.
> Thanks!
>
>>
>> Thoughts?
>> Vlastimil
>>
>>> ---
>>> include/linux/mmzone.h | 1 +
>>> mm/vmstat.c | 1 +
>>> 2 files changed, 2 insertions(+)
>>>
>>> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
>>> index e09fe563d5dc..15e783f29e21 100644
>>> --- a/include/linux/mmzone.h
>>> +++ b/include/linux/mmzone.h
>>> @@ -180,6 +180,7 @@ enum node_stat_item {
>>> NR_VMSCAN_IMMEDIATE, /* Prioritise for reclaim when writeback ends */
>>> NR_DIRTIED, /* page dirtyings since bootup */
>>> NR_WRITTEN, /* page writings since bootup */
>>> + NR_INDIRECTLY_RECLAIMABLE_BYTES, /* measured in bytes */
>>> NR_VM_NODE_STAT_ITEMS
>>> };
>>>
>>> diff --git a/mm/vmstat.c b/mm/vmstat.c
>>> index 40b2db6db6b1..b6b5684f31fe 100644
>>> --- a/mm/vmstat.c
>>> +++ b/mm/vmstat.c
>>> @@ -1161,6 +1161,7 @@ const char * const vmstat_text[] = {
>>> "nr_vmscan_immediate_reclaim",
>>> "nr_dirtied",
>>> "nr_written",
>>> + "nr_indirectly_reclaimable",
>>>
>>> /* enum writeback_stat_item counters */
>>> "nr_dirty_threshold",
>>>
>>
>
^ permalink raw reply
* Re: [PATCH 01/24] Add the ability to lock down access to the running kernel image
From: Andy Lutomirski @ 2018-04-12 2:57 UTC (permalink / raw)
To: David Howells
Cc: Linus Torvalds, linux-man, Linux API, James Morris, LKML,
LSM List
In-Reply-To: <152346388583.4030.15146667041427303547.stgit@warthog.procyon.org.uk>
On Wed, Apr 11, 2018 at 9:24 AM, David Howells <dhowells@redhat.com> wrote:
>
> (*) CONFIG_LOCK_DOWN_KERNEL
>
> This makes lockdown available and applies it to all the points that
> need to be locked down if the mode is set. Lockdown mode can be
> enabled by providing:
>
> lockdown=1
By doing this, you are basically committing to making the
protect-kernel-integrity vs protect-kernel-secrecy split be a
second-class citizen if it gets added.
How about lockdown=integrity_and_secrecy or lockdown=2 if you feel
like using numbers?
^ permalink raw reply
* Re: [PATCH 24/24] debugfs: Restrict debugfs when the kernel is locked down
From: Andy Lutomirski @ 2018-04-12 2:54 UTC (permalink / raw)
To: Greg KH
Cc: David Howells, Linus Torvalds, linux-man, Linux API, James Morris,
LKML, LSM List
In-Reply-To: <20180411203308.GA10167@kroah.com>
On Wed, Apr 11, 2018 at 1:33 PM, Greg KH <greg@kroah.com> wrote:
> On Wed, Apr 11, 2018 at 09:09:16PM +0100, David Howells wrote:
>> Greg KH <greg@kroah.com> wrote:
>>
>> > Why not just disable debugfs entirely? This half-hearted way to sorta
>> > lock it down is odd, it is meant to not be there at all, nothing in your
>> > normal system should ever depend on it.
>> >
>> > So again just don't allow it to be mounted at all, much simpler and more
>> > obvious as to what is going on.
>>
>> Yeah, I agree - and then I got complaints because it seems that it's been
>> abused to allow drivers and userspace components to communicate.
>
> With in-kernel code? Please let me know and I'll go fix it up to not
> allow that, as that is not ok.
>
> I do know of some bad examples of out-of-tree code abusing debugfs to do
> crazy things (battery level monitoring?), but that's their own fault...
>
> debugfs is for DEBUGGING! For anything you all feel should be "secure",
> then just disable it entirely.
>
Debugfs is very, very useful for, ahem, debugging. I really think
this is an example of why we should split lockdown into the read and
write varieties and allow mounting and reading debugfs when only write
is locked down.
^ permalink raw reply
* Re: [PATCH 01/24] Add the ability to lock down access to the running kernel image
From: Linus Torvalds @ 2018-04-11 22:38 UTC (permalink / raw)
To: Jordan Glover
Cc: David Howells, linux-man, Linux API, James Morris,
Linux Kernel Mailing List, LSM List
In-Reply-To: <8z0aRQyD-6Krqntk8UD9WQjK5JSqEai2Pt5oeFU2EplgxoWiHlX5nlJXwCDHQ1WcS1oIprXimgz7UvwHCWDB9Z3dYFrEmZmtkEJSqaYMel8=@protonmail.ch>
On Wed, Apr 11, 2018 at 2:05 PM, Jordan Glover
<Golden_Miller83@protonmail.ch> wrote:
>>
>> If that /dev/mem access prevention was just instead done as an even
>> stricter mode of the existing CONFIG_STRICT_DEVMEM, it could just be
>> enabled unconditionally.
>
> CONFIG_DEVMEM=n
It's actually CONFIG_DEVMEM, CONFIG_DEVKMEM and CONFIG_DEVPORT, it's
just not obvious from the patch.
But the important part is this part:
>> So I would seriously ask that the distros that have been using these
>> patches look at which parts of lockdown they could make unconditional
>> (because it doesn't break machines), and which ones need that escape
>> clause.
.. because I get the feeling that not a lot of people have actually
been testing this, because "turn off secure boot" is such a universal
thing when people boot Linux.
So it's really the whole claim that distributions have been running
for this for the last five years that I wonder about, and how often
people end up being told: "just disable secure boot":.
But if people really don't need DEVMEM/DEVKMEM/DEVPORT, maybe we
should just disable them in the default configs, and consider them
legacy.
I'm just surprised. I suspect a lot of people end up actually using
devmem as a fallback for dmidecode etc. Maybe those people don't boot
with EFI secure mode, but if so that just shows that this whole
"hardening" is just security theater.
Linus
^ permalink raw reply
* Re: [PATCH bpf-next v8 05/11] seccomp,landlock: Enforce Landlock programs per process hierarchy
From: Mickaël Salaün @ 2018-04-11 22:18 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Andy Lutomirski, Daniel Borkmann, LKML, Alexei Starovoitov,
Arnaldo Carvalho de Melo, Casey Schaufler, David Drysdale,
David S . Miller, Eric W . Biederman, Jann Horn, Jonathan Corbet,
Michael Kerrisk, Kees Cook, Paul Moore, Sargun Dhillon,
Serge E . Hallyn, Shuah Khan, Tejun Heo, Thomas Graf,
Tycho Andersen, Will Drewry, Kernel
In-Reply-To: <20180410044821.tllxbaq2uj6gtzpn@ast-mbp.dhcp.thefacebook.com>
[-- Attachment #1.1: Type: text/plain, Size: 25891 bytes --]
On 04/10/2018 06:48 AM, Alexei Starovoitov wrote:
> On Mon, Apr 09, 2018 at 12:01:59AM +0200, Mickaël Salaün wrote:
>>
>> On 04/08/2018 11:06 PM, Andy Lutomirski wrote:
>>> On Sun, Apr 8, 2018 at 6:13 AM, Mickaël Salaün <mic@digikod.net> wrote:
>>>>
>>>> On 02/27/2018 10:48 PM, Mickaël Salaün wrote:
>>>>>
>>>>> On 27/02/2018 17:39, Andy Lutomirski wrote:
>>>>>> On Tue, Feb 27, 2018 at 5:32 AM, Alexei Starovoitov
>>>>>> <alexei.starovoitov@gmail.com> wrote:
>>>>>>> On Tue, Feb 27, 2018 at 05:20:55AM +0000, Andy Lutomirski wrote:
>>>>>>>> On Tue, Feb 27, 2018 at 4:54 AM, Alexei Starovoitov
>>>>>>>> <alexei.starovoitov@gmail.com> wrote:
>>>>>>>>> On Tue, Feb 27, 2018 at 04:40:34AM +0000, Andy Lutomirski wrote:
>>>>>>>>>> On Tue, Feb 27, 2018 at 2:08 AM, Alexei Starovoitov
>>>>>>>>>> <alexei.starovoitov@gmail.com> wrote:
>>>>>>>>>>> On Tue, Feb 27, 2018 at 01:41:15AM +0100, Mickaël Salaün wrote:
>>>>>>>>>>>> The seccomp(2) syscall can be used by a task to apply a Landlock program
>>>>>>>>>>>> to itself. As a seccomp filter, a Landlock program is enforced for the
>>>>>>>>>>>> current task and all its future children. A program is immutable and a
>>>>>>>>>>>> task can only add new restricting programs to itself, forming a list of
>>>>>>>>>>>> programss.
>>>>>>>>>>>>
>>>>>>>>>>>> A Landlock program is tied to a Landlock hook. If the action on a kernel
>>>>>>>>>>>> object is allowed by the other Linux security mechanisms (e.g. DAC,
>>>>>>>>>>>> capabilities, other LSM), then a Landlock hook related to this kind of
>>>>>>>>>>>> object is triggered. The list of programs for this hook is then
>>>>>>>>>>>> evaluated. Each program return a 32-bit value which can deny the action
>>>>>>>>>>>> on a kernel object with a non-zero value. If every programs of the list
>>>>>>>>>>>> return zero, then the action on the object is allowed.
>>>>>>>>>>>>
>>>>>>>>>>>> Multiple Landlock programs can be chained to share a 64-bits value for a
>>>>>>>>>>>> call chain (e.g. evaluating multiple elements of a file path). This
>>>>>>>>>>>> chaining is restricted when a process construct this chain by loading a
>>>>>>>>>>>> program, but additional checks are performed when it requests to apply
>>>>>>>>>>>> this chain of programs to itself. The restrictions ensure that it is
>>>>>>>>>>>> not possible to call multiple programs in a way that would imply to
>>>>>>>>>>>> handle multiple shared values (i.e. cookies) for one chain. For now,
>>>>>>>>>>>> only a fs_pick program can be chained to the same type of program,
>>>>>>>>>>>> because it may make sense if they have different triggers (cf. next
>>>>>>>>>>>> commits). This restrictions still allows to reuse Landlock programs in
>>>>>>>>>>>> a safe way (e.g. use the same loaded fs_walk program with multiple
>>>>>>>>>>>> chains of fs_pick programs).
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>>>>>>>>>>>
>>>>>>>>>>> ...
>>>>>>>>>>>
>>>>>>>>>>>> +struct landlock_prog_set *landlock_prepend_prog(
>>>>>>>>>>>> + struct landlock_prog_set *current_prog_set,
>>>>>>>>>>>> + struct bpf_prog *prog)
>>>>>>>>>>>> +{
>>>>>>>>>>>> + struct landlock_prog_set *new_prog_set = current_prog_set;
>>>>>>>>>>>> + unsigned long pages;
>>>>>>>>>>>> + int err;
>>>>>>>>>>>> + size_t i;
>>>>>>>>>>>> + struct landlock_prog_set tmp_prog_set = {};
>>>>>>>>>>>> +
>>>>>>>>>>>> + if (prog->type != BPF_PROG_TYPE_LANDLOCK_HOOK)
>>>>>>>>>>>> + return ERR_PTR(-EINVAL);
>>>>>>>>>>>> +
>>>>>>>>>>>> + /* validate memory size allocation */
>>>>>>>>>>>> + pages = prog->pages;
>>>>>>>>>>>> + if (current_prog_set) {
>>>>>>>>>>>> + size_t i;
>>>>>>>>>>>> +
>>>>>>>>>>>> + for (i = 0; i < ARRAY_SIZE(current_prog_set->programs); i++) {
>>>>>>>>>>>> + struct landlock_prog_list *walker_p;
>>>>>>>>>>>> +
>>>>>>>>>>>> + for (walker_p = current_prog_set->programs[i];
>>>>>>>>>>>> + walker_p; walker_p = walker_p->prev)
>>>>>>>>>>>> + pages += walker_p->prog->pages;
>>>>>>>>>>>> + }
>>>>>>>>>>>> + /* count a struct landlock_prog_set if we need to allocate one */
>>>>>>>>>>>> + if (refcount_read(¤t_prog_set->usage) != 1)
>>>>>>>>>>>> + pages += round_up(sizeof(*current_prog_set), PAGE_SIZE)
>>>>>>>>>>>> + / PAGE_SIZE;
>>>>>>>>>>>> + }
>>>>>>>>>>>> + if (pages > LANDLOCK_PROGRAMS_MAX_PAGES)
>>>>>>>>>>>> + return ERR_PTR(-E2BIG);
>>>>>>>>>>>> +
>>>>>>>>>>>> + /* ensure early that we can allocate enough memory for the new
>>>>>>>>>>>> + * prog_lists */
>>>>>>>>>>>> + err = store_landlock_prog(&tmp_prog_set, current_prog_set, prog);
>>>>>>>>>>>> + if (err)
>>>>>>>>>>>> + return ERR_PTR(err);
>>>>>>>>>>>> +
>>>>>>>>>>>> + /*
>>>>>>>>>>>> + * Each task_struct points to an array of prog list pointers. These
>>>>>>>>>>>> + * tables are duplicated when additions are made (which means each
>>>>>>>>>>>> + * table needs to be refcounted for the processes using it). When a new
>>>>>>>>>>>> + * table is created, all the refcounters on the prog_list are bumped (to
>>>>>>>>>>>> + * track each table that references the prog). When a new prog is
>>>>>>>>>>>> + * added, it's just prepended to the list for the new table to point
>>>>>>>>>>>> + * at.
>>>>>>>>>>>> + *
>>>>>>>>>>>> + * Manage all the possible errors before this step to not uselessly
>>>>>>>>>>>> + * duplicate current_prog_set and avoid a rollback.
>>>>>>>>>>>> + */
>>>>>>>>>>>> + if (!new_prog_set) {
>>>>>>>>>>>> + /*
>>>>>>>>>>>> + * If there is no Landlock program set used by the current task,
>>>>>>>>>>>> + * then create a new one.
>>>>>>>>>>>> + */
>>>>>>>>>>>> + new_prog_set = new_landlock_prog_set();
>>>>>>>>>>>> + if (IS_ERR(new_prog_set))
>>>>>>>>>>>> + goto put_tmp_lists;
>>>>>>>>>>>> + } else if (refcount_read(¤t_prog_set->usage) > 1) {
>>>>>>>>>>>> + /*
>>>>>>>>>>>> + * If the current task is not the sole user of its Landlock
>>>>>>>>>>>> + * program set, then duplicate them.
>>>>>>>>>>>> + */
>>>>>>>>>>>> + new_prog_set = new_landlock_prog_set();
>>>>>>>>>>>> + if (IS_ERR(new_prog_set))
>>>>>>>>>>>> + goto put_tmp_lists;
>>>>>>>>>>>> + for (i = 0; i < ARRAY_SIZE(new_prog_set->programs); i++) {
>>>>>>>>>>>> + new_prog_set->programs[i] =
>>>>>>>>>>>> + READ_ONCE(current_prog_set->programs[i]);
>>>>>>>>>>>> + if (new_prog_set->programs[i])
>>>>>>>>>>>> + refcount_inc(&new_prog_set->programs[i]->usage);
>>>>>>>>>>>> + }
>>>>>>>>>>>> +
>>>>>>>>>>>> + /*
>>>>>>>>>>>> + * Landlock program set from the current task will not be freed
>>>>>>>>>>>> + * here because the usage is strictly greater than 1. It is
>>>>>>>>>>>> + * only prevented to be freed by another task thanks to the
>>>>>>>>>>>> + * caller of landlock_prepend_prog() which should be locked if
>>>>>>>>>>>> + * needed.
>>>>>>>>>>>> + */
>>>>>>>>>>>> + landlock_put_prog_set(current_prog_set);
>>>>>>>>>>>> + }
>>>>>>>>>>>> +
>>>>>>>>>>>> + /* prepend tmp_prog_set to new_prog_set */
>>>>>>>>>>>> + for (i = 0; i < ARRAY_SIZE(tmp_prog_set.programs); i++) {
>>>>>>>>>>>> + /* get the last new list */
>>>>>>>>>>>> + struct landlock_prog_list *last_list =
>>>>>>>>>>>> + tmp_prog_set.programs[i];
>>>>>>>>>>>> +
>>>>>>>>>>>> + if (last_list) {
>>>>>>>>>>>> + while (last_list->prev)
>>>>>>>>>>>> + last_list = last_list->prev;
>>>>>>>>>>>> + /* no need to increment usage (pointer replacement) */
>>>>>>>>>>>> + last_list->prev = new_prog_set->programs[i];
>>>>>>>>>>>> + new_prog_set->programs[i] = tmp_prog_set.programs[i];
>>>>>>>>>>>> + }
>>>>>>>>>>>> + }
>>>>>>>>>>>> + new_prog_set->chain_last = tmp_prog_set.chain_last;
>>>>>>>>>>>> + return new_prog_set;
>>>>>>>>>>>> +
>>>>>>>>>>>> +put_tmp_lists:
>>>>>>>>>>>> + for (i = 0; i < ARRAY_SIZE(tmp_prog_set.programs); i++)
>>>>>>>>>>>> + put_landlock_prog_list(tmp_prog_set.programs[i]);
>>>>>>>>>>>> + return new_prog_set;
>>>>>>>>>>>> +}
>>>>>>>>>>>
>>>>>>>>>>> Nack on the chaining concept.
>>>>>>>>>>> Please do not reinvent the wheel.
>>>>>>>>>>> There is an existing mechanism for attaching/detaching/quering multiple
>>>>>>>>>>> programs attached to cgroup and tracing hooks that are also
>>>>>>>>>>> efficiently executed via BPF_PROG_RUN_ARRAY.
>>>>>>>>>>> Please use that instead.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I don't see how that would help. Suppose you add a filter, then
>>>>>>>>>> fork(), and then the child adds another filter. Do you want to
>>>>>>>>>> duplicate the entire array? You certainly can't *modify* the array
>>>>>>>>>> because you'll affect processes that shouldn't be affected.
>>>>>>>>>>
>>>>>>>>>> In contrast, doing this through seccomp like the earlier patches
>>>>>>>>>> seemed just fine to me, and seccomp already had the right logic.
>>>>>>>>>
>>>>>>>>> it doesn't look to me that existing seccomp side of managing fork
>>>>>>>>> situation can be reused. Here there is an attempt to add 'chaining'
>>>>>>>>> concept which sort of an extension of existing seccomp style,
>>>>>>>>> but somehow heavily done on bpf side and contradicts cgroup/tracing.
>>>>>>>>>
>>>>>>>>
>>>>>>>> I don't see why the seccomp way can't be used. I agree with you that
>>>>>>>> the seccomp *style* shouldn't be used in bpf code like this, but I
>>>>>>>> think that Landlock programs can and should just live in the existing
>>>>>>>> seccomp chain. If the existing seccomp code needs some modification
>>>>>>>> to make this work, then so be it.
>>>>>>>
>>>>>>> +1
>>>>>>> if that was the case...
>>>>>>> but that's not my reading of the patch set.
>>>>>>
>>>>>> An earlier version of the patch set used the seccomp filter chain.
>>>>>> Mickaël, what exactly was wrong with that approach other than that the
>>>>>> seccomp() syscall was awkward for you to use? You could add a
>>>>>> seccomp_add_landlock_rule() syscall if you needed to.
>>>>>
>>>>> Nothing was wrong about about that, this part did not changed (see my
>>>>> next comment).
>>>>>
>>>>>>
>>>>>> As a side comment, why is this an LSM at all, let alone a non-stacking
>>>>>> LSM? It would make a lot more sense to me to make Landlock depend on
>>>>>> having LSMs configured in but to call the landlock hooks directly from
>>>>>> the security_xyz() hooks.
>>>>>
>>>>> See Casey's answer and his patch series: https://lwn.net/Articles/741963/
>>>>>
>>>>>>
>>>>>>>
>>>>>>>> In other words, the kernel already has two kinds of chaining:
>>>>>>>> seccomp's and bpf's. bpf's doesn't work right for this type of usage
>>>>>>>> across fork(), whereas seccomp's already handles that case correctly.
>>>>>>>> (In contrast, seccomp's is totally wrong for cgroup-attached filters.)
>>>>>>>> So IMO Landlock should use the seccomp core code and call into bpf
>>>>>>>> for the actual filtering.
>>>>>>>
>>>>>>> +1
>>>>>>> in cgroup we had to invent this new BPF_PROG_RUN_ARRAY mechanism,
>>>>>>> since cgroup hierarchy can be complicated with bpf progs attached
>>>>>>> at different levels with different override/multiprog properties,
>>>>>>> so walking link list and checking all flags at run-time would have
>>>>>>> been too slow. That's why we added compute_effective_progs().
>>>>>>
>>>>>> If we start adding override flags to Landlock, I think we're doing it
>>>>>> wrong. With cgroup bpf programs, the whole mess is set up by the
>>>>>> administrator. With seccomp, and with Landlock if done correctly, it
>>>>>> *won't* be set up by the administrator, so the chance that everyone
>>>>>> gets all the flags right is about zero. All attached filters should
>>>>>> run unconditionally.
>>>>>
>>>>>
>>>>> There is a misunderstanding about this chaining mechanism. This should
>>>>> not be confused with the list of seccomp filters nor the cgroup
>>>>> hierarchies. Landlock programs can be stacked the same way seccomp's
>>>>> filters can (cf. struct landlock_prog_set, the "chain_last" field is an
>>>>> optimization which is not used for this struct handling). This stackable
>>>>> property did not changed from the previous patch series. The chaining
>>>>> mechanism is for another use case, which does not make sense for seccomp
>>>>> filters nor other eBPF program types, at least for now, from what I can
>>>>> tell.
>>>>>
>>>>> You may want to get a look at my talk at FOSDEM
>>>>> (https://landlock.io/talks/2018-02-04_landlock-fosdem.pdf), especially
>>>>> slides 11 and 12.
>>>>>
>>>>> Let me explain my reasoning about this program chaining thing.
>>>>>
>>>>> To check if an action on a file is allowed, we first need to identify
>>>>> this file and match it to the security policy. In a previous
>>>>> (non-public) patch series, I tried to use one type of eBPF program to
>>>>> check every kind of access to a file. To be able to identify a file, I
>>>>> relied on an eBPF map, similar to the current inode map. This map store
>>>>> a set of references to file descriptors. I then created a function
>>>>> bpf_is_file_beneath() to check if the requested file was beneath a file
>>>>> in the map. This way, no chaining, only one eBPF program type to check
>>>>> an access to a file... but some issues then emerged. First, this design
>>>>> create a side-channel which help an attacker using such a program to
>>>>> infer some information not normally available, for example to get a hint
>>>>> on where a file descriptor (received from a UNIX socket) come from.
>>>>> Another issue is that this type of program would be called for each
>>>>> component of a path. Indeed, when the kernel check if an access to a
>>>>> file is allowed, it walk through all of the directories in its path
>>>>> (checking if the current process is allowed to execute them). That first
>>>>> attempt led me to rethink the way we could filter an access to a file
>>>>> *path*.
>>>>>
>>>>> To minimize the number of called to an eBPF program dedicated to
>>>>> validate an access to a file path, I decided to create three subtype of
>>>>> eBPF programs. The FS_WALK type is called when walking through every
>>>>> directory of a file path (except the last one if it is the target). We
>>>>> can then restrict this type of program to the minimum set of functions
>>>>> it is allowed to call and the minimum set of data available from its
>>>>> context. The first implicit chaining is for this type of program. To be
>>>>> able to evaluate a path while being called for all its components, this
>>>>> program need to store a state (to remember what was the parent directory
>>>>> of this path). There is no "previous" field in the subtype for this
>>>>> program because it is chained with itself, for each directories. This
>>>>> enable to create a FS_WALK program to evaluate a file hierarchy, thank
>>>>> to the inode map which can be used to check if a directory of this
>>>>> hierarchy is part of an allowed (or denied) list of directories. This
>>>>> design enables to express a file hierarchy in a programmatic way,
>>>>> without requiring an eBPF helper to do the job (unlike my first experiment).
>>>>>
>>>>> The explicit chaining is used to tied a path evaluation (with a FS_WALK
>>>>> program) to an access to the actual file being requested (the last
>>>>> component of a file path), with a FS_PICK program. It is only at this
>>>>> time that the kernel check for the requested action (e.g. read, write,
>>>>> chdir, append...). To be able to filter such access request we can have
>>>>> one call to the same program for every action and let this program check
>>>>> for which action it was called. However, this design does not allow the
>>>>> kernel to know if the current action is indeed handled by this program.
>>>>> Hence, it is not possible to implement a cache mechanism to only call
>>>>> this program if it knows how to handle this action.
>>>>>
>>>>> The approach I took for this FS_PICK type of program is to add to its
>>>>> subtype which action it can handle (with the "triggers" bitfield, seen
>>>>> as ORed actions). This way, the kernel knows if a call to a FS_PICK
>>>>> program is necessary. If the user wants to enforce a different security
>>>>> policy according to the action requested on a file, then it needs
>>>>> multiple FS_PICK programs. However, to reduce the number of such
>>>>> programs, this patch series allow a FS_PICK program to be chained with
>>>>> another, the same way a FS_WALK is chained with itself. This way, if the
>>>>> user want to check if the action is a for example an "open" and a "read"
>>>>> and not a "map" and a "read", then it can chain multiple FS_PICK
>>>>> programs with different triggers actions. The OR check performed by the
>>>>> kernel is not a limitation then, only a way to know if a call to an eBPF
>>>>> program is needed.
>>>>>
>>>>> The last type of program is FS_GET. This one is called when a process
>>>>> get a struct file or change its working directory. This is the only
>>>>> program type able (and allowed) to tag a file. This restriction is
>>>>> important to not being subject to resource exhaustion attacks (i.e.
>>>>> tagging every inode accessible to an attacker, which would allocate too
>>>>> much kernel memory).
>>>>>
>>>>> This design gives room for improvements to create a cache of eBPF
>>>>> context (input data, including maps if any), with the result of an eBPF
>>>>> program. This would help limit the number of call to an eBPF program the
>>>>> same way SELinux or other kernel components do to limit costly checks.
>>>>>
>>>>> The eBPF maps of progs are useful to call the same type of eBPF
>>>>> program. It does not fit with this use case because we may want multiple
>>>>> eBPF program according to the action requested on a kernel object (e.g.
>>>>> FS_GET). The other reason is because the eBPF program does not know what
>>>>> will be the next (type of) access check performed by the kernel.
>>>>>
>>>>> To say it another way, this chaining mechanism is a way to split a
>>>>> kernel object evaluation with multiple specialized programs, each of
>>>>> them being able to deal with data tied to their type. Using a monolithic
>>>>> eBPF program to check everything does not scale and does not fit with
>>>>> unprivileged use either.
>>>>>
>>>>> As a side note, the cookie value is only an ephemeral value to keep a
>>>>> state between multiple programs call. It can be used to create a state
>>>>> machine for an object evaluation.
>>>>>
>>>>> I don't see a way to do an efficient and programmatic path evaluation,
>>>>> with different access checks, with the current eBPF features. Please let
>>>>> me know if you know how to do it another way.
>>>>>
>>>>
>>>> Andy, Alexei, Daniel, what do you think about this Landlock program
>>>> chaining and cookie?
>>>>
>>>
>>> Can you give a small pseudocode real world example that acutally needs
>>> chaining? The mechanism is quite complicated and I'd like to
>>> understand how it'll be used.
>>>
>>
>> Here is the interesting part from the example (patch 09/11):
>>
>> +SEC("maps")
>> +struct bpf_map_def inode_map = {
>> + .type = BPF_MAP_TYPE_INODE,
>> + .key_size = sizeof(u32),
>> + .value_size = sizeof(u64),
>> + .max_entries = 20,
>> +};
>> +
>> +SEC("subtype/landlock1")
>> +static union bpf_prog_subtype _subtype1 = {
>> + .landlock_hook = {
>> + .type = LANDLOCK_HOOK_FS_WALK,
>> + }
>> +};
>> +
>> +static __always_inline __u64 update_cookie(__u64 cookie, __u8 lookup,
>> + void *inode, void *chain, bool freeze)
>> +{
>> + __u64 map_allow = 0;
>> +
>> + if (cookie == 0) {
>> + cookie = bpf_inode_get_tag(inode, chain);
>> + if (cookie)
>> + return cookie;
>> + /* only look for the first match in the map, ignore nested
>> + * paths in this example */
>> + map_allow = bpf_inode_map_lookup(&inode_map, inode);
>> + if (map_allow)
>> + cookie = 1 | map_allow;
>> + } else {
>> + if (cookie & COOKIE_VALUE_FREEZED)
>> + return cookie;
>> + map_allow = cookie & _MAP_MARK_MASK;
>> + cookie &= ~_MAP_MARK_MASK;
>> + switch (lookup) {
>> + case LANDLOCK_CTX_FS_WALK_INODE_LOOKUP_DOTDOT:
>> + cookie--;
>> + break;
>> + case LANDLOCK_CTX_FS_WALK_INODE_LOOKUP_DOT:
>> + break;
>> + default:
>> + /* ignore _MAP_MARK_MASK overflow in this example */
>> + cookie++;
>> + break;
>> + }
>> + if (cookie >= 1)
>> + cookie |= map_allow;
>> + }
>> + /* do not modify the cookie for each fs_pick */
>> + if (freeze && cookie)
>> + cookie |= COOKIE_VALUE_FREEZED;
>> + return cookie;
>> +}
>> +
>> +SEC("landlock1")
>> +int fs_walk(struct landlock_ctx_fs_walk *ctx)
>> +{
>> + ctx->cookie = update_cookie(ctx->cookie, ctx->inode_lookup,
>> + (void *)ctx->inode, (void *)ctx->chain, false);
>> + return LANDLOCK_RET_ALLOW;
>> +}
>>
>> The program "landlock1" is called for every directory execution (except
>> the last one if it is the leaf of a path). This enables to identify a
>> file hierarchy with only a (one dimension) list of file descriptors
>> (i.e. inode_map).
>>
>> Underneath, the Landlock LSM part looks if there is an associated path
>> walk (nameidata) with each inode access request. If there is one, then
>> the cookie associated with the path walk (if any) is made available
>> through the eBPF program context. This enables to develop a state
>> machine with an eBPF program to "evaluate" a file path (without string
>> parsing).
>>
>> The goal with this chaining mechanism is to be able to express a complex
>> kernel object like a file, with multiple run of one or more eBPF
>> programs, as a multilayer evaluation. This semantic may only make sense
>> for the user/developer and his security policy. We must keep in mind
>> that this object identification should be available to unprivileged
>> processes. This means that we must be very careful to what kind of
>> information are available to an eBPF program because this can then leak
>> to a process (e.g. through a map). With this mechanism, only information
>> already available to user space is available to the eBPF program.
>>
>> In this example, the complexity of the path evaluation is in the eBPF
>> program. We can then keep the kernel code more simple and generic. This
>> enables more flexibility for a security policy definition.
>
> it all sounds correct on paper, but it's pretty novel
> approach and I'm not sure I see all the details in the patch.
> When people say "inode" they most of the time mean inode integer number,
> whereas in this patch do you mean a raw pointer to in-kernel
> 'struct inode' ?
> To avoid confusion it should probably be called differently.
It's indeed a pointer to a "struct inode", not an inode number.
I was thinking about generalizing the BPF_MAP_TYPE_INODE by renaming it
to BPF_MAP_TYPE_FD. This map type could then be used either to identify
a set of inodes (pointers) or other kernel objects identifiable by a
file descriptor. A "subtype" (similar to the BPF prog subtype introduced
in this patch series) may be used to specialize such a map to statically
identify the kind of content it may hold. We could then add more
subtypes to identify sockets, devices, processes, and so on.
>
> If you meant inode as a number then why inode only?
> where is superblock, device, mount point?
> How bpf side can compare inodes without this additional info?
> How bpf side will know what inode to compare to?
> What if inode number is reused?
This pointer can identify if a giver inode is the same as one pointed by
a file descriptor (or a file path).
> This approach is an optimization to compare inodes
> instead of strings passed into sys_open ?
Comparing paths with strings is less efficient but it is also very
error-prone. Another advantage of using file descriptors is for
unprivileged processes: we can be sure that this processes are allowed
to access a file referred by a file descriptor (opened file). Indeed we
check (security_inode_getattr) that the process is allowed to stat an
opened file. This way, a malicious process can't infer information by
crafting path strings.
>
> If you meant inode as a pointer how bpf side will
> know the pointer before the walk begins?
The BPF map is filled by user space with file descriptors pointing to
opened files. When a path walk begin, the LSM part of Landlock is
notified that a process is requesting an access to the first element of
the path (e.g. "/"). This first element may be part of a map or not. The
BPF program can then choose if this request is legitimate or not.
> What guarantees that it's not a stale pointer?
When user space updates a map with a new file descriptor, the kernel
checks if this FD is valid. If this is the case, then the inode's usage
counter is incremented and its address is stored in the map.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 01/24] Add the ability to lock down access to the running kernel image
From: Jordan Glover @ 2018-04-11 21:05 UTC (permalink / raw)
To: Linus Torvalds
Cc: David Howells, linux-man, Linux API, James Morris,
Linux Kernel Mailing List, LSM List
In-Reply-To: <CA+55aFzPo-dip8dgyo0U+g5qai9SAJU+D1c+AFJ0zV9_PBAB8Q@mail.gmail.com>
On April 11, 2018 8:09 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Wed, Apr 11, 2018 at 9:24 AM, David Howells dhowells@redhat.com wrote:
>
> > Provide a single call to allow kernel code to determine whether the system
> >
> > should be locked down, thereby disallowing various accesses that might
> >
> > allow the running kernel image to be changed, including:
> >
> > - /dev/mem and similar
> > - Loading of unauthorised modules
> > - Fiddling with MSR registers
> > - Suspend to disk managed by the kernel
> > - Use of device DMA
>
> So what I stlll absolutely detest about this series is that I think
>
> many of these things should simply be done as separate config options.
>
> For example, if the distro is sure that it doesn't need /dev/mem, then
>
> why the hell is this tied to "lockdown" that then may have to be
>
> disabled because other changes may not be acceptable (eg people may
>
> need that device DMA, or whatever).
>
> If that /dev/mem access prevention was just instead done as an even
>
> stricter mode of the existing CONFIG_STRICT_DEVMEM, it could just be
>
> enabled unconditionally.
CONFIG_DEVMEM=n
>
> So none of these patches raise my hackles per se. But what continues
>
> to makes me very very uncomfortable is how this is all tied together.
>
> Why is this one magical mode that then - because it has such a big
>
> impact - has to be enabled/disabled as a single magical mode and with
>
> very odd rules?
>
> I think a lot of people would be happier if this wasn't so incestuous
>
> and mixing together independent things under one name, and one flag.
>
> I think a lot of the secure boot problems were exacerbated by that mixup.
>
> So I would seriously ask that the distros that have been using these
>
> patches look at which parts of lockdown they could make unconditional
>
> (because it doesn't break machines), and which ones need that escape
>
> clause.
>
> Linus
>
Jordan
^ permalink raw reply
* Re: [PATCH] mmap.2: document new MAP_FIXED_NOREPLACE flag
From: John Hubbard @ 2018-04-11 20:36 UTC (permalink / raw)
To: Jann Horn, Michal Hocko
Cc: Michael Kerrisk, Andrew Morton, Linux-MM, LKML, Linux API,
Michal Hocko
In-Reply-To: <CAG48ez3BS5EtnrhFQUGYY9MKGOUHzFbhauJQd361uTwy2pBEeg@mail.gmail.com>
On 04/11/2018 08:37 AM, Jann Horn wrote:
> On Wed, Apr 11, 2018 at 2:04 PM, <mhocko@kernel.org> wrote:
>> From: Michal Hocko <mhocko@suse.com>
>>
>> 4.17+ kernels offer a new MAP_FIXED_NOREPLACE flag which allows the caller to
>> atomicaly probe for a given address range.
>>
>> [wording heavily updated by John Hubbard <jhubbard@nvidia.com>]
>> Signed-off-by: Michal Hocko <mhocko@suse.com>
>> ---
>> Hi,
>> Andrew's sent the MAP_FIXED_NOREPLACE to Linus for the upcoming merge
>> window. So here we go with the man page update.
>>
>> man2/mmap.2 | 27 +++++++++++++++++++++++++++
>> 1 file changed, 27 insertions(+)
>>
>> diff --git a/man2/mmap.2 b/man2/mmap.2
>> index ea64eb8f0dcc..f702f3e4eba2 100644
>> --- a/man2/mmap.2
>> +++ b/man2/mmap.2
>> @@ -261,6 +261,27 @@ Examples include
>> and the PAM libraries
>> .UR http://www.linux-pam.org
>> .UE .
>> +Newer kernels
>> +(Linux 4.17 and later) have a
>> +.B MAP_FIXED_NOREPLACE
>> +option that avoids the corruption problem; if available, MAP_FIXED_NOREPLACE
>> +should be preferred over MAP_FIXED.
>
> This still looks wrong to me. There are legitimate uses for MAP_FIXED,
> and for most users of MAP_FIXED that I'm aware of, MAP_FIXED_NOREPLACE
> wouldn't work while MAP_FIXED works perfectly well.
>
> MAP_FIXED is for when you have already reserved the targeted memory
> area using another VMA; MAP_FIXED_NOREPLACE is for when you haven't.
That's a nice summary, I hope it shows up in your upcoming patch. I recall
that we went back and forth, trying to find a balance of explaining
this feature, without providing overly-elaborate examples (which I tend
toward).
> Please don't make it sound as if MAP_FIXED is always wrong.
>
Agreed.
thanks,
--
John Hubbard
NVIDIA
^ 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