* Re: [PATCH v12 1/8] mm: support madvise(MADV_FREE)
From: Kirill A. Shutemov @ 2014-07-17 11:28 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, linux-kernel, linux-mm, Michael Kerrisk, Linux API,
Hugh Dickins, Johannes Weiner, Rik van Riel, KOSAKI Motohiro,
Mel Gorman, Jason Evans, Zhang Yanfei
In-Reply-To: <1404886949-17695-2-git-send-email-minchan@kernel.org>
On Wed, Jul 09, 2014 at 03:22:22PM +0900, Minchan Kim wrote:
> Linux doesn't have an ability to free pages lazy while other OS
> already have been supported that named by madvise(MADV_FREE).
>
> The gain is clear that kernel can discard freed pages rather than
> swapping out or OOM if memory pressure happens.
>
> Without memory pressure, freed pages would be reused by userspace
> without another additional overhead(ex, page fault + allocation
> + zeroing).
>
> How to work is following as.
>
> When madvise syscall is called, VM clears dirty bit of ptes of
> the range. If memory pressure happens, VM checks dirty bit of
> page table and if it found still "clean", it means it's a
> "lazyfree pages" so VM could discard the page instead of swapping out.
> Once there was store operation for the page before VM peek a page
> to reclaim, dirty bit is set so VM can swap out the page instead of
> discarding.
>
> Firstly, heavy users would be general allocators(ex, jemalloc,
> tcmalloc and hope glibc supports it) and jemalloc/tcmalloc already
> have supported the feature for other OS(ex, FreeBSD)
>
> barrios@blaptop:~/benchmark/ebizzy$ lscpu
> Architecture: x86_64
> CPU op-mode(s): 32-bit, 64-bit
> Byte Order: Little Endian
> CPU(s): 4
> On-line CPU(s) list: 0-3
> Thread(s) per core: 2
> Core(s) per socket: 2
> Socket(s): 1
> NUMA node(s): 1
> Vendor ID: GenuineIntel
> CPU family: 6
> Model: 42
> Stepping: 7
> CPU MHz: 2801.000
> BogoMIPS: 5581.64
> Virtualization: VT-x
> L1d cache: 32K
> L1i cache: 32K
> L2 cache: 256K
> L3 cache: 4096K
> NUMA node0 CPU(s): 0-3
>
> ebizzy benchmark(./ebizzy -S 10 -n 512)
>
> vanilla-jemalloc MADV_free-jemalloc
>
> 1 thread
> records: 10 records: 10
> avg: 7682.10 avg: 15306.10
> std: 62.35(0.81%) std: 347.99(2.27%)
> max: 7770.00 max: 15622.00
> min: 7598.00 min: 14772.00
>
> 2 thread
> records: 10 records: 10
> avg: 12747.50 avg: 24171.00
> std: 792.06(6.21%) std: 895.18(3.70%)
> max: 13337.00 max: 26023.00
> min: 10535.00 min: 23152.00
>
> 4 thread
> records: 10 records: 10
> avg: 16474.60 avg: 33717.90
> std: 1496.45(9.08%) std: 2008.97(5.96%)
> max: 17877.00 max: 35958.00
> min: 12224.00 min: 29565.00
>
> 8 thread
> records: 10 records: 10
> avg: 16778.50 avg: 33308.10
> std: 825.53(4.92%) std: 1668.30(5.01%)
> max: 17543.00 max: 36010.00
> min: 14576.00 min: 29577.00
>
> 16 thread
> records: 10 records: 10
> avg: 20614.40 avg: 35516.30
> std: 602.95(2.92%) std: 1283.65(3.61%)
> max: 21753.00 max: 37178.00
> min: 19605.00 min: 33217.00
>
> 32 thread
> records: 10 records: 10
> avg: 22771.70 avg: 36018.50
> std: 598.94(2.63%) std: 1046.76(2.91%)
> max: 24035.00 max: 37266.00
> min: 22108.00 min: 34149.00
>
> In summary, MADV_FREE is about 2 time faster than MADV_DONTNEED.
>
> Cc: Michael Kerrisk <mtk.manpages@gmail.com>
> Cc: Linux API <linux-api@vger.kernel.org>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Jason Evans <je@fb.com>
> Acked-by: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>
> Acked-by: Rik van Riel <riel@redhat.com>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
> include/linux/rmap.h | 9 ++-
> include/linux/vm_event_item.h | 1 +
> include/uapi/asm-generic/mman-common.h | 1 +
> mm/madvise.c | 136 +++++++++++++++++++++++++++++++++
> mm/rmap.c | 42 +++++++++-
> mm/vmscan.c | 40 ++++++++--
> mm/vmstat.c | 1 +
> 7 files changed, 218 insertions(+), 12 deletions(-)
>
> diff --git a/include/linux/rmap.h b/include/linux/rmap.h
> index be574506e6a9..0ba377b97a38 100644
> --- a/include/linux/rmap.h
> +++ b/include/linux/rmap.h
> @@ -75,6 +75,7 @@ enum ttu_flags {
> TTU_UNMAP = 1, /* unmap mode */
> TTU_MIGRATION = 2, /* migration mode */
> TTU_MUNLOCK = 4, /* munlock mode */
> + TTU_FREE = 8, /* free mode */
>
> TTU_IGNORE_MLOCK = (1 << 8), /* ignore mlock */
> TTU_IGNORE_ACCESS = (1 << 9), /* don't age */
> @@ -181,7 +182,8 @@ static inline void page_dup_rmap(struct page *page)
> * Called from mm/vmscan.c to handle paging out
> */
> int page_referenced(struct page *, int is_locked,
> - struct mem_cgroup *memcg, unsigned long *vm_flags);
> + struct mem_cgroup *memcg, unsigned long *vm_flags,
> + int *is_dirty);
>
> #define TTU_ACTION(x) ((x) & TTU_ACTION_MASK)
>
> @@ -260,9 +262,12 @@ int rmap_walk(struct page *page, struct rmap_walk_control *rwc);
>
> static inline int page_referenced(struct page *page, int is_locked,
> struct mem_cgroup *memcg,
> - unsigned long *vm_flags)
> + unsigned long *vm_flags,
> + int *is_pte_dirty)
> {
> *vm_flags = 0;
> + if (is_pte_dirty)
> + *is_pte_dirty = 0;
> return 0;
> }
>
> diff --git a/include/linux/vm_event_item.h b/include/linux/vm_event_item.h
> index ced92345c963..e2d3fb1e9814 100644
> --- a/include/linux/vm_event_item.h
> +++ b/include/linux/vm_event_item.h
> @@ -25,6 +25,7 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
> FOR_ALL_ZONES(PGALLOC),
> PGFREE, PGACTIVATE, PGDEACTIVATE,
> PGFAULT, PGMAJFAULT,
> + PGLAZYFREED,
> FOR_ALL_ZONES(PGREFILL),
> FOR_ALL_ZONES(PGSTEAL_KSWAPD),
> FOR_ALL_ZONES(PGSTEAL_DIRECT),
> diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
> index ddc3b36f1046..7a94102b7a02 100644
> --- a/include/uapi/asm-generic/mman-common.h
> +++ b/include/uapi/asm-generic/mman-common.h
> @@ -34,6 +34,7 @@
> #define MADV_SEQUENTIAL 2 /* expect sequential page references */
> #define MADV_WILLNEED 3 /* will need these pages */
> #define MADV_DONTNEED 4 /* don't need these pages */
> +#define MADV_FREE 5 /* free pages only if memory pressure */
>
> /* common parameters: try to keep these consistent across architectures */
> #define MADV_REMOVE 9 /* remove these pages & resources */
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 0938b30da4ab..55b42e5e32a3 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -19,6 +19,14 @@
> #include <linux/blkdev.h>
> #include <linux/swap.h>
> #include <linux/swapops.h>
> +#include <linux/mmu_notifier.h>
> +
> +#include <asm/tlb.h>
> +
> +struct madvise_free_private {
> + struct vm_area_struct *vma;
> + struct mmu_gather *tlb;
> +};
>
> /*
> * Any behaviour which results in changes to the vma->vm_flags needs to
> @@ -31,6 +39,7 @@ static int madvise_need_mmap_write(int behavior)
> case MADV_REMOVE:
> case MADV_WILLNEED:
> case MADV_DONTNEED:
> + case MADV_FREE:
> return 0;
> default:
> /* be safe, default to 1. list exceptions explicitly */
> @@ -251,6 +260,124 @@ static long madvise_willneed(struct vm_area_struct *vma,
> return 0;
> }
>
> +static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
> + unsigned long end, struct mm_walk *walk)
> +
> +{
> + struct madvise_free_private *fp = walk->private;
> + struct mmu_gather *tlb = fp->tlb;
> + struct mm_struct *mm = tlb->mm;
> + struct vm_area_struct *vma = fp->vma;
> + spinlock_t *ptl;
> + pte_t *pte, ptent;
> + struct page *page;
> +
> + split_huge_page_pmd(vma, addr, pmd);
> + if (pmd_trans_unstable(pmd))
> + return 0;
> +
> + pte = pte_offset_map_lock(mm, pmd, addr, &ptl);
> + arch_enter_lazy_mmu_mode();
> + for (; addr != end; pte++, addr += PAGE_SIZE) {
> + ptent = *pte;
> +
> + if (!pte_present(ptent))
> + continue;
> +
> + page = vm_normal_page(vma, addr, ptent);
> + if (!page)
> + continue;
> +
> + if (PageSwapCache(page)) {
> + if (trylock_page(page)) {
> + if (try_to_free_swap(page))
> + ClearPageDirty(page);
> + unlock_page(page);
'continue' for !try_to_free_swap(page) case?
> + } else
> + continue;
> + }
> +
> + /*
> + * Some of architecture(ex, PPC) don't update TLB
> + * with set_pte_at and tlb_remove_tlb_entry so for
> + * the portability, remap the pte with old|clean
> + * after pte clearing.
> + */
> + ptent = ptep_get_and_clear_full(mm, addr, pte,
> + tlb->fullmm);
> + ptent = pte_mkold(ptent);
> + ptent = pte_mkclean(ptent);
> + set_pte_at(mm, addr, pte, ptent);
> + tlb_remove_tlb_entry(tlb, pte, addr);
> + }
> + arch_leave_lazy_mmu_mode();
> + pte_unmap_unlock(pte - 1, ptl);
> + cond_resched();
> + return 0;
> +}
> +
> +static void madvise_free_page_range(struct mmu_gather *tlb,
> + struct vm_area_struct *vma,
> + unsigned long addr, unsigned long end)
> +{
> + struct madvise_free_private fp = {
> + .vma = vma,
> + .tlb = tlb,
> + };
> +
> + struct mm_walk free_walk = {
> + .pmd_entry = madvise_free_pte_range,
> + .mm = vma->vm_mm,
> + .private = &fp,
> + };
> +
> + BUG_ON(addr >= end);
> + tlb_start_vma(tlb, vma);
> + walk_page_range(addr, end, &free_walk);
> + tlb_end_vma(tlb, vma);
> +}
> +
> +static int madvise_free_single_vma(struct vm_area_struct *vma,
> + unsigned long start_addr, unsigned long end_addr)
> +{
> + unsigned long start, end;
> + struct mm_struct *mm = vma->vm_mm;
> + struct mmu_gather tlb;
> +
> + if (vma->vm_flags & (VM_LOCKED|VM_HUGETLB|VM_PFNMAP))
THP uses VM_SPECIAL | VM_HUGETLB | VM_SHARED | VM_MAYSHARE to filter out
'special' VMAs. Looks reasonable here too.
Otherwise:
Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
--
Kirill A. Shutemov
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [net-next PATCH v2 0/3] Broadcast/Multicast rate limit via Ethtool Coalesce
From: Mugunthan V N @ 2014-07-17 6:13 UTC (permalink / raw)
To: David Miller
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20140709.164420.157865201153845876.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
On Thursday 10 July 2014 05:14 AM, David Miller wrote:
> From: Mugunthan V N <mugunthanvnm-l0cyMroinI0@public.gmane.org>
> Date: Wed, 9 Jul 2014 12:44:07 +0530
>
>> A system/cpu can be loaded by a hacker with flooding of broadcast or
>> multicast packets, to prevent this some Ethernet controllers like CPSW
>> provide a mechanism to limit the broadcast/multicast packet rate via
>> hardware limiters. This patch series enables this feature via
>> Ethtool Coalesce.
> This is pretty bogus if you ask me.
>
> What is the difference from accepting a high rate of unicast packets?
> I say it is no different.
>
> Therefore, this feature makes no sense to me at all.
Any packet storm can cause an endpoint some issues. Typically packet
storms will cause the system CPU to thrash resulting is very low system
performance.
Unicast storms only target a single destination end station, it can be
easily mitigated by the host adding a blocking entry in the LUT for each
aggressor.
Broadcast and multicast target multiple end stations, or an entire
network, not only can it cause CPU thrashing, it can result in loss of
other broadcast and multicast services. The rate limiting feature allow
the broadcast and or multicast traffic to be dropped if the rates are
too high. This eliminates the CPU thrashing issue. It also allows the
system to analyze the aggressors and block them for future transgressions.
Regards
Mugunthan V N
^ permalink raw reply
* Re: [PATCH v12 0/8] MADV_FREE support
From: Minchan Kim @ 2014-07-17 5:02 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Michael Kerrisk, Linux API,
Hugh Dickins, Johannes Weiner, Rik van Riel, KOSAKI Motohiro,
Mel Gorman, Jason Evans, Zhang Yanfei, Kirill A. Shutemov
In-Reply-To: <1404886949-17695-1-git-send-email-minchan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Kirill, Do you have any comment?
On Wed, Jul 09, 2014 at 03:22:21PM +0900, Minchan Kim wrote:
> This patch enable MADV_FREE hint for madvise syscall, which have
> been supported by other OSes. [PATCH 1] includes the details.
>
> [1] support MADVISE_FREE for !THP page so if VM encounter
> THP page in syscall context, it splits THP page.
> [2-7] is to preparing to call madvise syscall without THP plitting
> [8] enable THP page support for MADV_FREE.
>
>
> * From v11
> * Fix arm build - Steve
> * Separate patch for arm and arm64 - Steve
> * Remove unnecessary check - Kirill
> * Skip non-vm_normal page - Kirill
> * Add Acked-by - Zhang
> * Sparc64 build fix
> * Pagetable walker THP handling fix
>
> * From v10
> * Add Acked-by from arch stuff(x86, s390)
> * Pagewalker based pagetable working - Kirill
> * Fix try_to_unmap_one broken with hwpoison - Kirill
> * Use VM_BUG_ON_PAGE in madvise_free_pmd - Kirill
> * Fix pgtable-3level.h for arm - Steve
>
> * From v9
> * Add Acked-by - Rik
> * Add THP page support - Kirill
>
> * From v8
> * Rebased-on v3.16-rc2-mmotm-2014-06-25-16-44
>
> * From v7
> * Rebased-on next-20140613
>
> * From v6
> * Remove page from swapcache in syscal time
> * Move utility functions from memory.c to madvise.c - Johannes
> * Rename untilify functtions - Johannes
> * Remove unnecessary checks from vmscan.c - Johannes
> * Rebased-on v3.15-rc5-mmotm-2014-05-16-16-56
> * Drop Reviewe-by because there was some changes since then.
>
> * From v5
> * Fix PPC problem which don't flush TLB - Rik
> * Remove unnecessary lazyfree_range stub function - Rik
> * Rebased on v3.15-rc5
>
> * From v4
> * Add Reviewed-by: Zhang Yanfei
> * Rebase on v3.15-rc1-mmotm-2014-04-15-16-14
>
> * From v3
> * Add "how to work part" in description - Zhang
> * Add page_discardable utility function - Zhang
> * Clean up
>
> * From v2
> * Remove forceful dirty marking of swap-readed page - Johannes
> * Remove deactivation logic of lazyfreed page
> * Rebased on 3.14
> * Remove RFC tag
>
> * From v1
> * Use custom page table walker for madvise_free - Johannes
> * Remove PG_lazypage flag - Johannes
> * Do madvise_dontneed instead of madvise_freein swapless system
>
> Minchan Kim (8):
> [1] mm: support madvise(MADV_FREE)
> [2] x86: add pmd_[dirty|mkclean] for THP
> [3] sparc: add pmd_[dirty|mkclean] for THP
> [4] powerpc: add pmd_[dirty|mkclean] for THP
> [5] s390: add pmd_[dirty|mkclean] for THP
> [6] arm: add pmd_[dirty|mkclean] for THP
> [7] arm64: add pmd_[dirty|mkclean] for THP
> [8] mm: Don't split THP page when syscall is called
>
> arch/arm/include/asm/pgtable-3level.h | 3 +
> arch/arm64/include/asm/pgtable.h | 2 +
> arch/powerpc/include/asm/pgtable-ppc64.h | 2 +
> arch/s390/include/asm/pgtable.h | 12 +++
> arch/sparc/include/asm/pgtable_64.h | 16 ++++
> arch/x86/include/asm/pgtable.h | 10 ++
> include/linux/huge_mm.h | 4 +
> include/linux/rmap.h | 9 +-
> include/linux/vm_event_item.h | 1 +
> include/uapi/asm-generic/mman-common.h | 1 +
> mm/huge_memory.c | 35 +++++++
> mm/madvise.c | 155 +++++++++++++++++++++++++++++++
> mm/rmap.c | 46 ++++++++-
> mm/vmscan.c | 64 +++++++++----
> mm/vmstat.c | 1 +
> 15 files changed, 341 insertions(+), 20 deletions(-)
>
> --
> 2.0.0
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo-Bw31MaZKKs0EbZ0PF+XxCw@public.gmane.org For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org"> email-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org </a>
--
Kind regards,
Minchan Kim
^ permalink raw reply
* [PATCH v11 11/11] seccomp: implement SECCOMP_FILTER_FLAG_TSYNC
From: Kees Cook @ 2014-07-16 21:50 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Andy Lutomirski, Oleg Nesterov, James Morris,
Michael Kerrisk (man-pages), Alexei Starovoitov, Andrew Morton,
Daniel Borkmann, Will Drewry, Julien Tinnes, David Drysdale,
linux-api, x86, linux-arm-kernel, linux-mips, linux-arch,
linux-security-module
In-Reply-To: <1405547442-26641-1-git-send-email-keescook@chromium.org>
Applying restrictive seccomp filter programs to large or diverse
codebases often requires handling threads which may be started early in
the process lifetime (e.g., by code that is linked in). While it is
possible to apply permissive programs prior to process start up, it is
difficult to further restrict the kernel ABI to those threads after that
point.
This change adds a new seccomp syscall flag to SECCOMP_SET_MODE_FILTER for
synchronizing thread group seccomp filters at filter installation time.
When calling seccomp(SECCOMP_SET_MODE_FILTER, SECCOMP_FILTER_FLAG_TSYNC,
filter) an attempt will be made to synchronize all threads in current's
threadgroup to its new seccomp filter program. This is possible iff all
threads are using a filter that is an ancestor to the filter current is
attempting to synchronize to. NULL filters (where the task is running as
SECCOMP_MODE_NONE) are also treated as ancestors allowing threads to be
transitioned into SECCOMP_MODE_FILTER. If prctrl(PR_SET_NO_NEW_PRIVS,
...) has been set on the calling thread, no_new_privs will be set for
all synchronized threads too. On success, 0 is returned. On failure,
the pid of one of the failing threads will be returned and no filters
will have been applied.
The race conditions against another thread are:
- requesting TSYNC (already handled by sighand lock)
- performing a clone (already handled by sighand lock)
- changing its filter (already handled by sighand lock)
- calling exec (handled by cred_guard_mutex)
The clone case is assisted by the fact that new threads will have their
seccomp state duplicated from their parent before appearing on the tasklist.
Holding cred_guard_mutex means that seccomp filters cannot be assigned
while in the middle of another thread's exec (potentially bypassing
no_new_privs or similar). The call to de_thread() may kill threads waiting
for the mutex.
Changes across threads to the filter pointer includes a barrier.
Based on patches by Will Drewry.
Suggested-by: Julien Tinnes <jln@chromium.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Andy Lutomirski <luto@amacapital.net>
---
fs/exec.c | 2 +-
include/linux/seccomp.h | 2 +
include/uapi/linux/seccomp.h | 3 +
kernel/seccomp.c | 130 +++++++++++++++++++++++++++++++++++++++++-
4 files changed, 135 insertions(+), 2 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 0f5c272410f6..ab1f1200ce5d 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1216,7 +1216,7 @@ EXPORT_SYMBOL(install_exec_creds);
/*
* determine how safe it is to execute the proposed program
* - the caller must hold ->cred_guard_mutex to protect against
- * PTRACE_ATTACH
+ * PTRACE_ATTACH or seccomp thread-sync
*/
static void check_unsafe_exec(struct linux_binprm *bprm)
{
diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
index 9ff98b4bfe2e..5d586a45a319 100644
--- a/include/linux/seccomp.h
+++ b/include/linux/seccomp.h
@@ -3,6 +3,8 @@
#include <uapi/linux/seccomp.h>
+#define SECCOMP_FILTER_FLAG_MASK (SECCOMP_FILTER_FLAG_TSYNC)
+
#ifdef CONFIG_SECCOMP
#include <linux/thread_info.h>
diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
index b258878ba754..0f238a43ff1e 100644
--- a/include/uapi/linux/seccomp.h
+++ b/include/uapi/linux/seccomp.h
@@ -14,6 +14,9 @@
#define SECCOMP_SET_MODE_STRICT 0
#define SECCOMP_SET_MODE_FILTER 1
+/* Valid flags for SECCOMP_SET_MODE_FILTER */
+#define SECCOMP_FILTER_FLAG_TSYNC 1
+
/*
* All BPF programs must return a 32-bit value.
* The bottom 16-bits are for optional return data.
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 9065d2c79c56..2125b83ccfd4 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -26,6 +26,7 @@
#ifdef CONFIG_SECCOMP_FILTER
#include <asm/syscall.h>
#include <linux/filter.h>
+#include <linux/pid.h>
#include <linux/ptrace.h>
#include <linux/security.h>
#include <linux/tracehook.h>
@@ -225,6 +226,109 @@ static inline void seccomp_assign_mode(struct task_struct *task,
}
#ifdef CONFIG_SECCOMP_FILTER
+/* Returns 1 if the parent is an ancestor of the child. */
+static int is_ancestor(struct seccomp_filter *parent,
+ struct seccomp_filter *child)
+{
+ /* NULL is the root ancestor. */
+ if (parent == NULL)
+ return 1;
+ for (; child; child = child->prev)
+ if (child == parent)
+ return 1;
+ return 0;
+}
+
+/**
+ * seccomp_can_sync_threads: checks if all threads can be synchronized
+ *
+ * Expects sighand and cred_guard_mutex locks to be held.
+ *
+ * Returns 0 on success, -ve on error, or the pid of a thread which was
+ * either not in the correct seccomp mode or it did not have an ancestral
+ * seccomp filter.
+ */
+static inline pid_t seccomp_can_sync_threads(void)
+{
+ struct task_struct *thread, *caller;
+
+ BUG_ON(!mutex_is_locked(¤t->signal->cred_guard_mutex));
+ BUG_ON(!spin_is_locked(¤t->sighand->siglock));
+
+ if (current->seccomp.mode != SECCOMP_MODE_FILTER)
+ return -EACCES;
+
+ /* Validate all threads being eligible for synchronization. */
+ caller = current;
+ for_each_thread(caller, thread) {
+ pid_t failed;
+
+ if (thread->seccomp.mode == SECCOMP_MODE_DISABLED ||
+ (thread->seccomp.mode == SECCOMP_MODE_FILTER &&
+ is_ancestor(thread->seccomp.filter,
+ caller->seccomp.filter)))
+ continue;
+
+ /* Return the first thread that cannot be synchronized. */
+ failed = task_pid_vnr(thread);
+ /* If the pid cannot be resolved, then return -ESRCH */
+ if (unlikely(WARN_ON(failed == 0)))
+ failed = -ESRCH;
+ return failed;
+ }
+
+ return 0;
+}
+
+/**
+ * seccomp_sync_threads: sets all threads to use current's filter
+ *
+ * Expects sighand and cred_guard_mutex locks to be held, and for
+ * seccomp_can_sync_threads() to have returned success already
+ * without dropping the locks.
+ *
+ */
+static inline void seccomp_sync_threads(void)
+{
+ struct task_struct *thread, *caller;
+
+ BUG_ON(!mutex_is_locked(¤t->signal->cred_guard_mutex));
+ BUG_ON(!spin_is_locked(¤t->sighand->siglock));
+
+ /* Synchronize all threads. */
+ caller = current;
+ for_each_thread(caller, thread) {
+ /* Get a task reference for the new leaf node. */
+ get_seccomp_filter(caller);
+ /*
+ * Drop the task reference to the shared ancestor since
+ * current's path will hold a reference. (This also
+ * allows a put before the assignment.)
+ */
+ put_seccomp_filter(thread);
+ smp_store_release(&thread->seccomp.filter,
+ caller->seccomp.filter);
+ /*
+ * Opt the other thread into seccomp if needed.
+ * As threads are considered to be trust-realm
+ * equivalent (see ptrace_may_access), it is safe to
+ * allow one thread to transition the other.
+ */
+ if (thread->seccomp.mode == SECCOMP_MODE_DISABLED) {
+ /*
+ * Don't let an unprivileged task work around
+ * the no_new_privs restriction by creating
+ * a thread that sets it up, enters seccomp,
+ * then dies.
+ */
+ if (task_no_new_privs(caller))
+ task_set_no_new_privs(thread);
+
+ seccomp_assign_mode(thread, SECCOMP_MODE_FILTER);
+ }
+ }
+}
+
/**
* seccomp_prepare_filter: Prepares a seccomp filter for use.
* @fprog: BPF program to install
@@ -364,6 +468,15 @@ static long seccomp_attach_filter(unsigned int flags,
if (total_insns > MAX_INSNS_PER_PATH)
return -ENOMEM;
+ /* If thread sync has been requested, check that it is possible. */
+ if (flags & SECCOMP_FILTER_FLAG_TSYNC) {
+ int ret;
+
+ ret = seccomp_can_sync_threads();
+ if (ret)
+ return ret;
+ }
+
/*
* If there is an existing filter, make it the prev and don't drop its
* task reference.
@@ -371,6 +484,10 @@ static long seccomp_attach_filter(unsigned int flags,
filter->prev = current->seccomp.filter;
current->seccomp.filter = filter;
+ /* Now that the new filter is in place, synchronize to all threads. */
+ if (flags & SECCOMP_FILTER_FLAG_TSYNC)
+ seccomp_sync_threads();
+
return 0;
}
@@ -590,7 +707,7 @@ static long seccomp_set_mode_filter(unsigned int flags,
long ret = -EINVAL;
/* Validate flags. */
- if (flags != 0)
+ if (flags & ~SECCOMP_FILTER_FLAG_MASK)
return -EINVAL;
/* Prepare the new filter before holding any locks. */
@@ -598,6 +715,14 @@ static long seccomp_set_mode_filter(unsigned int flags,
if (IS_ERR(prepared))
return PTR_ERR(prepared);
+ /*
+ * Make sure we cannot change seccomp or nnp state via TSYNC
+ * while another thread is in the middle of calling exec.
+ */
+ if (flags & SECCOMP_FILTER_FLAG_TSYNC &&
+ mutex_lock_killable(¤t->signal->cred_guard_mutex))
+ goto out_free;
+
spin_lock_irq(¤t->sighand->siglock);
if (!seccomp_may_assign_mode(seccomp_mode))
@@ -612,6 +737,9 @@ static long seccomp_set_mode_filter(unsigned int flags,
seccomp_assign_mode(current, seccomp_mode);
out:
spin_unlock_irq(¤t->sighand->siglock);
+ if (flags & SECCOMP_FILTER_FLAG_TSYNC)
+ mutex_unlock(¤t->signal->cred_guard_mutex);
+out_free:
seccomp_filter_free(prepared);
return ret;
}
--
1.7.9.5
^ permalink raw reply related
* [PATCH v11 10/11] seccomp: allow mode setting across threads
From: Kees Cook @ 2014-07-16 21:50 UTC (permalink / raw)
To: linux-kernel
Cc: linux-arch, linux-mips, Will Drewry, Kees Cook,
linux-security-module, linux-api, x86, James Morris,
Oleg Nesterov, Daniel Borkmann, Julien Tinnes,
Michael Kerrisk (man-pages), Andrew Morton, Andy Lutomirski,
David Drysdale, linux-arm-kernel, Alexei Starovoitov
In-Reply-To: <1405547442-26641-1-git-send-email-keescook@chromium.org>
This changes the mode setting helper to allow threads to change the
seccomp mode from another thread. We must maintain barriers to keep
TIF_SECCOMP synchronized with the rest of the seccomp state.
Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Andy Lutomirski <luto@amacapital.net>
---
kernel/seccomp.c | 36 +++++++++++++++++++++++++-----------
1 file changed, 25 insertions(+), 11 deletions(-)
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index d5543e787e4e..9065d2c79c56 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -173,21 +173,24 @@ static int seccomp_check_filter(struct sock_filter *filter, unsigned int flen)
*/
static u32 seccomp_run_filters(int syscall)
{
- struct seccomp_filter *f;
+ struct seccomp_filter *f = ACCESS_ONCE(current->seccomp.filter);
struct seccomp_data sd;
u32 ret = SECCOMP_RET_ALLOW;
/* Ensure unexpected behavior doesn't result in failing open. */
- if (WARN_ON(current->seccomp.filter == NULL))
+ if (unlikely(WARN_ON(f == NULL)))
return SECCOMP_RET_KILL;
+ /* Make sure cross-thread synced filter points somewhere sane. */
+ smp_read_barrier_depends();
+
populate_seccomp_data(&sd);
/*
* All filters in the list are evaluated and the lowest BPF return
* value always takes priority (ignoring the DATA).
*/
- for (f = current->seccomp.filter; f; f = f->prev) {
+ for (; f; f = f->prev) {
u32 cur_ret = SK_RUN_FILTER(f->prog, (void *)&sd);
if ((cur_ret & SECCOMP_RET_ACTION) < (ret & SECCOMP_RET_ACTION))
@@ -207,12 +210,18 @@ static inline bool seccomp_may_assign_mode(unsigned long seccomp_mode)
return true;
}
-static inline void seccomp_assign_mode(unsigned long seccomp_mode)
+static inline void seccomp_assign_mode(struct task_struct *task,
+ unsigned long seccomp_mode)
{
- BUG_ON(!spin_is_locked(¤t->sighand->siglock));
+ BUG_ON(!spin_is_locked(&task->sighand->siglock));
- current->seccomp.mode = seccomp_mode;
- set_tsk_thread_flag(current, TIF_SECCOMP);
+ task->seccomp.mode = seccomp_mode;
+ /*
+ * Make sure TIF_SECCOMP cannot be set before the mode (and
+ * filter) is set.
+ */
+ smp_mb__before_atomic();
+ set_tsk_thread_flag(task, TIF_SECCOMP);
}
#ifdef CONFIG_SECCOMP_FILTER
@@ -435,12 +444,17 @@ static int mode1_syscalls_32[] = {
int __secure_computing(int this_syscall)
{
- int mode = current->seccomp.mode;
int exit_sig = 0;
int *syscall;
u32 ret;
- switch (mode) {
+ /*
+ * Make sure that any changes to mode from another thread have
+ * been seen after TIF_SECCOMP was seen.
+ */
+ rmb();
+
+ switch (current->seccomp.mode) {
case SECCOMP_MODE_STRICT:
syscall = mode1_syscalls;
#ifdef CONFIG_COMPAT
@@ -545,7 +559,7 @@ static long seccomp_set_mode_strict(void)
#ifdef TIF_NOTSC
disable_TSC();
#endif
- seccomp_assign_mode(seccomp_mode);
+ seccomp_assign_mode(current, seccomp_mode);
ret = 0;
out:
@@ -595,7 +609,7 @@ static long seccomp_set_mode_filter(unsigned int flags,
/* Do not free the successfully attached filter. */
prepared = NULL;
- seccomp_assign_mode(seccomp_mode);
+ seccomp_assign_mode(current, seccomp_mode);
out:
spin_unlock_irq(¤t->sighand->siglock);
seccomp_filter_free(prepared);
--
1.7.9.5
^ permalink raw reply related
* [PATCH v11 09/11] seccomp: introduce writer locking
From: Kees Cook @ 2014-07-16 21:50 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Kees Cook, Andy Lutomirski, Oleg Nesterov, James Morris,
Michael Kerrisk (man-pages), Alexei Starovoitov, Andrew Morton,
Daniel Borkmann, Will Drewry, Julien Tinnes, David Drysdale,
linux-api-u79uwXL29TY76Z2rM5mHXA, x86-DgEjT+Ai2ygdnm+yROfE0A,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-mips-6z/3iImG2C8G8FEW9MqTrA,
linux-arch-u79uwXL29TY76Z2rM5mHXA,
linux-security-module-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1405547442-26641-1-git-send-email-keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Normally, task_struct.seccomp.filter is only ever read or modified by
the task that owns it (current). This property aids in fast access
during system call filtering as read access is lockless.
Updating the pointer from another task, however, opens up race
conditions. To allow cross-thread filter pointer updates, writes to the
seccomp fields are now protected by the sighand spinlock (which is shared
by all threads in the thread group). Read access remains lockless because
pointer updates themselves are atomic. However, writes (or cloning)
often entail additional checking (like maximum instruction counts)
which require locking to perform safely.
In the case of cloning threads, the child is invisible to the system
until it enters the task list. To make sure a child can't be cloned from
a thread and left in a prior state, seccomp duplication is additionally
moved under the sighand lock. Then parent and child are certain have
the same seccomp state when they exit the lock.
Based on patches by Will Drewry and David Drysdale.
Signed-off-by: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Reviewed-by: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
---
include/linux/seccomp.h | 6 +++---
kernel/fork.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++-
kernel/seccomp.c | 16 +++++++++++++++-
3 files changed, 66 insertions(+), 5 deletions(-)
diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
index 4054b0994071..9ff98b4bfe2e 100644
--- a/include/linux/seccomp.h
+++ b/include/linux/seccomp.h
@@ -14,11 +14,11 @@ struct seccomp_filter;
*
* @mode: indicates one of the valid values above for controlled
* system calls available to a process.
- * @filter: The metadata and ruleset for determining what system calls
- * are allowed for a task.
+ * @filter: must always point to a valid seccomp-filter or NULL as it is
+ * accessed without locking during system call entry.
*
* @filter must only be accessed from the context of current as there
- * is no locking.
+ * is no read locking.
*/
struct seccomp {
int mode;
diff --git a/kernel/fork.c b/kernel/fork.c
index 6a13c46cd87d..ed4bc339c9dc 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -315,6 +315,15 @@ static struct task_struct *dup_task_struct(struct task_struct *orig)
goto free_ti;
tsk->stack = ti;
+#ifdef CONFIG_SECCOMP
+ /*
+ * We must handle setting up seccomp filters once we're under
+ * the sighand lock in case orig has changed between now and
+ * then. Until then, filter must be NULL to avoid messing up
+ * the usage counts on the error path calling free_task.
+ */
+ tsk->seccomp.filter = NULL;
+#endif
setup_thread_stack(tsk, orig);
clear_user_return_notifier(tsk);
@@ -1081,6 +1090,39 @@ static int copy_signal(unsigned long clone_flags, struct task_struct *tsk)
return 0;
}
+static void copy_seccomp(struct task_struct *p)
+{
+#ifdef CONFIG_SECCOMP
+ /*
+ * Must be called with sighand->lock held, which is common to
+ * all threads in the group. Holding cred_guard_mutex is not
+ * needed because this new task is not yet running and cannot
+ * be racing exec.
+ */
+ BUG_ON(!spin_is_locked(¤t->sighand->siglock));
+
+ /* Ref-count the new filter user, and assign it. */
+ get_seccomp_filter(current);
+ p->seccomp = current->seccomp;
+
+ /*
+ * Explicitly enable no_new_privs here in case it got set
+ * between the task_struct being duplicated and holding the
+ * sighand lock. The seccomp state and nnp must be in sync.
+ */
+ if (task_no_new_privs(current))
+ task_set_no_new_privs(p);
+
+ /*
+ * If the parent gained a seccomp mode after copying thread
+ * flags and between before we held the sighand lock, we have
+ * to manually enable the seccomp thread flag here.
+ */
+ if (p->seccomp.mode != SECCOMP_MODE_DISABLED)
+ set_tsk_thread_flag(p, TIF_SECCOMP);
+#endif
+}
+
SYSCALL_DEFINE1(set_tid_address, int __user *, tidptr)
{
current->clear_child_tid = tidptr;
@@ -1196,7 +1238,6 @@ static struct task_struct *copy_process(unsigned long clone_flags,
goto fork_out;
ftrace_graph_init_task(p);
- get_seccomp_filter(p);
rt_mutex_init_task(p);
@@ -1437,6 +1478,12 @@ static struct task_struct *copy_process(unsigned long clone_flags,
spin_lock(¤t->sighand->siglock);
/*
+ * Copy seccomp details explicitly here, in case they were changed
+ * before holding sighand lock.
+ */
+ copy_seccomp(p);
+
+ /*
* Process group and session signals need to be delivered to just the
* parent before the fork or both the parent and the child after the
* fork. Restart if a signal comes in before we add the new process to
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 58125160417c..d5543e787e4e 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -199,6 +199,8 @@ static u32 seccomp_run_filters(int syscall)
static inline bool seccomp_may_assign_mode(unsigned long seccomp_mode)
{
+ BUG_ON(!spin_is_locked(¤t->sighand->siglock));
+
if (current->seccomp.mode && current->seccomp.mode != seccomp_mode)
return false;
@@ -207,6 +209,8 @@ static inline bool seccomp_may_assign_mode(unsigned long seccomp_mode)
static inline void seccomp_assign_mode(unsigned long seccomp_mode)
{
+ BUG_ON(!spin_is_locked(¤t->sighand->siglock));
+
current->seccomp.mode = seccomp_mode;
set_tsk_thread_flag(current, TIF_SECCOMP);
}
@@ -332,6 +336,8 @@ out:
* @flags: flags to change filter behavior
* @filter: seccomp filter to add to the current process
*
+ * Caller must be holding current->sighand->siglock lock.
+ *
* Returns 0 on success, -ve on error.
*/
static long seccomp_attach_filter(unsigned int flags,
@@ -340,6 +346,8 @@ static long seccomp_attach_filter(unsigned int flags,
unsigned long total_insns;
struct seccomp_filter *walker;
+ BUG_ON(!spin_is_locked(¤t->sighand->siglock));
+
/* Validate resulting filter length. */
total_insns = filter->prog->len;
for (walker = current->seccomp.filter; walker; walker = walker->prev)
@@ -529,6 +537,8 @@ static long seccomp_set_mode_strict(void)
const unsigned long seccomp_mode = SECCOMP_MODE_STRICT;
long ret = -EINVAL;
+ spin_lock_irq(¤t->sighand->siglock);
+
if (!seccomp_may_assign_mode(seccomp_mode))
goto out;
@@ -539,6 +549,7 @@ static long seccomp_set_mode_strict(void)
ret = 0;
out:
+ spin_unlock_irq(¤t->sighand->siglock);
return ret;
}
@@ -566,13 +577,15 @@ static long seccomp_set_mode_filter(unsigned int flags,
/* Validate flags. */
if (flags != 0)
- goto out;
+ return -EINVAL;
/* Prepare the new filter before holding any locks. */
prepared = seccomp_prepare_user_filter(filter);
if (IS_ERR(prepared))
return PTR_ERR(prepared);
+ spin_lock_irq(¤t->sighand->siglock);
+
if (!seccomp_may_assign_mode(seccomp_mode))
goto out;
@@ -584,6 +597,7 @@ static long seccomp_set_mode_filter(unsigned int flags,
seccomp_assign_mode(seccomp_mode);
out:
+ spin_unlock_irq(¤t->sighand->siglock);
seccomp_filter_free(prepared);
return ret;
}
--
1.7.9.5
^ permalink raw reply related
* [PATCH v11 08/11] seccomp: split filter prep from check and apply
From: Kees Cook @ 2014-07-16 21:50 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Kees Cook, Andy Lutomirski, Oleg Nesterov, James Morris,
Michael Kerrisk (man-pages), Alexei Starovoitov, Andrew Morton,
Daniel Borkmann, Will Drewry, Julien Tinnes, David Drysdale,
linux-api-u79uwXL29TY76Z2rM5mHXA, x86-DgEjT+Ai2ygdnm+yROfE0A,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-mips-6z/3iImG2C8G8FEW9MqTrA,
linux-arch-u79uwXL29TY76Z2rM5mHXA,
linux-security-module-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1405547442-26641-1-git-send-email-keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
In preparation for adding seccomp locking, move filter creation away
from where it is checked and applied. This will allow for locking where
no memory allocation is happening. The validation, filter attachment,
and seccomp mode setting can all happen under the future locks.
For extreme defensiveness, I've added a BUG_ON check for the calculated
size of the buffer allocation in case BPF_MAXINSN ever changes, which
shouldn't ever happen. The compiler should actually optimize out this
check since the test above it makes it impossible.
Signed-off-by: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Reviewed-by: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
---
kernel/seccomp.c | 97 +++++++++++++++++++++++++++++++++++++-----------------
1 file changed, 67 insertions(+), 30 deletions(-)
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index d2596136b0d1..58125160417c 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -18,6 +18,7 @@
#include <linux/compat.h>
#include <linux/sched.h>
#include <linux/seccomp.h>
+#include <linux/slab.h>
#include <linux/syscalls.h>
/* #define SECCOMP_DEBUG 1 */
@@ -27,7 +28,6 @@
#include <linux/filter.h>
#include <linux/ptrace.h>
#include <linux/security.h>
-#include <linux/slab.h>
#include <linux/tracehook.h>
#include <linux/uaccess.h>
@@ -213,27 +213,23 @@ static inline void seccomp_assign_mode(unsigned long seccomp_mode)
#ifdef CONFIG_SECCOMP_FILTER
/**
- * seccomp_attach_filter: Attaches a seccomp filter to current.
+ * seccomp_prepare_filter: Prepares a seccomp filter for use.
* @fprog: BPF program to install
*
- * Returns 0 on success or an errno on failure.
+ * Returns filter on success or an ERR_PTR on failure.
*/
-static long seccomp_attach_filter(struct sock_fprog *fprog)
+static struct seccomp_filter *seccomp_prepare_filter(struct sock_fprog *fprog)
{
struct seccomp_filter *filter;
- unsigned long fp_size = fprog->len * sizeof(struct sock_filter);
- unsigned long total_insns = fprog->len;
+ unsigned long fp_size;
struct sock_filter *fp;
int new_len;
long ret;
if (fprog->len == 0 || fprog->len > BPF_MAXINSNS)
- return -EINVAL;
-
- for (filter = current->seccomp.filter; filter; filter = filter->prev)
- total_insns += filter->prog->len + 4; /* include a 4 instr penalty */
- if (total_insns > MAX_INSNS_PER_PATH)
- return -ENOMEM;
+ return ERR_PTR(-EINVAL);
+ BUG_ON(INT_MAX / fprog->len < sizeof(struct sock_filter));
+ fp_size = fprog->len * sizeof(struct sock_filter);
/*
* Installing a seccomp filter requires that the task has
@@ -244,11 +240,11 @@ static long seccomp_attach_filter(struct sock_fprog *fprog)
if (!task_no_new_privs(current) &&
security_capable_noaudit(current_cred(), current_user_ns(),
CAP_SYS_ADMIN) != 0)
- return -EACCES;
+ return ERR_PTR(-EACCES);
fp = kzalloc(fp_size, GFP_KERNEL|__GFP_NOWARN);
if (!fp)
- return -ENOMEM;
+ return ERR_PTR(-ENOMEM);
/* Copy the instructions from fprog. */
ret = -EFAULT;
@@ -292,13 +288,7 @@ static long seccomp_attach_filter(struct sock_fprog *fprog)
sk_filter_select_runtime(filter->prog);
- /*
- * If there is an existing filter, make it the prev and don't drop its
- * task reference.
- */
- filter->prev = current->seccomp.filter;
- current->seccomp.filter = filter;
- return 0;
+ return filter;
free_filter_prog:
kfree(filter->prog);
@@ -306,19 +296,20 @@ free_filter:
kfree(filter);
free_prog:
kfree(fp);
- return ret;
+ return ERR_PTR(ret);
}
/**
- * seccomp_attach_user_filter - attaches a user-supplied sock_fprog
+ * seccomp_prepare_user_filter - prepares a user-supplied sock_fprog
* @user_filter: pointer to the user data containing a sock_fprog.
*
* Returns 0 on success and non-zero otherwise.
*/
-static long seccomp_attach_user_filter(const char __user *user_filter)
+static struct seccomp_filter *
+seccomp_prepare_user_filter(const char __user *user_filter)
{
struct sock_fprog fprog;
- long ret = -EFAULT;
+ struct seccomp_filter *filter = ERR_PTR(-EFAULT);
#ifdef CONFIG_COMPAT
if (is_compat_task()) {
@@ -331,9 +322,39 @@ static long seccomp_attach_user_filter(const char __user *user_filter)
#endif
if (copy_from_user(&fprog, user_filter, sizeof(fprog)))
goto out;
- ret = seccomp_attach_filter(&fprog);
+ filter = seccomp_prepare_filter(&fprog);
out:
- return ret;
+ return filter;
+}
+
+/**
+ * seccomp_attach_filter: validate and attach filter
+ * @flags: flags to change filter behavior
+ * @filter: seccomp filter to add to the current process
+ *
+ * Returns 0 on success, -ve on error.
+ */
+static long seccomp_attach_filter(unsigned int flags,
+ struct seccomp_filter *filter)
+{
+ unsigned long total_insns;
+ struct seccomp_filter *walker;
+
+ /* Validate resulting filter length. */
+ total_insns = filter->prog->len;
+ for (walker = current->seccomp.filter; walker; walker = walker->prev)
+ total_insns += walker->prog->len + 4; /* 4 instr penalty */
+ if (total_insns > MAX_INSNS_PER_PATH)
+ return -ENOMEM;
+
+ /*
+ * If there is an existing filter, make it the prev and don't drop its
+ * task reference.
+ */
+ filter->prev = current->seccomp.filter;
+ current->seccomp.filter = filter;
+
+ return 0;
}
/* get_seccomp_filter - increments the reference count of the filter on @tsk */
@@ -346,6 +367,14 @@ void get_seccomp_filter(struct task_struct *tsk)
atomic_inc(&orig->usage);
}
+static inline void seccomp_filter_free(struct seccomp_filter *filter)
+{
+ if (filter) {
+ sk_filter_free(filter->prog);
+ kfree(filter);
+ }
+}
+
/* put_seccomp_filter - decrements the ref count of tsk->seccomp.filter */
void put_seccomp_filter(struct task_struct *tsk)
{
@@ -354,8 +383,7 @@ void put_seccomp_filter(struct task_struct *tsk)
while (orig && atomic_dec_and_test(&orig->usage)) {
struct seccomp_filter *freeme = orig;
orig = orig->prev;
- sk_filter_free(freeme->prog);
- kfree(freeme);
+ seccomp_filter_free(freeme);
}
}
@@ -533,21 +561,30 @@ static long seccomp_set_mode_filter(unsigned int flags,
const char __user *filter)
{
const unsigned long seccomp_mode = SECCOMP_MODE_FILTER;
+ struct seccomp_filter *prepared = NULL;
long ret = -EINVAL;
/* Validate flags. */
if (flags != 0)
goto out;
+ /* Prepare the new filter before holding any locks. */
+ prepared = seccomp_prepare_user_filter(filter);
+ if (IS_ERR(prepared))
+ return PTR_ERR(prepared);
+
if (!seccomp_may_assign_mode(seccomp_mode))
goto out;
- ret = seccomp_attach_user_filter(filter);
+ ret = seccomp_attach_filter(flags, prepared);
if (ret)
goto out;
+ /* Do not free the successfully attached filter. */
+ prepared = NULL;
seccomp_assign_mode(seccomp_mode);
out:
+ seccomp_filter_free(prepared);
return ret;
}
#else
--
1.7.9.5
^ permalink raw reply related
* [PATCH v11 07/11] sched: move no_new_privs into new atomic flags
From: Kees Cook @ 2014-07-16 21:50 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Andy Lutomirski, Oleg Nesterov, James Morris,
Michael Kerrisk (man-pages), Alexei Starovoitov, Andrew Morton,
Daniel Borkmann, Will Drewry, Julien Tinnes, David Drysdale,
linux-api, x86, linux-arm-kernel, linux-mips, linux-arch,
linux-security-module
In-Reply-To: <1405547442-26641-1-git-send-email-keescook@chromium.org>
Since seccomp transitions between threads requires updates to the
no_new_privs flag to be atomic, the flag must be part of an atomic flag
set. This moves the nnp flag into a separate task field, and introduces
accessors.
Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Andy Lutomirski <luto@amacapital.net>
---
fs/exec.c | 4 ++--
include/linux/sched.h | 18 +++++++++++++++---
kernel/seccomp.c | 2 +-
kernel/sys.c | 4 ++--
security/apparmor/domain.c | 4 ++--
5 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index a3d33fe592d6..0f5c272410f6 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1234,7 +1234,7 @@ static void check_unsafe_exec(struct linux_binprm *bprm)
* This isn't strictly necessary, but it makes it harder for LSMs to
* mess up.
*/
- if (current->no_new_privs)
+ if (task_no_new_privs(current))
bprm->unsafe |= LSM_UNSAFE_NO_NEW_PRIVS;
t = p;
@@ -1272,7 +1272,7 @@ int prepare_binprm(struct linux_binprm *bprm)
bprm->cred->egid = current_egid();
if (!(bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID) &&
- !current->no_new_privs &&
+ !task_no_new_privs(current) &&
kuid_has_mapping(bprm->cred->user_ns, inode->i_uid) &&
kgid_has_mapping(bprm->cred->user_ns, inode->i_gid)) {
/* Set-uid? */
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 306f4f0c987a..0fd19055bb64 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1307,13 +1307,12 @@ struct task_struct {
* execve */
unsigned in_iowait:1;
- /* task may not gain privileges */
- unsigned no_new_privs:1;
-
/* Revert to default priority/policy when forking */
unsigned sched_reset_on_fork:1;
unsigned sched_contributes_to_load:1;
+ unsigned long atomic_flags; /* Flags needing atomic access. */
+
pid_t pid;
pid_t tgid;
@@ -1967,6 +1966,19 @@ static inline void memalloc_noio_restore(unsigned int flags)
current->flags = (current->flags & ~PF_MEMALLOC_NOIO) | flags;
}
+/* Per-process atomic flags. */
+#define PFA_NO_NEW_PRIVS 0x00000001 /* May not gain new privileges. */
+
+static inline bool task_no_new_privs(struct task_struct *p)
+{
+ return test_bit(PFA_NO_NEW_PRIVS, &p->atomic_flags);
+}
+
+static inline void task_set_no_new_privs(struct task_struct *p)
+{
+ set_bit(PFA_NO_NEW_PRIVS, &p->atomic_flags);
+}
+
/*
* task->jobctl flags
*/
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index f0652578af75..d2596136b0d1 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -241,7 +241,7 @@ static long seccomp_attach_filter(struct sock_fprog *fprog)
* This avoids scenarios where unprivileged tasks can affect the
* behavior of privileged children.
*/
- if (!current->no_new_privs &&
+ if (!task_no_new_privs(current) &&
security_capable_noaudit(current_cred(), current_user_ns(),
CAP_SYS_ADMIN) != 0)
return -EACCES;
diff --git a/kernel/sys.c b/kernel/sys.c
index 66a751ebf9d9..ce8129192a26 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1990,12 +1990,12 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
if (arg2 != 1 || arg3 || arg4 || arg5)
return -EINVAL;
- current->no_new_privs = 1;
+ task_set_no_new_privs(current);
break;
case PR_GET_NO_NEW_PRIVS:
if (arg2 || arg3 || arg4 || arg5)
return -EINVAL;
- return current->no_new_privs ? 1 : 0;
+ return task_no_new_privs(current) ? 1 : 0;
case PR_GET_THP_DISABLE:
if (arg2 || arg3 || arg4 || arg5)
return -EINVAL;
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 452567d3a08e..d97cba3e3849 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -621,7 +621,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, bool permtest)
* There is no exception for unconfined as change_hat is not
* available.
*/
- if (current->no_new_privs)
+ if (task_no_new_privs(current))
return -EPERM;
/* released below */
@@ -776,7 +776,7 @@ int aa_change_profile(const char *ns_name, const char *hname, bool onexec,
* no_new_privs is set because this aways results in a reduction
* of permissions.
*/
- if (current->no_new_privs && !unconfined(profile)) {
+ if (task_no_new_privs(current) && !unconfined(profile)) {
put_cred(cred);
return -EPERM;
}
--
1.7.9.5
^ permalink raw reply related
* [PATCH v11 06/11] MIPS: add seccomp syscall
From: Kees Cook @ 2014-07-16 21:50 UTC (permalink / raw)
To: linux-kernel
Cc: linux-arch, linux-mips, Will Drewry, Kees Cook,
linux-security-module, linux-api, x86, James Morris,
Oleg Nesterov, Daniel Borkmann, Julien Tinnes,
Michael Kerrisk (man-pages), Andrew Morton, Andy Lutomirski,
David Drysdale, linux-arm-kernel, Alexei Starovoitov
In-Reply-To: <1405547442-26641-1-git-send-email-keescook@chromium.org>
Wires up the new seccomp syscall.
Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
---
arch/mips/include/uapi/asm/unistd.h | 15 +++++++++------
arch/mips/kernel/scall32-o32.S | 1 +
arch/mips/kernel/scall64-64.S | 1 +
arch/mips/kernel/scall64-n32.S | 1 +
arch/mips/kernel/scall64-o32.S | 1 +
5 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/arch/mips/include/uapi/asm/unistd.h b/arch/mips/include/uapi/asm/unistd.h
index 5805414777e0..9bc13eaf9d67 100644
--- a/arch/mips/include/uapi/asm/unistd.h
+++ b/arch/mips/include/uapi/asm/unistd.h
@@ -372,16 +372,17 @@
#define __NR_sched_setattr (__NR_Linux + 349)
#define __NR_sched_getattr (__NR_Linux + 350)
#define __NR_renameat2 (__NR_Linux + 351)
+#define __NR_seccomp (__NR_Linux + 352)
/*
* Offset of the last Linux o32 flavoured syscall
*/
-#define __NR_Linux_syscalls 351
+#define __NR_Linux_syscalls 352
#endif /* _MIPS_SIM == _MIPS_SIM_ABI32 */
#define __NR_O32_Linux 4000
-#define __NR_O32_Linux_syscalls 351
+#define __NR_O32_Linux_syscalls 352
#if _MIPS_SIM == _MIPS_SIM_ABI64
@@ -701,16 +702,17 @@
#define __NR_sched_setattr (__NR_Linux + 309)
#define __NR_sched_getattr (__NR_Linux + 310)
#define __NR_renameat2 (__NR_Linux + 311)
+#define __NR_seccomp (__NR_Linux + 312)
/*
* Offset of the last Linux 64-bit flavoured syscall
*/
-#define __NR_Linux_syscalls 311
+#define __NR_Linux_syscalls 312
#endif /* _MIPS_SIM == _MIPS_SIM_ABI64 */
#define __NR_64_Linux 5000
-#define __NR_64_Linux_syscalls 311
+#define __NR_64_Linux_syscalls 312
#if _MIPS_SIM == _MIPS_SIM_NABI32
@@ -1034,15 +1036,16 @@
#define __NR_sched_setattr (__NR_Linux + 313)
#define __NR_sched_getattr (__NR_Linux + 314)
#define __NR_renameat2 (__NR_Linux + 315)
+#define __NR_seccomp (__NR_Linux + 316)
/*
* Offset of the last N32 flavoured syscall
*/
-#define __NR_Linux_syscalls 315
+#define __NR_Linux_syscalls 316
#endif /* _MIPS_SIM == _MIPS_SIM_NABI32 */
#define __NR_N32_Linux 6000
-#define __NR_N32_Linux_syscalls 315
+#define __NR_N32_Linux_syscalls 316
#endif /* _UAPI_ASM_UNISTD_H */
diff --git a/arch/mips/kernel/scall32-o32.S b/arch/mips/kernel/scall32-o32.S
index 3245474f19d5..ab02d14f1b5c 100644
--- a/arch/mips/kernel/scall32-o32.S
+++ b/arch/mips/kernel/scall32-o32.S
@@ -578,3 +578,4 @@ EXPORT(sys_call_table)
PTR sys_sched_setattr
PTR sys_sched_getattr /* 4350 */
PTR sys_renameat2
+ PTR sys_seccomp
diff --git a/arch/mips/kernel/scall64-64.S b/arch/mips/kernel/scall64-64.S
index be2fedd4ae33..010dccf128ec 100644
--- a/arch/mips/kernel/scall64-64.S
+++ b/arch/mips/kernel/scall64-64.S
@@ -431,4 +431,5 @@ EXPORT(sys_call_table)
PTR sys_sched_setattr
PTR sys_sched_getattr /* 5310 */
PTR sys_renameat2
+ PTR sys_seccomp
.size sys_call_table,.-sys_call_table
diff --git a/arch/mips/kernel/scall64-n32.S b/arch/mips/kernel/scall64-n32.S
index c1dbcda4b816..c3b3b6525df5 100644
--- a/arch/mips/kernel/scall64-n32.S
+++ b/arch/mips/kernel/scall64-n32.S
@@ -424,4 +424,5 @@ EXPORT(sysn32_call_table)
PTR sys_sched_setattr
PTR sys_sched_getattr
PTR sys_renameat2 /* 6315 */
+ PTR sys_seccomp
.size sysn32_call_table,.-sysn32_call_table
diff --git a/arch/mips/kernel/scall64-o32.S b/arch/mips/kernel/scall64-o32.S
index f1343ccd7ed7..bb1550b1f501 100644
--- a/arch/mips/kernel/scall64-o32.S
+++ b/arch/mips/kernel/scall64-o32.S
@@ -557,4 +557,5 @@ EXPORT(sys32_call_table)
PTR sys_sched_setattr
PTR sys_sched_getattr /* 4350 */
PTR sys_renameat2
+ PTR sys_seccomp
.size sys32_call_table,.-sys32_call_table
--
1.7.9.5
^ permalink raw reply related
* [PATCH v11 05/11] ARM: add seccomp syscall
From: Kees Cook @ 2014-07-16 21:50 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Andy Lutomirski, Oleg Nesterov, James Morris,
Michael Kerrisk (man-pages), Alexei Starovoitov, Andrew Morton,
Daniel Borkmann, Will Drewry, Julien Tinnes, David Drysdale,
linux-api, x86, linux-arm-kernel, linux-mips, linux-arch,
linux-security-module
In-Reply-To: <1405547442-26641-1-git-send-email-keescook@chromium.org>
Wires up the new seccomp syscall.
Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
---
arch/arm/include/uapi/asm/unistd.h | 1 +
arch/arm/kernel/calls.S | 1 +
2 files changed, 2 insertions(+)
diff --git a/arch/arm/include/uapi/asm/unistd.h b/arch/arm/include/uapi/asm/unistd.h
index ba94446c72d9..e21b4a069701 100644
--- a/arch/arm/include/uapi/asm/unistd.h
+++ b/arch/arm/include/uapi/asm/unistd.h
@@ -409,6 +409,7 @@
#define __NR_sched_setattr (__NR_SYSCALL_BASE+380)
#define __NR_sched_getattr (__NR_SYSCALL_BASE+381)
#define __NR_renameat2 (__NR_SYSCALL_BASE+382)
+#define __NR_seccomp (__NR_SYSCALL_BASE+383)
/*
* This may need to be greater than __NR_last_syscall+1 in order to
diff --git a/arch/arm/kernel/calls.S b/arch/arm/kernel/calls.S
index 8f51bdcdacbb..bea85f97f363 100644
--- a/arch/arm/kernel/calls.S
+++ b/arch/arm/kernel/calls.S
@@ -392,6 +392,7 @@
/* 380 */ CALL(sys_sched_setattr)
CALL(sys_sched_getattr)
CALL(sys_renameat2)
+ CALL(sys_seccomp)
#ifndef syscalls_counted
.equ syscalls_padding, ((NR_syscalls + 3) & ~3) - NR_syscalls
#define syscalls_counted
--
1.7.9.5
^ permalink raw reply related
* [PATCH v11 04/11] seccomp: add "seccomp" syscall
From: Kees Cook @ 2014-07-16 21:50 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Andy Lutomirski, Oleg Nesterov, James Morris,
Michael Kerrisk (man-pages), Alexei Starovoitov, Andrew Morton,
Daniel Borkmann, Will Drewry, Julien Tinnes, David Drysdale,
linux-api, x86, linux-arm-kernel, linux-mips, linux-arch,
linux-security-module
In-Reply-To: <1405547442-26641-1-git-send-email-keescook@chromium.org>
This adds the new "seccomp" syscall with both an "operation" and "flags"
parameter for future expansion. The third argument is a pointer value,
used with the SECCOMP_SET_MODE_FILTER operation. Currently, flags must
be 0. This is functionally equivalent to prctl(PR_SET_SECCOMP, ...).
In addition to the TSYNC flag later in this patch series, there is a
non-zero chance that this syscall could be used for configuring a fixed
argument area for seccomp-tracer-aware processes to pass syscall arguments
in the future. Hence, the use of "seccomp" not simply "seccomp_add_filter"
for this syscall. Additionally, this syscall uses operation, flags,
and user pointer for arguments because strictly passing arguments via
a user pointer would mean seccomp itself would be unable to trivially
filter the seccomp syscall itself.
Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Andy Lutomirski <luto@amacapital.net>
---
arch/Kconfig | 1 +
arch/x86/syscalls/syscall_32.tbl | 1 +
arch/x86/syscalls/syscall_64.tbl | 1 +
include/linux/syscalls.h | 2 ++
include/uapi/asm-generic/unistd.h | 4 ++-
include/uapi/linux/seccomp.h | 4 +++
kernel/seccomp.c | 55 +++++++++++++++++++++++++++++++++----
kernel/sys_ni.c | 3 ++
8 files changed, 65 insertions(+), 6 deletions(-)
diff --git a/arch/Kconfig b/arch/Kconfig
index 97ff872c7acc..0eae9df35b88 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -321,6 +321,7 @@ config HAVE_ARCH_SECCOMP_FILTER
- secure_computing is called from a ptrace_event()-safe context
- secure_computing return value is checked and a return value of -1
results in the system call being skipped immediately.
+ - seccomp syscall wired up
config SECCOMP_FILTER
def_bool y
diff --git a/arch/x86/syscalls/syscall_32.tbl b/arch/x86/syscalls/syscall_32.tbl
index d6b867921612..7527eac24122 100644
--- a/arch/x86/syscalls/syscall_32.tbl
+++ b/arch/x86/syscalls/syscall_32.tbl
@@ -360,3 +360,4 @@
351 i386 sched_setattr sys_sched_setattr
352 i386 sched_getattr sys_sched_getattr
353 i386 renameat2 sys_renameat2
+354 i386 seccomp sys_seccomp
diff --git a/arch/x86/syscalls/syscall_64.tbl b/arch/x86/syscalls/syscall_64.tbl
index ec255a1646d2..16272a6c12b7 100644
--- a/arch/x86/syscalls/syscall_64.tbl
+++ b/arch/x86/syscalls/syscall_64.tbl
@@ -323,6 +323,7 @@
314 common sched_setattr sys_sched_setattr
315 common sched_getattr sys_sched_getattr
316 common renameat2 sys_renameat2
+317 common seccomp sys_seccomp
#
# x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index b0881a0ed322..1713977ee26f 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -866,4 +866,6 @@ asmlinkage long sys_process_vm_writev(pid_t pid,
asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
unsigned long idx1, unsigned long idx2);
asmlinkage long sys_finit_module(int fd, const char __user *uargs, int flags);
+asmlinkage long sys_seccomp(unsigned int op, unsigned int flags,
+ const char __user *uargs);
#endif
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 333640608087..65acbf0e2867 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -699,9 +699,11 @@ __SYSCALL(__NR_sched_setattr, sys_sched_setattr)
__SYSCALL(__NR_sched_getattr, sys_sched_getattr)
#define __NR_renameat2 276
__SYSCALL(__NR_renameat2, sys_renameat2)
+#define __NR_seccomp 277
+__SYSCALL(__NR_seccomp, sys_seccomp)
#undef __NR_syscalls
-#define __NR_syscalls 277
+#define __NR_syscalls 278
/*
* All syscalls below here should go away really,
diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
index ac2dc9f72973..b258878ba754 100644
--- a/include/uapi/linux/seccomp.h
+++ b/include/uapi/linux/seccomp.h
@@ -10,6 +10,10 @@
#define SECCOMP_MODE_STRICT 1 /* uses hard-coded filter. */
#define SECCOMP_MODE_FILTER 2 /* uses user-supplied filter. */
+/* Valid operations for seccomp syscall. */
+#define SECCOMP_SET_MODE_STRICT 0
+#define SECCOMP_SET_MODE_FILTER 1
+
/*
* All BPF programs must return a 32-bit value.
* The bottom 16-bits are for optional return data.
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 05cac2c2eca1..f0652578af75 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -18,6 +18,7 @@
#include <linux/compat.h>
#include <linux/sched.h>
#include <linux/seccomp.h>
+#include <linux/syscalls.h>
/* #define SECCOMP_DEBUG 1 */
@@ -314,7 +315,7 @@ free_prog:
*
* Returns 0 on success and non-zero otherwise.
*/
-static long seccomp_attach_user_filter(char __user *user_filter)
+static long seccomp_attach_user_filter(const char __user *user_filter)
{
struct sock_fprog fprog;
long ret = -EFAULT;
@@ -517,6 +518,7 @@ out:
#ifdef CONFIG_SECCOMP_FILTER
/**
* seccomp_set_mode_filter: internal function for setting seccomp filter
+ * @flags: flags to change filter behavior
* @filter: struct sock_fprog containing filter
*
* This function may be called repeatedly to install additional filters.
@@ -527,11 +529,16 @@ out:
*
* Returns 0 on success or -EINVAL on failure.
*/
-static long seccomp_set_mode_filter(char __user *filter)
+static long seccomp_set_mode_filter(unsigned int flags,
+ const char __user *filter)
{
const unsigned long seccomp_mode = SECCOMP_MODE_FILTER;
long ret = -EINVAL;
+ /* Validate flags. */
+ if (flags != 0)
+ goto out;
+
if (!seccomp_may_assign_mode(seccomp_mode))
goto out;
@@ -544,12 +551,35 @@ out:
return ret;
}
#else
-static inline long seccomp_set_mode_filter(char __user *filter)
+static inline long seccomp_set_mode_filter(unsigned int flags,
+ const char __user *filter)
{
return -EINVAL;
}
#endif
+/* Common entry point for both prctl and syscall. */
+static long do_seccomp(unsigned int op, unsigned int flags,
+ const char __user *uargs)
+{
+ switch (op) {
+ case SECCOMP_SET_MODE_STRICT:
+ if (flags != 0 || uargs != NULL)
+ return -EINVAL;
+ return seccomp_set_mode_strict();
+ case SECCOMP_SET_MODE_FILTER:
+ return seccomp_set_mode_filter(flags, uargs);
+ default:
+ return -EINVAL;
+ }
+}
+
+SYSCALL_DEFINE3(seccomp, unsigned int, op, unsigned int, flags,
+ const char __user *, uargs)
+{
+ return do_seccomp(op, flags, uargs);
+}
+
/**
* prctl_set_seccomp: configures current->seccomp.mode
* @seccomp_mode: requested mode to use
@@ -559,12 +589,27 @@ static inline long seccomp_set_mode_filter(char __user *filter)
*/
long prctl_set_seccomp(unsigned long seccomp_mode, char __user *filter)
{
+ unsigned int op;
+ char __user *uargs;
+
switch (seccomp_mode) {
case SECCOMP_MODE_STRICT:
- return seccomp_set_mode_strict();
+ op = SECCOMP_SET_MODE_STRICT;
+ /*
+ * Setting strict mode through prctl always ignored filter,
+ * so make sure it is always NULL here to pass the internal
+ * check in do_seccomp().
+ */
+ uargs = NULL;
+ break;
case SECCOMP_MODE_FILTER:
- return seccomp_set_mode_filter(filter);
+ op = SECCOMP_SET_MODE_FILTER;
+ uargs = filter;
+ break;
default:
return -EINVAL;
}
+
+ /* prctl interface doesn't have flags, so they are always zero. */
+ return do_seccomp(op, 0, uargs);
}
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index 36441b51b5df..2904a2105914 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -213,3 +213,6 @@ cond_syscall(compat_sys_open_by_handle_at);
/* compare kernel pointers */
cond_syscall(sys_kcmp);
+
+/* operate on Secure Computing state */
+cond_syscall(sys_seccomp);
--
1.7.9.5
^ permalink raw reply related
* [PATCH v11 03/11] seccomp: split mode setting routines
From: Kees Cook @ 2014-07-16 21:50 UTC (permalink / raw)
To: linux-kernel
Cc: linux-arch, linux-mips, Will Drewry, Kees Cook,
linux-security-module, linux-api, x86, James Morris,
Oleg Nesterov, Daniel Borkmann, Julien Tinnes,
Michael Kerrisk (man-pages), Andrew Morton, Andy Lutomirski,
David Drysdale, linux-arm-kernel, Alexei Starovoitov
In-Reply-To: <1405547442-26641-1-git-send-email-keescook@chromium.org>
Separates the two mode setting paths to make things more readable with
fewer #ifdefs within function bodies.
Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Andy Lutomirski <luto@amacapital.net>
---
kernel/seccomp.c | 71 ++++++++++++++++++++++++++++++++++++------------------
1 file changed, 48 insertions(+), 23 deletions(-)
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 9df7def86c3b..05cac2c2eca1 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -489,48 +489,66 @@ long prctl_get_seccomp(void)
}
/**
- * seccomp_set_mode: internal function for setting seccomp mode
- * @seccomp_mode: requested mode to use
- * @filter: optional struct sock_fprog for use with SECCOMP_MODE_FILTER
- *
- * This function may be called repeatedly with a @seccomp_mode of
- * SECCOMP_MODE_FILTER to install additional filters. Every filter
- * successfully installed will be evaluated (in reverse order) for each system
- * call the task makes.
+ * seccomp_set_mode_strict: internal function for setting strict seccomp
*
* Once current->seccomp.mode is non-zero, it may not be changed.
*
* Returns 0 on success or -EINVAL on failure.
*/
-static long seccomp_set_mode(unsigned long seccomp_mode, char __user *filter)
+static long seccomp_set_mode_strict(void)
{
+ const unsigned long seccomp_mode = SECCOMP_MODE_STRICT;
long ret = -EINVAL;
if (!seccomp_may_assign_mode(seccomp_mode))
goto out;
- switch (seccomp_mode) {
- case SECCOMP_MODE_STRICT:
- ret = 0;
#ifdef TIF_NOTSC
- disable_TSC();
+ disable_TSC();
#endif
- break;
+ seccomp_assign_mode(seccomp_mode);
+ ret = 0;
+
+out:
+
+ return ret;
+}
+
#ifdef CONFIG_SECCOMP_FILTER
- case SECCOMP_MODE_FILTER:
- ret = seccomp_attach_user_filter(filter);
- if (ret)
- goto out;
- break;
-#endif
- default:
+/**
+ * seccomp_set_mode_filter: internal function for setting seccomp filter
+ * @filter: struct sock_fprog containing filter
+ *
+ * This function may be called repeatedly to install additional filters.
+ * Every filter successfully installed will be evaluated (in reverse order)
+ * for each system call the task makes.
+ *
+ * Once current->seccomp.mode is non-zero, it may not be changed.
+ *
+ * Returns 0 on success or -EINVAL on failure.
+ */
+static long seccomp_set_mode_filter(char __user *filter)
+{
+ const unsigned long seccomp_mode = SECCOMP_MODE_FILTER;
+ long ret = -EINVAL;
+
+ if (!seccomp_may_assign_mode(seccomp_mode))
+ goto out;
+
+ ret = seccomp_attach_user_filter(filter);
+ if (ret)
goto out;
- }
seccomp_assign_mode(seccomp_mode);
out:
return ret;
}
+#else
+static inline long seccomp_set_mode_filter(char __user *filter)
+{
+ return -EINVAL;
+}
+#endif
/**
* prctl_set_seccomp: configures current->seccomp.mode
@@ -541,5 +559,12 @@ out:
*/
long prctl_set_seccomp(unsigned long seccomp_mode, char __user *filter)
{
- return seccomp_set_mode(seccomp_mode, filter);
+ switch (seccomp_mode) {
+ case SECCOMP_MODE_STRICT:
+ return seccomp_set_mode_strict();
+ case SECCOMP_MODE_FILTER:
+ return seccomp_set_mode_filter(filter);
+ default:
+ return -EINVAL;
+ }
}
--
1.7.9.5
^ permalink raw reply related
* [PATCH v11 02/11] seccomp: extract check/assign mode helpers
From: Kees Cook @ 2014-07-16 21:50 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Kees Cook, Andy Lutomirski, Oleg Nesterov, James Morris,
Michael Kerrisk (man-pages), Alexei Starovoitov, Andrew Morton,
Daniel Borkmann, Will Drewry, Julien Tinnes, David Drysdale,
linux-api-u79uwXL29TY76Z2rM5mHXA, x86-DgEjT+Ai2ygdnm+yROfE0A,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-mips-6z/3iImG2C8G8FEW9MqTrA,
linux-arch-u79uwXL29TY76Z2rM5mHXA,
linux-security-module-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1405547442-26641-1-git-send-email-keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
To support splitting mode 1 from mode 2, extract the mode checking and
assignment logic into common functions.
Signed-off-by: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Reviewed-by: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
---
kernel/seccomp.c | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index afb916c7e890..9df7def86c3b 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -194,7 +194,23 @@ static u32 seccomp_run_filters(int syscall)
}
return ret;
}
+#endif /* CONFIG_SECCOMP_FILTER */
+static inline bool seccomp_may_assign_mode(unsigned long seccomp_mode)
+{
+ if (current->seccomp.mode && current->seccomp.mode != seccomp_mode)
+ return false;
+
+ return true;
+}
+
+static inline void seccomp_assign_mode(unsigned long seccomp_mode)
+{
+ current->seccomp.mode = seccomp_mode;
+ set_tsk_thread_flag(current, TIF_SECCOMP);
+}
+
+#ifdef CONFIG_SECCOMP_FILTER
/**
* seccomp_attach_filter: Attaches a seccomp filter to current.
* @fprog: BPF program to install
@@ -490,8 +506,7 @@ static long seccomp_set_mode(unsigned long seccomp_mode, char __user *filter)
{
long ret = -EINVAL;
- if (current->seccomp.mode &&
- current->seccomp.mode != seccomp_mode)
+ if (!seccomp_may_assign_mode(seccomp_mode))
goto out;
switch (seccomp_mode) {
@@ -512,8 +527,7 @@ static long seccomp_set_mode(unsigned long seccomp_mode, char __user *filter)
goto out;
}
- current->seccomp.mode = seccomp_mode;
- set_thread_flag(TIF_SECCOMP);
+ seccomp_assign_mode(seccomp_mode);
out:
return ret;
}
--
1.7.9.5
^ permalink raw reply related
* [PATCH v11 01/11] seccomp: create internal mode-setting function
From: Kees Cook @ 2014-07-16 21:50 UTC (permalink / raw)
To: linux-kernel
Cc: linux-arch, linux-mips, Will Drewry, Kees Cook,
linux-security-module, linux-api, x86, James Morris,
Oleg Nesterov, Daniel Borkmann, Julien Tinnes,
Michael Kerrisk (man-pages), Andrew Morton, Andy Lutomirski,
David Drysdale, linux-arm-kernel, Alexei Starovoitov
In-Reply-To: <1405547442-26641-1-git-send-email-keescook@chromium.org>
In preparation for having other callers of the seccomp mode setting
logic, split the prctl entry point away from the core logic that performs
seccomp mode setting.
Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Andy Lutomirski <luto@amacapital.net>
---
kernel/seccomp.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 301bbc24739c..afb916c7e890 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -473,7 +473,7 @@ long prctl_get_seccomp(void)
}
/**
- * prctl_set_seccomp: configures current->seccomp.mode
+ * seccomp_set_mode: internal function for setting seccomp mode
* @seccomp_mode: requested mode to use
* @filter: optional struct sock_fprog for use with SECCOMP_MODE_FILTER
*
@@ -486,7 +486,7 @@ long prctl_get_seccomp(void)
*
* Returns 0 on success or -EINVAL on failure.
*/
-long prctl_set_seccomp(unsigned long seccomp_mode, char __user *filter)
+static long seccomp_set_mode(unsigned long seccomp_mode, char __user *filter)
{
long ret = -EINVAL;
@@ -517,3 +517,15 @@ long prctl_set_seccomp(unsigned long seccomp_mode, char __user *filter)
out:
return ret;
}
+
+/**
+ * prctl_set_seccomp: configures current->seccomp.mode
+ * @seccomp_mode: requested mode to use
+ * @filter: optional struct sock_fprog for use with SECCOMP_MODE_FILTER
+ *
+ * Returns 0 on success or -EINVAL on failure.
+ */
+long prctl_set_seccomp(unsigned long seccomp_mode, char __user *filter)
+{
+ return seccomp_set_mode(seccomp_mode, filter);
+}
--
1.7.9.5
^ permalink raw reply related
* [PATCH v11 0/11] seccomp: add thread sync ability
From: Kees Cook @ 2014-07-16 21:50 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Andy Lutomirski, Oleg Nesterov, James Morris,
Michael Kerrisk (man-pages), Alexei Starovoitov, Andrew Morton,
Daniel Borkmann, Will Drewry, Julien Tinnes, David Drysdale,
linux-api, x86, linux-arm-kernel, linux-mips, linux-arch,
linux-security-module
This adds the ability for threads to request seccomp filter
synchronization across their thread group (at filter attach time).
For example, for Chrome to make sure graphic driver threads are fully
confined after seccomp filters have been attached.
To support this, locking on seccomp changes via thread-group-shared
sighand lock is introduced, along with refactoring of no_new_privs. Races
with thread creation are handled via delayed duplication of the seccomp
task struct field and cred_guard_mutex.
This includes a new syscall (instead of adding a new prctl option),
as suggested by Andy Lutomirski and Michael Kerrisk.
Thanks!
-Kees
v11:
- updated writer locking commit log for clarity (luto)
- clarified writer lock thread flag setting comment (luto)
- inverted SECCOMP_FILTER_FLAG_MASK (luto)
- renamed is_acestor parameter (luto)
- added BUG_ON to catch currently impossible integer overflow (luto)
v10:
- dropped pending-kill checks (oleg)
- tweaked memory barriers (oleg)
v9:
- rearranged/split patches to make things more reviewable
- added use of cred_guard_mutex to solve exec race (oleg, luto)
- added barriers for TIF_SECCOMP vs seccomp.mode race (oleg, luto)
- fixed missed copying of nnp state after v8 refactor (oleg)
v8:
- drop use of tasklist_lock, appears redundant against sighand (oleg)
- reduced use of smp_load_acquire to logical minimum (oleg)
- change nnp to a task struct held atomic flags field (oleg, luto)
- drop needless irqflags changes in fork.c for holding sighand lock (oleg)
- cleaned up use of thread for-each loop (oleg)
- rearranged patch order to keep syscall changes adjacent
- added example code to manpage (mtk)
v7:
- rebase on Linus's tree (merged with network bpf changes)
- wrote manpage text documenting API (follows this series)
v6:
- switch from seccomp-specific lock to thread-group lock to gain atomicity
- implement seccomp syscall across all architectures with seccomp filter
- clean up sparse warnings around locking
v5:
- move includes around (drysdale)
- drop set_nnp return value (luto)
- use smp_load_acquire/store_release (luto)
- merge nnp changes to seccomp always, fewer ifdef (luto)
v4:
- cleaned up locking further, as noticed by David Drysdale
v3:
- added SECCOMP_EXT_ACT_FILTER for new filter install options
v2:
- reworked to avoid clone races
^ permalink raw reply
* Re: [PATCH v10 0/11] seccomp: add thread sync ability
From: Andy Lutomirski @ 2014-07-16 21:27 UTC (permalink / raw)
To: Kees Cook
Cc: Oleg Nesterov, LKML, Alexei Starovoitov,
Michael Kerrisk (man-pages), Andrew Morton, Daniel Borkmann,
Will Drewry, Julien Tinnes, David Drysdale, Linux API,
x86@kernel.org, linux-arm-kernel@lists.infradead.org,
Linux MIPS Mailing List, linux-arch, linux-security-module
In-Reply-To: <CAGXu5jKMwiAxeCVZVPS72XWCd2KhiR-uc4VFKT0mvD9s-cTO_Q@mail.gmail.com>
On Wed, Jul 16, 2014 at 2:23 PM, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Jul 16, 2014 at 12:45 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>> In seccomp_prepare_user_filter, would it make sense to return -EINVAL
>> if !user_filter? That will make it slightly more pleasant to
>> implement TSYNC-without-change if anyone ever wants it. (This isn't
>> really necessary -- it's just slightly more polite.)
>
> I can't do this since EFAULT is already used to detect seccomp
> capabilities from userspace.
Aha. In that case, can you (separately) send a prctl.2 manpage patch
documenting that? Also, I'm pretty sure you can get away with doing
this for seccomp(2) -- EINVAL and ENOSYS are easily distinguishable,
but the current behavior is IMO also fine if documented.
--Andy
^ permalink raw reply
* Re: [PATCH v10 0/11] seccomp: add thread sync ability
From: Kees Cook @ 2014-07-16 21:23 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Oleg Nesterov, LKML, Alexei Starovoitov,
Michael Kerrisk (man-pages), Andrew Morton, Daniel Borkmann,
Will Drewry, Julien Tinnes, David Drysdale, Linux API,
x86@kernel.org, linux-arm-kernel@lists.infradead.org, linux-mips,
linux-arch, linux-security-module
In-Reply-To: <CALCETrX48nuR6wqEV4Nu47BF4Z3XFvBpYer_fzwaqJdi0fpdKw@mail.gmail.com>
On Wed, Jul 16, 2014 at 12:45 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Wed, Jul 16, 2014 at 10:54 AM, Kees Cook <keescook@chromium.org> wrote:
>> On Mon, Jul 14, 2014 at 12:04 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>> On Fri, Jul 11, 2014 at 10:55 AM, Kees Cook <keescook@chromium.org> wrote:
>>>> On Fri, Jul 11, 2014 at 9:49 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>>>>> On 07/10, Kees Cook wrote:
>>>>>>
>>>>>> This adds the ability for threads to request seccomp filter
>>>>>> synchronization across their thread group (at filter attach time).
>>>>>> For example, for Chrome to make sure graphic driver threads are fully
>>>>>> confined after seccomp filters have been attached.
>>>>>>
>>>>>> To support this, locking on seccomp changes via thread-group-shared
>>>>>> sighand lock is introduced, along with refactoring of no_new_privs. Races
>>>>>> with thread creation are handled via delayed duplication of the seccomp
>>>>>> task struct field and cred_guard_mutex.
>>>>>>
>>>>>> This includes a new syscall (instead of adding a new prctl option),
>>>>>> as suggested by Andy Lutomirski and Michael Kerrisk.
>>>>>
>>>>> I do not not see any problems in this version,
>>>>
>>>> Awesome! Thank you for all the reviews. :) If Andy and Michael are
>>>> happy with this too, I think this is in good shape. \o/
>>>
>>> I think I'm happy with it. Is it in git somewhere for easy perusal?
>>> I have a cold, so my reviewing ability is a bit off, but I want to
>>> take a look at the final version, and git is a little easier than
>>> email for this.
>>
>> Hi Andy,
>>
>> Have you had a chance to look v10 over? I'd like to send a v11 with
>> Oleg's Reviewed-by added (at James Morris's request). Should I add one
>> from you as well?
>
> Trivial nits (take them or leave them):
>
> seccomp_check_mode confused me. Maybe seccomp_may_assign_mode would
> be a better name.
Good idea; I was unhappy with this name too. Done for v11.
> In the writer locking changelog, should "(which is unique to the
> thread group)" be "(which is shared by all threads in the thread
> group)"?
Done.
> This bit:
>
> /*
> * Explicitly enable no_new_privs here in case it got set
> * between the task_struct being duplicated and holding the
> * sighand lock. The seccomp state and nnp must be in sync.
> */
> if (task_no_new_privs(current))
> task_set_no_new_privs(p);
>
> should arguably move the very end of task duplication -- someone will
> want it for some other purpose some day.
That certainly seems plausible, but for the moment, I want to keep nnp
near seccomp, since that's where we have a race.
> This bit:
>
> /* If we have a seccomp mode, enable the thread flag. */
> if (p->seccomp.mode != SECCOMP_MODE_DISABLED)
> set_tsk_thread_flag(p, TIF_SECCOMP);
>
> is not quite obvious to me -- it's not obvious why it's needed. If
> it's to eliminate a race, can you explain the race in the comment? If
> it's just paranoia, a WARN_ON or BUG_ON might be worthwhile.
I've expanded the comment; it's the same issue as nnp: seccomp could
have changed, so if we gained a mode, we need to set the flag too.
> SECCOMP_FILTER_FLAG_MASK seems backwards to me. Maybe rename it to
> SECCOMP_FILTER_ALLOWED_FLAGS and invert it?
Excellent point. Other kernel users of the _MASK name aren't inverted. Fixed.
> is_ancestor: calling the first parameter "candidate" is just a tiny
> bit odd -- it's the child that's up for consideration. (This is
> barely even worth the time it took me to type it.)
Switch argument and comment to "parent".
> Less trivial nits:
>
> Can you change:
>
> fp = kzalloc(fp_size, GFP_KERNEL|__GFP_NOWARN);
>
> to
>
> fp = kcalloc(fprog->len, sizeof(struct sock_filter), GFP_KERNEL|__GFP_NOWARN);
>
> somewhere in this series (w/ a changelog comment that it's not
> exploitable to avoid scaring people).
Done, though I used a BUG_ON since the compiler will optimize it out
(since it's an impossible state to be in).
> In seccomp_prepare_user_filter, would it make sense to return -EINVAL
> if !user_filter? That will make it slightly more pleasant to
> implement TSYNC-without-change if anyone ever wants it. (This isn't
> really necessary -- it's just slightly more polite.)
I can't do this since EFAULT is already used to detect seccomp
capabilities from userspace.
> Once James picks this up, I'll rebase my series on top of it. I think
> they'll conflict slightly.
It's not bad. I tripped over sd -> sd_local and the related sd vs &sd
in the SK_RUN_FILTER call, but otherwise it was pretty trivial.
> Feel free to add my Reviewed-by to the whole series except the ARM and
> MIPS patches.
Thanks!
> And some thoughts:
>
> Your changelog comment for the seccomp syscall suggests that you're
> thinking about extending the tracer interface. I'd suggest a rather
> different design: add a concept of a seccomp monitor associated with a
> particular filter and an action SECCOMP_RET_MONITOR.
> SECCOMP_RET_MONITOR will tell the monitor what syscall was attempted
> and will wait for further instructions. The monitor can then ask for
> zero or more syscalls to be issued on behalf of the waiting process
> and then resume it. Each of those additional syscalls will be further
> filtered through all seccomp filters outside of the one that returned
> SECCOMP_RET_MONITOR.
>
> This would avoid all of the nastiness inherent in using ptrace and
> would nest much more nicely. And it could be far faster.
>
> If you decide to do something to ARM along the lines of what I'm doing
> to x86, you may want to fix this:
>
> /*
> * Make sure that any changes to mode from another thread have
> * been seen after TIF_SECCOMP was seen.
> */
> rmb();
>
> It should have essentially no effect on x86, though.
Noted, thanks!
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH v10 0/11] seccomp: add thread sync ability
From: Andy Lutomirski @ 2014-07-16 19:45 UTC (permalink / raw)
To: Kees Cook
Cc: Oleg Nesterov, LKML, Alexei Starovoitov,
Michael Kerrisk (man-pages), Andrew Morton, Daniel Borkmann,
Will Drewry, Julien Tinnes, David Drysdale, Linux API,
x86@kernel.org, linux-arm-kernel@lists.infradead.org, linux-mips,
linux-arch, linux-security-module
In-Reply-To: <CAGXu5j+r-CpJiXipCT=j09+ZfJjF9jTc3kuZFAH3ZxgUEvktTA@mail.gmail.com>
C
On Wed, Jul 16, 2014 at 10:54 AM, Kees Cook <keescook@chromium.org> wrote:
> On Mon, Jul 14, 2014 at 12:04 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>> On Fri, Jul 11, 2014 at 10:55 AM, Kees Cook <keescook@chromium.org> wrote:
>>> On Fri, Jul 11, 2014 at 9:49 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>>>> On 07/10, Kees Cook wrote:
>>>>>
>>>>> This adds the ability for threads to request seccomp filter
>>>>> synchronization across their thread group (at filter attach time).
>>>>> For example, for Chrome to make sure graphic driver threads are fully
>>>>> confined after seccomp filters have been attached.
>>>>>
>>>>> To support this, locking on seccomp changes via thread-group-shared
>>>>> sighand lock is introduced, along with refactoring of no_new_privs. Races
>>>>> with thread creation are handled via delayed duplication of the seccomp
>>>>> task struct field and cred_guard_mutex.
>>>>>
>>>>> This includes a new syscall (instead of adding a new prctl option),
>>>>> as suggested by Andy Lutomirski and Michael Kerrisk.
>>>>
>>>> I do not not see any problems in this version,
>>>
>>> Awesome! Thank you for all the reviews. :) If Andy and Michael are
>>> happy with this too, I think this is in good shape. \o/
>>
>> I think I'm happy with it. Is it in git somewhere for easy perusal?
>> I have a cold, so my reviewing ability is a bit off, but I want to
>> take a look at the final version, and git is a little easier than
>> email for this.
>
> Hi Andy,
>
> Have you had a chance to look v10 over? I'd like to send a v11 with
> Oleg's Reviewed-by added (at James Morris's request). Should I add one
> from you as well?
Trivial nits (take them or leave them):
seccomp_check_mode confused me. Maybe seccomp_may_assign_mode would
be a better name.
In the writer locking changelog, should "(which is unique to the
thread group)" be "(which is shared by all threads in the thread
group)"?
This bit:
/*
* Explicitly enable no_new_privs here in case it got set
* between the task_struct being duplicated and holding the
* sighand lock. The seccomp state and nnp must be in sync.
*/
if (task_no_new_privs(current))
task_set_no_new_privs(p);
should arguably move the very end of task duplication -- someone will
want it for some other purpose some day.
This bit:
/* If we have a seccomp mode, enable the thread flag. */
if (p->seccomp.mode != SECCOMP_MODE_DISABLED)
set_tsk_thread_flag(p, TIF_SECCOMP);
is not quite obvious to me -- it's not obvious why it's needed. If
it's to eliminate a race, can you explain the race in the comment? If
it's just paranoia, a WARN_ON or BUG_ON might be worthwhile.
SECCOMP_FILTER_FLAG_MASK seems backwards to me. Maybe rename it to
SECCOMP_FILTER_ALLOWED_FLAGS and invert it?
is_ancestor: calling the first parameter "candidate" is just a tiny
bit odd -- it's the child that's up for consideration. (This is
barely even worth the time it took me to type it.)
Less trivial nits:
Can you change:
fp = kzalloc(fp_size, GFP_KERNEL|__GFP_NOWARN);
to
fp = kcalloc(fprog->len, sizeof(struct sock_filter), GFP_KERNEL|__GFP_NOWARN);
somewhere in this series (w/ a changelog comment that it's not
exploitable to avoid scaring people).
In seccomp_prepare_user_filter, would it make sense to return -EINVAL
if !user_filter? That will make it slightly more pleasant to
implement TSYNC-without-change if anyone ever wants it. (This isn't
really necessary -- it's just slightly more polite.)
Once James picks this up, I'll rebase my series on top of it. I think
they'll conflict slightly.
Feel free to add my Reviewed-by to the whole series except the ARM and
MIPS patches.
And some thoughts:
Your changelog comment for the seccomp syscall suggests that you're
thinking about extending the tracer interface. I'd suggest a rather
different design: add a concept of a seccomp monitor associated with a
particular filter and an action SECCOMP_RET_MONITOR.
SECCOMP_RET_MONITOR will tell the monitor what syscall was attempted
and will wait for further instructions. The monitor can then ask for
zero or more syscalls to be issued on behalf of the waiting process
and then resume it. Each of those additional syscalls will be further
filtered through all seccomp filters outside of the one that returned
SECCOMP_RET_MONITOR.
This would avoid all of the nastiness inherent in using ptrace and
would nest much more nicely. And it could be far faster.
If you decide to do something to ARM along the lines of what I'm doing
to x86, you may want to fix this:
/*
* Make sure that any changes to mode from another thread have
* been seen after TIF_SECCOMP was seen.
*/
rmb();
It should have essentially no effect on x86, though.
--Andy
^ permalink raw reply
* Re: [PATCH v10 0/11] seccomp: add thread sync ability
From: Kees Cook @ 2014-07-16 17:54 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Oleg Nesterov, LKML, Alexei Starovoitov,
Michael Kerrisk (man-pages), Andrew Morton, Daniel Borkmann,
Will Drewry, Julien Tinnes, David Drysdale, Linux API,
x86@kernel.org, linux-arm-kernel@lists.infradead.org, linux-mips,
linux-arch, linux-security-module
In-Reply-To: <CALCETrVXgA9a2f7VwnCYW4_XB+JAPRSR8xsuH_ZYbA82=ZozRw@mail.gmail.com>
On Mon, Jul 14, 2014 at 12:04 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Fri, Jul 11, 2014 at 10:55 AM, Kees Cook <keescook@chromium.org> wrote:
>> On Fri, Jul 11, 2014 at 9:49 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>>> On 07/10, Kees Cook wrote:
>>>>
>>>> This adds the ability for threads to request seccomp filter
>>>> synchronization across their thread group (at filter attach time).
>>>> For example, for Chrome to make sure graphic driver threads are fully
>>>> confined after seccomp filters have been attached.
>>>>
>>>> To support this, locking on seccomp changes via thread-group-shared
>>>> sighand lock is introduced, along with refactoring of no_new_privs. Races
>>>> with thread creation are handled via delayed duplication of the seccomp
>>>> task struct field and cred_guard_mutex.
>>>>
>>>> This includes a new syscall (instead of adding a new prctl option),
>>>> as suggested by Andy Lutomirski and Michael Kerrisk.
>>>
>>> I do not not see any problems in this version,
>>
>> Awesome! Thank you for all the reviews. :) If Andy and Michael are
>> happy with this too, I think this is in good shape. \o/
>
> I think I'm happy with it. Is it in git somewhere for easy perusal?
> I have a cold, so my reviewing ability is a bit off, but I want to
> take a look at the final version, and git is a little easier than
> email for this.
Hi Andy,
Have you had a chance to look v10 over? I'd like to send a v11 with
Oleg's Reviewed-by added (at James Morris's request). Should I add one
from you as well?
Thanks!
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH v10 0/11] seccomp: add thread sync ability
From: James Morris @ 2014-07-16 16:44 UTC (permalink / raw)
To: Kees Cook
Cc: Oleg Nesterov, LKML, Andy Lutomirski, Alexei Starovoitov,
Michael Kerrisk (man-pages), Andrew Morton, Daniel Borkmann,
Will Drewry, Julien Tinnes, David Drysdale, Linux API,
x86@kernel.org, linux-arm-kernel@lists.infradead.org, linux-mips,
linux-arch, linux-security-module
In-Reply-To: <CAGXu5jK-x0=Rr7kX2a=b4Z8ueA77uwmhNZZAayG8cwmNOKa8Ug@mail.gmail.com>
On Fri, 11 Jul 2014, Kees Cook wrote:
> On Fri, Jul 11, 2014 at 9:49 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 07/10, Kees Cook wrote:
> >>
> >> This adds the ability for threads to request seccomp filter
> >> synchronization across their thread group (at filter attach time).
> >> For example, for Chrome to make sure graphic driver threads are fully
> >> confined after seccomp filters have been attached.
> >>
> >> To support this, locking on seccomp changes via thread-group-shared
> >> sighand lock is introduced, along with refactoring of no_new_privs. Races
> >> with thread creation are handled via delayed duplication of the seccomp
> >> task struct field and cred_guard_mutex.
> >>
> >> This includes a new syscall (instead of adding a new prctl option),
> >> as suggested by Andy Lutomirski and Michael Kerrisk.
> >
> > I do not not see any problems in this version,
>
> Awesome! Thank you for all the reviews. :) If Andy and Michael are
> happy with this too, I think this is in good shape. \o/
>
> -Kees
If you re-send this, please add Oleg's Reviewed-by to each patch.
>
> >
> > Reviewed-by: Oleg Nesterov <oleg@redhat.com>
> >
>
>
>
>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [RFC v3 6/7] shm: wait for pins to be released when sealing
From: Hugh Dickins @ 2014-07-16 10:09 UTC (permalink / raw)
To: David Herrmann
Cc: linux-kernel, Michael Kerrisk, Ryan Lortie, Linus Torvalds,
Andrew Morton, linux-mm, linux-fsdevel, linux-api,
Greg Kroah-Hartman, john.stultz, Lennart Poettering, Daniel Mack,
Kay Sievers, Hugh Dickins, Tony Battersby, Andy Lutomirski
In-Reply-To: <1402655819-14325-7-git-send-email-dh.herrmann@gmail.com>
On Fri, 13 Jun 2014, David Herrmann wrote:
> We currently fail setting SEAL_WRITE in case there're pending page
> references. This patch extends the pin-tests to wait up to 150ms for all
> references to be dropped. This is still not perfect in that it doesn't
> account for harmless read-only pins, but it's much better than a hard
> failure.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Right, I didn't look through the patch itself, just compared the result
with what I sent. Okay, you prefer to separate out shmem_tag_pins().
Yes, it looks fine. There's just one change I'd like at this stage,
something I realized shortly after sending the code fragment: please
add a call to lru_add_drain() at the head of shmem_tag_pins(). The
reason being that lru_add_drain() is local to the cpu, so cheap, and
in many cases will bring down all the raised refcounts right then.
Whereas lru_add_drain_all() in the first scan of shmem_wait_for_pins()
is much more expensive, involving inter-processor interrupts to do
that on all cpus: it is appropriate to call it at that point, but we
really ought to try the cheaper lru_add_drain() at the earlier stage.
I would also like never to embark on this scan of the radix_tree
and wait for pins, if the pages were never given out in a VM_SHARED
mapping - or is that unrealistic, because every memfd is read-write,
and typical initialization expected to be by mmap() rather than write()?
But anyway, you're quite right not to get into that at this stage:
it's best left as an optimization once the basics are safely in.
Hugh
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v3 5/7] selftests: add memfd/sealing page-pinning tests
From: Hugh Dickins @ 2014-07-16 10:08 UTC (permalink / raw)
To: David Herrmann
Cc: linux-kernel, Michael Kerrisk, Ryan Lortie, Linus Torvalds,
Andrew Morton, linux-mm, linux-fsdevel, linux-api,
Greg Kroah-Hartman, john.stultz, Lennart Poettering, Daniel Mack,
Kay Sievers, Hugh Dickins, Tony Battersby, Andy Lutomirski
In-Reply-To: <1402655819-14325-6-git-send-email-dh.herrmann@gmail.com>
On Fri, 13 Jun 2014, David Herrmann wrote:
> Setting SEAL_WRITE is not possible if there're pending GUP users. This
> commit adds selftests for memfd+sealing that use FUSE to create pending
> page-references. FUSE is very helpful here in that it allows us to delay
> direct-IO operations for an arbitrary amount of time. This way, we can
> force the kernel to pin pages and then run our normal selftests.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
I had a number of problems in getting this working (on openSUSE 13.1):
rpm told me I had fuse installed, yet I had to download and install
the tarball to get header files needed; then "make fuse_mnt" told me
to add -D_FILE_OFFSET_BITS=64 to the compile flags; after which I
got "undefined reference to `fuse_main_real'"; but then I tried
"make run_fuse" as root, and it seemed to sort these issues out
for itself, aside from "./run_fuse_test.sh: Permission denied" -
which was within my bounds of comprehension unlike the rest!
No complaint, thanks for providing the test (though I didn't check
the source to convince myself that "DONE" has done what's claimed):
some rainy day someone can get the Makefile working more smoothly,
no need to delay the patchset for this.
Hugh
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v3 4/7] selftests: add memfd_create() + sealing tests
From: Hugh Dickins @ 2014-07-16 10:07 UTC (permalink / raw)
To: David Herrmann
Cc: linux-kernel, Michael Kerrisk, Ryan Lortie, Linus Torvalds,
Andrew Morton, linux-mm, linux-fsdevel, linux-api,
Greg Kroah-Hartman, john.stultz, Lennart Poettering, Daniel Mack,
Kay Sievers, Hugh Dickins, Tony Battersby, Andy Lutomirski
In-Reply-To: <1402655819-14325-5-git-send-email-dh.herrmann@gmail.com>
On Fri, 13 Jun 2014, David Herrmann wrote:
> Some basic tests to verify sealing on memfds works as expected and
> guarantees the advertised semantics.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Thanks for whatever the fix was, I didn't hit any problem running
this version repeatedly, on 64-bit and on 32-bit.
Hugh
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v3 3/7] shm: add memfd_create() syscall
From: Hugh Dickins @ 2014-07-16 10:07 UTC (permalink / raw)
To: David Herrmann
Cc: linux-kernel, Michael Kerrisk, Ryan Lortie, Linus Torvalds,
Andrew Morton, linux-mm, linux-fsdevel, linux-api,
Greg Kroah-Hartman, john.stultz, Lennart Poettering, Daniel Mack,
Kay Sievers, Hugh Dickins, Tony Battersby, Andy Lutomirski
In-Reply-To: <1402655819-14325-4-git-send-email-dh.herrmann@gmail.com>
On Fri, 13 Jun 2014, David Herrmann wrote:
> memfd_create() is similar to mmap(MAP_ANON), but returns a file-descriptor
> that you can pass to mmap(). It can support sealing and avoids any
> connection to user-visible mount-points. Thus, it's not subject to quotas
> on mounted file-systems, but can be used like malloc()'ed memory, but
> with a file-descriptor to it.
>
> memfd_create() returns the raw shmem file, so calls like ftruncate() can
> be used to modify the underlying inode. Also calls like fstat()
> will return proper information and mark the file as regular file. If you
> want sealing, you can specify MFD_ALLOW_SEALING. Otherwise, sealing is not
> supported (like on all other regular files).
>
> Compared to O_TMPFILE, it does not require a tmpfs mount-point and is not
> subject to quotas and alike. It is still properly accounted to memcg
> limits, though.
It's an important point, but unclear quite what "quotas and alike" means.
There's never been any quota support in shmem/tmpfs, but filesystem size
can be limited. Maybe say "and is not subject to a filesystem size limit.
It is still properly accounted to memcg limits, though, and to the same
overcommit or no-overcommit accounting as all user memory."
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
A comment or two below, but this is okay by me. I'm not wildly excited
to be getting a new system call in mm/shmem.c. I do like it much better
now that you've dropped the size arg, thank you, but I still find it an
odd system call: if it were not for the name, that you want so much for
debugging, I think we would just implement this with a /dev/sealable
alongside /dev/zero, which gave you your own object on opening (in the
way that /dev/zero gives you your own object on mmap'ing).
I haven't checked the manpage, I hope it's made very clear that
there's no uniqueness imposed on the name, that it's merely a tag
attached to the object.
But from a shmem point of view this seems fine: if everyone else
is happy with memfd_create(), it's fine by me.
> ---
> arch/x86/syscalls/syscall_32.tbl | 1 +
> arch/x86/syscalls/syscall_64.tbl | 1 +
> include/linux/syscalls.h | 1 +
> include/uapi/linux/memfd.h | 8 +++++
> kernel/sys_ni.c | 1 +
> mm/shmem.c | 72 ++++++++++++++++++++++++++++++++++++++++
> 6 files changed, 84 insertions(+)
> create mode 100644 include/uapi/linux/memfd.h
>
> diff --git a/arch/x86/syscalls/syscall_32.tbl b/arch/x86/syscalls/syscall_32.tbl
> index d6b8679..e7495b4 100644
> --- a/arch/x86/syscalls/syscall_32.tbl
> +++ b/arch/x86/syscalls/syscall_32.tbl
> @@ -360,3 +360,4 @@
> 351 i386 sched_setattr sys_sched_setattr
> 352 i386 sched_getattr sys_sched_getattr
> 353 i386 renameat2 sys_renameat2
> +354 i386 memfd_create sys_memfd_create
> diff --git a/arch/x86/syscalls/syscall_64.tbl b/arch/x86/syscalls/syscall_64.tbl
> index ec255a1..28be0e1 100644
> --- a/arch/x86/syscalls/syscall_64.tbl
> +++ b/arch/x86/syscalls/syscall_64.tbl
> @@ -323,6 +323,7 @@
> 314 common sched_setattr sys_sched_setattr
> 315 common sched_getattr sys_sched_getattr
> 316 common renameat2 sys_renameat2
> +317 common memfd_create sys_memfd_create
>
> #
> # x32-specific system call numbers start at 512 to avoid cache impact
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index b0881a0..0be5d4d 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -802,6 +802,7 @@ asmlinkage long sys_timerfd_settime(int ufd, int flags,
> asmlinkage long sys_timerfd_gettime(int ufd, struct itimerspec __user *otmr);
> asmlinkage long sys_eventfd(unsigned int count);
> asmlinkage long sys_eventfd2(unsigned int count, int flags);
> +asmlinkage long sys_memfd_create(const char *uname_ptr, unsigned int flags);
> asmlinkage long sys_fallocate(int fd, int mode, loff_t offset, loff_t len);
> asmlinkage long sys_old_readdir(unsigned int, struct old_linux_dirent __user *, unsigned int);
> asmlinkage long sys_pselect6(int, fd_set __user *, fd_set __user *,
> diff --git a/include/uapi/linux/memfd.h b/include/uapi/linux/memfd.h
> new file mode 100644
> index 0000000..534e364
> --- /dev/null
> +++ b/include/uapi/linux/memfd.h
> @@ -0,0 +1,8 @@
> +#ifndef _UAPI_LINUX_MEMFD_H
> +#define _UAPI_LINUX_MEMFD_H
> +
> +/* flags for memfd_create(2) (unsigned int) */
> +#define MFD_CLOEXEC 0x0001U
> +#define MFD_ALLOW_SEALING 0x0002U
> +
> +#endif /* _UAPI_LINUX_MEMFD_H */
> diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
> index 36441b5..489a4e6 100644
> --- a/kernel/sys_ni.c
> +++ b/kernel/sys_ni.c
> @@ -197,6 +197,7 @@ cond_syscall(compat_sys_timerfd_settime);
> cond_syscall(compat_sys_timerfd_gettime);
> cond_syscall(sys_eventfd);
> cond_syscall(sys_eventfd2);
> +cond_syscall(sys_memfd_create);
>
> /* performance counters: */
> cond_syscall(sys_perf_event_open);
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 1438b3e..e7c5fe1 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -66,7 +66,9 @@ static struct vfsmount *shm_mnt;
> #include <linux/highmem.h>
> #include <linux/seq_file.h>
> #include <linux/magic.h>
> +#include <linux/syscalls.h>
> #include <linux/fcntl.h>
> +#include <uapi/linux/memfd.h>
>
> #include <asm/uaccess.h>
> #include <asm/pgtable.h>
> @@ -2662,6 +2664,76 @@ static int shmem_show_options(struct seq_file *seq, struct dentry *root)
> shmem_show_mpol(seq, sbinfo->mpol);
> return 0;
> }
> +
> +#define MFD_NAME_PREFIX "memfd:"
> +#define MFD_NAME_PREFIX_LEN (sizeof(MFD_NAME_PREFIX) - 1)
> +#define MFD_NAME_MAX_LEN (NAME_MAX - MFD_NAME_PREFIX_LEN)
> +
> +#define MFD_ALL_FLAGS (MFD_CLOEXEC | MFD_ALLOW_SEALING)
> +
> +SYSCALL_DEFINE2(memfd_create,
> + const char*, uname,
Jann Horn suggested "const char __user *" rather than "const char *",
here and in syscalls.h, I think that's right (for sparse: compare
with sys_open, for example).
> + unsigned int, flags)
> +{
> + struct shmem_inode_info *info;
> + struct file *file;
> + int fd, error;
> + char *name;
> + long len;
> +
> + if (flags & ~(unsigned int)MFD_ALL_FLAGS)
> + return -EINVAL;
> +
> + /* length includes terminating zero */
> + len = strnlen_user(uname, MFD_NAME_MAX_LEN + 1);
> + if (len <= 0)
> + return -EFAULT;
> + if (len > MFD_NAME_MAX_LEN + 1)
> + return -EINVAL;
> +
> + name = kmalloc(len + MFD_NAME_PREFIX_LEN, GFP_TEMPORARY);
> + if (!name)
> + return -ENOMEM;
> +
> + strcpy(name, MFD_NAME_PREFIX);
> + if (copy_from_user(&name[MFD_NAME_PREFIX_LEN], uname, len)) {
> + error = -EFAULT;
> + goto err_name;
> + }
> +
> + /* terminating-zero may have changed after strnlen_user() returned */
> + if (name[len + MFD_NAME_PREFIX_LEN - 1]) {
> + error = -EFAULT;
> + goto err_name;
> + }
> +
> + fd = get_unused_fd_flags((flags & MFD_CLOEXEC) ? O_CLOEXEC : 0);
Perhaps we should throw O_LARGEFILE in there too? So 32-bit is not
surprised when it accesses beyond MAX_NON_LFS. I guess it's almost
a non-issue, since the file is in memory, so not expected to be very
large; but I seem to recall being caught out by a missing O_LARGEFILE
in the past, and a new interface like this might do better to force it.
But I'm not very sure of my ground here: please ask around, an fsdevel
person will have a better idea than me, whether it's best included.
> + if (fd < 0) {
> + error = fd;
> + goto err_name;
> + }
> +
> + file = shmem_file_setup(name, 0, VM_NORESERVE);
> + if (IS_ERR(file)) {
> + error = PTR_ERR(file);
> + goto err_fd;
> + }
> + info = SHMEM_I(file_inode(file));
> + file->f_mode |= FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE;
> + if (flags & MFD_ALLOW_SEALING)
> + info->seals &= ~F_SEAL_SEAL;
> +
> + fd_install(fd, file);
> + kfree(name);
> + return fd;
> +
> +err_fd:
> + put_unused_fd(fd);
> +err_name:
> + kfree(name);
> + return error;
> +}
> +
> #endif /* CONFIG_TMPFS */
>
> static void shmem_put_super(struct super_block *sb)
> --
> 2.0.0
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v3 2/7] shm: add sealing API
From: Hugh Dickins @ 2014-07-16 10:06 UTC (permalink / raw)
To: David Herrmann
Cc: linux-kernel, Michael Kerrisk, Ryan Lortie, Linus Torvalds,
Andrew Morton, linux-mm, linux-fsdevel, linux-api,
Greg Kroah-Hartman, john.stultz, Lennart Poettering, Daniel Mack,
Kay Sievers, Hugh Dickins, Tony Battersby, Andy Lutomirski
In-Reply-To: <1402655819-14325-3-git-send-email-dh.herrmann@gmail.com>
On Fri, 13 Jun 2014, David Herrmann wrote:
> If two processes share a common memory region, they usually want some
> guarantees to allow safe access. This often includes:
> - one side cannot overwrite data while the other reads it
> - one side cannot shrink the buffer while the other accesses it
> - one side cannot grow the buffer beyond previously set boundaries
>
> If there is a trust-relationship between both parties, there is no need
> for policy enforcement. However, if there's no trust relationship (eg.,
> for general-purpose IPC) sharing memory-regions is highly fragile and
> often not possible without local copies. Look at the following two
> use-cases:
> 1) A graphics client wants to share its rendering-buffer with a
> graphics-server. The memory-region is allocated by the client for
> read/write access and a second FD is passed to the server. While
> scanning out from the memory region, the server has no guarantee that
> the client doesn't shrink the buffer at any time, requiring rather
> cumbersome SIGBUS handling.
> 2) A process wants to perform an RPC on another process. To avoid huge
> bandwidth consumption, zero-copy is preferred. After a message is
> assembled in-memory and a FD is passed to the remote side, both sides
> want to be sure that neither modifies this shared copy, anymore. The
> source may have put sensible data into the message without a separate
> copy and the target may want to parse the message inline, to avoid a
> local copy.
>
> While SIGBUS handling, POSIX mandatory locking and MAP_DENYWRITE provide
> ways to achieve most of this, the first one is unproportionally ugly to
> use in libraries and the latter two are broken/racy or even disabled due
> to denial of service attacks.
>
> This patch introduces the concept of SEALING. If you seal a file, a
> specific set of operations is blocked on that file forever.
> Unlike locks, seals can only be set, never removed. Hence, once you
> verified a specific set of seals is set, you're guaranteed that no-one can
> perform the blocked operations on this file, anymore.
>
> An initial set of SEALS is introduced by this patch:
> - SHRINK: If SEAL_SHRINK is set, the file in question cannot be reduced
> in size. This affects ftruncate() and open(O_TRUNC).
> - GROW: If SEAL_GROW is set, the file in question cannot be increased
> in size. This affects ftruncate(), fallocate() and write().
> - WRITE: If SEAL_WRITE is set, no write operations (besides resizing)
> are possible. This affects fallocate(PUNCH_HOLE), mmap() and
> write().
> - SEAL: If SEAL_SEAL is set, no further seals can be added to a file.
> This basically prevents the F_ADD_SEAL operation on a file and
> can be set to prevent others from adding further seals that you
> don't want.
>
> The described use-cases can easily use these seals to provide safe use
> without any trust-relationship:
> 1) The graphics server can verify that a passed file-descriptor has
> SEAL_SHRINK set. This allows safe scanout, while the client is
> allowed to increase buffer size for window-resizing on-the-fly.
> Concurrent writes are explicitly allowed.
> 2) For general-purpose IPC, both processes can verify that SEAL_SHRINK,
> SEAL_GROW and SEAL_WRITE are set. This guarantees that neither
> process can modify the data while the other side parses it.
> Furthermore, it guarantees that even with writable FDs passed to the
> peer, it cannot increase the size to hit memory-limits of the source
> process (in case the file-storage is accounted to the source).
>
> The new API is an extension to fcntl(), adding two new commands:
> F_GET_SEALS: Return a bitset describing the seals on the file. This
> can be called on any FD if the underlying file supports
> sealing.
> F_ADD_SEALS: Change the seals of a given file. This requires WRITE
> access to the file and F_SEAL_SEAL may not already be set.
> Furthermore, the underlying file must support sealing and
> there may not be any existing shared mapping of that file.
> Otherwise, EBADF/EPERM is returned.
> The given seals are _added_ to the existing set of seals
> on the file. You cannot remove seals again.
>
> The fcntl() handler is currently specific to shmem and disabled on all
> files. A file needs to explicitly support sealing for this interface to
> work. A separate syscall is added in a follow-up, which creates files that
> support sealing. There is no intention to support this on other
> file-systems. Semantics are unclear for non-volatile files and we lack any
> use-case right now. Therefore, the implementation is specific to shmem.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Looks pretty good to me, minor comments below.
> ---
> fs/fcntl.c | 5 ++
> include/linux/shmem_fs.h | 17 ++++
> include/uapi/linux/fcntl.h | 15 ++++
> mm/shmem.c | 189 ++++++++++++++++++++++++++++++++++++++++++++-
> 4 files changed, 223 insertions(+), 3 deletions(-)
>
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index 72c82f6..22d1c3d 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -21,6 +21,7 @@
> #include <linux/rcupdate.h>
> #include <linux/pid_namespace.h>
> #include <linux/user_namespace.h>
> +#include <linux/shmem_fs.h>
>
> #include <asm/poll.h>
> #include <asm/siginfo.h>
> @@ -336,6 +337,10 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
> case F_GETPIPE_SZ:
> err = pipe_fcntl(filp, cmd, arg);
> break;
> + case F_ADD_SEALS:
> + case F_GET_SEALS:
> + err = shmem_fcntl(filp, cmd, arg);
> + break;
> default:
> break;
> }
> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
> index 4d1771c..50777b5 100644
> --- a/include/linux/shmem_fs.h
> +++ b/include/linux/shmem_fs.h
> @@ -1,6 +1,7 @@
> #ifndef __SHMEM_FS_H
> #define __SHMEM_FS_H
>
> +#include <linux/file.h>
> #include <linux/swap.h>
> #include <linux/mempolicy.h>
> #include <linux/pagemap.h>
> @@ -11,6 +12,7 @@
>
> struct shmem_inode_info {
> spinlock_t lock;
> + unsigned int seals; /* shmem seals */
> unsigned long flags;
> unsigned long alloced; /* data pages alloced to file */
> union {
> @@ -65,4 +67,19 @@ static inline struct page *shmem_read_mapping_page(
> mapping_gfp_mask(mapping));
> }
>
> +#ifdef CONFIG_TMPFS
> +
> +extern int shmem_add_seals(struct file *file, unsigned int seals);
> +extern int shmem_get_seals(struct file *file);
> +extern long shmem_fcntl(struct file *file, unsigned int cmd, unsigned long arg);
> +
> +#else
> +
> +static inline long shmem_fcntl(struct file *f, unsigned int c, unsigned long a)
> +{
> + return -EINVAL;
> +}
> +
> +#endif
> +
> #endif
> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> index 074b886..beed138 100644
> --- a/include/uapi/linux/fcntl.h
> +++ b/include/uapi/linux/fcntl.h
> @@ -28,6 +28,21 @@
> #define F_GETPIPE_SZ (F_LINUX_SPECIFIC_BASE + 8)
>
> /*
> + * Set/Get seals
> + */
> +#define F_ADD_SEALS (F_LINUX_SPECIFIC_BASE + 9)
> +#define F_GET_SEALS (F_LINUX_SPECIFIC_BASE + 10)
> +
> +/*
> + * Types of seals
> + */
> +#define F_SEAL_SEAL 0x0001 /* prevent further seals from being set */
> +#define F_SEAL_SHRINK 0x0002 /* prevent file from shrinking */
> +#define F_SEAL_GROW 0x0004 /* prevent file from growing */
> +#define F_SEAL_WRITE 0x0008 /* prevent writes */
> +/* (1U << 31) is reserved for signed error codes */
> +
> +/*
> * Types of directory notifications that may be requested.
> */
> #define DN_ACCESS 0x00000001 /* File accessed */
> diff --git a/mm/shmem.c b/mm/shmem.c
> index f484c27..1438b3e 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -66,6 +66,7 @@ static struct vfsmount *shm_mnt;
> #include <linux/highmem.h>
> #include <linux/seq_file.h>
> #include <linux/magic.h>
> +#include <linux/fcntl.h>
>
> #include <asm/uaccess.h>
> #include <asm/pgtable.h>
> @@ -531,16 +532,23 @@ EXPORT_SYMBOL_GPL(shmem_truncate_range);
> static int shmem_setattr(struct dentry *dentry, struct iattr *attr)
> {
> struct inode *inode = dentry->d_inode;
> + struct shmem_inode_info *info = SHMEM_I(inode);
> + loff_t oldsize = inode->i_size;
> + loff_t newsize = attr->ia_size;
> int error;
>
> error = inode_change_ok(inode, attr);
> if (error)
> return error;
>
> - if (S_ISREG(inode->i_mode) && (attr->ia_valid & ATTR_SIZE)) {
> - loff_t oldsize = inode->i_size;
> - loff_t newsize = attr->ia_size;
> + /* protected by i_mutex */
> + if (attr->ia_valid & ATTR_SIZE) {
> + if ((newsize < oldsize && (info->seals & F_SEAL_SHRINK)) ||
> + (newsize > oldsize && (info->seals & F_SEAL_GROW)))
> + return -EPERM;
> + }
Not important but...
I'd have thought all that was better inside the S_ISREG(inode->i_mode) &&
(attr->ia_valid & ATTR_SIZE) block. Less unnecessary change above, and
more efficient for non-size attrs. You cannot seal anything but a regular
file anyway, right?
>
> + if (S_ISREG(inode->i_mode) && (attr->ia_valid & ATTR_SIZE)) {
> if (newsize != oldsize) {
> i_size_write(inode, newsize);
> inode->i_ctime = inode->i_mtime = CURRENT_TIME;
> @@ -1315,6 +1323,7 @@ static struct inode *shmem_get_inode(struct super_block *sb, const struct inode
> info = SHMEM_I(inode);
> memset(info, 0, (char *)inode - (char *)info);
> spin_lock_init(&info->lock);
> + info->seals = F_SEAL_SEAL;
> info->flags = flags & VM_NORESERVE;
> INIT_LIST_HEAD(&info->swaplist);
> simple_xattrs_init(&info->xattrs);
> @@ -1374,7 +1383,15 @@ shmem_write_begin(struct file *file, struct address_space *mapping,
> {
> int ret;
> struct inode *inode = mapping->host;
> + struct shmem_inode_info *info = SHMEM_I(inode);
> pgoff_t index = pos >> PAGE_CACHE_SHIFT;
> +
> + /* i_mutex is held by caller */
> + if (info->seals & F_SEAL_WRITE)
> + return -EPERM;
> + if ((info->seals & F_SEAL_GROW) && pos + len > inode->i_size)
> + return -EPERM;
I think this is your only addition which comes in a hot path.
Mel has been shaving nanoseconds off this path recently: you're not
introducing any atomic ops here, good, but I wonder if it would make any
measurable difference to include this pair of tests inside a single
"if (unlikely(info->seals)) {". Maybe not, but it wouldn't hurt.
> +
> ret = shmem_getpage(inode, index, pagep, SGP_WRITE, NULL);
> if (ret == 0 && *pagep)
> init_page_accessed(*pagep);
> @@ -1715,11 +1732,166 @@ static loff_t shmem_file_llseek(struct file *file, loff_t offset, int whence)
> return offset;
> }
>
> +/*
> + * Setting SEAL_WRITE requires us to verify there's no pending writer. However,
> + * via get_user_pages(), drivers might have some pending I/O without any active
> + * user-space mappings (eg., direct-IO, AIO). Therefore, we look at all pages
> + * and see whether it has an elevated ref-count. If so, we abort.
> + * The caller must guarantee that no new user will acquire writable references
> + * to those pages to avoid races.
> + */
> +static int shmem_test_for_pins(struct address_space *mapping)
> +{
> + struct radix_tree_iter iter;
> + void **slot;
> + pgoff_t start;
> + struct page *page;
> + int error;
> +
> + /* flush additional refs in lru_add early */
> + lru_add_drain_all();
> +
> + error = 0;
> + start = 0;
> + rcu_read_lock();
> +
> +restart:
> + radix_tree_for_each_slot(slot, &mapping->page_tree, &iter, start) {
> + page = radix_tree_deref_slot(slot);
> + if (!page || radix_tree_exception(page)) {
> + if (radix_tree_deref_retry(page))
> + goto restart;
> + } else if (page_count(page) - page_mapcount(page) > 1) {
> + error = -EBUSY;
> + break;
> + }
> +
> + if (need_resched()) {
> + cond_resched_rcu();
> + start = iter.index + 1;
> + goto restart;
> + }
> + }
> + rcu_read_unlock();
> +
> + return error;
> +}
Please leave shmem_test_for_pins() (and the comment above it)
out of this particular patch.
The implementation here satisfies none of us, make it harder to
figure out the patch improving it later, and distracts from the basic
sealing interface and functionality that you introduce in this patch.
A brief comment on the issue instead - "But what if a page of the
object is pinned for pending I/O? See later patch" - maybe, but on the
whole I think it's better to raise and settle the issue in later patch.
> +
> +#define F_ALL_SEALS (F_SEAL_SEAL | \
> + F_SEAL_SHRINK | \
> + F_SEAL_GROW | \
> + F_SEAL_WRITE)
> +
> +int shmem_add_seals(struct file *file, unsigned int seals)
> +{
> + struct dentry *dentry = file->f_path.dentry;
> + struct inode *inode = dentry->d_inode;
struct inode *inode = file_inode(file), and forget about dentry?
> + struct shmem_inode_info *info = SHMEM_I(inode);
> + int error;
> +
> + /*
> + * SEALING
> + * Sealing allows multiple parties to share a shmem-file but restrict
> + * access to a specific subset of file operations. Seals can only be
> + * added, but never removed. This way, mutually untrusted parties can
> + * share common memory regions with a well-defined policy. A malicious
> + * peer can thus never perform unwanted operations on a shared object.
> + *
> + * Seals are only supported on special shmem-files and always affect
> + * the whole underlying inode. Once a seal is set, it may prevent some
> + * kinds of access to the file. Currently, the following seals are
> + * defined:
> + * SEAL_SEAL: Prevent further seals from being set on this file
> + * SEAL_SHRINK: Prevent the file from shrinking
> + * SEAL_GROW: Prevent the file from growing
> + * SEAL_WRITE: Prevent write access to the file
> + *
> + * As we don't require any trust relationship between two parties, we
> + * must prevent seals from being removed. Therefore, sealing a file
> + * only adds a given set of seals to the file, it never touches
> + * existing seals. Furthermore, the "setting seals"-operation can be
> + * sealed itself, which basically prevents any further seal from being
> + * added.
> + *
> + * Semantics of sealing are only defined on volatile files. Only
> + * anonymous shmem files support sealing. More importantly, seals are
> + * never written to disk. Therefore, there's no plan to support it on
> + * other file types.
> + */
> +
> + if (file->f_op != &shmem_file_operations)
> + return -EINVAL;
> + if (!(file->f_mode & FMODE_WRITE))
> + return -EINVAL;
I would expect -EBADF there (like when you write to read-only fd).
Though I was okay with the -EPERM you had the previous version.
> + if (seals & ~(unsigned int)F_ALL_SEALS)
> + return -EINVAL;
> +
> + mutex_lock(&inode->i_mutex);
> +
> + if (info->seals & F_SEAL_SEAL) {
I notice this is inconsistent with F_SEAL_WRITE just below:
we're allowed to SEAL_WRITE what's already SEAL_WRITEd,
but not to SEAL_SEAL what's already SEAL_SEALed.
Oh, never mind, I can see that makes some sense.
> + error = -EPERM;
> + goto unlock;
> + }
> +
> + if ((seals & F_SEAL_WRITE) && !(info->seals & F_SEAL_WRITE)) {
> + error = mapping_deny_writable(file->f_mapping);
> + if (error)
Which would be -EBUSY, yes, that seems okay.
And with your atomic i_mmap_writable changes in 1/7, and the i_mutex
here, the locking is now solid, and accomplished simply: nice.
> + goto unlock;
> +
> + error = shmem_test_for_pins(file->f_mapping);
> + if (error) {
> + mapping_allow_writable(file->f_mapping);
> + goto unlock;
> + }
Right, although I ask you to remove shmem_test_for_pins() from this
patch, I can see that you might want to include a "return 0" stub for
shmem_wait_for_pins() in this patch, just so that this can appear here
now, and we consider the non-atomicity of it. Yes, I agree this is
how it should proceed: first deny, then re-allow if waiting fails.
> + }
> +
> + info->seals |= seals;
> + error = 0;
> +
> +unlock:
> + mutex_unlock(&inode->i_mutex);
> + return error;
> +}
> +EXPORT_SYMBOL_GPL(shmem_add_seals);
> +
> +int shmem_get_seals(struct file *file)
> +{
> + if (file->f_op != &shmem_file_operations)
> + return -EINVAL;
That's fine, though it is worth considering whether return 0
might be preferable. No, I suppose this is easier, fits with
shmem_fcntl() just returning -EINVAL when !TMPFS or !SHMEM.
> +
> + return SHMEM_I(file_inode(file))->seals & F_ALL_SEALS;
& F_ALL_SEALS? Okay, that may be some kind of future proofing that you
have in mind; but it may just be a leftover from when you were using bit
31 for internal use.
> +}
> +EXPORT_SYMBOL_GPL(shmem_get_seals);
> +
> +long shmem_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> + long error;
> +
> + switch (cmd) {
> + case F_ADD_SEALS:
> + /* disallow upper 32bit */
> + if (arg >> 32)
> + return -EINVAL;
That is worth checking, but gives
mm/shmem.c:1948:3: warning: right shift count >= width of type
on a 32-bit build. I expect there's an accepted way to do it;
I've used "arg > UINT_MAX" myself in some places.
> +
> + error = shmem_add_seals(file, arg);
> + break;
> + case F_GET_SEALS:
> + error = shmem_get_seals(file);
> + break;
> + default:
> + error = -EINVAL;
> + break;
> + }
> +
> + return error;
> +}
> +
> static long shmem_fallocate(struct file *file, int mode, loff_t offset,
> loff_t len)
> {
> struct inode *inode = file_inode(file);
> struct shmem_sb_info *sbinfo = SHMEM_SB(inode->i_sb);
> + struct shmem_inode_info *info = SHMEM_I(inode);
> struct shmem_falloc shmem_falloc;
> pgoff_t start, index, end;
> int error;
> @@ -1731,6 +1903,12 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,
> loff_t unmap_start = round_up(offset, PAGE_SIZE);
> loff_t unmap_end = round_down(offset + len, PAGE_SIZE) - 1;
>
> + /* protected by i_mutex */
> + if (info->seals & F_SEAL_WRITE) {
> + error = -EPERM;
> + goto out;
> + }
> +
> if ((u64)unmap_end > (u64)unmap_start)
> unmap_mapping_range(mapping, unmap_start,
> 1 + unmap_end - unmap_start, 0);
> @@ -1745,6 +1923,11 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,
> if (error)
> goto out;
>
> + if ((info->seals & F_SEAL_GROW) && offset + len > inode->i_size) {
> + error = -EPERM;
> + goto out;
> + }
> +
> start = offset >> PAGE_CACHE_SHIFT;
> end = (offset + len + PAGE_CACHE_SIZE - 1) >> PAGE_CACHE_SHIFT;
> /* Try to avoid a swapstorm if len is impossible to satisfy */
> --
> 2.0.0
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox