date:20181002

This patch makes the untagged_addr macro accept all kinds of address types
(void *, unsigned long, etc.) and allows not to specify type casts in each
place where it is used. This is done by using __typeof__.

Signed-off-by: Andrey Konovalov 
---
 arch/arm64/include/asm/uaccess.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index e66b0fca99c2..2d6451cbaa86 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -102,7 +102,8 @@ static inline unsigned long __range_ok(const void __user 
*addr, unsigned long si
  * up with a tagged userland pointer. Clear the tag to get a sane pointer to
  * pass on to access_ok(), for instance.
  */
-#define untagged_addr(addr)sign_extend64(addr, 55)
+#define untagged_addr(addr)\
+   ((__typeof__(addr))sign_extend64((__u64)(addr), 55))
 
 #define access_ok(type, addr, size)__range_ok(addr, size)
 #define user_addr_max  get_fs
-- 
2.19.0.605.g01d371f741-goog

[PATCH v7 0/8] arm64: untag user pointers passed to the kernel

arm64 has a feature called Top Byte Ignore, which allows to embed pointer
tags into the top byte of each pointer. Userspace programs (such as
HWASan, a memory debugging tool [1]) might use this feature and pass
tagged user pointers to the kernel through syscalls or other interfaces.

Right now the kernel is already able to handle user faults with tagged
pointers, due to these patches:

1. 81cddd65 ("arm64: traps: fix userspace cache maintenance emulation on a
tagged pointer")
2. 7dcd9dd8 ("arm64: hw_breakpoint: fix watchpoint matching for tagged
pointers")
3. 276e9327 ("arm64: entry: improve data abort handling of tagged
pointers")

When passing tagged pointers to syscalls, there's a special case of such a
pointer being passed to one of the memory syscalls (mmap, mprotect, etc.).
These syscalls don't do memory accesses but rather deal with memory
ranges, hence an untagged pointer is better suited.

This patchset extends tagged pointer support to non-memory syscalls. This
is done by reusing the untagged_addr macro to untag user pointers when the
kernel performs pointer checking to find out whether the pointer comes
from userspace (most notably in access_ok).

The following testing approaches has been taken to find potential issues
with user pointer untagging:

1. Static testing (with sparse [2] and separately with a custom static
analyzer based on Clang) to track casts of __user pointers to integer
types to find places where untagging needs to be done.

2. Dynamic testing: adding BUG_ON(has_tag(addr)) to find_vma() and running
a modified syzkaller version that passes tagged pointers to the kernel.

Based on the results of the testing the requried patches have been added
to the patchset.

This patchset is a prerequisite for ARM's memory tagging hardware feature
support [3].

Thanks!

[1] http://clang.llvm.org/docs/HardwareAssistedAddressSanitizerDesign.html

[2]
https://github.com/lucvoo/sparse-dev/commit/5f960cb10f56ec2017c128ef9d16060e0145f292

[3]
https://community.arm.com/processors/b/blog/posts/arm-a-profile-architecture-2018-developments-armv85a

Changes in v7:
- Rebased onto 17b57b18 (4.19-rc6).
- Dropped the "arm64: untag user address in __do_user_fault" patch, since
the existing patches already handle user faults properly.
- Dropped the "usb, arm64: untag user addresses in devio" patch, since the
passed pointer must come from a vma and therefore be untagged.
- Dropped the "arm64: annotate user pointers casts detected by sparse"
patch (see the discussion to the replies of the v6 of this patchset).
- Added more context to the cover letter.
- Updated Documentation/arm64/tagged-pointers.txt.

Changes in v6:
- Added annotations for user pointer casts found by sparse.
- Rebased onto 050cdc6c (4.19-rc1+).

Changes in v5:
- Added 3 new patches that add untagging to places found with static
analysis.
- Rebased onto 44c929e1 (4.18-rc8).

Changes in v4:
- Added a selftest for checking that passing tagged pointers to the
kernel succeeds.
- Rebased onto 81e97f013 (4.18-rc1+).

Changes in v3:
- Rebased onto e5c51f30 (4.17-rc6+).
- Added linux-arch@ to the list of recipients.

Changes in v2:
- Rebased onto 2d618bdf (4.17-rc3+).
- Removed excessive untagging in gup.c.
- Removed untagging pointers returned from __uaccess_mask_ptr.

Changes in v1:
- Rebased onto 4.17-rc1.

Changes in RFC v2:
- Added "#ifndef untagged_addr..." fallback in linux/uaccess.h instead of
defining it for each arch individually.
- Updated Documentation/arm64/tagged-pointers.txt.
- Dropped "mm, arm64: untag user addresses in memory syscalls".
- Rebased onto 3eb2ce82 (4.16-rc7).

Andrey Konovalov (8):
arm64: add type casts to untagged_addr macro
uaccess: add untagged_addr definition for other arches
arm64: untag user addresses in access_ok and __uaccess_mask_ptr
mm, arm64: untag user addresses in mm/gup.c
lib, arm64: untag addrs passed to strncpy_from_user and strnlen_user
fs, arm64: untag user address in copy_mount_options
arm64: update Documentation/arm64/tagged-pointers.txt
selftests, arm64: add a selftest for passing tagged pointers to kernel

Documentation/arm64/tagged-pointers.txt | 24 +++
arch/arm64/include/asm/uaccess.h | 14 +++
fs/namespace.c| 2 +-
include/linux/uaccess.h | 4
lib/strncpy_from_user.c | 2 ++
lib/strnlen_user.c| 2 ++
mm/gup.c | 4
tools/testing/selftests/arm64/.gitignore | 1 +
tools/testing/selftests/arm64/Makefile| 11 +
.../testing/selftests/arm64/run_tags_test.sh | 12 ++
tools/testing/selftests/arm64/tags_test.c | 19 +++
11 files changed, 79 insertions(+), 16 deletions(-)
create mode 100644 tools/testing/selftests/arm64/.gitignore
create mode 100644 tools/testing/selftests/arm64/Makefile

[PATCH v7 8/8] selftests, arm64: add a selftest for passing tagged pointers to kernel

This patch adds a simple test, that calls the uname syscall with a
tagged user pointer as an argument. Without the kernel accepting tagged
user pointers the test fails with EFAULT.

Signed-off-by: Andrey Konovalov 
---
 tools/testing/selftests/arm64/.gitignore  |  1 +
 tools/testing/selftests/arm64/Makefile| 11 +++
 .../testing/selftests/arm64/run_tags_test.sh  | 12 
 tools/testing/selftests/arm64/tags_test.c | 19 +++
 4 files changed, 43 insertions(+)
 create mode 100644 tools/testing/selftests/arm64/.gitignore
 create mode 100644 tools/testing/selftests/arm64/Makefile
 create mode 100755 tools/testing/selftests/arm64/run_tags_test.sh
 create mode 100644 tools/testing/selftests/arm64/tags_test.c

diff --git a/tools/testing/selftests/arm64/.gitignore 
b/tools/testing/selftests/arm64/.gitignore
new file mode 100644
index ..e8fae8d61ed6
--- /dev/null
+++ b/tools/testing/selftests/arm64/.gitignore
@@ -0,0 +1 @@
+tags_test
diff --git a/tools/testing/selftests/arm64/Makefile 
b/tools/testing/selftests/arm64/Makefile
new file mode 100644
index ..a61b2e743e99
--- /dev/null
+++ b/tools/testing/selftests/arm64/Makefile
@@ -0,0 +1,11 @@
+# SPDX-License-Identifier: GPL-2.0
+
+# ARCH can be overridden by the user for cross compiling
+ARCH ?= $(shell uname -m 2>/dev/null || echo not)
+
+ifneq (,$(filter $(ARCH),aarch64 arm64))
+TEST_GEN_PROGS := tags_test
+TEST_PROGS := run_tags_test.sh
+endif
+
+include ../lib.mk
diff --git a/tools/testing/selftests/arm64/run_tags_test.sh 
b/tools/testing/selftests/arm64/run_tags_test.sh
new file mode 100755
index ..745f11379930
--- /dev/null
+++ b/tools/testing/selftests/arm64/run_tags_test.sh
@@ -0,0 +1,12 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+
+echo ""
+echo "running tags test"
+echo ""
+./tags_test
+if [ $? -ne 0 ]; then
+   echo "[FAIL]"
+else
+   echo "[PASS]"
+fi
diff --git a/tools/testing/selftests/arm64/tags_test.c 
b/tools/testing/selftests/arm64/tags_test.c
new file mode 100644
index ..1452ed7d33f9
--- /dev/null
+++ b/tools/testing/selftests/arm64/tags_test.c
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include 
+#include 
+#include 
+#include 
+
+#define SHIFT_TAG(tag) ((uint64_t)(tag) << 56)
+#define SET_TAG(ptr, tag)  (((uint64_t)(ptr) & ~SHIFT_TAG(0xff)) | \
+   SHIFT_TAG(tag))
+
+int main(void)
+{
+   struct utsname utsname;
+   void *ptr = &utsname;
+   void *tagged_ptr = (void *)SET_TAG(ptr, 0x42);
+   int err = uname(tagged_ptr);
+   return err;
+}
-- 
2.19.0.605.g01d371f741-goog

[PATCH v7 4/8] mm, arm64: untag user addresses in mm/gup.c

mm/gup.c provides a kernel interface that accepts user addresses and
manipulates user pages directly (for example get_user_pages, that is used
by the futex syscall). Since a user can provided tagged addresses, we need
to handle such case.

Add untagging to gup.c functions that use user addresses for vma lookup.

Signed-off-by: Andrey Konovalov 
---
 mm/gup.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/mm/gup.c b/mm/gup.c
index 1abc8b4afff6..6f09132c654e 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -666,6 +666,8 @@ static long __get_user_pages(struct task_struct *tsk, 
struct mm_struct *mm,
if (!nr_pages)
return 0;
 
+   start = untagged_addr(start);
+
VM_BUG_ON(!!pages != !!(gup_flags & FOLL_GET));
 
/*
@@ -820,6 +822,8 @@ int fixup_user_fault(struct task_struct *tsk, struct 
mm_struct *mm,
struct vm_area_struct *vma;
vm_fault_t ret, major = 0;
 
+   address = untagged_addr(address);
+
if (unlocked)
fault_flags |= FAULT_FLAG_ALLOW_RETRY;
 
-- 
2.19.0.605.g01d371f741-goog

[PATCH v7 6/8] fs, arm64: untag user address in copy_mount_options

In copy_mount_options a user address is being subtracted from TASK_SIZE.
If the address is lower than TASK_SIZE, the size is calculated to not
allow the exact_copy_from_user() call to cross TASK_SIZE boundary.
However if the address is tagged, then the size will be calculated
incorrectly.

Untag the address before subtracting.

Signed-off-by: Andrey Konovalov 
---
 fs/namespace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 99186556f8d3..51f763fb9430 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2672,7 +2672,7 @@ void *copy_mount_options(const void __user * data)
 * the remainder of the page.
 */
/* copy_from_user cannot cross TASK_SIZE ! */
-   size = TASK_SIZE - (unsigned long)data;
+   size = TASK_SIZE - (unsigned long)untagged_addr(data);
if (size > PAGE_SIZE)
size = PAGE_SIZE;
 
-- 
2.19.0.605.g01d371f741-goog

[PATCH v7 7/8] arm64: update Documentation/arm64/tagged-pointers.txt

Document the changes in Documentation/arm64/tagged-pointers.txt.

Signed-off-by: Andrey Konovalov 
---
 Documentation/arm64/tagged-pointers.txt | 24 ++--
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/Documentation/arm64/tagged-pointers.txt 
b/Documentation/arm64/tagged-pointers.txt
index a25a99e82bb1..ae877d185fdb 100644
--- a/Documentation/arm64/tagged-pointers.txt
+++ b/Documentation/arm64/tagged-pointers.txt
@@ -17,13 +17,21 @@ this byte for application use.
 Passing tagged addresses to the kernel
 --
 
-All interpretation of userspace memory addresses by the kernel assumes
-an address tag of 0x00.
+Some initial work for supporting non-zero address tags passed to the
+kernel has been done. As of now, the kernel supports tags in:
 
-This includes, but is not limited to, addresses found in:
+  - user fault addresses
 
- - pointer arguments to system calls, including pointers in structures
-   passed to system calls,
+  - pointer arguments (including pointers in structures), which don't
+describe virtual memory ranges, passed to system calls
+
+All other interpretations of userspace memory addresses by the kernel
+assume an address tag of 0x00. This includes, but is not limited to,
+addresses found in:
+
+ - pointer arguments (including pointers in structures), which describe
+   virtual memory ranges, passed to memory system calls (mmap, mprotect,
+   etc.)
 
  - the stack pointer (sp), e.g. when interpreting it to deliver a
signal,
@@ -33,11 +41,7 @@ This includes, but is not limited to, addresses found in:
 
 Using non-zero address tags in any of these locations may result in an
 error code being returned, a (fatal) signal being raised, or other modes
-of failure.
-
-For these reasons, passing non-zero address tags to the kernel via
-system calls is forbidden, and using a non-zero address tag for sp is
-strongly discouraged.
+of failure. Using a non-zero address tag for sp is strongly discouraged.
 
 Programs maintaining a frame pointer and frame records that use non-zero
 address tags may suffer impaired or inaccurate debug and profiling
-- 
2.19.0.605.g01d371f741-goog

[PATCH v7 2/8] uaccess: add untagged_addr definition for other arches

To allow arm64 syscalls accept tagged pointers from userspace, we must
untag them when they are passed to the kernel. Since untagging is done in
generic parts of the kernel, the untagged_addr macro needs to be defined
for all architectures.

Define it as a noop for other architectures besides arm64.

Signed-off-by: Andrey Konovalov 
---
 include/linux/uaccess.h | 4 
 1 file changed, 4 insertions(+)

diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index efe79c1cdd47..c045b4eff95e 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -13,6 +13,10 @@
 
 #include 
 
+#ifndef untagged_addr
+#define untagged_addr(addr) addr
+#endif
+
 /*
  * Architectures should provide two primitives (raw_copy_{to,from}_user())
  * and get rid of their private instances of copy_{to,from}_user() and
-- 
2.19.0.605.g01d371f741-goog

[PATCH v7 5/8] lib, arm64: untag addrs passed to strncpy_from_user and strnlen_user

strncpy_from_user and strnlen_user accept user addresses as arguments, and
do not go through the same path as copy_from_user and others, so here we
need to handle the case of tagged user addresses separately.

Untag user pointers passed to these functions.

Signed-off-by: Andrey Konovalov 
---
 lib/strncpy_from_user.c | 2 ++
 lib/strnlen_user.c  | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c
index b53e1b5d80f4..97467cd2bc59 100644
--- a/lib/strncpy_from_user.c
+++ b/lib/strncpy_from_user.c
@@ -106,6 +106,8 @@ long strncpy_from_user(char *dst, const char __user *src, 
long count)
if (unlikely(count <= 0))
return 0;
 
+   src = untagged_addr(src);
+
max_addr = user_addr_max();
src_addr = (unsigned long)src;
if (likely(src_addr < max_addr)) {
diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c
index 60d0bbda8f5e..8b5f56466e00 100644
--- a/lib/strnlen_user.c
+++ b/lib/strnlen_user.c
@@ -108,6 +108,8 @@ long strnlen_user(const char __user *str, long count)
if (unlikely(count <= 0))
return 0;
 
+   str = untagged_addr(str);
+
max_addr = user_addr_max();
src_addr = (unsigned long)str;
if (likely(src_addr < max_addr)) {
-- 
2.19.0.605.g01d371f741-goog

[PATCH v7 3/8] arm64: untag user addresses in access_ok and __uaccess_mask_ptr

copy_from_user (and a few other similar functions) are used to copy data
from user memory into the kernel memory or vice versa. Since a user can
provided a tagged pointer to one of the syscalls that use copy_from_user,
we need to correctly handle such pointers.

Do this by untagging user pointers in access_ok and in __uaccess_mask_ptr,
before performing access validity checks.

Signed-off-by: Andrey Konovalov 
---
 arch/arm64/include/asm/uaccess.h | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index 2d6451cbaa86..fa7318d3d7d5 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -105,7 +105,8 @@ static inline unsigned long __range_ok(const void __user 
*addr, unsigned long si
 #define untagged_addr(addr)\
((__typeof__(addr))sign_extend64((__u64)(addr), 55))
 
-#define access_ok(type, addr, size)__range_ok(addr, size)
+#define access_ok(type, addr, size)\
+   __range_ok(untagged_addr(addr), size)
 #define user_addr_max  get_fs
 
 #define _ASM_EXTABLE(from, to) \
@@ -237,7 +238,8 @@ static inline void uaccess_enable_not_uao(void)
 
 /*
  * Sanitise a uaccess pointer such that it becomes NULL if above the
- * current addr_limit.
+ * current addr_limit. In case the pointer is tagged (has the top byte set),
+ * untag the pointer before checking.
  */
 #define uaccess_mask_ptr(ptr) (__typeof__(ptr))__uaccess_mask_ptr(ptr)
 static inline void __user *__uaccess_mask_ptr(const void __user *ptr)
@@ -245,10 +247,11 @@ static inline void __user *__uaccess_mask_ptr(const void 
__user *ptr)
void __user *safe_ptr;
 
asm volatile(
-   "   bicsxzr, %1, %2\n"
+   "   bicsxzr, %3, %2\n"
"   csel%0, %1, xzr, eq\n"
: "=&r" (safe_ptr)
-   : "r" (ptr), "r" (current_thread_info()->addr_limit)
+   : "r" (ptr), "r" (current_thread_info()->addr_limit),
+ "r" (untagged_addr(ptr))
: "cc");
 
csdb();
-- 
2.19.0.605.g01d371f741-goog

Re: [PATCH v6 11/11] arm64: annotate user pointers casts detected by sparse

On Fri, Sep 28, 2018 at 7:50 PM, Catalin Marinas
 wrote:
> On Mon, Sep 17, 2018 at 07:01:00PM +0200, Andrey Konovalov wrote:

>> Looking at patch #8 ("usb, arm64: untag user addresses in devio") in
>> this series, it seems that that devio ioctl actually accepts a pointer
>> into a vma, so we shouldn't actually be untagging its argument and the
>> patch needs to be dropped.
>
> You are right, the pointer seems to have originated from the kernel as
> already untagged (mmap() on the driver), so we would expect the user to
> pass it back an untagged pointer.

OK, dropped this patch in v7.

>> As for case 1, the places where pointers are compared with TASK_SIZE
>> and others can be found with grep. Maybe it makes sense to introduce
>> some kind of routine like is_user_pointer() that handles tagged
>> pointers and refactor the existing code to use it? And maybe add a
>> rule to checkpatch.pl that forbids the direct usage of TASK_SIZE and
>> others.
>>
>> So I think detecting direct comparisons with TASK_SIZE and others
>> would more useful than finding __user pointer casts (it seems that the
>> latter requires a lot of annotations to be fixed/added), and I should
>> just drop this patch with annotations.
>
> I think point (1) is not too bad, usually found with grep.
>
> As I've said in my previous reply, I kind of came to the same conclusion
> that searching __user pointer casts to long may not actually scale. If
> we could add an __untagged annotation to ulong where it matters (e.g.
> find_vma()), we could identify a ulong (default tagged) and annotate
> some of those.
>
> However, this analysis on __user * casting was useful even if we don't
> end up using it. If we come up with a clearer definition of the ABI
> (which syscalls accept tagged pointers), we may conclude that the only
> places where untagging matters are a few find_vma() calls in the arch
> and mm code and can ignore the rest.

So what exactly should I do now?

For now I've posted v7 with the sparse annotation patch dropped (to
have the most up-do-date version posted).

Re: [PATCH v9 19/20] kasan: update documentation

On Fri, Sep 21, 2018 at 5:13 PM, Andrey Konovalov  wrote:
> This patch updates KASAN documentation to reflect the addition of the new
> tag-based mode.
>
> Signed-off-by: Andrey Konovalov 
> ---
>  Documentation/dev-tools/kasan.rst | 232 ++
>  1 file changed, 138 insertions(+), 94 deletions(-)
>
> diff --git a/Documentation/dev-tools/kasan.rst 
> b/Documentation/dev-tools/kasan.rst
> index aabc8738b3d8..a407e18afd32 100644
> --- a/Documentation/dev-tools/kasan.rst
> +++ b/Documentation/dev-tools/kasan.rst
> @@ -4,15 +4,25 @@ The Kernel Address Sanitizer (KASAN)
>  Overview
>  
>
> -KernelAddressSANitizer (KASAN) is a dynamic memory error detector. It 
> provides
> -a fast and comprehensive solution for finding use-after-free and 
> out-of-bounds
> -bugs.
> +KernelAddressSANitizer (KASAN) is a dynamic memory error detector designed to
> +find out-of-bound and use-after-free bugs. KASAN has two modes: generic KASAN
> +(similar to userspace ASan) and software tag-based KASAN (similar to 
> userspace
> +HWASan).
>
> -KASAN uses compile-time instrumentation for checking every memory access,
> -therefore you will need a GCC version 4.9.2 or later. GCC 5.0 or later is
> -required for detection of out-of-bounds accesses to stack or global 
> variables.
> +KASAN uses compile-time instrumentation to insert validity checks before 
> every
> +memory access, and therefore requires a compiler version that supports that.
>
> -Currently KASAN is supported only for the x86_64 and arm64 architectures.
> +Generic KASAN is supported in both GCC and Clang. With GCC it requires 
> version
> +4.9.2 or later for basic support and version 5.0 or later for detection of
> +out-of-bounds accesses for stack and global variables and for inline
> +instrumentation mode (see the Usage section). With Clang it requires version
> +3.7.0 or later and it doesn't support detection of out-of-bounds accesses for

Note: this should actually be 7.0.0 and not 3.7.0 (as we need rL329612).

> +global variables yet.
> +
> +Tag-based KASAN is only supported in Clang and requires version 7.0.0 or 
> later.
> +
> +Currently generic KASAN is supported for the x86_64, arm64 and xtensa
> +architectures, and tag-based KASAN is supported only for arm64.
>
>  Usage
>  -
> @@ -21,12 +31,14 @@ To enable KASAN configure kernel with::
>
>   CONFIG_KASAN = y
>
> -and choose between CONFIG_KASAN_OUTLINE and CONFIG_KASAN_INLINE. Outline and
> -inline are compiler instrumentation types. The former produces smaller binary
> -the latter is 1.1 - 2 times faster. Inline instrumentation requires a GCC
> -version 5.0 or later.
> +and choose between CONFIG_KASAN_GENERIC (to enable generic KASAN) and
> +CONFIG_KASAN_SW_TAGS (to enable software tag-based KASAN).
>
> -KASAN works with both SLUB and SLAB memory allocators.
> +You also need to choose between CONFIG_KASAN_OUTLINE and CONFIG_KASAN_INLINE.
> +Outline and inline are compiler instrumentation types. The former produces
> +smaller binary while the latter is 1.1 - 2 times faster.
> +
> +Both KASAN modes work with both SLUB and SLAB memory allocators.
>  For better bug detection and nicer reporting, enable CONFIG_STACKTRACE.
>
>  To disable instrumentation for specific files or directories, add a line
> @@ -43,85 +55,85 @@ similar to the following to the respective kernel 
> Makefile:
>  Error reports
>  ~
>
> -A typical out of bounds access report looks like this::
> +A typical out-of-bounds access generic KASAN report looks like this::
>
>  ==
> -BUG: AddressSanitizer: out of bounds access in 
> kmalloc_oob_right+0x65/0x75 [test_kasan] at addr 8800693bc5d3
> -Write of size 1 by task modprobe/1689
> -
> =
> -BUG kmalloc-128 (Not tainted): kasan error
> -
> -
> -
> -Disabling lock debugging due to kernel taint
> -INFO: Allocated in kmalloc_oob_right+0x3d/0x75 [test_kasan] age=0 cpu=0 
> pid=1689
> - __slab_alloc+0x4b4/0x4f0
> - kmem_cache_alloc_trace+0x10b/0x190
> - kmalloc_oob_right+0x3d/0x75 [test_kasan]
> - init_module+0x9/0x47 [test_kasan]
> - do_one_initcall+0x99/0x200
> - load_module+0x2cb3/0x3b20
> - SyS_finit_module+0x76/0x80
> - system_call_fastpath+0x12/0x17
> -INFO: Slab 0xea0001a4ef00 objects=17 used=7 fp=0x8800693bd728 
> flags=0x1004080
> -INFO: Object 0x8800693bc558 @offset=1368 fp=0x8800693bc720
> -
> -Bytes b4 8800693bc548: 00 00 00 00 00 00 00 00 5a 5a 5a 5a 5a 5a 5a 
> 5a  
> -Object 8800693bc558: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 
>  
> -Object 8800693bc568: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 
>  
> -Object 8800693bc578: 6b 6b 6b

[GIT PULL linux-next] Add Compiler Attributes tree

2018-10-02 Thread Miguel Ojeda

Hi Stephen,

The Compiler Attributes series has been stable for 10+ days. To
increase testing before 4.20, I would to request it being picked up
for -next.

The changes w.r.t. v5 in the LKML:

  - Rebased on top of next-20180928, which required removing
aligned_largest, which was removed by 9503cd9cbaba
("include/linux/compiler*.h: add version detection to
asm_volatile_goto").
  - Added latest Reviewed-by's and Tested-by's.

Thanks!

Cheers,
Miguel

The following changes since commit 4794a36bf08dfa89fe636e5080db9d8350e255dd:

  Add linux-next specific files for 20180928 (2018-09-28 15:26:51 +1000)

are available in the Git repository at:

  https://github.com/ojeda/linux.git compiler-attributes

for you to fetch changes up to dbce062c0b519db1cdad8d87ab46851f0be6bdea:

  Compiler Attributes: ext4: remove local __nonstring definition
(2018-10-02 15:11:26 +0200)


Miguel Ojeda (15):
  Compiler Attributes: remove unused attributes
  Compiler Attributes: always use the extra-underscores syntax
  Compiler Attributes: remove unneeded tests
  Compiler Attributes: homogenize __must_be_array
  Compiler Attributes: remove unneeded sparse (__CHECKER__) tests
  Compiler Attributes: add missing SPDX ID in compiler_types.h
  Compiler Attributes: use feature checks instead of version checks
  Compiler Attributes: KENTRY used twice the "used" attribute
  Compiler Attributes: remove uses of __attribute__ from compiler.h
  Compiler Attributes: add Doc/process/programming-language.rst
  Compiler Attributes: add MAINTAINERS entry
  Compiler Attributes: add support for __nonstring (gcc >= 8)
  Compiler Attributes: enable -Wstringop-truncation on W=1 (gcc >= 8)
  Compiler Attributes: auxdisplay: panel: use __nonstring
  Compiler Attributes: ext4: remove local __nonstring definition

 Documentation/process/index.rst|   1 +
 Documentation/process/programming-language.rst |  45 +
 MAINTAINERS|   5 +
 drivers/auxdisplay/panel.c |   7 +-
 fs/ext4/ext4.h |   9 -
 include/linux/compiler-clang.h |   5 -
 include/linux/compiler-gcc.h   |  70 +--
 include/linux/compiler-intel.h |   9 -
 include/linux/compiler.h   |  19 +-
 include/linux/compiler_attributes.h| 257 +
 include/linux/compiler_types.h | 100 ++
 scripts/Makefile.extrawarn |   1 +
 12 files changed, 340 insertions(+), 188 deletions(-)
 create mode 100644 Documentation/process/programming-language.rst
 create mode 100644 include/linux/compiler_attributes.h

Re: [PATCH security-next v4 23/32] selinux: Remove boot parameter

2018-10-02 Thread Stephen Smalley


On 10/02/2018 08:12 AM, Paul Moore wrote:

On Mon, Oct 1, 2018 at 9:04 PM Kees Cook  wrote:

Since LSM enabling is now centralized with CONFIG_LSM_ENABLE and
"lsm.enable=...", this removes the LSM-specific enabling logic from
SELinux.

Signed-off-by: Kees Cook 
---
  .../admin-guide/kernel-parameters.txt |  9 --
  security/selinux/Kconfig  | 29 ---
  security/selinux/hooks.c  | 15 +-
  3 files changed, 1 insertion(+), 52 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt 
b/Documentation/admin-guide/kernel-parameters.txt
index cf963febebb0..0d10ab3d020e 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4045,15 +4045,6 @@
 loaded. An invalid security module name will be treated
 as if no module has been chosen.

-   selinux=[SELINUX] Disable or enable SELinux at boot time.
-   Format: { "0" | "1" }
-   See security/selinux/Kconfig help text.
-   0 -- disable.
-   1 -- enable.
-   Default value is set via kernel config option.
-   If enabled at boot time, /selinux/disable can be used
-   later to disable prior to initial policy load.


No comments yet on the rest of the patchset, but the subject line of
this patch caught my eye and I wanted to comment quickly on this one
...

Not a fan unfortunately.

Much like the SELinux bits under /proc/self/attr, this is a user
visible thing which has made its way into a lot of docs, scripts, and
minds; I believe removing it would be a big mistake.


Yes, we can't suddenly break existing systems that had selinux=0 in 
their grub config.  We have to retain the support.





 serialnumber[BUGS=X86-32]

 shapers=[NET]
diff --git a/security/selinux/Kconfig b/security/selinux/Kconfig
index 8af7a690eb40..86936528a0bb 100644
--- a/security/selinux/Kconfig
+++ b/security/selinux/Kconfig
@@ -8,35 +8,6 @@ config SECURITY_SELINUX
   You will also need a policy configuration and a labeled filesystem.
   If you are unsure how to answer this question, answer N.

-config SECURITY_SELINUX_BOOTPARAM
-   bool "NSA SELinux boot parameter"
-   depends on SECURITY_SELINUX
-   default n
-   help
- This option adds a kernel parameter 'selinux', which allows SELinux
- to be disabled at boot.  If this option is selected, SELinux
- functionality can be disabled with selinux=0 on the kernel
- command line.  The purpose of this option is to allow a single
- kernel image to be distributed with SELinux built in, but not
- necessarily enabled.
-
- If you are unsure how to answer this question, answer N.
-
-config SECURITY_SELINUX_BOOTPARAM_VALUE
-   int "NSA SELinux boot parameter default value"
-   depends on SECURITY_SELINUX_BOOTPARAM
-   range 0 1
-   default 1
-   help
- This option sets the default value for the kernel parameter
- 'selinux', which allows SELinux to be disabled at boot.  If this
- option is set to 0 (zero), the SELinux kernel parameter will
- default to 0, disabling SELinux at bootup.  If this option is
- set to 1 (one), the SELinux kernel parameter will default to 1,
- enabling SELinux at bootup.
-
- If you are unsure how to answer this question, answer 1.
-
  config SECURITY_SELINUX_DISABLE
 bool "NSA SELinux runtime disable"
 depends on SECURITY_SELINUX
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 71a10fedecb3..8f5eea097612 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -120,20 +120,7 @@ __setup("enforcing=", enforcing_setup);
  #define selinux_enforcing_boot 1
  #endif

-#ifdef CONFIG_SECURITY_SELINUX_BOOTPARAM
-int selinux_enabled = CONFIG_SECURITY_SELINUX_BOOTPARAM_VALUE;
-
-static int __init selinux_enabled_setup(char *str)
-{
-   unsigned long enabled;
-   if (!kstrtoul(str, 0, &enabled))
-   selinux_enabled = enabled ? 1 : 0;
-   return 1;
-}
-__setup("selinux=", selinux_enabled_setup);
-#else
-int selinux_enabled = 1;
-#endif
+int selinux_enabled __lsm_ro_after_init;

  static unsigned int selinux_checkreqprot_boot =
 CONFIG_SECURITY_SELINUX_CHECKREQPROT_VALUE;
--
2.17.1

Re: [PATCH security-next v4 23/32] selinux: Remove boot parameter

On Tue, Oct 2, 2018 at 6:42 AM, Stephen Smalley  wrote:
> On 10/02/2018 08:12 AM, Paul Moore wrote:
>>
>> On Mon, Oct 1, 2018 at 9:04 PM Kees Cook  wrote:
>>>
>>> Since LSM enabling is now centralized with CONFIG_LSM_ENABLE and
>>> "lsm.enable=...", this removes the LSM-specific enabling logic from
>>> SELinux.
>>>
>>> Signed-off-by: Kees Cook 
>>> ---
>>>   .../admin-guide/kernel-parameters.txt |  9 --
>>>   security/selinux/Kconfig  | 29 ---
>>>   security/selinux/hooks.c  | 15 +-
>>>   3 files changed, 1 insertion(+), 52 deletions(-)
>>>
>>> diff --git a/Documentation/admin-guide/kernel-parameters.txt
>>> b/Documentation/admin-guide/kernel-parameters.txt
>>> index cf963febebb0..0d10ab3d020e 100644
>>> --- a/Documentation/admin-guide/kernel-parameters.txt
>>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>>> @@ -4045,15 +4045,6 @@
>>>  loaded. An invalid security module name will be
>>> treated
>>>  as if no module has been chosen.
>>>
>>> -   selinux=[SELINUX] Disable or enable SELinux at boot time.
>>> -   Format: { "0" | "1" }
>>> -   See security/selinux/Kconfig help text.
>>> -   0 -- disable.
>>> -   1 -- enable.
>>> -   Default value is set via kernel config option.
>>> -   If enabled at boot time, /selinux/disable can be
>>> used
>>> -   later to disable prior to initial policy load.
>>
>>
>> No comments yet on the rest of the patchset, but the subject line of
>> this patch caught my eye and I wanted to comment quickly on this one
>> ...
>>
>> Not a fan unfortunately.
>>
>> Much like the SELinux bits under /proc/self/attr, this is a user
>> visible thing which has made its way into a lot of docs, scripts, and
>> minds; I believe removing it would be a big mistake.
>
>
> Yes, we can't suddenly break existing systems that had selinux=0 in their
> grub config.  We have to retain the support.

Is it okay to only support selinux=0 (instead of also selinux=1)?

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH security-next v4 23/32] selinux: Remove boot parameter

2018-10-02 Thread Stephen Smalley


On 10/02/2018 10:44 AM, Kees Cook wrote:

On Tue, Oct 2, 2018 at 6:42 AM, Stephen Smalley  wrote:

On 10/02/2018 08:12 AM, Paul Moore wrote:


On Mon, Oct 1, 2018 at 9:04 PM Kees Cook  wrote:


Since LSM enabling is now centralized with CONFIG_LSM_ENABLE and
"lsm.enable=...", this removes the LSM-specific enabling logic from
SELinux.

Signed-off-by: Kees Cook 
---
   .../admin-guide/kernel-parameters.txt |  9 --
   security/selinux/Kconfig  | 29 ---
   security/selinux/hooks.c  | 15 +-
   3 files changed, 1 insertion(+), 52 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt
b/Documentation/admin-guide/kernel-parameters.txt
index cf963febebb0..0d10ab3d020e 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4045,15 +4045,6 @@
  loaded. An invalid security module name will be
treated
  as if no module has been chosen.

-   selinux=[SELINUX] Disable or enable SELinux at boot time.
-   Format: { "0" | "1" }
-   See security/selinux/Kconfig help text.
-   0 -- disable.
-   1 -- enable.
-   Default value is set via kernel config option.
-   If enabled at boot time, /selinux/disable can be
used
-   later to disable prior to initial policy load.



No comments yet on the rest of the patchset, but the subject line of
this patch caught my eye and I wanted to comment quickly on this one
...

Not a fan unfortunately.

Much like the SELinux bits under /proc/self/attr, this is a user
visible thing which has made its way into a lot of docs, scripts, and
minds; I believe removing it would be a big mistake.



Yes, we can't suddenly break existing systems that had selinux=0 in their
grub config.  We have to retain the support.


Is it okay to only support selinux=0 (instead of also selinux=1)?


For Fedora/RHEL kernels, selinux=1 would be redundant since it is the 
default.  However, in other distros where SELinux is not the default, I 
think they have documented selinux=1 as the way to enable SELinux.  So 
users may be relying on that as well. I don't think we can safely drop 
support for either one.  Sorry.

Re: [RFC PATCH v4 02/27] x86/fpu/xstate: Change some names to separate XSAVES system and user states

2018-10-02 Thread Borislav Petkov

On Fri, Sep 21, 2018 at 08:03:26AM -0700, Yu-cheng Yu wrote:
> To support XSAVES system states, change some names to distinguish
> user and system states.

I don't understand what the logic here is. SDM says:

XSAVES—Save Processor Extended States Supervisor

the stress being on "Supervisor" - why does it need to be renamed to
"system" now?

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [RFC PATCH v4 02/27] x86/fpu/xstate: Change some names to separate XSAVES system and user states

2018-10-02 Thread Yu-cheng Yu

On Tue, 2018-10-02 at 17:29 +0200, Borislav Petkov wrote:
> On Fri, Sep 21, 2018 at 08:03:26AM -0700, Yu-cheng Yu wrote:
> > To support XSAVES system states, change some names to distinguish
> > user and system states.
> 
> I don't understand what the logic here is. SDM says:
> 
> XSAVES—Save Processor Extended States Supervisor
> 
> the stress being on "Supervisor" - why does it need to be renamed to
> "system" now?
> 

Good point.  However, "system" is more indicative; CET states are per-task and
not "Supervisor".  Do we want to go back to "Supervisor" or add comments?

Yu-cheng

Re: [RFC PATCH v4 02/27] x86/fpu/xstate: Change some names to separate XSAVES system and user states

2018-10-02 Thread Dave Hansen

On 10/02/2018 09:21 AM, Yu-cheng Yu wrote:
> On Tue, 2018-10-02 at 17:29 +0200, Borislav Petkov wrote:
>> On Fri, Sep 21, 2018 at 08:03:26AM -0700, Yu-cheng Yu wrote:
>>> To support XSAVES system states, change some names to distinguish
>>> user and system states.
>> I don't understand what the logic here is. SDM says:
>>
>> XSAVES—Save Processor Extended States Supervisor
>>
>> the stress being on "Supervisor" - why does it need to be renamed to
>> "system" now?
>>
> Good point.  However, "system" is more indicative; CET states are per-task and
> not "Supervisor".  Do we want to go back to "Supervisor" or add comments?

This is one of those things where the SDM language does not match what
we use in the kernel.  I think it's fine to call them "system" or
"kernel" states to make it consistent with our existing in-kernel
nomenclature.

I say add comments to clarify what the SDM calls it vs. what we do.

Re: [PATCH v8 3/5] ipc: Allow boot time extension of IPCMNI from 32k to 2M

2018-10-02 Thread Manfred Spraul


Hello together,

On 8/18/18 3:15 AM, Waiman Long wrote:

On 08/17/2018 12:45 PM, Davidlohr Bueso wrote:

Cc'ing Manfred.

On Mon, 18 Jun 2018, Waiman Long wrote:


The maximum number of unique System V IPC identifiers was limited to
32k.  That limit should be big enough for most use cases.

However, there are some users out there requesting for more. To satisfy
the need of those users, a new boot time kernel option "ipcmni_extend"
is added to extend the IPCMNI value to 2M. This is a 64X increase which
hopefully is big enough for them.

Could you please provide more info on the need of these users and how
you came up with this new value (which just seems quite arbitrary)?

Thanks,
Davidlohr

Red Hat has a customer that is migrating from Solaris to Linux. Some of
their applications just happen to use more than 32k of shared memory
segments. I think Solaris allows up to 16M unique ID.

Yes, the amount of increase is a bit arbitrary. I was trying to balance
how many bits should be left for sequence number. Maybe I should just
take 8 more bits for ID and leave 8 bits for sequence number to match
Solaris.


- I think we should use the same numbers as Solaris.
Otherwise we later have to touch it again.

- What is the performance when using shmget() with already 10M segments 
present?


- I like the new logic for updating the sequence counter.

Is there a reason why you only enable it for extended mode?

You create a rarely used codepath, and I don't understand what speaks 
against switching to the 'deleted' approach for all systems.



--

    Manfred

Re: [PATCH security-next v4 23/32] selinux: Remove boot parameter

2018-10-02 Thread Jordan Glover

‐‐‐ Original Message ‐‐‐
On Tuesday, October 2, 2018 4:57 PM, Stephen Smalley  wrote:

> On 10/02/2018 10:44 AM, Kees Cook wrote:
>
> > On Tue, Oct 2, 2018 at 6:42 AM, Stephen Smalley s...@tycho.nsa.gov wrote:
> >
> > > On 10/02/2018 08:12 AM, Paul Moore wrote:
> > >
> > > > On Mon, Oct 1, 2018 at 9:04 PM Kees Cook keesc...@chromium.org wrote:
> > > >
> > > > > Since LSM enabling is now centralized with CONFIG_LSM_ENABLE and
> > > > > "lsm.enable=...", this removes the LSM-specific enabling logic from
> > > > > SELinux.
> > > > >
> > > > > Signed-off-by: Kees Cook keesc...@chromium.org
> > > > >
> > > > > ---
> > > > >
> > > > > .../admin-guide/kernel-parameters.txt | 9 --
> > > > > security/selinux/Kconfig | 29 ---
> > > > > security/selinux/hooks.c | 15 +-
> > > > > 3 files changed, 1 insertion(+), 52 deletions(-)
> > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt
> > > > > b/Documentation/admin-guide/kernel-parameters.txt
> > > > > index cf963febebb0..0d10ab3d020e 100644
> > > > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > > > @@ -4045,15 +4045,6 @@
> > > > > loaded. An invalid security module name will be
> > > > > treated
> > > > > as if no module has been chosen.
> > > > >
> > > > > - selinux=[SELINUX] Disable or enable SELinux at boot 
> > > > > time.
> > > > >
> > > > >
> > > > > - Format: { "0" | "1" }
> > > > >
> > > > >
> > > > > - See security/selinux/Kconfig help text.
> > > > >
> > > > >
> > > > > - 0 -- disable.
> > > > >
> > > > >
> > > > > - 1 -- enable.
> > > > >
> > > > >
> > > > > - Default value is set via kernel config 
> > > > > option.
> > > > >
> > > > >
> > > > > - If enabled at boot time, /selinux/disable 
> > > > > can be
> > > > >
> > > > >
> > > > >
> > > > > used
> > > > >
> > > > > - later to disable prior to initial policy 
> > > > > load.
> > > > >
> > > > >
> > > >
> > > > No comments yet on the rest of the patchset, but the subject line of
> > > > this patch caught my eye and I wanted to comment quickly on this one
> > > > ...
> > > > Not a fan unfortunately.
> > > > Much like the SELinux bits under /proc/self/attr, this is a user
> > > > visible thing which has made its way into a lot of docs, scripts, and
> > > > minds; I believe removing it would be a big mistake.
> > >
> > > Yes, we can't suddenly break existing systems that had selinux=0 in their
> > > grub config. We have to retain the support.
> >
> > Is it okay to only support selinux=0 (instead of also selinux=1)?
>
> For Fedora/RHEL kernels, selinux=1 would be redundant since it is the
> default. However, in other distros where SELinux is not the default, I
> think they have documented selinux=1 as the way to enable SELinux. So
> users may be relying on that as well. I don't think we can safely drop
> support for either one. Sorry.

It's always documented as: "selinux=1 security=selinux" so security= should
still do the job and selinux=1 become no-op, no?

Jordan

Re: [PATCH security-next v4 23/32] selinux: Remove boot parameter

On Tue, Oct 2, 2018 at 7:58 AM, Stephen Smalley  wrote:
> On 10/02/2018 10:44 AM, Kees Cook wrote:
>>
>> On Tue, Oct 2, 2018 at 6:42 AM, Stephen Smalley  wrote:
>>>
>>> On 10/02/2018 08:12 AM, Paul Moore wrote:


 On Mon, Oct 1, 2018 at 9:04 PM Kees Cook  wrote:
>
>
> Since LSM enabling is now centralized with CONFIG_LSM_ENABLE and
> "lsm.enable=...", this removes the LSM-specific enabling logic from
> SELinux.
>
> Signed-off-by: Kees Cook 
> ---
>.../admin-guide/kernel-parameters.txt |  9 --
>security/selinux/Kconfig  | 29
> ---
>security/selinux/hooks.c  | 15 +-
>3 files changed, 1 insertion(+), 52 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt
> b/Documentation/admin-guide/kernel-parameters.txt
> index cf963febebb0..0d10ab3d020e 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4045,15 +4045,6 @@
>   loaded. An invalid security module name will
> be
> treated
>   as if no module has been chosen.
>
> -   selinux=[SELINUX] Disable or enable SELinux at boot
> time.
> -   Format: { "0" | "1" }
> -   See security/selinux/Kconfig help text.
> -   0 -- disable.
> -   1 -- enable.
> -   Default value is set via kernel config option.
> -   If enabled at boot time, /selinux/disable can
> be
> used
> -   later to disable prior to initial policy load.



 No comments yet on the rest of the patchset, but the subject line of
 this patch caught my eye and I wanted to comment quickly on this one
 ...

 Not a fan unfortunately.

 Much like the SELinux bits under /proc/self/attr, this is a user
 visible thing which has made its way into a lot of docs, scripts, and
 minds; I believe removing it would be a big mistake.
>>>
>>>
>>>
>>> Yes, we can't suddenly break existing systems that had selinux=0 in their
>>> grub config.  We have to retain the support.
>>
>>
>> Is it okay to only support selinux=0 (instead of also selinux=1)?
>
>
> For Fedora/RHEL kernels, selinux=1 would be redundant since it is the
> default.  However, in other distros where SELinux is not the default, I
> think they have documented selinux=1 as the way to enable SELinux.  So users
> may be relying on that as well. I don't think we can safely drop support for
> either one.  Sorry.

Okay. How would you like to resolve this? Should SELinux remain
"enable special", and AppArmor is okay to remove the LSM-specific
enabling?

The trouble is with handling CONFIG_LSM_ENABLE vs lsm.enable=... boot
param vs the SELinux bootparam. I.e. CONFIG_LSM_ENABLE is redundant to
SECURITY_SELINUX_BOOTPARAM_VALUE, and selinux= is redundant to
lsm.enable=. Specifically, how should the kernel distinguish between
the four settings?

-Kees

-- 
Kees Cook
Pixel Security

Re: [RFC PATCH v4 02/27] x86/fpu/xstate: Change some names to separate XSAVES system and user states

2018-10-02 Thread Borislav Petkov

On Tue, Oct 02, 2018 at 09:30:52AM -0700, Dave Hansen wrote:
> > Good point.  However, "system" is more indicative; CET states are per-task 
> > and
> > not "Supervisor".  Do we want to go back to "Supervisor" or add comments?
> 
> This is one of those things where the SDM language does not match what
> we use in the kernel.  I think it's fine to call them "system" or
> "kernel" states to make it consistent with our existing in-kernel
> nomenclature.
> 
> I say add comments to clarify what the SDM calls it vs. what we do.

So AFAIU, the difference is that XSAVES is a CPL0 insn. Thus the
supervisor thing, I'd guess.

Now it looks like CET uses XSAVES (from skimming the patchset forward)
but then what our nomenclature is and how it all gets tied together,
needs to be explained somewhere prominent so that we're all on the same
page.

This patch's commit message is not even close. So I'd very much
appreciate a more verbose explanation, even if it repeats itself at
places.

Thx.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [RFC PATCH v4 02/27] x86/fpu/xstate: Change some names to separate XSAVES system and user states

2018-10-02 Thread Dave Hansen

On 10/02/2018 09:37 AM, Borislav Petkov wrote:
> This patch's commit message is not even close. So I'd very much
> appreciate a more verbose explanation, even if it repeats itself at
> places.

Yep, totally agree.

Re: [RFC PATCH v4 02/27] x86/fpu/xstate: Change some names to separate XSAVES system and user states

2018-10-02 Thread Yu-cheng Yu

On Tue, 2018-10-02 at 09:39 -0700, Dave Hansen wrote:
> On 10/02/2018 09:37 AM, Borislav Petkov wrote:
> > This patch's commit message is not even close. So I'd very much
> > appreciate a more verbose explanation, even if it repeats itself at
> > places.
> 
> Yep, totally agree.

Ok, I will work on that.

Yu-cheng

Re: [PATCH security-next v4 23/32] selinux: Remove boot parameter

On Tue, Oct 2, 2018 at 9:33 AM, Jordan Glover
 wrote:
> It's always documented as: "selinux=1 security=selinux" so security= should
> still do the job and selinux=1 become no-op, no?

The v3 patch set worked this way, yes. (The per-LSM enable defaults
were set by the LSM. Only in the case of "lsm.disable=selinux" would
the above stop working.)

John did not like the separation of having two CONFIG and two
bootparams mixing the controls. The v3 resolution rules were:

SECURITY_SELINUX_BOOTPARAM_VALUE overrides CONFIG_LSM_ENABLE.
SECURITY_APPARMOR_BOOTPARAM_VALUE overrides CONFIG_LSM_ENABLE.
selinux= overrides SECURITY_SELINUX_BOOTPARAM_VALUE.
apparmor.enabled= overrides SECURITY_APPARMOR_BOOTPARAM_VALUE.
apparmor= overrides apparmor.enabled=.
lsm.enable= overrides selinux=.
lsm.enable= overrides apparmor=.
lsm.disable= overrides lsm.enable=.
major LSM _omission_ from security= (if present) overrides lsm.enable.

v4 removed the per-LSM boot params and CONFIGs at John's request, but
Paul and Stephen don't want this for SELinux.

The pieces for reducing conflict with CONFIG_LSM_ENABLE and
lsm.{enable,disable}= were:

1- Remove SECURITY_APPARMOR_BOOTPARAM_VALUE.
2- Remove apparmor= and apparmor.enabled=.
3- Remove SECURITY_SELINUX_BOOTPARAM_VALUE.
4- Remove selinux=.

v4 used all of 1-4 above. SELinux says "4" cannot happen as it's too
commonly used. Would 3 be okay for SELinux?

John, with 4 not happening, do you prefer to not have 2 happen?

With CONFIGs removed, then the boot time defaults are controlled by
CONFIG_LSM_ENABLE, but the boot params continue to work as before.
Only the use of the new lsm.enable= and lsm.disable= would override
the per-LSM boot params. This would clean up the build-time CONFIG
weirdness, and leave the existing boot params as before (putting us
functionally in between the v3 and v4 series).

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v9 1/2] dt-bindings: hwmon: Add ina3221 documentation

2018-10-02 Thread Guenter Roeck

Hi Nicolin,

On Mon, Oct 01, 2018 at 06:05:22PM -0700, Nicolin Chen wrote:
> Texas Instruments INA3221 is a triple-channel shunt and bus
> voltage monitor. This patch adds a DT binding doc for it.
> 
> Signed-off-by: Nicolin Chen 
> ---

This version of the series looks good to me. I'll add both patches to
hwmon-next, with the understanding that I may pull them if Rob has
bindings related concerns.

Thanks,
Guenter

> Changelog
> v7->v9:
>  * N/A
> v6->v7:
>  * Restored three channel examples and merged them with the parent one
> v5->v6:
>  * Removed status property as no need to explicitly list it.
>  * Combined all examples into a complete one.
> v4->v5:
>  * Replaced "input-id" with "reg" and added address-cells and size-cells
>  * Replaced "input-label" with "label"
>  * Replaced "shunt-resistor" with "shunt-resistor-micro-ohms"
> v3->v4:
>  * Removed the attempt of putting labels in the node names
>  * Added a new optional label property in the child node
>  * Updated examples accordingly
> v2->v3:
>  * Added a simple subject in the line 1
>  * Fixed the shunt resistor value in the example
> v1->v2:
>  * Dropped channel name properties
>  * Added child node definitions.
>  * * Added shunt resistor property in the child node
>  * * Added status property to indicate connection status
>  * * Changed to use child node name as the label of input source
> 
>  .../devicetree/bindings/hwmon/ina3221.txt | 44 +++
>  1 file changed, 44 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/ina3221.txt
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ina3221.txt 
> b/Documentation/devicetree/bindings/hwmon/ina3221.txt
> new file mode 100644
> index ..a7b25caa2b8e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/ina3221.txt
> @@ -0,0 +1,44 @@
> +Texas Instruments INA3221 Device Tree Bindings
> +
> +1) ina3221 node
> +  Required properties:
> +  - compatible: Must be "ti,ina3221"
> +  - reg: I2C address
> +
> +  Optional properties:
> +  = The node contains optional child nodes for three channels =
> +  = Each child node describes the information of input source =
> +
> +  - #address-cells: Required only if a child node is present. Must be 1.
> +  - #size-cells: Required only if a child node is present. Must be 0.
> +
> +2) child nodes
> +  Required properties:
> +  - reg: Must be 0, 1 or 2, corresponding to IN1, IN2 or IN3 port of INA3221
> +
> +  Optional properties:
> +  - label: Name of the input source
> +  - shunt-resistor-micro-ohms: Shunt resistor value in micro-Ohm
> +
> +Example:
> +
> +ina3221@40 {
> + compatible = "ti,ina3221";
> + reg = <0x40>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + input@0 {
> + reg = <0x0>;
> + status = "disabled";
> + };
> + input@1 {
> + reg = <0x1>;
> + shunt-resistor-micro-ohms = <5000>;
> + };
> + input@2 {
> + reg = <0x2>;
> + label = "VDD_5V";
> + shunt-resistor-micro-ohms = <5000>;
> + };
> +};
> -- 
> 2.17.1
>

Re: [PATCH v9 1/2] dt-bindings: hwmon: Add ina3221 documentation

2018-10-02 Thread Nicolin Chen

On Tue, Oct 02, 2018 at 10:05:18AM -0700, Guenter Roeck wrote:
> Hi Nicolin,
> 
> On Mon, Oct 01, 2018 at 06:05:22PM -0700, Nicolin Chen wrote:
> > Texas Instruments INA3221 is a triple-channel shunt and bus
> > voltage monitor. This patch adds a DT binding doc for it.
> > 
> > Signed-off-by: Nicolin Chen 
> > ---
> 
> This version of the series looks good to me. I'll add both patches to
> hwmon-next, with the understanding that I may pull them if Rob has
> bindings related concerns.

Thanks a lot, especially for the review.

Nicolin

Re: [RFC PATCH v4 03/27] x86/fpu/xstate: Enable XSAVES system states

2018-10-02 Thread Borislav Petkov

On Fri, Sep 21, 2018 at 08:03:27AM -0700, Yu-cheng Yu wrote:
> XSAVES saves both system and user states.  The Linux kernel
> currently does not save/restore any system states.  This patch
> creates the framework for supporting system states.

... and needs a lot more text explaining *why* it is doing that.

> 
> Signed-off-by: Yu-cheng Yu 
> ---
>  arch/x86/include/asm/fpu/internal.h |   3 +-
>  arch/x86/include/asm/fpu/xstate.h   |   9 ++-
>  arch/x86/kernel/fpu/core.c  |   7 +-
>  arch/x86/kernel/fpu/init.c  |  10 ---
>  arch/x86/kernel/fpu/xstate.c| 112 +---
>  5 files changed, 80 insertions(+), 61 deletions(-)
> 
> diff --git a/arch/x86/include/asm/fpu/internal.h 
> b/arch/x86/include/asm/fpu/internal.h
> index f1f9bf91a0ab..1f447865db3a 100644
> --- a/arch/x86/include/asm/fpu/internal.h
> +++ b/arch/x86/include/asm/fpu/internal.h
> @@ -45,7 +45,6 @@ extern void fpu__init_cpu_xstate(void);
>  extern void fpu__init_system(struct cpuinfo_x86 *c);
>  extern void fpu__init_check_bugs(void);
>  extern void fpu__resume_cpu(void);
> -extern u64 fpu__get_supported_xfeatures_mask(void);
>  
>  /*
>   * Debugging facility:
> @@ -94,7 +93,7 @@ static inline void fpstate_init_xstate(struct xregs_state 
> *xsave)
>* trigger #GP:
>*/
>   xsave->header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT |
> - xfeatures_mask_user;
> + xfeatures_mask_all;
>  }
>  
>  static inline void fpstate_init_fxstate(struct fxregs_state *fx)
> diff --git a/arch/x86/include/asm/fpu/xstate.h 
> b/arch/x86/include/asm/fpu/xstate.h
> index 9b382e5157ed..a32dc5f8c963 100644
> --- a/arch/x86/include/asm/fpu/xstate.h
> +++ b/arch/x86/include/asm/fpu/xstate.h
> @@ -19,10 +19,10 @@
>  #define XSAVE_YMM_SIZE   256
>  #define XSAVE_YMM_OFFSET(XSAVE_HDR_SIZE + XSAVE_HDR_OFFSET)
>  
> -/* System features */
> -#define XFEATURE_MASK_SYSTEM (XFEATURE_MASK_PT)

Previous patch renames it, this patch deletes it. Why do we need all
that unnecessary churn?

Also, this patch is trying to do a couple of things at once and
reviewing it is not trivial. Please split the changes logically.

> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 19f8df54c72a..dd2c561c4544 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -51,13 +51,16 @@ static short xsave_cpuid_features[] __initdata = {
>  };
>  
>  /*
> - * Mask of xstate features supported by the CPU and the kernel:
> + * Mask of xstate features supported by the CPU and the kernel.
> + * This is the result from CPUID query, SUPPORTED_XFEATURES_MASK,
> + * and boot_cpu_has().
>   */

This needs to explain what both masks are - user and system. "CPU" and
"kernel" is not "user" and "all".

>  u64 xfeatures_mask_user __read_mostly;
> +u64 xfeatures_mask_all __read_mostly;



> @@ -219,30 +222,31 @@ void fpstate_sanitize_xstate(struct fpu *fpu)
>   */
>  void fpu__init_cpu_xstate(void)
>  {
> - if (!boot_cpu_has(X86_FEATURE_XSAVE) || !xfeatures_mask_user)
> + if (!boot_cpu_has(X86_FEATURE_XSAVE) || !xfeatures_mask_all)
>   return;
> +
> + cr4_set_bits(X86_CR4_OSXSAVE);
> +
>   /*
> -  * Make it clear that XSAVES system states are not yet
> -  * implemented should anyone expect it to work by changing
> -  * bits in XFEATURE_MASK_* macros and XCR0.
> +  * XCR_XFEATURE_ENABLED_MASK sets the features that are managed
> +  * by XSAVE{C, OPT} and XRSTOR.  Only XSAVE user states can be
> +  * set here.
>*/
> - WARN_ONCE((xfeatures_mask_user & XFEATURE_MASK_SYSTEM),
> - "x86/fpu: XSAVES system states are not yet implemented.\n");
> + xsetbv(XCR_XFEATURE_ENABLED_MASK,
> +xfeatures_mask_user);

No need to break the line here.

Also, you have a couple more places in your patches where you
unnecessarily break lines. Please don't do that, even if it exceeds 80
cols by a couple of chars.

>  
> - xfeatures_mask_user &= ~XFEATURE_MASK_SYSTEM;
> -
> - cr4_set_bits(X86_CR4_OSXSAVE);
> - xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask_user);
> + /*
> +  * MSR_IA32_XSS sets which XSAVES system states to be managed by
> +  * XSAVES.  Only XSAVES system states can be set here.
> +  */
> + if (boot_cpu_has(X86_FEATURE_XSAVES))
> + wrmsrl(MSR_IA32_XSS,
> +xfeatures_mask_all & ~xfeatures_mask_user);

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v8 3/5] ipc: Allow boot time extension of IPCMNI from 32k to 2M

2018-10-02 Thread Waiman Long

On 10/02/2018 12:32 PM, Manfred Spraul wrote:
> Hello together,
>
> On 8/18/18 3:15 AM, Waiman Long wrote:
>> On 08/17/2018 12:45 PM, Davidlohr Bueso wrote:
>>> Cc'ing Manfred.
>>>
>>> On Mon, 18 Jun 2018, Waiman Long wrote:
>>>
 The maximum number of unique System V IPC identifiers was limited to
 32k.  That limit should be big enough for most use cases.

 However, there are some users out there requesting for more. To
 satisfy
 the need of those users, a new boot time kernel option "ipcmni_extend"
 is added to extend the IPCMNI value to 2M. This is a 64X increase
 which
 hopefully is big enough for them.
>>> Could you please provide more info on the need of these users and how
>>> you came up with this new value (which just seems quite arbitrary)?
>>>
>>> Thanks,
>>> Davidlohr
>> Red Hat has a customer that is migrating from Solaris to Linux. Some of
>> their applications just happen to use more than 32k of shared memory
>> segments. I think Solaris allows up to 16M unique ID.
>>
>> Yes, the amount of increase is a bit arbitrary. I was trying to balance
>> how many bits should be left for sequence number. Maybe I should just
>> take 8 more bits for ID and leave 8 bits for sequence number to match
>> Solaris.
>
> - I think we should use the same numbers as Solaris.
> Otherwise we later have to touch it again.

As said in my patch, it is a trade-off between # of uniq identifiers
versus the chance of id reuse. I don't have an objection to increase it
further, but I don't see the customers to really need such a large value.

>
> - What is the performance when using shmget() with already 10M
> segments present?

I am not sure the performance impact as I had not measure it myself. The
shmget() function is considered in slowpath. We are generally less
concern about its performance than other code paths that are in a
performance critical path.

>
> - I like the new logic for updating the sequence counter.
>
> Is there a reason why you only enable it for extended mode?

I tried not to disturb the existing logic for backward compatibility
concern. I don't mind switching it all over to use the new "deleted"
approach if other people have no objection.

Cheers,
Longman

> You create a rarely used codepath, and I don't understand what speaks
> against switching to the 'deleted' approach for all systems.
>
>
> -- 
>
> Manfred
>

Re: [PATCH security-next v4 23/32] selinux: Remove boot parameter

2018-10-02 Thread Stephen Smalley


On 10/02/2018 12:54 PM, Kees Cook wrote:

On Tue, Oct 2, 2018 at 9:33 AM, Jordan Glover
 wrote:

It's always documented as: "selinux=1 security=selinux" so security= should
still do the job and selinux=1 become no-op, no?


The v3 patch set worked this way, yes. (The per-LSM enable defaults
were set by the LSM. Only in the case of "lsm.disable=selinux" would
the above stop working.)

John did not like the separation of having two CONFIG and two
bootparams mixing the controls. The v3 resolution rules were:

SECURITY_SELINUX_BOOTPARAM_VALUE overrides CONFIG_LSM_ENABLE.
SECURITY_APPARMOR_BOOTPARAM_VALUE overrides CONFIG_LSM_ENABLE.
selinux= overrides SECURITY_SELINUX_BOOTPARAM_VALUE.
apparmor.enabled= overrides SECURITY_APPARMOR_BOOTPARAM_VALUE.
apparmor= overrides apparmor.enabled=.
lsm.enable= overrides selinux=.
lsm.enable= overrides apparmor=.
lsm.disable= overrides lsm.enable=.
major LSM _omission_ from security= (if present) overrides lsm.enable.

v4 removed the per-LSM boot params and CONFIGs at John's request, but
Paul and Stephen don't want this for SELinux.

The pieces for reducing conflict with CONFIG_LSM_ENABLE and
lsm.{enable,disable}= were:

1- Remove SECURITY_APPARMOR_BOOTPARAM_VALUE.
2- Remove apparmor= and apparmor.enabled=.
3- Remove SECURITY_SELINUX_BOOTPARAM_VALUE.
4- Remove selinux=.

v4 used all of 1-4 above. SELinux says "4" cannot happen as it's too
commonly used. Would 3 be okay for SELinux?


Let's say a user/packager/distro has been building kernels for the past 
14 years (*) with a config that has SECURITY_SELINUX_BOOTPARAM_VALUE=0, 
and now they build a new kernel that includes these patches using that 
same config.  Won't SELinux be enabled by default because 
SECURITY_SELINUX_BOOTPARAM_VALUE is now ignored and LSM_ENABLE defaults 
to all?  Is it ok to require them to specify a new config option to 
preserve old behavior?


(*) how long this config option has been around



John, with 4 not happening, do you prefer to not have 2 happen?

With CONFIGs removed, then the boot time defaults are controlled by
CONFIG_LSM_ENABLE, but the boot params continue to work as before.
Only the use of the new lsm.enable= and lsm.disable= would override
the per-LSM boot params. This would clean up the build-time CONFIG
weirdness, and leave the existing boot params as before (putting us
functionally in between the v3 and v4 series).

-Kees

Re: [PATCH security-next v4 23/32] selinux: Remove boot parameter

On 10/02/2018 09:54 AM, Kees Cook wrote:
> On Tue, Oct 2, 2018 at 9:33 AM, Jordan Glover
>  wrote:
>> It's always documented as: "selinux=1 security=selinux" so security= should
>> still do the job and selinux=1 become no-op, no?
> 
> The v3 patch set worked this way, yes. (The per-LSM enable defaults
> were set by the LSM. Only in the case of "lsm.disable=selinux" would
> the above stop working.)
> 
> John did not like the separation of having two CONFIG and two

still don't

> bootparams mixing the controls. The v3 resolution rules were:
> 
> SECURITY_SELINUX_BOOTPARAM_VALUE overrides CONFIG_LSM_ENABLE.
> SECURITY_APPARMOR_BOOTPARAM_VALUE overrides CONFIG_LSM_ENABLE.
> selinux= overrides SECURITY_SELINUX_BOOTPARAM_VALUE.
> apparmor.enabled= overrides SECURITY_APPARMOR_BOOTPARAM_VALUE.
> apparmor= overrides apparmor.enabled=.
> lsm.enable= overrides selinux=.
> lsm.enable= overrides apparmor=.
> lsm.disable= overrides lsm.enable=.
> major LSM _omission_ from security= (if present) overrides lsm.enable.
> 

Yeah I would really like to remove the potential confusion for the
user. The user now has 4 kernel options to play with, and get confused
by

LSM= (I'll count apparmor.enabled= here as well)
security=
lsm.enabled=
lsm.disabled=

I really don't like this, it will be very confusing for users. I also
think an authoritative list of what is enabled is easier for users
than mixing a list of whats enabled with kernel config default
settings.

Under the current scheme

lsm.enabled=selinux

could actually mean selinux,yama,loadpin,something_else are
enabled. If we extend this behavior to when full stacking lands

lsm.enabled=selinux,yama

might mean selinux,yama,apparmor,loadpin,something_else

and what that list is will vary from kernel to kernel, which I think
is harder for the user than the lsm.enabled list being what is
actually enabled at boot

If we have to have multiple kernel parameter, I prefer a behvior where
if you hav conflicting kernel parameters specified

  apparmor=0 lsm.enabled=apparmor

that the conflict is logged and the lsm is left disabled, as I think
it is easier for users to understand than the overrides scheme of v3,
and sans logging of the conflict is effectively what we had in the
past

  apparmor=0 security=apparmor
or
  apparmor=1 security=selinux

would result in apparmor being disabed


That being said I get we have a mess currently, and there really
doesn't seem to be a good way to fix it. I think getting this right
for the user is important enough that I am willing to break current
apparmor userspace api. While apparmor=0 is documented we have also
documented security=X for years and apparmor=0 isn't used too often
so I think we can drop it to help clean this mess up abit.

I am not going to Nak, or block on v3 behavior if that is considered
the best path forward after this discussion/rant.


> v4 removed the per-LSM boot params and CONFIGs at John's request, but
> Paul and Stephen don't want this for SELinux.
> 
> The pieces for reducing conflict with CONFIG_LSM_ENABLE and
> lsm.{enable,disable}= were:
> 
> 1- Remove SECURITY_APPARMOR_BOOTPARAM_VALUE.
> 2- Remove apparmor= and apparmor.enabled=.
> 3- Remove SECURITY_SELINUX_BOOTPARAM_VALUE.
> 4- Remove selinux=.
> 
> v4 used all of 1-4 above. SELinux says "4" cannot happen as it's too
> commonly used. Would 3 be okay for SELinux?
> 
> John, with 4 not happening, do you prefer to not have 2 happen?
> 
> With CONFIGs removed, then the boot time defaults are controlled by
> CONFIG_LSM_ENABLE, but the boot params continue to work as before.
> Only the use of the new lsm.enable= and lsm.disable= would override
> the per-LSM boot params. This would clean up the build-time CONFIG
> weirdness, and leave the existing boot params as before (putting us
> functionally in between the v3 and v4 series).
> 
I'm ambivalent. I really want to clean this up but atm it doesn't
really look like 2 is going to provide much of a benefit. If you
think it helps clean this up, do it. Regardless 1 is going to
happen, and I will start updating documentation and try to get
users moving away from using the apparmor= kernel parameter.

Re: [PATCH security-next v4 23/32] selinux: Remove boot parameter

On Tue, Oct 2, 2018 at 11:33 AM, Stephen Smalley  wrote:
> On 10/02/2018 12:54 PM, Kees Cook wrote:
>>
>> On Tue, Oct 2, 2018 at 9:33 AM, Jordan Glover
>>  wrote:
>>>
>>> It's always documented as: "selinux=1 security=selinux" so security=
>>> should
>>> still do the job and selinux=1 become no-op, no?
>>
>>
>> The v3 patch set worked this way, yes. (The per-LSM enable defaults
>> were set by the LSM. Only in the case of "lsm.disable=selinux" would
>> the above stop working.)
>>
>> John did not like the separation of having two CONFIG and two
>> bootparams mixing the controls. The v3 resolution rules were:
>>
>> SECURITY_SELINUX_BOOTPARAM_VALUE overrides CONFIG_LSM_ENABLE.
>> SECURITY_APPARMOR_BOOTPARAM_VALUE overrides CONFIG_LSM_ENABLE.
>> selinux= overrides SECURITY_SELINUX_BOOTPARAM_VALUE.
>> apparmor.enabled= overrides SECURITY_APPARMOR_BOOTPARAM_VALUE.
>> apparmor= overrides apparmor.enabled=.
>> lsm.enable= overrides selinux=.
>> lsm.enable= overrides apparmor=.
>> lsm.disable= overrides lsm.enable=.
>> major LSM _omission_ from security= (if present) overrides lsm.enable.
>>
>> v4 removed the per-LSM boot params and CONFIGs at John's request, but
>> Paul and Stephen don't want this for SELinux.
>>
>> The pieces for reducing conflict with CONFIG_LSM_ENABLE and
>> lsm.{enable,disable}= were:
>>
>> 1- Remove SECURITY_APPARMOR_BOOTPARAM_VALUE.
>> 2- Remove apparmor= and apparmor.enabled=.
>> 3- Remove SECURITY_SELINUX_BOOTPARAM_VALUE.
>> 4- Remove selinux=.
>>
>> v4 used all of 1-4 above. SELinux says "4" cannot happen as it's too
>> commonly used. Would 3 be okay for SELinux?
>
>
> Let's say a user/packager/distro has been building kernels for the past 14
> years (*) with a config that has SECURITY_SELINUX_BOOTPARAM_VALUE=0, and now
> they build a new kernel that includes these patches using that same config.
> Won't SELinux be enabled by default because SECURITY_SELINUX_BOOTPARAM_VALUE
> is now ignored and LSM_ENABLE defaults to all?  Is it ok to require them to
> specify a new config option to preserve old behavior?

Yes, I think that's fine -- kernel CONFIGs change all the time. System
builders are used to examining these changes.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH security-next v4 23/32] selinux: Remove boot parameter

On Tue, Oct 2, 2018 at 11:57 AM, John Johansen
 wrote:
> Under the current scheme
>
> lsm.enabled=selinux
>
> could actually mean selinux,yama,loadpin,something_else are
> enabled. If we extend this behavior to when full stacking lands
>
> lsm.enabled=selinux,yama
>
> might mean selinux,yama,apparmor,loadpin,something_else
>
> and what that list is will vary from kernel to kernel, which I think
> is harder for the user than the lsm.enabled list being what is
> actually enabled at boot

Ah, I think I missed this in your earlier emails. What you don't like
here is that "lsm.enable=" is additive. You want it to be explicit.

Are you okay with lsm.order= having fallback?

The situation we were trying to solve was with new LSMs getting
implicitly disabled if someone is booting with an explicit list. For
example:

lsm.enable=yama,apparmor

means when "landlock" gets added to the kernel, it will be implicitly disabled.

> If we have to have multiple kernel parameter, I prefer a behvior where
> if you hav conflicting kernel parameters specified
>
>   apparmor=0 lsm.enabled=apparmor
>
> that the conflict is logged and the lsm is left disabled, as I think
> it is easier for users to understand than the overrides scheme of v3,
> and sans logging of the conflict is effectively what we had in the
> past
>
>   apparmor=0 security=apparmor
> or
>   apparmor=1 security=selinux
>
> would result in apparmor being disabed

Okay, so for this part you want per-LSM boot param to have priority
(which seems to match SELinux's concerns), possibly logging the
conflict, but still accepting the apparmor= and selinux= state.
security= would still driving initialization ordering (so I think the
behavior I have in the series would be correct).

> That being said I get we have a mess currently, and there really
> doesn't seem to be a good way to fix it. I think getting this right
> for the user is important enough that I am willing to break current
> apparmor userspace api. While apparmor=0 is documented we have also
> documented security=X for years and apparmor=0 isn't used too often
> so I think we can drop it to help clean this mess up abit.
>
> I am not going to Nak, or block on v3 behavior if that is considered
> the best path forward after this discussion/rant.

I could define CONFIG_LSM_ENABLE as being "additive" to
SECURITY_APPARMOR_BOOTPARAM_VALUE and
SECURITY_SELINUX_BOOTPARAM_VALUE?

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH security-next v4 23/32] selinux: Remove boot parameter

On 10/02/2018 12:17 PM, Kees Cook wrote:
> On Tue, Oct 2, 2018 at 11:57 AM, John Johansen
>  wrote:
>> Under the current scheme
>>
>> lsm.enabled=selinux
>>
>> could actually mean selinux,yama,loadpin,something_else are
>> enabled. If we extend this behavior to when full stacking lands
>>
>> lsm.enabled=selinux,yama
>>
>> might mean selinux,yama,apparmor,loadpin,something_else
>>
>> and what that list is will vary from kernel to kernel, which I think
>> is harder for the user than the lsm.enabled list being what is
>> actually enabled at boot
> 
> Ah, I think I missed this in your earlier emails. What you don't like
> here is that "lsm.enable=" is additive. You want it to be explicit.
> 
> Are you okay with lsm.order= having fallback?
> 

yeah, if we are going to separate order, fallbacks are fine for
anything that isn't specified.

I am still not convinced that separating order from enablement is
right, but its generally something a user should care about so I can
live with it.

> The situation we were trying to solve was with new LSMs getting
> implicitly disabled if someone is booting with an explicit list. For
> example:
> 
> lsm.enable=yama,apparmor
> 
> means when "landlock" gets added to the kernel, it will be implicitly 
> disabled.
> 

And here is the point of contention, I wouldn't call that implicitly
disabled. The user explicitly selected a set of LSMs to enable. Having
other LSMs enable when they aren't specified is confusing to a user,
as now they have to consider what is enabled by default in the
Kconfig.

I think requiring distros/builders to consider Kconfig options is
fine, but its a lot higher hurdle for regular users.


>> If we have to have multiple kernel parameter, I prefer a behvior where
>> if you hav conflicting kernel parameters specified
>>
>>   apparmor=0 lsm.enabled=apparmor
>>
>> that the conflict is logged and the lsm is left disabled, as I think
>> it is easier for users to understand than the overrides scheme of v3,
>> and sans logging of the conflict is effectively what we had in the
>> past
>>
>>   apparmor=0 security=apparmor
>> or
>>   apparmor=1 security=selinux
>>
>> would result in apparmor being disabed
> 
> Okay, so for this part you want per-LSM boot param to have priority
> (which seems to match SELinux's concerns), possibly logging the

hrmmm I wouldn't call it priority :)

If you look at the above logic its a boolean AND operation. The LSM is
only enabled if $LSM=1 AND security=$LSM all other combinations result
in $LSM being disabled

> conflict, but still accepting the apparmor= and selinux= state.

logging is nice for the user but certainly isn't required and is more
than we are doing today

> security= would still driving initialization ordering (so I think the
> behavior I have in the series would be correct).
> 
>> That being said I get we have a mess currently, and there really
>> doesn't seem to be a good way to fix it. I think getting this right
>> for the user is important enough that I am willing to break current
>> apparmor userspace api. While apparmor=0 is documented we have also
>> documented security=X for years and apparmor=0 isn't used too often
>> so I think we can drop it to help clean this mess up abit.
>>
>> I am not going to Nak, or block on v3 behavior if that is considered
>> the best path forward after this discussion/rant.
> 
> I could define CONFIG_LSM_ENABLE as being "additive" to
> SECURITY_APPARMOR_BOOTPARAM_VALUE and
> SECURITY_SELINUX_BOOTPARAM_VALUE?
> 

Oh sure lets deal with my complaint about too many ways to configure
this beast by adding yet another config option :P

seriously though, please no. That just adds another layer of confusion
even if it is only being foisted on the distro/builder

Re: [PATCH v12 9/9] cpuset: Support forced turning off of partition flag

2018-10-02 Thread Tejun Heo

Hello, Waiman.

My apologies for the delay.

On Mon, Aug 27, 2018 at 01:50:18PM -0400, Waiman Long wrote:
> My current code has explicitly assumed the following relationship for
> partition root.
> 
> cpus_allowed = effective_cpus + reserved_cpus
> 
> Also effective_cpus cannot be empty. Specifically, cpus_allowed has to
> be equal to effective_cpus before a cpuset can be made a partition root.
> 
> Any changes that break the above conditions will turn off the partition
> flag forcefully. The only exception is cpu offlining where cpus_allowed
> > effective_cpus + reserved_cpus can happen.
> 
> One reason for doing so is because reserved_cpus is hidden. So the main
> way to infer that is to do cpus_allowed - effective_cpus.
> 
> It is probably doable to make cpus_allowed >= effective_cpus +
> reserved_cpus in general, but we may need to expose reserved_cpus as a
> read-only file, for instance. There may also be other complications that
> we will need to take care of if this is supported. My current preference
> is to not doing that unless there is compelling reason to do so.

So, if we're gonna make this hierarchical, I think it probably would
be better to go in all the way.  It's kinda weird to mix the two
approaches - the normal cpuset operation following the usual
convention (it'd be really great to fix the removal part too) and
parition code doing something else.

I think adding another interface file should be fine here.

Thanks.

-- 
tejun

Re: [PATCH security-next v4 23/32] selinux: Remove boot parameter

On Tue, Oct 2, 2018 at 12:47 PM, John Johansen
 wrote:
> On 10/02/2018 12:17 PM, Kees Cook wrote:
>> I could define CONFIG_LSM_ENABLE as being "additive" to
>> SECURITY_APPARMOR_BOOTPARAM_VALUE and
>> SECURITY_SELINUX_BOOTPARAM_VALUE?
>
> Oh sure lets deal with my complaint about too many ways to configure
> this beast by adding yet another config option :P

This is what v3 already does: SEC...BOOTPARAM_VALUE trumps ...LSM_ENABLE.

> seriously though, please no. That just adds another layer of confusion
> even if it is only being foisted on the distro/builder

You've already sent a patch removing
SECURITY_APPARMOR_BOOTPARAM_VALUE. If SELinux is fine to do that too,
then I think we'll be sorted out. I'll just need to make "lsm.enable="
be an explicit list. (Do you have a problem with "lsm.disable=..." ?)

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v12 9/9] cpuset: Support forced turning off of partition flag

2018-10-02 Thread Waiman Long

On 10/02/2018 04:06 PM, Tejun Heo wrote:
> Hello, Waiman.
>
> My apologies for the delay.
>
> On Mon, Aug 27, 2018 at 01:50:18PM -0400, Waiman Long wrote:
>> My current code has explicitly assumed the following relationship for
>> partition root.
>>
>> cpus_allowed = effective_cpus + reserved_cpus
>>
>> Also effective_cpus cannot be empty. Specifically, cpus_allowed has to
>> be equal to effective_cpus before a cpuset can be made a partition root.
>>
>> Any changes that break the above conditions will turn off the partition
>> flag forcefully. The only exception is cpu offlining where cpus_allowed
>>> effective_cpus + reserved_cpus can happen.
>> One reason for doing so is because reserved_cpus is hidden. So the main
>> way to infer that is to do cpus_allowed - effective_cpus.
>>
>> It is probably doable to make cpus_allowed >= effective_cpus +
>> reserved_cpus in general, but we may need to expose reserved_cpus as a
>> read-only file, for instance. There may also be other complications that
>> we will need to take care of if this is supported. My current preference
>> is to not doing that unless there is compelling reason to do so.
> So, if we're gonna make this hierarchical, I think it probably would
> be better to go in all the way.  It's kinda weird to mix the two
> approaches - the normal cpuset operation following the usual
> convention (it'd be really great to fix the removal part too) and
> parition code doing something else.
>
> I think adding another interface file should be fine here.
>
> Thanks.
>
OK, I will revise the patch to make it work without the removal part.

Thanks,
Longman

[PATCH] yama: clarify ptrace_scope=2 in Yama documentation

2018-10-02 Thread Yves-Alexis Perez

Current phrasing is ambiguous since it's unclear if attaching to a
children through PTRACE_TRACEME requires CAP_SYS_PTRACE. Rephrase the
sentence to make that clear.

Signed-off-by: Yves-Alexis Perez 
---
 Documentation/admin-guide/LSM/Yama.rst | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/LSM/Yama.rst 
b/Documentation/admin-guide/LSM/Yama.rst
index 13468ea696b7..d0a060de3973 100644
--- a/Documentation/admin-guide/LSM/Yama.rst
+++ b/Documentation/admin-guide/LSM/Yama.rst
@@ -64,8 +64,8 @@ The sysctl settings (writable only with ``CAP_SYS_PTRACE``) 
are:
 Using ``PTRACE_TRACEME`` is unchanged.
 
 2 - admin-only attach:
-only processes with ``CAP_SYS_PTRACE`` may use ptrace
-with ``PTRACE_ATTACH``, or through children calling ``PTRACE_TRACEME``.
+only processes with ``CAP_SYS_PTRACE`` may use ptrace, either with
+``PTRACE_ATTACH`` or through children calling ``PTRACE_TRACEME``.
 
 3 - no attach:
 no processes may use ptrace with ``PTRACE_ATTACH`` nor via
-- 
2.19.0


-- 
Yves-Alexis

Re: [PATCH] yama: clarify ptrace_scope=2 in Yama documentation

On Tue, Oct 2, 2018 at 1:47 PM, Yves-Alexis Perez  wrote:
> Current phrasing is ambiguous since it's unclear if attaching to a
> children through PTRACE_TRACEME requires CAP_SYS_PTRACE. Rephrase the
> sentence to make that clear.
>
> Signed-off-by: Yves-Alexis Perez 

Thanks! Yes, this makes things more clear.

Acked-by: Kees Cook 

Jon, can you take this in your tree?

-Kees

> ---
>  Documentation/admin-guide/LSM/Yama.rst | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/admin-guide/LSM/Yama.rst 
> b/Documentation/admin-guide/LSM/Yama.rst
> index 13468ea696b7..d0a060de3973 100644
> --- a/Documentation/admin-guide/LSM/Yama.rst
> +++ b/Documentation/admin-guide/LSM/Yama.rst
> @@ -64,8 +64,8 @@ The sysctl settings (writable only with ``CAP_SYS_PTRACE``) 
> are:
>  Using ``PTRACE_TRACEME`` is unchanged.
>
>  2 - admin-only attach:
> -only processes with ``CAP_SYS_PTRACE`` may use ptrace
> -with ``PTRACE_ATTACH``, or through children calling ``PTRACE_TRACEME``.
> +only processes with ``CAP_SYS_PTRACE`` may use ptrace, either with
> +``PTRACE_ATTACH`` or through children calling ``PTRACE_TRACEME``.
>
>  3 - no attach:
>  no processes may use ptrace with ``PTRACE_ATTACH`` nor via
> --
> 2.19.0
>
>
> --
> Yves-Alexis



-- 
Kees Cook
Pixel Security

Re: [PATCH] yama: clarify ptrace_scope=2 in Yama documentation

2018-10-02 Thread Matthew Wilcox

On Tue, Oct 02, 2018 at 10:47:23PM +0200, Yves-Alexis Perez wrote:
> Current phrasing is ambiguous since it's unclear if attaching to a
> children through PTRACE_TRACEME requires CAP_SYS_PTRACE. Rephrase the
> sentence to make that clear.

I disagree that your sentence makes that clear.  How about:

>  2 - admin-only attach:
> -only processes with ``CAP_SYS_PTRACE`` may use ptrace
> -with ``PTRACE_ATTACH``, or through children calling ``PTRACE_TRACEME``.
> +only processes with ``CAP_SYS_PTRACE`` may use ptrace, either with
> +``PTRACE_ATTACH`` or through children calling ``PTRACE_TRACEME``.

+only processes with ``CAP_SYS_PTRACE`` may use ptrace.  This
+restricts both ``PTRACE_ATTACH`` and ``PTRACE_TRACEME``.

Re: [PATCH] yama: clarify ptrace_scope=2 in Yama documentation

On Tue, Oct 2, 2018 at 1:52 PM, Matthew Wilcox  wrote:
> On Tue, Oct 02, 2018 at 10:47:23PM +0200, Yves-Alexis Perez wrote:
>> Current phrasing is ambiguous since it's unclear if attaching to a
>> children through PTRACE_TRACEME requires CAP_SYS_PTRACE. Rephrase the
>> sentence to make that clear.
>
> I disagree that your sentence makes that clear.  How about:
>
>>  2 - admin-only attach:
>> -only processes with ``CAP_SYS_PTRACE`` may use ptrace
>> -with ``PTRACE_ATTACH``, or through children calling ``PTRACE_TRACEME``.
>> +only processes with ``CAP_SYS_PTRACE`` may use ptrace, either with
>> +``PTRACE_ATTACH`` or through children calling ``PTRACE_TRACEME``.
>
> +only processes with ``CAP_SYS_PTRACE`` may use ptrace.  This
> +restricts both ``PTRACE_ATTACH`` and ``PTRACE_TRACEME``.

PTRACE_TRACEME is done by the child, not the process with
CAP_SYS_PTRACE, so I still think the Yves-Alexis's is clearer. But if
other agree, I'm fine with it. :)

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] yama: clarify ptrace_scope=2 in Yama documentation

2018-10-02 Thread Yves-Alexis Perez

On Tue, 2018-10-02 at 13:52 -0700, Matthew Wilcox wrote:
> On Tue, Oct 02, 2018 at 10:47:23PM +0200, Yves-Alexis Perez wrote:
> > Current phrasing is ambiguous since it's unclear if attaching to a
> > children through PTRACE_TRACEME requires CAP_SYS_PTRACE. Rephrase the
> > sentence to make that clear.
> 
> I disagree that your sentence makes that clear.  How about:
> 
> >  2 - admin-only attach:
> > -only processes with ``CAP_SYS_PTRACE`` may use ptrace
> > -with ``PTRACE_ATTACH``, or through children calling
> > ``PTRACE_TRACEME``.
> > +only processes with ``CAP_SYS_PTRACE`` may use ptrace, either with
> > +``PTRACE_ATTACH`` or through children calling ``PTRACE_TRACEME``.
> 
> +only processes with ``CAP_SYS_PTRACE`` may use ptrace.  This
> +restricts both ``PTRACE_ATTACH`` and ``PTRACE_TRACEME``.

Hi Matthew,

I'm no native speaker, both versions are fine by me but I liked keeping the
“children calling” part since the semantics are quite different for
PTRACE_ATTACH and PTRACE_TRACEME.

Regards,
-- 
Yves-Alexis

Re: [PATCH security-next v4 23/32] selinux: Remove boot parameter

On 10/02/2018 01:29 PM, Kees Cook wrote:
> On Tue, Oct 2, 2018 at 12:47 PM, John Johansen
>  wrote:
>> On 10/02/2018 12:17 PM, Kees Cook wrote:
>>> I could define CONFIG_LSM_ENABLE as being "additive" to
>>> SECURITY_APPARMOR_BOOTPARAM_VALUE and
>>> SECURITY_SELINUX_BOOTPARAM_VALUE?
>>
>> Oh sure lets deal with my complaint about too many ways to configure
>> this beast by adding yet another config option :P
> 
> This is what v3 already does: SEC...BOOTPARAM_VALUE trumps ...LSM_ENABLE.
> 
sure but I sent in a patch to kill SECURITY_APPARMOR_BOOTPARAM_VALUE
because I really dislike the extra levels of config and getting rid
of the  SEC..BOOTPARAM_VALUE seems to be the easy way to fix it

Now if only we can convince Paul and Stephen :)

>> seriously though, please no. That just adds another layer of confusion
>> even if it is only being foisted on the distro/builder
> 
> You've already sent a patch removing
> SECURITY_APPARMOR_BOOTPARAM_VALUE. If SELinux is fine to do that too,
> then I think we'll be sorted out. I'll just need to make "lsm.enable="
> be an explicit list. (Do you have a problem with "lsm.disable=..." ?)
> 

why yes, glad you asked

If lsm.enabled is an explicit list lsm.disabled isn't required its a
convenience option that can introduce confusion and conflicts. If
both lsm.enabled and lsm.disabled are being used at the same time.

I realize that some times the convenience of specifying

  lsm.disable=$LSM

is easier than specifying an entire list of what should be enabled
when you just want to disable a single LSM.

I don't think the convenience is worth the potential confusion, but
I don't feel strongly about it and realize others feel the other
way.


tldr: I can live with it, but don't like it if you are asking :)

Re: [GIT PULL linux-next] Add Compiler Attributes tree

2018-10-02 Thread Stephen Rothwell

Hi Miguel,

On Tue, 2 Oct 2018 15:47:12 +0200 Miguel Ojeda 
 wrote:
>
> The Compiler Attributes series has been stable for 10+ days. To
> increase testing before 4.20, I would to request it being picked up
> for -next.
> 
> The changes w.r.t. v5 in the LKML:
> 
>   - Rebased on top of next-20180928, which required removing

Unfortunately, trees/branches included in linux-next must be based on
something stable (usually Linus' tree, but it could be another
tree/branch that is included in linux-next that does not rebase).
Linux-next itself rebases every day, so snything based on it would drag
in a previous version of all the other trees :-(

> aligned_largest, which was removed by 9503cd9cbaba
> ("include/linux/compiler*.h: add version detection to
> asm_volatile_goto").

That commit is from Andrew's patch series which also rebases (usually
at least every week), so you cannot depend on it.

-- 
Cheers,
Stephen Rothwell

pgpD8Ix5lWAuc.pgp
Description: OpenPGP digital signature

Re: [PATCH security-next v4 04/32] LSM: Remove initcall tracing

On Mon, 1 Oct 2018, Kees Cook wrote:

> This partially reverts commit 58eacfffc417 ("init, tracing: instrument
> security and console initcall trace events") since security init calls
> are about to no longer resemble regular init calls.
> 
> Signed-off-by: Kees Cook 
> Reviewed-by: Casey Schaufler 


Reviewed-by: James Morris 


-- 
James Morris

Re: [PATCH security-next v4 06/32] vmlinux.lds.h: Move LSM_TABLE into INIT_DATA

On Mon, 1 Oct 2018, Kees Cook wrote:

> Since the struct lsm_info table is not an initcall, we can just move it
> into INIT_DATA like all the other tables.
> 
> Signed-off-by: Kees Cook 
> Reviewed-by: Casey Schaufler 
> Reviewed-by: John Johansen 


Reviewed-by: James Morris 


-- 
James Morris

Re: [PATCH security-next v4 07/32] LSM: Convert security_initcall() into DEFINE_LSM()

On Mon, 1 Oct 2018, Kees Cook wrote:

> Instead of using argument-based initializers, switch to defining the
> contents of struct lsm_info on a per-LSM basis. This also drops
> the final use of the now inaccurate "initcall" naming.
> 
> Signed-off-by: Kees Cook 
> Reviewed-by: Casey Schaufler 


Reviewed-by: James Morris 

-- 
James Morris

Re: [GIT PULL linux-next] Add Compiler Attributes tree

2018-10-02 Thread Nick Desaulniers

On Tue, Oct 2, 2018 at 2:11 PM Stephen Rothwell  wrote:
>
> Hi Miguel,
>
> On Tue, 2 Oct 2018 15:47:12 +0200 Miguel Ojeda 
>  wrote:
> >
> > The Compiler Attributes series has been stable for 10+ days. To
> > increase testing before 4.20, I would to request it being picked up
> > for -next.
> >
> > The changes w.r.t. v5 in the LKML:
> >
> >   - Rebased on top of next-20180928, which required removing
>
> Unfortunately, trees/branches included in linux-next must be based on
> something stable (usually Linus' tree, but it could be another
> tree/branch that is included in linux-next that does not rebase).
> Linux-next itself rebases every day, so snything based on it would drag
> in a previous version of all the other trees :-(

I think of this like a branch that's force pushed to.  Can't base
other branches or trees off of it cause it's always moving/force
rewriting history.

>
> > aligned_largest, which was removed by 9503cd9cbaba
> > ("include/linux/compiler*.h: add version detection to
> > asm_volatile_goto").
>
> That commit is from Andrew's patch series which also rebases (usually
> at least every week), so you cannot depend on it.

Miguel, you should be able to drop that patch from your set then,
since Andrew's -mm tree flows into this -next tree as well, IIUC.
We'll take up that patch from there.

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH security-next v4 09/32] LSM: Provide init debugging infrastructure

On Mon, 1 Oct 2018, Kees Cook wrote:

> Booting with "lsm.debug" will report future details on how LSM ordering
> decisions are being made.
> 
> Signed-off-by: Kees Cook 
> Reviewed-by: Casey Schaufler 
> Reviewed-by: John Johansen 



Reviewed-by: James Morris 


-- 
James Morris

Re: [PATCH security-next v4 10/32] LSM: Don't ignore initialization failures

On Mon, 1 Oct 2018, Kees Cook wrote:

> LSM initialization failures have traditionally been ignored. We should
> at least WARN when something goes wrong.

I guess we could have a boot param which specifies what to do if any LSM 
fails to init, as I think some folks will want to stop execution at that 
point.

Thoughts?


> 
> Signed-off-by: Kees Cook 
> Reviewed-by: Casey Schaufler 
> Reviewed-by: John Johansen 
> ---
>  security/security.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/security/security.c b/security/security.c
> index 395f804f6a91..2055af907eba 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -55,10 +55,12 @@ static __initdata bool debug;
>  static void __init major_lsm_init(void)
>  {
>   struct lsm_info *lsm;
> + int ret;
>  
>   for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
>   init_debug("initializing %s\n", lsm->name);
> - lsm->init();
> + ret = lsm->init();
> + WARN(ret, "%s failed to initialize: %d\n", lsm->name, ret);
>   }
>  }
>  
> 

-- 
James Morris

Re: [PATCH security-next v4 10/32] LSM: Don't ignore initialization failures

On Tue, Oct 2, 2018 at 2:20 PM, James Morris  wrote:
> On Mon, 1 Oct 2018, Kees Cook wrote:
>
>> LSM initialization failures have traditionally been ignored. We should
>> at least WARN when something goes wrong.
>
> I guess we could have a boot param which specifies what to do if any LSM
> fails to init, as I think some folks will want to stop execution at that
> point.
>
> Thoughts?

I'm not opposed, but I won't author it because Linus will yell at me
about introducing a "machine killing" option.

-Kees

-- 
Kees Cook
Pixel Security

Re: [GIT PULL linux-next] Add Compiler Attributes tree

2018-10-02 Thread Miguel Ojeda

Hi Stephen,

On Tue, Oct 2, 2018 at 11:11 PM Stephen Rothwell  wrote:
>
> Hi Miguel,
>
> On Tue, 2 Oct 2018 15:47:12 +0200 Miguel Ojeda 
>  wrote:
> >
> > The Compiler Attributes series has been stable for 10+ days. To
> > increase testing before 4.20, I would to request it being picked up
> > for -next.
> >
> > The changes w.r.t. v5 in the LKML:
> >
> >   - Rebased on top of next-20180928, which required removing
>
> Unfortunately, trees/branches included in linux-next must be based on
> something stable (usually Linus' tree, but it could be another
> tree/branch that is included in linux-next that does not rebase).
> Linux-next itself rebases every day, so snything based on it would drag
> in a previous version of all the other trees :-(

I assumed you could apply changes as a diff/patches/cherry-pick, not
as a merge, for those that went on top of others (so that at the new
merge window, conflicts were already solved). Otherwise, why are
next-* tags/branches provided anyway?

>
> > aligned_largest, which was removed by 9503cd9cbaba
> > ("include/linux/compiler*.h: add version detection to
> > asm_volatile_goto").
>
> That commit is from Andrew's patch series which also rebases (usually
> at least every week), so you cannot depend on it.

Then who is solving the conflict?

Thanks!

Cheers,
Miguel

Re: [PATCH security-next v4 23/32] selinux: Remove boot parameter

On Tue, 2 Oct 2018, Kees Cook wrote:

> On Tue, Oct 2, 2018 at 11:57 AM, John Johansen
>  wrote:
> > Under the current scheme
> >
> > lsm.enabled=selinux
> >
> > could actually mean selinux,yama,loadpin,something_else are
> > enabled. If we extend this behavior to when full stacking lands
> >
> > lsm.enabled=selinux,yama
> >
> > might mean selinux,yama,apparmor,loadpin,something_else
> >
> > and what that list is will vary from kernel to kernel, which I think
> > is harder for the user than the lsm.enabled list being what is
> > actually enabled at boot
> 
> Ah, I think I missed this in your earlier emails. What you don't like
> here is that "lsm.enable=" is additive. You want it to be explicit.
> 

This is a path to madness.

How about enable flags set ONLY per LSM:

lsm.selinux.enable=x
lsm.apparmor.enable=x

With no lsm.enable, and removing selinux=x and apparmor=x.

Yes this will break existing docs, but they can be updated for newer 
kernel versions to say "replace selinux=0 with lsm.selinux.enable=0" from 
kernel X onwards.

Surely distro packages and bootloaders are able to cope with changes to 
kernel parameters?

We can either take a one-time hit now, or build new usability debt, which 
will confuse people forever.

-- 
James Morris

Re: [GIT PULL linux-next] Add Compiler Attributes tree

2018-10-02 Thread Miguel Ojeda

Hi Nick,

On Tue, Oct 2, 2018 at 11:16 PM Nick Desaulniers
 wrote:
>
> On Tue, Oct 2, 2018 at 2:11 PM Stephen Rothwell  wrote:
> >
> > Hi Miguel,
> >
> > On Tue, 2 Oct 2018 15:47:12 +0200 Miguel Ojeda 
> >  wrote:
> > >
> > > The Compiler Attributes series has been stable for 10+ days. To
> > > increase testing before 4.20, I would to request it being picked up
> > > for -next.
> > >
> > > The changes w.r.t. v5 in the LKML:
> > >
> > >   - Rebased on top of next-20180928, which required removing
> >
> > Unfortunately, trees/branches included in linux-next must be based on
> > something stable (usually Linus' tree, but it could be another
> > tree/branch that is included in linux-next that does not rebase).
> > Linux-next itself rebases every day, so snything based on it would drag
> > in a previous version of all the other trees :-(
>
> I think of this like a branch that's force pushed to.  Can't base
> other branches or trees off of it cause it's always moving/force
> rewriting history.

As I have read, -next is supposed to be a vision of what the merge
window will look like after merging everything, i.e. ideally -rc1. For
that to work for files out-of-tree (like these ones, which are not
maintained by a single tree), changes should be allowed to be stacked
on each other; otherwise, we cannot handle conflicts :-(

>
> >
> > > aligned_largest, which was removed by 9503cd9cbaba
> > > ("include/linux/compiler*.h: add version detection to
> > > asm_volatile_goto").
> >
> > That commit is from Andrew's patch series which also rebases (usually
> > at least every week), so you cannot depend on it.
>
> Miguel, you should be able to drop that patch from your set then,
> since Andrew's -mm tree flows into this -next tree as well, IIUC.
> We'll take up that patch from there.

Not sure what you mean by "drop that patch". There is no patch to
drop, there is a conflict with a change already in -next.

Cheers,
Miguel

Re: [GIT PULL linux-next] Add Compiler Attributes tree

2018-10-02 Thread Dominique Martinet

Miguel Ojeda wrote on Wed, Oct 03, 2018:
> As I have read, -next is supposed to be a vision of what the merge
> window will look like after merging everything, i.e. ideally -rc1. For
> that to work for files out-of-tree (like these ones, which are not
> maintained by a single tree), changes should be allowed to be stacked
> on each other; otherwise, we cannot handle conflicts :-(

The rule is the same as with a regular mainline pull; I don't have the
reference at hand but in some recent-ish pull request Linus said he
prefers the stable version with the conflict, and optionally you can
provide a second branch with the conflict resolved for reference, but
the pull request should be based on something stable even if it has
conflicts

If there is a conflict Stefen will resolve it like Linus/Greg would, and
the resolved bit will be carried over everyday so it's not much more
work -- exactly like a regular pull request for inclusion in the main
tree :)


(Found another mail arguing for stable base[1], but can't find the mail I
was thinking of)
 [1] 
http://lkml.kernel.org/r/ca+55afw+dwofadgvzrm-ucmsih+f1chocww+xffm3apjorq...@mail.gmail.com

-- 
Dominique Martinet

Re: [GIT PULL linux-next] Add Compiler Attributes tree

2018-10-02 Thread Stephen Rothwell

Hi Miguel,

On Wed, 3 Oct 2018 00:36:52 +0200 Dominique Martinet  
wrote:
>
> Miguel Ojeda wrote on Wed, Oct 03, 2018:
> > As I have read, -next is supposed to be a vision of what the merge
> > window will look like after merging everything, i.e. ideally -rc1. For
> > that to work for files out-of-tree (like these ones, which are not
> > maintained by a single tree), changes should be allowed to be stacked
> > on each other; otherwise, we cannot handle conflicts :-(  
> 
> The rule is the same as with a regular mainline pull; I don't have the
> reference at hand but in some recent-ish pull request Linus said he
> prefers the stable version with the conflict, and optionally you can
> provide a second branch with the conflict resolved for reference, but
> the pull request should be based on something stable even if it has
> conflicts
> 
> If there is a conflict Stefen will resolve it like Linus/Greg would, and
> the resolved bit will be carried over everyday so it's not much more
> work -- exactly like a regular pull request for inclusion in the main
> tree :)

Exactly what Dominique said.  I will fix up the conflict (unless it is
a very complex conflict, in which case the author(s) should help) and
the Linus (or Greg) will do the same.  If you do depend on a patch in
Andrew's series, what happens if that patch does not get sent to Linus
during the merge window or Linus rejects it?

-- 
Cheers,
Stephen Rothwell


pgpeLXsLBPMsC.pgp
Description: OpenPGP digital signature

Re: [PATCH security-next v4 23/32] selinux: Remove boot parameter

On Tue, Oct 2, 2018 at 3:06 PM, James Morris  wrote:
> On Tue, 2 Oct 2018, Kees Cook wrote:
>
>> On Tue, Oct 2, 2018 at 11:57 AM, John Johansen
>>  wrote:
>> > Under the current scheme
>> >
>> > lsm.enabled=selinux
>> >
>> > could actually mean selinux,yama,loadpin,something_else are
>> > enabled. If we extend this behavior to when full stacking lands
>> >
>> > lsm.enabled=selinux,yama
>> >
>> > might mean selinux,yama,apparmor,loadpin,something_else
>> >
>> > and what that list is will vary from kernel to kernel, which I think
>> > is harder for the user than the lsm.enabled list being what is
>> > actually enabled at boot
>>
>> Ah, I think I missed this in your earlier emails. What you don't like
>> here is that "lsm.enable=" is additive. You want it to be explicit.
>>
>
> This is a path to madness.
>
> How about enable flags set ONLY per LSM:
>
> lsm.selinux.enable=x
> lsm.apparmor.enable=x
>
> With no lsm.enable, and removing selinux=x and apparmor=x.
>
> Yes this will break existing docs, but they can be updated for newer
> kernel versions to say "replace selinux=0 with lsm.selinux.enable=0" from
> kernel X onwards.
>
> Surely distro packages and bootloaders are able to cope with changes to
> kernel parameters?
>
> We can either take a one-time hit now, or build new usability debt, which
> will confuse people forever.

I'd like to avoid this for a few reasons:

- this requires per-LSM plumbing instead of centralized plumbing
  - each LSM needs to have its own CONFIG flag
  - each LSM needs to have its own bootparam flag
- SELinux has explicited stated they do not want to lose selinux=
- this doesn't meet John's goal of having a "single explicit enable list"

I think the current proposal (in the other thread) is likely the
sanest approach:

- Drop CONFIG_SECURITY_SELINUX_BOOTPARAM_VALUE
- Drop CONFIG_SECURITY_APPARMOR_BOOTPARAM_VALUE
- All enabled LSMs are listed at build-time in CONFIG_LSM_ENABLE
- Boot time enabling for selinux= and apparmor= remain
- lsm.enable= is explicit: overrides above and omissions are disabled
- maybe include lsm.disable= to disable anything

-Kees

-- 
Kees Cook
Pixel Security

Re: [GIT PULL linux-next] Add Compiler Attributes tree

2018-10-02 Thread Theodore Y. Ts'o

On Wed, Oct 03, 2018 at 12:12:10AM +0200, Miguel Ojeda wrote:
> As I have read, -next is supposed to be a vision of what the merge
> window will look like after merging everything, i.e. ideally -rc1. For
> that to work for files out-of-tree (like these ones, which are not
> maintained by a single tree), changes should be allowed to be stacked
> on each other; otherwise, we cannot handle conflicts :-(

In general, best practice is to base tree on an -rcX commit.  I
usually will use something like -rc4 which is after most of the major
changes have gone in.  This tends to reduce conflicts for most git
trees.

There are times when a commit in one tree needs to depend on a commit
in another tree.  What to do depends on the circumstances.

One solution is to base one subsystem's git tree on another
subsystem's git tree --- *if* that git tree is one that doesn't get
rebase.  You'll need to talk to the subsystem maintainer to find out
whether or not that's the case.  But that's actually not a great
solution, because what can happen is if the tree A is based on tree B,
and there is something in tree B which Linus objects to, tree B won't
get pulled.  And since tree A depends on tree B, Linus will refuse to
pull tree A as well.  We recently had a case where a subsystem pull
got delayed by a full development cycle because of this.

So another solution is to simply evade the problem.  If the reason why
tree A needs to depend on tree B is that tree B is using some
interface which has changed, if it's a minor change, then Stephen will
fix it up when he pulls the changes; just as Linus will.

If the issue is that there is some common infrastructure which two git
tree needs, what will often happen is that just those patches which
provide the new infastructure will get put on a branch based on -rcX
on one of the git trees.  And then the subsystems will base their work
on that sub-branche.  For example, suppose the file system developers
have collectively decided that there should be a new fallocate(2)
flag, FALLOC_FL_DONT_RANDOMLY_LOSE (ala RFC 748).  The work to define
and enable that feature in the VFS layer might be placed on the
randomly-lose git branch on xfs.git.  And then the xfs and ext4
development branches will both be based on the randomly-lose branch.

Yet another solution is to arrange the code changes to avoid needing
commits that might conflict.  For example, in fs/ext4/ext4.h, I very
deliberately did this.

/* Until this gets included into linux/compiler-gcc.h */
#ifndef __nonstring
#if defined(GCC_VERSION) && (GCC_VERSION >= 8)
#define __nonstring __attribute__((nonstring))
#else
#define __nonstring
#endif
#endif

You included a cleanup patch to remove it in your git tree, but it
wasn't actually necessary.  If there was a merge conflict, it would be
simple enough to just drop your cleanup patch, since I had carefully
used #ifndef __nonstring... #endif.  So the idea was that if someone
defined __nonstring somewhere else, it wouldn't break the build with a
duplicate #define since it was protected with an #ifndef.

I didn't mind that you included a cleanup patch; but I set things up
so that it would not be necessary, since often the best way to solve a
merge conflict is by avoiding the need for the change (in some other
git tree) in the first place.  :-)

Cheers,

- Ted

Re: [PATCH security-next v4 23/32] selinux: Remove boot parameter

On 10/02/2018 03:06 PM, James Morris wrote:
> On Tue, 2 Oct 2018, Kees Cook wrote:
> 
>> On Tue, Oct 2, 2018 at 11:57 AM, John Johansen
>>  wrote:
>>> Under the current scheme
>>>
>>> lsm.enabled=selinux
>>>
>>> could actually mean selinux,yama,loadpin,something_else are
>>> enabled. If we extend this behavior to when full stacking lands
>>>
>>> lsm.enabled=selinux,yama
>>>
>>> might mean selinux,yama,apparmor,loadpin,something_else
>>>
>>> and what that list is will vary from kernel to kernel, which I think
>>> is harder for the user than the lsm.enabled list being what is
>>> actually enabled at boot
>>
>> Ah, I think I missed this in your earlier emails. What you don't like
>> here is that "lsm.enable=" is additive. You want it to be explicit.
>>
> 
> This is a path to madness.
> 
> How about enable flags set ONLY per LSM:
> 
> lsm.selinux.enable=x
> lsm.apparmor.enable=x
> 
why add the lsm. prefix? I think if we go this route
selinux.enable=x
apparmor.enable=x

is a little cleaner

the question then becomes is this easier for users? Doing something
similar to this was discussed earlier but its more tedious to type
each of these out, and since they are separate options they can
get spread out making it easy to miss one when you are changing
your boot options.

I honestly don't think we are going to come to a consensus on what is
best for users because different sets of users have different priorities.
But I do think we need to come up with something cleaner than v3

> With no lsm.enable, and removing selinux=x and apparmor=x.
>
this will break the user api, not just the distro/builder kernel
config. I do think it is probably worth doing, but not everyone agrees.

> Yes this will break existing docs, but they can be updated for newer 
> kernel versions to say "replace selinux=0 with lsm.selinux.enable=0" from 
> kernel X onwards.
> 
yes docs can be updated but it does take time to propagate out and their
are always the dozens of blog, and forum posts etc that google will
drag up that won't get updated

> Surely distro packages and bootloaders are able to cope with changes to 
> kernel parameters?
> 
yes, but users who have been taught to add certain incantations to their
kernel parameters find it a lot harder

> We can either take a one-time hit now, or build new usability debt, which 
> will confuse people forever.
> 

I'm not opposed to taking a one-time hit for usability in the future.

Re: [PATCH security-next v4 23/32] selinux: Remove boot parameter

On 10/02/2018 04:06 PM, Kees Cook wrote:
> On Tue, Oct 2, 2018 at 3:06 PM, James Morris  wrote:
>> On Tue, 2 Oct 2018, Kees Cook wrote:
>>
>>> On Tue, Oct 2, 2018 at 11:57 AM, John Johansen
>>>  wrote:
 Under the current scheme

 lsm.enabled=selinux

 could actually mean selinux,yama,loadpin,something_else are
 enabled. If we extend this behavior to when full stacking lands

 lsm.enabled=selinux,yama

 might mean selinux,yama,apparmor,loadpin,something_else

 and what that list is will vary from kernel to kernel, which I think
 is harder for the user than the lsm.enabled list being what is
 actually enabled at boot
>>>
>>> Ah, I think I missed this in your earlier emails. What you don't like
>>> here is that "lsm.enable=" is additive. You want it to be explicit.
>>>
>>
>> This is a path to madness.
>>
>> How about enable flags set ONLY per LSM:
>>
>> lsm.selinux.enable=x
>> lsm.apparmor.enable=x
>>
>> With no lsm.enable, and removing selinux=x and apparmor=x.
>>
>> Yes this will break existing docs, but they can be updated for newer
>> kernel versions to say "replace selinux=0 with lsm.selinux.enable=0" from
>> kernel X onwards.
>>
>> Surely distro packages and bootloaders are able to cope with changes to
>> kernel parameters?
>>
>> We can either take a one-time hit now, or build new usability debt, which
>> will confuse people forever.
> 
> I'd like to avoid this for a few reasons:
> 
> - this requires per-LSM plumbing instead of centralized plumbing
>   - each LSM needs to have its own CONFIG flag
>   - each LSM needs to have its own bootparam flag
> - SELinux has explicited stated they do not want to lose selinux=
> - this doesn't meet John's goal of having a "single explicit enable list"
> 
I can compromise on a single explicit list. My main goal is something that
is consistent and easy for the end user, and I would like to avoid
potential points of confusion if possible.

To me a list like
  lsm.enable=X,Y,Z

is best as a single explicit enable list, and it would be best to avoid
lsm.disable as it just introduces confusion.

I do think per-LSM bootparams looses the advantages of centralization,
and still requires the user to know some Kconfig info but it also gets
rid of the lsm.disable confusion.

With ordering separated out from being enabled there is a certain
cleanness to it. And perhaps most users are looking to enable/disable
a single lsm, instead of specifying exactly what security they want
on their system.

If we were to go this route I would rather drop the lsm. prefix


> I think the current proposal (in the other thread) is likely the
> sanest approach:
> 
> - Drop CONFIG_SECURITY_SELINUX_BOOTPARAM_VALUE
> - Drop CONFIG_SECURITY_APPARMOR_BOOTPARAM_VALUE
> - All enabled LSMs are listed at build-time in CONFIG_LSM_ENABLE

Hrrmmm isn't this a Kconfig selectable list, with each built-in LSM
available to be enabled by default at boot.

> - Boot time enabling for selinux= and apparmor= remain
> - lsm.enable= is explicit: overrides above and omissions are disabled
wfm

> - maybe include lsm.disable= to disable anything

Re: [PATCH security-next v4 23/32] selinux: Remove boot parameter

On Tue, Oct 2, 2018 at 4:46 PM, John Johansen
 wrote:
> On 10/02/2018 04:06 PM, Kees Cook wrote:
>> I think the current proposal (in the other thread) is likely the
>> sanest approach:
>>
>> - Drop CONFIG_SECURITY_SELINUX_BOOTPARAM_VALUE
>> - Drop CONFIG_SECURITY_APPARMOR_BOOTPARAM_VALUE
>> - All enabled LSMs are listed at build-time in CONFIG_LSM_ENABLE
>
> Hrrmmm isn't this a Kconfig selectable list, with each built-in LSM
> available to be enabled by default at boot.

That's not how I have it currently. It's a comma-separated a string,
including the reserved name "all". The default would just be
"CONFIG_LSM_ENABLE=all". Casey and I wanted this to have a way to
capture new LSMs by default at build-time.

>> - Boot time enabling for selinux= and apparmor= remain
>> - lsm.enable= is explicit: overrides above and omissions are disabled
> wfm

Okay, this is closer to v3 than v4. Paul or Stephen, how do you feel
about losing the SELinux bootparam CONFIG? (i.e. CONFIG_LSM_ENABLE
would be replacing its functionality.)

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH security-next v4 23/32] selinux: Remove boot parameter

On 10/02/2018 04:54 PM, Kees Cook wrote:
> On Tue, Oct 2, 2018 at 4:46 PM, John Johansen
>  wrote:
>> On 10/02/2018 04:06 PM, Kees Cook wrote:
>>> I think the current proposal (in the other thread) is likely the
>>> sanest approach:
>>>
>>> - Drop CONFIG_SECURITY_SELINUX_BOOTPARAM_VALUE
>>> - Drop CONFIG_SECURITY_APPARMOR_BOOTPARAM_VALUE
>>> - All enabled LSMs are listed at build-time in CONFIG_LSM_ENABLE
>>
>> Hrrmmm isn't this a Kconfig selectable list, with each built-in LSM
>> available to be enabled by default at boot.
> 
> That's not how I have it currently. It's a comma-separated a string,
> including the reserved name "all". The default would just be
> "CONFIG_LSM_ENABLE=all". Casey and I wanted this to have a way to
> capture new LSMs by default at build-time.
> 

I understand where you are coming from, but speaking with my distro
hat on, that is not going to work. As a distro Ubuntu very much wants
to be able to offer all the LSMs built in to the kernel so the user
can select them. But very much wants to be able to specify a default
supported subset that is enabled at boot.

I expect RH and Suse will feel similarily. Speaking for Ubuntu if this
isn't available as part of lsm stacking it will get distro patched in.

>>> - Boot time enabling for selinux= and apparmor= remain
>>> - lsm.enable= is explicit: overrides above and omissions are disabled
>> wfm
> 
> Okay, this is closer to v3 than v4. Paul or Stephen, how do you feel
> about losing the SELinux bootparam CONFIG? (i.e. CONFIG_LSM_ENABLE
> would be replacing its functionality.)
> 
> -Kees
>

Re: [PATCH security-next v4 23/32] selinux: Remove boot parameter