* [PATCH v3 0/3] introduce PIDFD_SELF* sentinels
From: Lorenzo Stoakes @ 2024-10-16 10:20 UTC (permalink / raw)
To: Christian Brauner
Cc: Shuah Khan, Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel, Oliver Sang
If you wish to utilise a pidfd interface to refer to the current process or
thread it is rather cumbersome, requiring something like:
int pidfd = pidfd_open(getpid(), 0 or PIDFD_THREAD);
...
close(pidfd);
Or the equivalent call opening /proc/self. It is more convenient to use a
sentinel value to indicate to an interface that accepts a pidfd that we
simply wish to refer to the current process thread.
This series introduces sentinels for this purposes which can be passed as
the pidfd in this instance rather than having to establish a dummy fd for
this purpose.
It is useful to refer to both the current thread from the userland's
perspective for which we use PIDFD_SELF, and the current process from the
userland's perspective, for which we use PIDFD_SELF_PROCESS.
There is unfortunately some confusion between the kernel and userland as to
what constitutes a process - a thread from the userland perspective is a
process in userland, and a userland process is a thread group (more
specifically the thread group leader from the kernel perspective). We
therefore alias things thusly:
* PIDFD_SELF_THREAD aliased by PIDFD_SELF - use PIDTYPE_PID.
* PIDFD_SELF_THREAD_GROUP alised by PIDFD_SELF_PROCESS - use PIDTYPE_TGID.
In all of the kernel code we refer to PIDFD_SELF_THREAD and
PIDFD_SELF_THREAD_GROUP. However we expect users to use PIDFD_SELF and
PIDFD_SELF_PROCESS.
This matters for cases where, for instance, a user unshare()'s FDs or does
thread-specific signal handling and where the user would be hugely confused
if the FDs referenced or signal processed referred to the thread group
leader rather than the individual thread.
We ensure that pidfd_send_signal() and pidfd_getfd() work correctly, and
assert as much in selftests. All other interfaces except setns() will work
implicitly with this new interface, however it doesn't make sense to test
waitid(P_PIDFD, ...) as waiting on ourselves is a blocking operation.
In the case of setns() we explicitly disallow use of PIDFD_SELF* as it
doesn't make sense to obtain the namespaces of our own process, and it
would require work to implement this functionality there that would be of
no use.
We also do not provide the ability to utilise PIDFD_SELF* in ordinary fd
operations such as open() or poll(), as this would require extensive work
and be of no real use.
v3:
* Do not fput() an invalid fd as reported by kernel test bot.
* Fix unintended churn from moving variable declaration.
v2:
* Fix tests as reported by Shuah.
* Correct RFC version lore link.
https://lore.kernel.org/linux-mm/cover.1728643714.git.lorenzo.stoakes@oracle.com/
Non-RFC v1:
* Removed RFC tag - there seems to be general consensus that this change is
a good idea, but perhaps some debate to be had on implementation. It
seems sensible then to move forward with the RFC flag removed.
* Introduced PIDFD_SELF_THREAD, PIDFD_SELF_THREAD_GROUP and their aliases
PIDFD_SELF and PIDFD_SELF_PROCESS respectively.
* Updated testing accordingly.
https://lore.kernel.org/linux-mm/cover.1728578231.git.lorenzo.stoakes@oracle.com/
RFC version:
https://lore.kernel.org/linux-mm/cover.1727644404.git.lorenzo.stoakes@oracle.com/
Lorenzo Stoakes (3):
pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process
selftests: pidfd: add tests for PIDFD_SELF_*
include/linux/pid.h | 43 +++++-
include/uapi/linux/pidfd.h | 15 ++
kernel/exit.c | 3 +-
kernel/nsproxy.c | 1 +
kernel/pid.c | 73 ++++++---
kernel/signal.c | 26 +---
tools/testing/selftests/pidfd/pidfd.h | 8 +
.../selftests/pidfd/pidfd_getfd_test.c | 141 ++++++++++++++++++
.../selftests/pidfd/pidfd_setns_test.c | 11 ++
tools/testing/selftests/pidfd/pidfd_test.c | 76 ++++++++--
10 files changed, 342 insertions(+), 55 deletions(-)
--
2.46.2
^ permalink raw reply
* Re: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
From: Lorenzo Stoakes @ 2024-10-16 9:46 UTC (permalink / raw)
To: kernel test robot
Cc: oe-lkp, lkp, linux-kernel, Christian Brauner, Shuah Khan,
Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api
In-Reply-To: <202410161634.abca3854-lkp@intel.com>
On Wed, Oct 16, 2024 at 04:50:56PM +0800, kernel test robot wrote:
>
>
> Hello,
>
> kernel test robot noticed "BUG:unable_to_handle_page_fault_for_address" on:
Thanks, see below for analysis.
>
> commit: e65dbb5c9051a4da2305787fd558e1d60de2275a ("[PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup")
> url: https://github.com/intel-lab-lkp/linux/commits/Lorenzo-Stoakes/pidfd-extend-pidfd_get_pid-and-de-duplicate-pid-lookup/20241011-191241
> base: https://git.kernel.org/cgit/linux/kernel/git/shuah/linux-kselftest.git next
> patch link: https://lore.kernel.org/all/8e7edaf2f648fb01a71def749f17f76c0502dee1.1728643714.git.lorenzo.stoakes@oracle.com/
> patch subject: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
>
> in testcase: trinity
> version: trinity-i386-abe9de86-1_20230429
> with following parameters:
>
> runtime: 600s
>
>
>
> config: x86_64-randconfig-072-20241015
> compiler: gcc-12
> test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
>
> (please refer to attached dmesg/kmsg for entire log/backtrace)
>
>
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <oliver.sang@intel.com>
> | Closes: https://lore.kernel.org/oe-lkp/202410161634.abca3854-lkp@intel.com
>
>
> [ 416.054386][ T1959] BUG: unable to handle page fault for address: ffffffff8fed9474
> [ 416.055651][ T1959] #PF: supervisor write access in kernel mode
> [ 416.056550][ T1959] #PF: error_code(0x0003) - permissions violation
> [ 416.057502][ T1959] PGD 3e90f5067 P4D 3e90f5067 PUD 3e90f6063 PMD 3e50001a1
> [ 416.058587][ T1959] Oops: Oops: 0003 [#1] PREEMPT SMP KASAN
> [ 416.059414][ T1959] CPU: 1 UID: 65534 PID: 1959 Comm: trinity-c3 Not tainted 6.12.0-rc1-00004-ge65dbb5c9051 #1 d7a38916ac9252f968706afc2c77f70fbdabe689
> [ 416.061328][ T1959] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
> [ 416.062850][ T1959] RIP: 0010:fput (arch/x86/include/asm/atomic64_64.h:61 include/linux/atomic/atomic-arch-fallback.h:4404 include/linux/atomic/atomic-long.h:1571 include/linux/atomic/atomic-instrumented.h:4540 fs/file_table.c:482)
> [ 416.063578][ T1959] Code: ff ff 66 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 f3 0f 1e fa 55 48 89 e5 41 55 41 54 53 48 89 fb be 08 00 00 00 e8 96 c6 f7 ff <f0> 48 ff 0b 0f 85 dd 00 00 00 65 4c 8b 25 04 ff 0e 70 4c 8d 6b 48
> All code
> ========
> 0: ff (bad)
> 1: ff 66 66 jmp *0x66(%rsi)
> 4: 2e 0f 1f 84 00 00 00 cs nopl 0x0(%rax,%rax,1)
> b: 00 00
> d: 0f 1f 00 nopl (%rax)
> 10: f3 0f 1e fa endbr64
> 14: 55 push %rbp
> 15: 48 89 e5 mov %rsp,%rbp
> 18: 41 55 push %r13
> 1a: 41 54 push %r12
> 1c: 53 push %rbx
> 1d: 48 89 fb mov %rdi,%rbx
> 20: be 08 00 00 00 mov $0x8,%esi
> 25: e8 96 c6 f7 ff call 0xfffffffffff7c6c0
> 2a:* f0 48 ff 0b lock decq (%rbx) <-- trapping instruction
OK so this looks like the fput() invoking atomic_long_dec_and_test() on an
invalid &file->f_count.
It looks like 0xffffffff8fed9474 in RBX is the file...
And that's because I'm not setting f in
SYSCALL_DEFINE4(pidfd_send_signal, ...) at:
pidfd_to_pid_proc(pidfd, &f_flags, &f);
On error and yet then jump to
err:
fdput(f);
return ret;
Which is trying to fdput() (thus fput()) the f, ugh.
OK I will fix this + respin, thanks for the report!
[snip]
^ permalink raw reply
* Re: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
From: Suren Baghdasaryan @ 2024-10-16 9:02 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Christian Brauner, Shuah Khan, Liam R . Howlett, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel
In-Reply-To: <2c783f7d-78d4-4d91-9999-12d7772b6272@lucifer.local>
On Wed, Oct 16, 2024 at 1:22 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Wed, Oct 16, 2024 at 01:16:15AM -0700, Suren Baghdasaryan wrote:
> > On Tue, Oct 15, 2024 at 11:05 PM Lorenzo Stoakes
> > <lorenzo.stoakes@oracle.com> wrote:
> > >
> > > On Tue, Oct 15, 2024 at 12:40:41PM -0700, Suren Baghdasaryan wrote:
> > > [snip]
> > > > > -struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
> > > > > +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
> > > > > + bool allow_proc, unsigned int *flags,
> > > > > + struct fd *fd)
> > > > > {
> > > > > - struct fd f;
> > > > > + struct file *file;
> > > > > struct pid *pid;
> > > > > + struct fd f = fdget(pidfd);
> > > > >
> > > > > - f = fdget(fd);
> > > > > - if (!fd_file(f))
> > > > > + file = fd_file(f);
> > > > > + if (!file)
> > > > > return ERR_PTR(-EBADF);
> > > > >
> > > > > - pid = pidfd_pid(fd_file(f));
> > > > > - if (!IS_ERR(pid)) {
> > > > > - get_pid(pid);
> > > > > - *flags = fd_file(f)->f_flags;
> > > > > + pid = pidfd_pid(file);
> > > > > + /* If we allow opening a pidfd via /proc/<pid>, do so. */
> > > > > + if (IS_ERR(pid) && allow_proc)
> > > > > + pid = tgid_pidfd_to_pid(file);
> > > > > +
> > > > > + if (IS_ERR(pid)) {
> > > > > + fdput(f);
> > > > > + return pid;
> > > > > }
> > > > >
> > > > > - fdput(f);
> > > > > + if (pin_pid)
> > > > > + get_pid(pid);
> > > > > + else
> > > > > + WARN_ON_ONCE(!fd); /* Nothing to keep pid/pidfd around? */
> > > > > +
> > > > > + if (flags)
> > > > > + *flags = file->f_flags;
> > > > > +
> > > > > + /*
> > > > > + * If the user provides an fd output then it will handle decrementing
> > > > > + * its reference counter.
> > > > > + */
> > > > > + if (fd)
> > > > > + *fd = f;
> > > > > + else
> > > > > + /* Otherwise we release it. */
> > > > > + fdput(f);
> > > > > +
> > > > > return pid;
> > > > > }
> > > >
> > > > There is an EXPORT_SYMBOL_GPL(pidfd_get_pid) right after this line. It
> > > > should also be changed to EXPORT_SYMBOL_GPL(__pidfd_get_pid),
> > > > otherwise __pidfd_get_pid() will not be exported. A module calling
> > > > pidfd_get_pid() now inlined in the header file will try to call
> > > > __pidfd_get_pid() and will have trouble resolving this symbol.
> > >
> > > Hmm hang on not there isn't? I don't see that anywhere?
> >
> > Doh! Sorry, I didn't realize the export was an out-of-tree Android
> > change. Never mind...
>
> No probs :P just glad I didn't miss something in this series!
>
> Hey maybe a motivation to upstream some of this? ;)
I wish... Without an upstream user the exports are not accepted
upstream and unfortunately Android vendors often resist upstreaming
their modules.
>
> >
> > >
> > > [snip]
^ permalink raw reply
* Re: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
From: kernel test robot @ 2024-10-16 8:50 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: oe-lkp, lkp, linux-kernel, Christian Brauner, Shuah Khan,
Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, oliver.sang
In-Reply-To: <8e7edaf2f648fb01a71def749f17f76c0502dee1.1728643714.git.lorenzo.stoakes@oracle.com>
Hello,
kernel test robot noticed "BUG:unable_to_handle_page_fault_for_address" on:
commit: e65dbb5c9051a4da2305787fd558e1d60de2275a ("[PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup")
url: https://github.com/intel-lab-lkp/linux/commits/Lorenzo-Stoakes/pidfd-extend-pidfd_get_pid-and-de-duplicate-pid-lookup/20241011-191241
base: https://git.kernel.org/cgit/linux/kernel/git/shuah/linux-kselftest.git next
patch link: https://lore.kernel.org/all/8e7edaf2f648fb01a71def749f17f76c0502dee1.1728643714.git.lorenzo.stoakes@oracle.com/
patch subject: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
in testcase: trinity
version: trinity-i386-abe9de86-1_20230429
with following parameters:
runtime: 600s
config: x86_64-randconfig-072-20241015
compiler: gcc-12
test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
(please refer to attached dmesg/kmsg for entire log/backtrace)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202410161634.abca3854-lkp@intel.com
[ 416.054386][ T1959] BUG: unable to handle page fault for address: ffffffff8fed9474
[ 416.055651][ T1959] #PF: supervisor write access in kernel mode
[ 416.056550][ T1959] #PF: error_code(0x0003) - permissions violation
[ 416.057502][ T1959] PGD 3e90f5067 P4D 3e90f5067 PUD 3e90f6063 PMD 3e50001a1
[ 416.058587][ T1959] Oops: Oops: 0003 [#1] PREEMPT SMP KASAN
[ 416.059414][ T1959] CPU: 1 UID: 65534 PID: 1959 Comm: trinity-c3 Not tainted 6.12.0-rc1-00004-ge65dbb5c9051 #1 d7a38916ac9252f968706afc2c77f70fbdabe689
[ 416.061328][ T1959] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[ 416.062850][ T1959] RIP: 0010:fput (arch/x86/include/asm/atomic64_64.h:61 include/linux/atomic/atomic-arch-fallback.h:4404 include/linux/atomic/atomic-long.h:1571 include/linux/atomic/atomic-instrumented.h:4540 fs/file_table.c:482)
[ 416.063578][ T1959] Code: ff ff 66 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 f3 0f 1e fa 55 48 89 e5 41 55 41 54 53 48 89 fb be 08 00 00 00 e8 96 c6 f7 ff <f0> 48 ff 0b 0f 85 dd 00 00 00 65 4c 8b 25 04 ff 0e 70 4c 8d 6b 48
All code
========
0: ff (bad)
1: ff 66 66 jmp *0x66(%rsi)
4: 2e 0f 1f 84 00 00 00 cs nopl 0x0(%rax,%rax,1)
b: 00 00
d: 0f 1f 00 nopl (%rax)
10: f3 0f 1e fa endbr64
14: 55 push %rbp
15: 48 89 e5 mov %rsp,%rbp
18: 41 55 push %r13
1a: 41 54 push %r12
1c: 53 push %rbx
1d: 48 89 fb mov %rdi,%rbx
20: be 08 00 00 00 mov $0x8,%esi
25: e8 96 c6 f7 ff call 0xfffffffffff7c6c0
2a:* f0 48 ff 0b lock decq (%rbx) <-- trapping instruction
2e: 0f 85 dd 00 00 00 jne 0x111
34: 65 4c 8b 25 04 ff 0e mov %gs:0x700eff04(%rip),%r12 # 0x700eff40
3b: 70
3c: 4c 8d 6b 48 lea 0x48(%rbx),%r13
Code starting with the faulting instruction
===========================================
0: f0 48 ff 0b lock decq (%rbx)
4: 0f 85 dd 00 00 00 jne 0xe7
a: 65 4c 8b 25 04 ff 0e mov %gs:0x700eff04(%rip),%r12 # 0x700eff16
11: 70
12: 4c 8d 6b 48 lea 0x48(%rbx),%r13
[ 416.066250][ T1959] RSP: 0018:ffffc9000299fa70 EFLAGS: 00010246
[ 416.067156][ T1959] RAX: 0000000000000001 RBX: ffffffff8fed9474 RCX: 0000000000000000
[ 416.068377][ T1959] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
[ 416.069091][ T1980] module: module-autoload: duplicate request for module net-pf-12
[ 416.069532][ T1959] RBP: ffffc9000299fa88 R08: 0000000000000000 R09: 0000000000000000
[ 416.069538][ T1959] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
[ 416.069541][ T1959] R13: fffffffffffffff7 R14: ffffc9000299fb70 R15: dffffc0000000000
[ 416.078460][ T1959] FS: 0000000000000000(0000) GS:ffff8883a8500000(0063) knlGS:00000000f7ef8280
[ 416.079775][ T1959] CS: 0010 DS: 002b ES: 002b CR0: 0000000080050033
[ 416.080740][ T1959] CR2: ffffffff8fed9474 CR3: 0000000120fe6000 CR4: 00000000000406f0
[ 416.081938][ T1959] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 416.083156][ T1959] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 416.084359][ T1959] Call Trace:
[ 416.084939][ T1959] <TASK>
[ 416.085461][ T1959] ? show_regs (arch/x86/kernel/dumpstack.c:479)
[ 416.088241][ T1964] module: module-autoload: duplicate request for module net-pf-32
[ 416.089149][ T1959] ? __die (arch/x86/kernel/dumpstack.c:421 arch/x86/kernel/dumpstack.c:434)
[ 416.089165][ T1959] ? __kasan_check_read (mm/kasan/shadow.c:31)
[ 416.089175][ T1959] ? page_fault_oops (arch/x86/mm/fault.c:710)
[ 416.092872][ T1959] ? fput (arch/x86/include/asm/atomic64_64.h:61 include/linux/atomic/atomic-arch-fallback.h:4404 include/linux/atomic/atomic-long.h:1571 include/linux/atomic/atomic-instrumented.h:4540 fs/file_table.c:482)
[ 416.093516][ T1959] ? show_fault_oops (arch/x86/mm/fault.c:643)
[ 416.094304][ T1959] ? fput (arch/x86/include/asm/atomic64_64.h:61 include/linux/atomic/atomic-arch-fallback.h:4404 include/linux/atomic/atomic-long.h:1571 include/linux/atomic/atomic-instrumented.h:4540 fs/file_table.c:482)
[ 416.094957][ T1959] ? search_exception_tables (kernel/extable.c:64)
[ 416.095760][ T1959] ? fixup_exception (arch/x86/mm/extable.c:320)
[ 416.096496][ T1959] ? validate_chain (arch/x86/include/asm/bitops.h:227 arch/x86/include/asm/bitops.h:239 include/asm-generic/bitops/instrumented-non-atomic.h:142 kernel/locking/lockdep.c:228 kernel/locking/lockdep.c:3816 kernel/locking/lockdep.c:3872)
[ 416.097269][ T1959] ? __kasan_check_read (mm/kasan/shadow.c:31)
[ 416.098069][ T1959] ? kernelmode_fixup_or_oops+0x84/0xb0
[ 416.099036][ T1959] ? __kasan_check_read (mm/kasan/shadow.c:31)
[ 416.099822][ T1959] ? __bad_area_nosemaphore (arch/x86/mm/fault.c:828)
[ 416.100680][ T1959] ? __kasan_check_read (mm/kasan/shadow.c:32)
[ 416.101464][ T1959] ? check_prev_add (kernel/locking/lockdep.c:3860)
[ 416.102255][ T1959] ? __kasan_check_read (mm/kasan/shadow.c:31)
[ 416.103036][ T1959] ? __kasan_check_read (mm/kasan/shadow.c:31)
[ 416.103805][ T1959] ? bad_area_nosemaphore (arch/x86/mm/fault.c:835)
[ 416.104574][ T1959] ? do_kern_addr_fault (arch/x86/mm/fault.c:862 arch/x86/mm/fault.c:881)
[ 416.105445][ T1959] ? __kasan_check_read (mm/kasan/shadow.c:31)
[ 416.106242][ T1959] ? do_kern_addr_fault (arch/x86/mm/fault.c:1199)
[ 416.107023][ T1959] ? exc_page_fault (arch/x86/mm/fault.c:1479 arch/x86/mm/fault.c:1539)
[ 416.107781][ T1959] ? asm_exc_page_fault (arch/x86/include/asm/idtentry.h:623)
[ 416.108538][ T1959] ? __kasan_check_read (mm/kasan/shadow.c:31)
[ 416.109332][ T1959] ? fput (arch/x86/include/asm/atomic64_64.h:61 include/linux/atomic/atomic-arch-fallback.h:4404 include/linux/atomic/atomic-long.h:1571 include/linux/atomic/atomic-instrumented.h:4540 fs/file_table.c:482)
[ 416.110003][ T1959] __do_sys_pidfd_send_signal (kernel/signal.c:3968)
[ 416.110881][ T1959] ? copy_siginfo_from_user32 (kernel/signal.c:3898)
[ 416.111737][ T1959] ? __kasan_check_read (mm/kasan/shadow.c:32)
[ 416.112533][ T1959] ? check_prev_add (kernel/locking/lockdep.c:3860)
[ 416.113327][ T1959] __ia32_sys_pidfd_send_signal (kernel/signal.c:3896)
[ 416.115877][ T1959] ? trace_hardirqs_on (kernel/trace/trace_preemptirq.c:63 (discriminator 22))
[ 416.116669][ T1959] ia32_sys_call (arch/x86/entry/syscall_32.c:44)
[ 416.117417][ T1959] __do_fast_syscall_32 (arch/x86/entry/common.c:165 arch/x86/entry/common.c:386)
[ 416.118224][ T1959] ? __lock_acquire (kernel/locking/lockdep.c:5202)
[ 416.119009][ T1959] ? __task_pid_nr_ns (include/linux/rcupdate.h:337 include/linux/rcupdate.h:849 kernel/pid.c:511)
[ 416.119778][ T1959] ? lock_acquire (include/trace/events/lock.h:24 kernel/locking/lockdep.c:5796)
[ 416.120563][ T1959] ? __task_pid_nr_ns (include/linux/rcupdate.h:337 include/linux/rcupdate.h:849 kernel/pid.c:511)
[ 416.121348][ T1959] ? find_held_lock (kernel/locking/lockdep.c:5315)
[ 416.122114][ T1959] ? __lock_release+0x100/0x530
[ 416.122949][ T1959] ? __task_pid_nr_ns (include/linux/rcupdate.h:347 include/linux/rcupdate.h:880 kernel/pid.c:515)
[ 416.123737][ T1959] ? reacquire_held_locks (kernel/locking/lockdep.c:5476)
[ 416.124570][ T1959] ? __task_pid_nr_ns (include/linux/rcupdate.h:347 include/linux/rcupdate.h:880 kernel/pid.c:515)
[ 416.125359][ T1959] ? syscall_exit_to_user_mode (include/linux/entry-common.h:321 kernel/entry/common.c:207 kernel/entry/common.c:218)
[ 416.126234][ T1959] ? syscall_exit_to_user_mode (kernel/entry/common.c:221)
[ 416.127090][ T1959] ? __do_fast_syscall_32 (arch/x86/entry/common.c:390)
[ 416.127922][ T1959] ? syscall_exit_to_user_mode (kernel/entry/common.c:221)
[ 416.128760][ T1959] ? __do_fast_syscall_32 (arch/x86/entry/common.c:390)
[ 416.129599][ T1959] ? __do_fast_syscall_32 (arch/x86/entry/common.c:390)
[ 416.130419][ T1959] do_fast_syscall_32 (arch/x86/entry/common.c:411)
[ 416.131158][ T1959] do_SYSENTER_32 (arch/x86/entry/common.c:450)
[ 416.131840][ T1959] entry_SYSENTER_compat_after_hwframe (arch/x86/entry/entry_64_compat.S:127)
[ 416.132788][ T1959] RIP: 0023:0xf7efd579
[ 416.133446][ T1959] Code: b8 01 10 06 03 74 b4 01 10 07 03 74 b0 01 10 08 03 74 d8 01 00 00 00 00 00 00 00 00 00 00 00 00 00 51 52 55 89 e5 0f 34 cd 80 <5d> 5a 59 c3 90 90 90 90 8d b4 26 00 00 00 00 8d b4 26 00 00 00 00
All code
========
0: b8 01 10 06 03 mov $0x3061001,%eax
5: 74 b4 je 0xffffffffffffffbb
7: 01 10 add %edx,(%rax)
9: 07 (bad)
a: 03 74 b0 01 add 0x1(%rax,%rsi,4),%esi
e: 10 08 adc %cl,(%rax)
10: 03 74 d8 01 add 0x1(%rax,%rbx,8),%esi
...
20: 00 51 52 add %dl,0x52(%rcx)
23: 55 push %rbp
24:* 89 e5 mov %esp,%ebp <-- trapping instruction
26: 0f 34 sysenter
28: cd 80 int $0x80
2a: 5d pop %rbp
2b: 5a pop %rdx
2c: 59 pop %rcx
2d: c3 ret
2e: 90 nop
2f: 90 nop
30: 90 nop
31: 90 nop
32: 8d b4 26 00 00 00 00 lea 0x0(%rsi,%riz,1),%esi
39: 8d b4 26 00 00 00 00 lea 0x0(%rsi,%riz,1),%esi
Code starting with the faulting instruction
===========================================
0: 5d pop %rbp
1: 5a pop %rdx
2: 59 pop %rcx
3: c3 ret
4: 90 nop
5: 90 nop
6: 90 nop
7: 90 nop
8: 8d b4 26 00 00 00 00 lea 0x0(%rsi,%riz,1),%esi
f: 8d b4 26 00 00 00 00 lea 0x0(%rsi,%riz,1),%esi
The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20241016/202410161634.abca3854-lkp@intel.com
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
From: Lorenzo Stoakes @ 2024-10-16 8:22 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: Christian Brauner, Shuah Khan, Liam R . Howlett, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel
In-Reply-To: <CAJuCfpFXCKAH+fc6=fg-nVC5tjpGG--Pvk4D2NOn-zdA1LXS=Q@mail.gmail.com>
On Wed, Oct 16, 2024 at 01:16:15AM -0700, Suren Baghdasaryan wrote:
> On Tue, Oct 15, 2024 at 11:05 PM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> > On Tue, Oct 15, 2024 at 12:40:41PM -0700, Suren Baghdasaryan wrote:
> > [snip]
> > > > -struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
> > > > +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
> > > > + bool allow_proc, unsigned int *flags,
> > > > + struct fd *fd)
> > > > {
> > > > - struct fd f;
> > > > + struct file *file;
> > > > struct pid *pid;
> > > > + struct fd f = fdget(pidfd);
> > > >
> > > > - f = fdget(fd);
> > > > - if (!fd_file(f))
> > > > + file = fd_file(f);
> > > > + if (!file)
> > > > return ERR_PTR(-EBADF);
> > > >
> > > > - pid = pidfd_pid(fd_file(f));
> > > > - if (!IS_ERR(pid)) {
> > > > - get_pid(pid);
> > > > - *flags = fd_file(f)->f_flags;
> > > > + pid = pidfd_pid(file);
> > > > + /* If we allow opening a pidfd via /proc/<pid>, do so. */
> > > > + if (IS_ERR(pid) && allow_proc)
> > > > + pid = tgid_pidfd_to_pid(file);
> > > > +
> > > > + if (IS_ERR(pid)) {
> > > > + fdput(f);
> > > > + return pid;
> > > > }
> > > >
> > > > - fdput(f);
> > > > + if (pin_pid)
> > > > + get_pid(pid);
> > > > + else
> > > > + WARN_ON_ONCE(!fd); /* Nothing to keep pid/pidfd around? */
> > > > +
> > > > + if (flags)
> > > > + *flags = file->f_flags;
> > > > +
> > > > + /*
> > > > + * If the user provides an fd output then it will handle decrementing
> > > > + * its reference counter.
> > > > + */
> > > > + if (fd)
> > > > + *fd = f;
> > > > + else
> > > > + /* Otherwise we release it. */
> > > > + fdput(f);
> > > > +
> > > > return pid;
> > > > }
> > >
> > > There is an EXPORT_SYMBOL_GPL(pidfd_get_pid) right after this line. It
> > > should also be changed to EXPORT_SYMBOL_GPL(__pidfd_get_pid),
> > > otherwise __pidfd_get_pid() will not be exported. A module calling
> > > pidfd_get_pid() now inlined in the header file will try to call
> > > __pidfd_get_pid() and will have trouble resolving this symbol.
> >
> > Hmm hang on not there isn't? I don't see that anywhere?
>
> Doh! Sorry, I didn't realize the export was an out-of-tree Android
> change. Never mind...
No probs :P just glad I didn't miss something in this series!
Hey maybe a motivation to upstream some of this? ;)
>
> >
> > [snip]
^ permalink raw reply
* Re: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
From: Suren Baghdasaryan @ 2024-10-16 8:16 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Christian Brauner, Shuah Khan, Liam R . Howlett, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel
In-Reply-To: <8c75620d-8920-4c19-8ebc-0f2b056d49fa@lucifer.local>
On Tue, Oct 15, 2024 at 11:05 PM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Tue, Oct 15, 2024 at 12:40:41PM -0700, Suren Baghdasaryan wrote:
> [snip]
> > > -struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
> > > +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
> > > + bool allow_proc, unsigned int *flags,
> > > + struct fd *fd)
> > > {
> > > - struct fd f;
> > > + struct file *file;
> > > struct pid *pid;
> > > + struct fd f = fdget(pidfd);
> > >
> > > - f = fdget(fd);
> > > - if (!fd_file(f))
> > > + file = fd_file(f);
> > > + if (!file)
> > > return ERR_PTR(-EBADF);
> > >
> > > - pid = pidfd_pid(fd_file(f));
> > > - if (!IS_ERR(pid)) {
> > > - get_pid(pid);
> > > - *flags = fd_file(f)->f_flags;
> > > + pid = pidfd_pid(file);
> > > + /* If we allow opening a pidfd via /proc/<pid>, do so. */
> > > + if (IS_ERR(pid) && allow_proc)
> > > + pid = tgid_pidfd_to_pid(file);
> > > +
> > > + if (IS_ERR(pid)) {
> > > + fdput(f);
> > > + return pid;
> > > }
> > >
> > > - fdput(f);
> > > + if (pin_pid)
> > > + get_pid(pid);
> > > + else
> > > + WARN_ON_ONCE(!fd); /* Nothing to keep pid/pidfd around? */
> > > +
> > > + if (flags)
> > > + *flags = file->f_flags;
> > > +
> > > + /*
> > > + * If the user provides an fd output then it will handle decrementing
> > > + * its reference counter.
> > > + */
> > > + if (fd)
> > > + *fd = f;
> > > + else
> > > + /* Otherwise we release it. */
> > > + fdput(f);
> > > +
> > > return pid;
> > > }
> >
> > There is an EXPORT_SYMBOL_GPL(pidfd_get_pid) right after this line. It
> > should also be changed to EXPORT_SYMBOL_GPL(__pidfd_get_pid),
> > otherwise __pidfd_get_pid() will not be exported. A module calling
> > pidfd_get_pid() now inlined in the header file will try to call
> > __pidfd_get_pid() and will have trouble resolving this symbol.
>
> Hmm hang on not there isn't? I don't see that anywhere?
Doh! Sorry, I didn't realize the export was an out-of-tree Android
change. Never mind...
>
> [snip]
^ permalink raw reply
* Re: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
From: Lorenzo Stoakes @ 2024-10-16 6:05 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: Christian Brauner, Shuah Khan, Liam R . Howlett, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel
In-Reply-To: <CAJuCfpHLGyrBWZ9JyJ5FdJQtGO1-tuUqHawjKX_mtwnAhSY6Ow@mail.gmail.com>
On Tue, Oct 15, 2024 at 12:40:41PM -0700, Suren Baghdasaryan wrote:
[snip]
> > -struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
> > +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
> > + bool allow_proc, unsigned int *flags,
> > + struct fd *fd)
> > {
> > - struct fd f;
> > + struct file *file;
> > struct pid *pid;
> > + struct fd f = fdget(pidfd);
> >
> > - f = fdget(fd);
> > - if (!fd_file(f))
> > + file = fd_file(f);
> > + if (!file)
> > return ERR_PTR(-EBADF);
> >
> > - pid = pidfd_pid(fd_file(f));
> > - if (!IS_ERR(pid)) {
> > - get_pid(pid);
> > - *flags = fd_file(f)->f_flags;
> > + pid = pidfd_pid(file);
> > + /* If we allow opening a pidfd via /proc/<pid>, do so. */
> > + if (IS_ERR(pid) && allow_proc)
> > + pid = tgid_pidfd_to_pid(file);
> > +
> > + if (IS_ERR(pid)) {
> > + fdput(f);
> > + return pid;
> > }
> >
> > - fdput(f);
> > + if (pin_pid)
> > + get_pid(pid);
> > + else
> > + WARN_ON_ONCE(!fd); /* Nothing to keep pid/pidfd around? */
> > +
> > + if (flags)
> > + *flags = file->f_flags;
> > +
> > + /*
> > + * If the user provides an fd output then it will handle decrementing
> > + * its reference counter.
> > + */
> > + if (fd)
> > + *fd = f;
> > + else
> > + /* Otherwise we release it. */
> > + fdput(f);
> > +
> > return pid;
> > }
>
> There is an EXPORT_SYMBOL_GPL(pidfd_get_pid) right after this line. It
> should also be changed to EXPORT_SYMBOL_GPL(__pidfd_get_pid),
> otherwise __pidfd_get_pid() will not be exported. A module calling
> pidfd_get_pid() now inlined in the header file will try to call
> __pidfd_get_pid() and will have trouble resolving this symbol.
Hmm hang on not there isn't? I don't see that anywhere?
[snip]
^ permalink raw reply
* Re: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
From: Suren Baghdasaryan @ 2024-10-15 19:40 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Christian Brauner, Shuah Khan, Liam R . Howlett, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel
In-Reply-To: <8e7edaf2f648fb01a71def749f17f76c0502dee1.1728643714.git.lorenzo.stoakes@oracle.com>
On Fri, Oct 11, 2024 at 4:06 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> The means by which a pid is determined from a pidfd is duplicated, with
> some callers holding a reference to the (pid)fd, and others explicitly
> pinning the pid.
>
> Introduce __pidfd_get_pid() which abstracts both approaches and provide
> optional output parameters for file->f_flags and the fd (the latter of
> which, if provided, prevents the function from decrementing the fd's
> refernce count).
>
> Additionally, allow the ability to open a pidfd by opening a /proc/<pid>
> directory, utilised by the pidfd_send_signal() system call, providing a
> pidfd_get_pid_proc() helper function to do so.
>
> Doing this allows us to eliminate open-coded pidfd pid lookup and to
> consistently handle this in one place.
>
> This lays the groundwork for a subsequent patch which adds a new sentinel
> pidfd to explicitly reference the current process (i.e. thread group
> leader) without the need for a pidfd.
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
> include/linux/pid.h | 42 +++++++++++++++++++++++++++++++-
> kernel/pid.c | 58 ++++++++++++++++++++++++++++++---------------
> kernel/signal.c | 22 ++++-------------
> 3 files changed, 84 insertions(+), 38 deletions(-)
>
> diff --git a/include/linux/pid.h b/include/linux/pid.h
> index a3aad9b4074c..68b02eab7509 100644
> --- a/include/linux/pid.h
> +++ b/include/linux/pid.h
> @@ -2,6 +2,7 @@
> #ifndef _LINUX_PID_H
> #define _LINUX_PID_H
>
> +#include <linux/file.h>
> #include <linux/pid_types.h>
> #include <linux/rculist.h>
> #include <linux/rcupdate.h>
> @@ -72,8 +73,47 @@ extern struct pid init_struct_pid;
>
> struct file;
>
> +
> +/**
> + * __pidfd_get_pid() - Retrieve a pid associated with the specified pidfd.
> + *
> + * @pidfd: The pidfd whose pid we want, or the fd of a /proc/<pid> file if
> + * @alloc_proc is also set.
> + * @pin_pid: If set, then the reference counter of the returned pid is
> + * incremented. If not set, then @fd should be provided to pin the
> + * pidfd.
> + * @allow_proc: If set, then an fd of a /proc/<pid> file can be passed instead
> + * of a pidfd, and this will be used to determine the pid.
> + * @flags: Output variable, if non-NULL, then the file->f_flags of the
> + * pidfd will be set here.
> + * @fd: Output variable, if non-NULL, then the pidfd reference will
> + * remain elevated and the caller will need to decrement it
> + * themselves.
> + *
> + * Returns: If successful, the pid associated with the pidfd, otherwise an
> + * error.
> + */
> +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
> + bool allow_proc, unsigned int *flags,
> + struct fd *fd);
> +
> +static inline struct pid *pidfd_get_pid(unsigned int pidfd, unsigned int *flags)
> +{
> + return __pidfd_get_pid(pidfd, /* pin_pid = */ true,
> + /* allow_proc = */ false,
> + flags, /* fd = */ NULL);
> +}
> +
> +static inline struct pid *pidfd_to_pid_proc(unsigned int pidfd,
> + unsigned int *flags,
> + struct fd *fd)
> +{
> + return __pidfd_get_pid(pidfd, /* pin_pid = */ false,
> + /* allow_proc = */ true,
> + flags, fd);
> +}
> +
> struct pid *pidfd_pid(const struct file *file);
> -struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags);
> struct task_struct *pidfd_get_task(int pidfd, unsigned int *flags);
> int pidfd_prepare(struct pid *pid, unsigned int flags, struct file **ret);
> void do_notify_pidfd(struct task_struct *task);
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 2715afb77eab..25cc1c36a1b1 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -36,6 +36,7 @@
> #include <linux/pid_namespace.h>
> #include <linux/init_task.h>
> #include <linux/syscalls.h>
> +#include <linux/proc_fs.h>
> #include <linux/proc_ns.h>
> #include <linux/refcount.h>
> #include <linux/anon_inodes.h>
> @@ -534,22 +535,46 @@ struct pid *find_ge_pid(int nr, struct pid_namespace *ns)
> }
> EXPORT_SYMBOL_GPL(find_ge_pid);
>
> -struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
> +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
> + bool allow_proc, unsigned int *flags,
> + struct fd *fd)
> {
> - struct fd f;
> + struct file *file;
> struct pid *pid;
> + struct fd f = fdget(pidfd);
>
> - f = fdget(fd);
> - if (!fd_file(f))
> + file = fd_file(f);
> + if (!file)
> return ERR_PTR(-EBADF);
>
> - pid = pidfd_pid(fd_file(f));
> - if (!IS_ERR(pid)) {
> - get_pid(pid);
> - *flags = fd_file(f)->f_flags;
> + pid = pidfd_pid(file);
> + /* If we allow opening a pidfd via /proc/<pid>, do so. */
> + if (IS_ERR(pid) && allow_proc)
> + pid = tgid_pidfd_to_pid(file);
> +
> + if (IS_ERR(pid)) {
> + fdput(f);
> + return pid;
> }
>
> - fdput(f);
> + if (pin_pid)
> + get_pid(pid);
> + else
> + WARN_ON_ONCE(!fd); /* Nothing to keep pid/pidfd around? */
> +
> + if (flags)
> + *flags = file->f_flags;
> +
> + /*
> + * If the user provides an fd output then it will handle decrementing
> + * its reference counter.
> + */
> + if (fd)
> + *fd = f;
> + else
> + /* Otherwise we release it. */
> + fdput(f);
> +
> return pid;
> }
There is an EXPORT_SYMBOL_GPL(pidfd_get_pid) right after this line. It
should also be changed to EXPORT_SYMBOL_GPL(__pidfd_get_pid),
otherwise __pidfd_get_pid() will not be exported. A module calling
pidfd_get_pid() now inlined in the header file will try to call
__pidfd_get_pid() and will have trouble resolving this symbol.
>
> @@ -747,23 +772,18 @@ SYSCALL_DEFINE3(pidfd_getfd, int, pidfd, int, fd,
> unsigned int, flags)
> {
> struct pid *pid;
> - struct fd f;
> int ret;
>
> /* flags is currently unused - make sure it's unset */
> if (flags)
> return -EINVAL;
>
> - f = fdget(pidfd);
> - if (!fd_file(f))
> - return -EBADF;
> -
> - pid = pidfd_pid(fd_file(f));
> + pid = pidfd_get_pid(pidfd, NULL);
> if (IS_ERR(pid))
> - ret = PTR_ERR(pid);
> - else
> - ret = pidfd_getfd(pid, fd);
> + return PTR_ERR(pid);
>
> - fdput(f);
> + ret = pidfd_getfd(pid, fd);
> +
> + put_pid(pid);
> return ret;
> }
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 4344860ffcac..868bfa674c62 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -3875,17 +3875,6 @@ static int copy_siginfo_from_user_any(kernel_siginfo_t *kinfo,
> return copy_siginfo_from_user(kinfo, info);
> }
>
> -static struct pid *pidfd_to_pid(const struct file *file)
> -{
> - struct pid *pid;
> -
> - pid = pidfd_pid(file);
> - if (!IS_ERR(pid))
> - return pid;
> -
> - return tgid_pidfd_to_pid(file);
> -}
> -
> #define PIDFD_SEND_SIGNAL_FLAGS \
> (PIDFD_SIGNAL_THREAD | PIDFD_SIGNAL_THREAD_GROUP | \
> PIDFD_SIGNAL_PROCESS_GROUP)
> @@ -3908,10 +3897,11 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
> siginfo_t __user *, info, unsigned int, flags)
> {
> int ret;
> - struct fd f;
> struct pid *pid;
> kernel_siginfo_t kinfo;
> enum pid_type type;
> + unsigned int f_flags;
> + struct fd f;
>
> /* Enforce flags be set to 0 until we add an extension. */
> if (flags & ~PIDFD_SEND_SIGNAL_FLAGS)
> @@ -3921,12 +3911,8 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
> if (hweight32(flags & PIDFD_SEND_SIGNAL_FLAGS) > 1)
> return -EINVAL;
>
> - f = fdget(pidfd);
> - if (!fd_file(f))
> - return -EBADF;
> -
> /* Is this a pidfd? */
> - pid = pidfd_to_pid(fd_file(f));
> + pid = pidfd_to_pid_proc(pidfd, &f_flags, &f);
> if (IS_ERR(pid)) {
> ret = PTR_ERR(pid);
> goto err;
> @@ -3939,7 +3925,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
> switch (flags) {
> case 0:
> /* Infer scope from the type of pidfd. */
> - if (fd_file(f)->f_flags & PIDFD_THREAD)
> + if (f_flags & PIDFD_THREAD)
> type = PIDTYPE_PID;
> else
> type = PIDTYPE_TGID;
> --
> 2.46.2
>
^ permalink raw reply
* Re: [f2fs-dev] [PATCH v5] f2fs: introduce device aliasing file
From: Jaegeuk Kim @ 2024-10-15 16:56 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Daeho Jeong, Daeho Jeong, kernel-team, linux-kernel,
linux-f2fs-devel, linux-fsdevel, linux-api
In-Reply-To: <Zw34CMxJB-THlGW0@infradead.org>
On 10/14, Christoph Hellwig wrote:
> On Mon, Oct 14, 2024 at 04:42:07PM +0000, Jaegeuk Kim wrote:
> > >
> > > Plz, refer to this patch and the description there.
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs-tools.git/commit/?h=dev-test&id=8cc4e257ec20bee207bb034d5ac406e1ab31eaea
> >
> > Also, I added this in the description.
> >
> > ---
> > For example,
> > "mkfs.f2fs -c /dev/block/test@test_alias /dev/block/main" gives
> > a file $root/test_alias which carves out /dev/block/test partition.
>
> What partition?
>
> So mkfs.f2fs adds additional devices based on the man page.
>
> So the above creates a file system with two devices, but the second
> device is not added to the general space pool, but mapped to a specific
> file? How does this file work. I guess it can't be unlinked and
> renamed. It probably also can't be truncated and hole punched,
> or use insert/collapse range. How does the user find out about this
> magic file? What is the use case? Are the exact semantics documented
> somewhere?
Let me ask for putting some design in Documentation. Just for a quick reference,
the use-case looks like:
# ls /dev/vd*
/dev/vdb (32GB) /dev/vdc (32GB)
# mkfs.ext4 /dev/vdc
# mkfs.f2fs -c /dev/vdc@vdc.file /dev/vdb
# mount /dev/vdb /mnt/f2fs
# ls -l /mnt/f2fs
vdc.file
# df -h
/dev/vdb 64G 33G 32G 52% /mnt/f2fs
# mount -o loop /dev/vdc /mnt/ext4
# df -h
/dev/vdb 64G 33G 32G 52% /mnt/f2fs
/dev/loop7 32G 24K 30G 1% /mnt/ext4
# umount /mnt/ext4
# f2fs_io getflags /mnt/f2fs/vdc.file
get a flag on /mnt/f2fs/vdc.file ret=0, flags=nocow(pinned),immutable
# f2fs_io setflags noimmutable /mnt/f2fs/vdc.file
get a flag on noimmutable ret=0, flags=800010
set a flag on /mnt/f2fs/vdc.file ret=0, flags=noimmutable
# rm /mnt/f2fs/vdc.file
# df -h
/dev/vdb 64G 753M 64G 2% /mnt/f2fs
So, key idea is, user can do any file operations on /dev/vdc, and
reclaim the space after the use, while the space is counted as /data.
That doesn't require modifying partition size and filesystem format.
^ permalink raw reply
* Re: [f2fs-dev] [PATCH v5] f2fs: introduce device aliasing file
From: Christoph Hellwig @ 2024-10-15 5:05 UTC (permalink / raw)
To: Jaegeuk Kim
Cc: Daeho Jeong, Christoph Hellwig, Daeho Jeong, kernel-team,
linux-kernel, linux-f2fs-devel, linux-fsdevel, linux-api
In-Reply-To: <Zw1J30Fn48uYCwK7@google.com>
On Mon, Oct 14, 2024 at 04:42:07PM +0000, Jaegeuk Kim wrote:
> >
> > Plz, refer to this patch and the description there.
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs-tools.git/commit/?h=dev-test&id=8cc4e257ec20bee207bb034d5ac406e1ab31eaea
>
> Also, I added this in the description.
>
> ---
> For example,
> "mkfs.f2fs -c /dev/block/test@test_alias /dev/block/main" gives
> a file $root/test_alias which carves out /dev/block/test partition.
What partition?
So mkfs.f2fs adds additional devices based on the man page.
So the above creates a file system with two devices, but the second
device is not added to the general space pool, but mapped to a specific
file? How does this file work. I guess it can't be unlinked and
renamed. It probably also can't be truncated and hole punched,
or use insert/collapse range. How does the user find out about this
magic file? What is the use case? Are the exact semantics documented
somewhere?
^ permalink raw reply
* Re: [PATCH v20 2/6] security: Add EXEC_RESTRICT_FILE and EXEC_DENY_INTERACTIVE securebits
From: sergeh @ 2024-10-15 3:26 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Serge Hallyn, Theodore Ts'o, Adhemerval Zanella Netto,
Alejandro Colomar, Aleksa Sarai, Andrew Morton, Andy Lutomirski,
Arnd Bergmann, Casey Schaufler, Christian Heimes, Dmitry Vyukov,
Elliott Hughes, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module, Andy Lutomirski
In-Reply-To: <20241011184422.977903-3-mic@digikod.net>
On Fri, Oct 11, 2024 at 08:44:18PM +0200, Mickaël Salaün wrote:
> The new SECBIT_EXEC_RESTRICT_FILE, SECBIT_EXEC_DENY_INTERACTIVE, and
> their *_LOCKED counterparts are designed to be set by processes setting
> up an execution environment, such as a user session, a container, or a
> security sandbox. Unlike other securebits, these ones can be set by
> unprivileged processes. Like seccomp filters or Landlock domains, the
> securebits are inherited across processes.
>
> When SECBIT_EXEC_RESTRICT_FILE is set, programs interpreting code should
> control executable resources according to execveat(2) + AT_CHECK (see
> previous commit).
>
> When SECBIT_EXEC_DENY_INTERACTIVE is set, a process should deny
> execution of user interactive commands (which excludes executable
> regular files).
>
> Being able to configure each of these securebits enables system
> administrators or owner of image containers to gradually validate the
> related changes and to identify potential issues (e.g. with interpreter
> or audit logs).
>
> It should be noted that unlike other security bits, the
> SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE bits are
> dedicated to user space willing to restrict itself. Because of that,
> they only make sense in the context of a trusted environment (e.g.
> sandbox, container, user session, full system) where the process
> changing its behavior (according to these bits) and all its parent
> processes are trusted. Otherwise, any parent process could just execute
> its own malicious code (interpreting a script or not), or even enforce a
> seccomp filter to mask these bits.
>
> Such a secure environment can be achieved with an appropriate access
> control (e.g. mount's noexec option, file access rights, LSM policy) and
> an enlighten ld.so checking that libraries are allowed for execution
> e.g., to protect against illegitimate use of LD_PRELOAD.
>
> Ptrace restrictions according to these securebits would not make sense
> because of the processes' trust assumption.
>
> Scripts may need some changes to deal with untrusted data (e.g. stdin,
> environment variables), but that is outside the scope of the kernel.
>
> See chromeOS's documentation about script execution control and the
> related threat model:
> https://www.chromium.org/chromium-os/developer-library/guides/security/noexec-shell-scripts/
>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Serge Hallyn <serge@hallyn.com>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
thanks,
-serge
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Link: https://lore.kernel.org/r/20241011184422.977903-3-mic@digikod.net
> ---
>
> Changes since v19:
> * Replace SECBIT_SHOULD_EXEC_CHECK and SECBIT_SHOULD_EXEC_RESTRICT with
> SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE:
> https://lore.kernel.org/all/20240710.eiKohpa4Phai@digikod.net/
> * Remove the ptrace restrictions, suggested by Andy.
> * Improve documentation according to the discussion with Jeff.
>
> New design since v18:
> https://lore.kernel.org/r/20220104155024.48023-3-mic@digikod.net
> ---
> include/uapi/linux/securebits.h | 113 +++++++++++++++++++++++++++++++-
> security/commoncap.c | 29 ++++++--
> 2 files changed, 135 insertions(+), 7 deletions(-)
>
> diff --git a/include/uapi/linux/securebits.h b/include/uapi/linux/securebits.h
> index d6d98877ff1a..351b6ecefc76 100644
> --- a/include/uapi/linux/securebits.h
> +++ b/include/uapi/linux/securebits.h
> @@ -52,10 +52,121 @@
> #define SECBIT_NO_CAP_AMBIENT_RAISE_LOCKED \
> (issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE_LOCKED))
>
> +/*
> + * The SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE securebits
> + * are intended for script interpreters and dynamic linkers to enforce a
> + * consistent execution security policy handled by the kernel.
> + *
> + * Whether an interpreter should check these securebits or not depends on the
> + * security risk of running malicious scripts with respect to the execution
> + * environment, and whether the kernel can check if a script is trustworthy or
> + * not. For instance, Python scripts running on a server can use arbitrary
> + * syscalls and access arbitrary files. Such interpreters should then be
> + * enlighten to use these securebits and let users define their security
> + * policy. However, a JavaScript engine running in a web browser should
> + * already be sandboxed and then should not be able to harm the user's
> + * environment.
> + *
> + * When SECBIT_EXEC_RESTRICT_FILE is set, a process should only interpret or
> + * execute a file if a call to execveat(2) with the related file descriptor and
> + * the AT_CHECK flag succeed.
> + *
> + * This secure bit may be set by user session managers, service managers,
> + * container runtimes, sandboxer tools... Except for test environments, the
> + * related SECBIT_EXEC_RESTRICT_FILE_LOCKED bit should also be set.
> + *
> + * Programs should only enforce consistent restrictions according to the
> + * securebits but without relying on any other user-controlled configuration.
> + * Indeed, the use case for these securebits is to only trust executable code
> + * vetted by the system configuration (through the kernel), so we should be
> + * careful to not let untrusted users control this configuration.
> + *
> + * However, script interpreters may still use user configuration such as
> + * environment variables as long as it is not a way to disable the securebits
> + * checks. For instance, the PATH and LD_PRELOAD variables can be set by a
> + * script's caller. Changing these variables may lead to unintended code
> + * executions, but only from vetted executable programs, which is OK. For this
> + * to make sense, the system should provide a consistent security policy to
> + * avoid arbitrary code execution e.g., by enforcing a write xor execute
> + * policy.
> + *
> + * SECBIT_EXEC_RESTRICT_FILE is complementary and should also be checked.
> + */
> +#define SECURE_EXEC_RESTRICT_FILE 8
> +#define SECURE_EXEC_RESTRICT_FILE_LOCKED 9 /* make bit-8 immutable */
> +
> +#define SECBIT_EXEC_RESTRICT_FILE (issecure_mask(SECURE_EXEC_RESTRICT_FILE))
> +#define SECBIT_EXEC_RESTRICT_FILE_LOCKED \
> + (issecure_mask(SECURE_EXEC_RESTRICT_FILE_LOCKED))
> +
> +/*
> + * When SECBIT_EXEC_DENY_INTERACTIVE is set, a process should never interpret
> + * interactive user commands (e.g. scripts). However, if such commands are
> + * passed through a file descriptor (e.g. stdin), its content should be
> + * interpreted if a call to execveat(2) with the related file descriptor and
> + * the AT_CHECK flag succeed.
> + *
> + * For instance, script interpreters called with a script snippet as argument
> + * should always deny such execution if SECBIT_EXEC_DENY_INTERACTIVE is set.
> + *
> + * This secure bit may be set by user session managers, service managers,
> + * container runtimes, sandboxer tools... Except for test environments, the
> + * related SECBIT_EXEC_DENY_INTERACTIVE_LOCKED bit should also be set.
> + *
> + * See the SECBIT_EXEC_RESTRICT_FILE documentation.
> + *
> + * Here is the expected behavior for a script interpreter according to
> + * combination of any exec securebits:
> + *
> + * 1. SECURE_EXEC_RESTRICT_FILE=0 SECURE_EXEC_DENY_INTERACTIVE=0 (default)
> + * Always interpret scripts, and allow arbitrary user commands.
> + * => No threat, everyone and everything is trusted, but we can get ahead of
> + * potential issues thanks to the call to execveat with AT_CHECK which
> + * should always be performed but ignored by the script interpreter.
> + * Indeed, this check is still important to enable systems administrators
> + * to verify requests (e.g. with audit) and prepare for migration to a
> + * secure mode.
> + *
> + * 2. SECURE_EXEC_RESTRICT_FILE=1 SECURE_EXEC_DENY_INTERACTIVE=0
> + * Deny script interpretation if they are not executable, but allow
> + * arbitrary user commands.
> + * => The threat is (potential) malicious scripts run by trusted (and not
> + * fooled) users. That can protect against unintended script executions
> + * (e.g. sh /tmp/*.sh). This makes sense for (semi-restricted) user
> + * sessions.
> + *
> + * 3. SECURE_EXEC_RESTRICT_FILE=0 SECURE_EXEC_DENY_INTERACTIVE=1
> + * Always interpret scripts, but deny arbitrary user commands.
> + * => This use case may be useful for secure services (i.e. without
> + * interactive user session) where scripts' integrity is verified (e.g.
> + * with IMA/EVM or dm-verity/IPE) but where access rights might not be
> + * ready yet. Indeed, arbitrary interactive commands would be much more
> + * difficult to check.
> + *
> + * 4. SECURE_EXEC_RESTRICT_FILE=1 SECURE_EXEC_DENY_INTERACTIVE=1
> + * Deny script interpretation if they are not executable, and also deny
> + * any arbitrary user commands.
> + * => The threat is malicious scripts run by untrusted users (but trusted
> + * code). This makes sense for system services that may only execute
> + * trusted scripts.
> + */
> +#define SECURE_EXEC_DENY_INTERACTIVE 10
> +#define SECURE_EXEC_DENY_INTERACTIVE_LOCKED 11 /* make bit-10 immutable */
> +
> +#define SECBIT_EXEC_DENY_INTERACTIVE \
> + (issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> +#define SECBIT_EXEC_DENY_INTERACTIVE_LOCKED \
> + (issecure_mask(SECURE_EXEC_DENY_INTERACTIVE_LOCKED))
> +
> #define SECURE_ALL_BITS (issecure_mask(SECURE_NOROOT) | \
> issecure_mask(SECURE_NO_SETUID_FIXUP) | \
> issecure_mask(SECURE_KEEP_CAPS) | \
> - issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE))
> + issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE) | \
> + issecure_mask(SECURE_EXEC_RESTRICT_FILE) | \
> + issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> #define SECURE_ALL_LOCKS (SECURE_ALL_BITS << 1)
>
> +#define SECURE_ALL_UNPRIVILEGED (issecure_mask(SECURE_EXEC_RESTRICT_FILE) | \
> + issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> +
> #endif /* _UAPI_LINUX_SECUREBITS_H */
> diff --git a/security/commoncap.c b/security/commoncap.c
> index cefad323a0b1..52ea01acb453 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -1302,21 +1302,38 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
> & (old->securebits ^ arg2)) /*[1]*/
> || ((old->securebits & SECURE_ALL_LOCKS & ~arg2)) /*[2]*/
> || (arg2 & ~(SECURE_ALL_LOCKS | SECURE_ALL_BITS)) /*[3]*/
> - || (cap_capable(current_cred(),
> - current_cred()->user_ns,
> - CAP_SETPCAP,
> - CAP_OPT_NONE) != 0) /*[4]*/
> /*
> * [1] no changing of bits that are locked
> * [2] no unlocking of locks
> * [3] no setting of unsupported bits
> - * [4] doing anything requires privilege (go read about
> - * the "sendmail capabilities bug")
> */
> )
> /* cannot change a locked bit */
> return -EPERM;
>
> + /*
> + * Doing anything requires privilege (go read about the
> + * "sendmail capabilities bug"), except for unprivileged bits.
> + * Indeed, the SECURE_ALL_UNPRIVILEGED bits are not
> + * restrictions enforced by the kernel but by user space on
> + * itself.
> + */
> + if (cap_capable(current_cred(), current_cred()->user_ns,
> + CAP_SETPCAP, CAP_OPT_NONE) != 0) {
> + const unsigned long unpriv_and_locks =
> + SECURE_ALL_UNPRIVILEGED |
> + SECURE_ALL_UNPRIVILEGED << 1;
> + const unsigned long changed = old->securebits ^ arg2;
> +
> + /* For legacy reason, denies non-change. */
> + if (!changed)
> + return -EPERM;
> +
> + /* Denies privileged changes. */
> + if (changed & ~unpriv_and_locks)
> + return -EPERM;
> + }
> +
> new = prepare_creds();
> if (!new)
> return -ENOMEM;
> --
> 2.46.1
>
^ permalink raw reply
* Re: [PATCH v20 1/6] exec: Add a new AT_CHECK flag to execveat(2)
From: sergeh @ 2024-10-15 3:20 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Serge E. Hallyn, Al Viro, Christian Brauner, Kees Cook,
Linus Torvalds, Paul Moore, Theodore Ts'o,
Adhemerval Zanella Netto, Alejandro Colomar, Aleksa Sarai,
Andrew Morton, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Christian Heimes, Dmitry Vyukov, Elliott Hughes, Eric Biggers,
Eric Chiang, Fan Wu, Florian Weimer, Geert Uytterhoeven,
James Morris, Jan Kara, Jann Horn, Jeff Xu, Jonathan Corbet,
Jordan R Abrahams, Lakshmi Ramasubramanian, Luca Boccassi,
Luis Chamberlain, Madhavan T . Venkataraman, Matt Bobrowski,
Matthew Garrett, Matthew Wilcox, Miklos Szeredi, Mimi Zohar,
Nicolas Bouchinet, Scott Shell, Shuah Khan, Stephen Rothwell,
Steve Dower, Steve Grubb, Thibaut Sautereau, Vincent Strubel,
Xiaoming Ni, Yin Fengwei, kernel-hardening, linux-api,
linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20241014.ke5eeKoo6doh@digikod.net>
On Mon, Oct 14, 2024 at 09:39:52AM +0200, Mickaël Salaün wrote:
> On Sat, Oct 12, 2024 at 10:04:16PM -0500, Serge E. Hallyn wrote:
> > On Fri, Oct 11, 2024 at 08:44:17PM +0200, Mickaël Salaün wrote:
> > > Add a new AT_CHECK flag to execveat(2) to check if a file would be
> >
> > Apologies for both bikeshedding and missing earlier discussions.
> >
> > But AT_CHECK sounds quite generic. How about AT_EXEC_CHECK, or
> > AT_CHECK_EXEC_CREDS? (I would suggest just AT_CHECK_CREDS since
> > it's for use in execveat(2), but as it's an AT_ flag, it's
> > probably worth being more precise).
>
> As Amir pointed out, we need at least to use the AT_EXECVE_CHECK_
> prefix, and I agree with the AT_EXECVE_CHECK name because it's about
> checking the whole execve request, not sepcifically a "creds" part.
Well, not the whole. You are explicitly not checking the validity of the
files.
But ok. With that,
Reviewed-by: Serge Hallyn <sergeh@kernel.org>
thanks,
-serge
^ permalink raw reply
* Re: [PATCH v20 2/6] security: Add EXEC_RESTRICT_FILE and EXEC_DENY_INTERACTIVE securebits
From: sergeh @ 2024-10-15 3:15 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Serge E. Hallyn, Al Viro, Christian Brauner, Kees Cook,
Linus Torvalds, Paul Moore, Theodore Ts'o,
Adhemerval Zanella Netto, Alejandro Colomar, Aleksa Sarai,
Andrew Morton, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Christian Heimes, Dmitry Vyukov, Elliott Hughes, Eric Biggers,
Eric Chiang, Fan Wu, Florian Weimer, Geert Uytterhoeven,
James Morris, Jan Kara, Jann Horn, Jeff Xu, Jonathan Corbet,
Jordan R Abrahams, Lakshmi Ramasubramanian, Luca Boccassi,
Luis Chamberlain, Madhavan T . Venkataraman, Matt Bobrowski,
Matthew Garrett, Matthew Wilcox, Miklos Szeredi, Mimi Zohar,
Nicolas Bouchinet, Scott Shell, Shuah Khan, Stephen Rothwell,
Steve Dower, Steve Grubb, Thibaut Sautereau, Vincent Strubel,
Xiaoming Ni, Yin Fengwei, kernel-hardening, linux-api,
linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module, Andy Lutomirski
In-Reply-To: <20241014.jahHeitoo0uo@digikod.net>
On Mon, Oct 14, 2024 at 09:40:34AM +0200, Mickaël Salaün wrote:
> On Sat, Oct 12, 2024 at 09:51:50PM -0500, Serge E. Hallyn wrote:
> > On Fri, Oct 11, 2024 at 08:44:18PM +0200, Mickaël Salaün wrote:
> > > The new SECBIT_EXEC_RESTRICT_FILE, SECBIT_EXEC_DENY_INTERACTIVE, and
> > > their *_LOCKED counterparts are designed to be set by processes setting
> > > up an execution environment, such as a user session, a container, or a
> > > security sandbox. Unlike other securebits, these ones can be set by
> > > unprivileged processes. Like seccomp filters or Landlock domains, the
> > > securebits are inherited across processes.
> > >
> > > When SECBIT_EXEC_RESTRICT_FILE is set, programs interpreting code should
> > > control executable resources according to execveat(2) + AT_CHECK (see
> > > previous commit).
> > >
> > > When SECBIT_EXEC_DENY_INTERACTIVE is set, a process should deny
> > > execution of user interactive commands (which excludes executable
> > > regular files).
> > >
> > > Being able to configure each of these securebits enables system
> > > administrators or owner of image containers to gradually validate the
> > > related changes and to identify potential issues (e.g. with interpreter
> > > or audit logs).
> > >
> > > It should be noted that unlike other security bits, the
> > > SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE bits are
> > > dedicated to user space willing to restrict itself. Because of that,
> > > they only make sense in the context of a trusted environment (e.g.
> > > sandbox, container, user session, full system) where the process
> > > changing its behavior (according to these bits) and all its parent
> > > processes are trusted. Otherwise, any parent process could just execute
> > > its own malicious code (interpreting a script or not), or even enforce a
> > > seccomp filter to mask these bits.
> > >
> > > Such a secure environment can be achieved with an appropriate access
> > > control (e.g. mount's noexec option, file access rights, LSM policy) and
> > > an enlighten ld.so checking that libraries are allowed for execution
> > > e.g., to protect against illegitimate use of LD_PRELOAD.
> > >
> > > Ptrace restrictions according to these securebits would not make sense
> > > because of the processes' trust assumption.
> > >
> > > Scripts may need some changes to deal with untrusted data (e.g. stdin,
> > > environment variables), but that is outside the scope of the kernel.
> > >
> > > See chromeOS's documentation about script execution control and the
> > > related threat model:
> > > https://www.chromium.org/chromium-os/developer-library/guides/security/noexec-shell-scripts/
> > >
> > > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > > Cc: Andy Lutomirski <luto@amacapital.net>
> > > Cc: Christian Brauner <brauner@kernel.org>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Cc: Paul Moore <paul@paul-moore.com>
> > > Cc: Serge Hallyn <serge@hallyn.com>
> > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > Link: https://lore.kernel.org/r/20241011184422.977903-3-mic@digikod.net
> > > ---
> > >
> > > Changes since v19:
> > > * Replace SECBIT_SHOULD_EXEC_CHECK and SECBIT_SHOULD_EXEC_RESTRICT with
> > > SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE:
> > > https://lore.kernel.org/all/20240710.eiKohpa4Phai@digikod.net/
> > > * Remove the ptrace restrictions, suggested by Andy.
> > > * Improve documentation according to the discussion with Jeff.
> > >
> > > New design since v18:
> > > https://lore.kernel.org/r/20220104155024.48023-3-mic@digikod.net
> > > ---
> > > include/uapi/linux/securebits.h | 113 +++++++++++++++++++++++++++++++-
> > > security/commoncap.c | 29 ++++++--
> > > 2 files changed, 135 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/include/uapi/linux/securebits.h b/include/uapi/linux/securebits.h
> > > index d6d98877ff1a..351b6ecefc76 100644
> > > --- a/include/uapi/linux/securebits.h
> > > +++ b/include/uapi/linux/securebits.h
> > > @@ -52,10 +52,121 @@
> > > #define SECBIT_NO_CAP_AMBIENT_RAISE_LOCKED \
> > > (issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE_LOCKED))
> > >
> > > +/*
> > > + * The SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE securebits
> > > + * are intended for script interpreters and dynamic linkers to enforce a
> > > + * consistent execution security policy handled by the kernel.
> > > + *
> > > + * Whether an interpreter should check these securebits or not depends on the
> > > + * security risk of running malicious scripts with respect to the execution
> > > + * environment, and whether the kernel can check if a script is trustworthy or
> > > + * not. For instance, Python scripts running on a server can use arbitrary
> > > + * syscalls and access arbitrary files. Such interpreters should then be
> > > + * enlighten to use these securebits and let users define their security
> > > + * policy. However, a JavaScript engine running in a web browser should
> > > + * already be sandboxed and then should not be able to harm the user's
> > > + * environment.
> > > + *
> > > + * When SECBIT_EXEC_RESTRICT_FILE is set, a process should only interpret or
> > > + * execute a file if a call to execveat(2) with the related file descriptor and
> > > + * the AT_CHECK flag succeed.
> > > + *
> > > + * This secure bit may be set by user session managers, service managers,
> > > + * container runtimes, sandboxer tools... Except for test environments, the
> > > + * related SECBIT_EXEC_RESTRICT_FILE_LOCKED bit should also be set.
> > > + *
> > > + * Programs should only enforce consistent restrictions according to the
> > > + * securebits but without relying on any other user-controlled configuration.
> > > + * Indeed, the use case for these securebits is to only trust executable code
> > > + * vetted by the system configuration (through the kernel), so we should be
> > > + * careful to not let untrusted users control this configuration.
> > > + *
> > > + * However, script interpreters may still use user configuration such as
> > > + * environment variables as long as it is not a way to disable the securebits
> > > + * checks. For instance, the PATH and LD_PRELOAD variables can be set by a
> > > + * script's caller. Changing these variables may lead to unintended code
> > > + * executions, but only from vetted executable programs, which is OK. For this
> > > + * to make sense, the system should provide a consistent security policy to
> > > + * avoid arbitrary code execution e.g., by enforcing a write xor execute
> > > + * policy.
> > > + *
> > > + * SECBIT_EXEC_RESTRICT_FILE is complementary and should also be checked.
> > > + */
> > > +#define SECURE_EXEC_RESTRICT_FILE 8
> > > +#define SECURE_EXEC_RESTRICT_FILE_LOCKED 9 /* make bit-8 immutable */
> > > +
> > > +#define SECBIT_EXEC_RESTRICT_FILE (issecure_mask(SECURE_EXEC_RESTRICT_FILE))
> > > +#define SECBIT_EXEC_RESTRICT_FILE_LOCKED \
> > > + (issecure_mask(SECURE_EXEC_RESTRICT_FILE_LOCKED))
> > > +
> > > +/*
> > > + * When SECBIT_EXEC_DENY_INTERACTIVE is set, a process should never interpret
> > > + * interactive user commands (e.g. scripts). However, if such commands are
> > > + * passed through a file descriptor (e.g. stdin), its content should be
> > > + * interpreted if a call to execveat(2) with the related file descriptor and
> > > + * the AT_CHECK flag succeed.
> > > + *
> > > + * For instance, script interpreters called with a script snippet as argument
> > > + * should always deny such execution if SECBIT_EXEC_DENY_INTERACTIVE is set.
> > > + *
> > > + * This secure bit may be set by user session managers, service managers,
> > > + * container runtimes, sandboxer tools... Except for test environments, the
> > > + * related SECBIT_EXEC_DENY_INTERACTIVE_LOCKED bit should also be set.
> > > + *
> > > + * See the SECBIT_EXEC_RESTRICT_FILE documentation.
> > > + *
> > > + * Here is the expected behavior for a script interpreter according to
> > > + * combination of any exec securebits:
> > > + *
> > > + * 1. SECURE_EXEC_RESTRICT_FILE=0 SECURE_EXEC_DENY_INTERACTIVE=0 (default)
> > > + * Always interpret scripts, and allow arbitrary user commands.
> > > + * => No threat, everyone and everything is trusted, but we can get ahead of
> > > + * potential issues thanks to the call to execveat with AT_CHECK which
> > > + * should always be performed but ignored by the script interpreter.
> > > + * Indeed, this check is still important to enable systems administrators
> > > + * to verify requests (e.g. with audit) and prepare for migration to a
> > > + * secure mode.
> > > + *
> > > + * 2. SECURE_EXEC_RESTRICT_FILE=1 SECURE_EXEC_DENY_INTERACTIVE=0
> > > + * Deny script interpretation if they are not executable, but allow
> > > + * arbitrary user commands.
> > > + * => The threat is (potential) malicious scripts run by trusted (and not
> > > + * fooled) users. That can protect against unintended script executions
> > > + * (e.g. sh /tmp/*.sh). This makes sense for (semi-restricted) user
> > > + * sessions.
> > > + *
> > > + * 3. SECURE_EXEC_RESTRICT_FILE=0 SECURE_EXEC_DENY_INTERACTIVE=1
> > > + * Always interpret scripts, but deny arbitrary user commands.
> > > + * => This use case may be useful for secure services (i.e. without
> > > + * interactive user session) where scripts' integrity is verified (e.g.
> > > + * with IMA/EVM or dm-verity/IPE) but where access rights might not be
> > > + * ready yet. Indeed, arbitrary interactive commands would be much more
> > > + * difficult to check.
> > > + *
> > > + * 4. SECURE_EXEC_RESTRICT_FILE=1 SECURE_EXEC_DENY_INTERACTIVE=1
> > > + * Deny script interpretation if they are not executable, and also deny
> > > + * any arbitrary user commands.
> > > + * => The threat is malicious scripts run by untrusted users (but trusted
> > > + * code). This makes sense for system services that may only execute
> > > + * trusted scripts.
> > > + */
> > > +#define SECURE_EXEC_DENY_INTERACTIVE 10
> > > +#define SECURE_EXEC_DENY_INTERACTIVE_LOCKED 11 /* make bit-10 immutable */
> > > +
> > > +#define SECBIT_EXEC_DENY_INTERACTIVE \
> > > + (issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> > > +#define SECBIT_EXEC_DENY_INTERACTIVE_LOCKED \
> > > + (issecure_mask(SECURE_EXEC_DENY_INTERACTIVE_LOCKED))
> > > +
> > > #define SECURE_ALL_BITS (issecure_mask(SECURE_NOROOT) | \
> > > issecure_mask(SECURE_NO_SETUID_FIXUP) | \
> > > issecure_mask(SECURE_KEEP_CAPS) | \
> > > - issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE))
> > > + issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE) | \
> > > + issecure_mask(SECURE_EXEC_RESTRICT_FILE) | \
> > > + issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> > > #define SECURE_ALL_LOCKS (SECURE_ALL_BITS << 1)
> > >
> > > +#define SECURE_ALL_UNPRIVILEGED (issecure_mask(SECURE_EXEC_RESTRICT_FILE) | \
> > > + issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> > > +
> > > #endif /* _UAPI_LINUX_SECUREBITS_H */
> > > diff --git a/security/commoncap.c b/security/commoncap.c
> > > index cefad323a0b1..52ea01acb453 100644
> > > --- a/security/commoncap.c
> > > +++ b/security/commoncap.c
> > > @@ -1302,21 +1302,38 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
> > > & (old->securebits ^ arg2)) /*[1]*/
> > > || ((old->securebits & SECURE_ALL_LOCKS & ~arg2)) /*[2]*/
> > > || (arg2 & ~(SECURE_ALL_LOCKS | SECURE_ALL_BITS)) /*[3]*/
> > > - || (cap_capable(current_cred(),
> > > - current_cred()->user_ns,
> > > - CAP_SETPCAP,
> > > - CAP_OPT_NONE) != 0) /*[4]*/
> > > /*
> > > * [1] no changing of bits that are locked
> > > * [2] no unlocking of locks
> > > * [3] no setting of unsupported bits
> > > - * [4] doing anything requires privilege (go read about
> > > - * the "sendmail capabilities bug")
> > > */
> > > )
> > > /* cannot change a locked bit */
> > > return -EPERM;
> > >
> > > + /*
> > > + * Doing anything requires privilege (go read about the
> > > + * "sendmail capabilities bug"), except for unprivileged bits.
> > > + * Indeed, the SECURE_ALL_UNPRIVILEGED bits are not
> > > + * restrictions enforced by the kernel but by user space on
> > > + * itself.
> > > + */
> > > + if (cap_capable(current_cred(), current_cred()->user_ns,
> > > + CAP_SETPCAP, CAP_OPT_NONE) != 0) {
> > > + const unsigned long unpriv_and_locks =
> > > + SECURE_ALL_UNPRIVILEGED |
> > > + SECURE_ALL_UNPRIVILEGED << 1;
> > > + const unsigned long changed = old->securebits ^ arg2;
> > > +
> > > + /* For legacy reason, denies non-change. */
> > > + if (!changed)
> > > + return -EPERM;
> >
> > This is odd to me. You say for legacy reasons, but, currently, calling
> > PR_SET_SECUREBITS with no changes returns 0. So you may be breaking
> > a lot of programs here, unless I'm mistaken.
>
> When we call PR_SET_SECUREBITS with 0 (and if it was 0 too), it
> currently goes through the capability check and return -EPERM if the
> caller doesn't have CAP_SETCAP. This is tested with
> TEST_F(secbits, legacy) in tools/testing/selftests/exec/check-exec.c
> (patch 3/6).
Drat, my manual test case had a typo. Right you are - it fails now.
thanks,
-serge
^ permalink raw reply
* Re: [PATCH v20 1/6] exec: Add a new AT_CHECK flag to execveat(2)
From: Amir Goldstein @ 2024-10-14 8:24 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Serge Hallyn, Theodore Ts'o, Adhemerval Zanella Netto,
Alejandro Colomar, Aleksa Sarai, Andrew Morton, Andy Lutomirski,
Arnd Bergmann, Casey Schaufler, Christian Heimes, Dmitry Vyukov,
Elliott Hughes, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20241014.waet2se6Jeiw@digikod.net>
On Mon, Oct 14, 2024 at 9:39 AM Mickaël Salaün <mic@digikod.net> wrote:
>
> On Sun, Oct 13, 2024 at 11:25:11AM +0200, Amir Goldstein wrote:
> > On Fri, Oct 11, 2024 at 8:45 PM Mickaël Salaün <mic@digikod.net> wrote:
> > >
> > > Add a new AT_CHECK flag to execveat(2) to check if a file would be
> > > allowed for execution. The main use case is for script interpreters and
> > > dynamic linkers to check execution permission according to the kernel's
> > > security policy. Another use case is to add context to access logs e.g.,
> > > which script (instead of interpreter) accessed a file. As any
> > > executable code, scripts could also use this check [1].
> > >
> > > This is different from faccessat(2) + X_OK which only checks a subset of
> > > access rights (i.e. inode permission and mount options for regular
> > > files), but not the full context (e.g. all LSM access checks). The main
> > > use case for access(2) is for SUID processes to (partially) check access
> > > on behalf of their caller. The main use case for execveat(2) + AT_CHECK
> > > is to check if a script execution would be allowed, according to all the
> > > different restrictions in place. Because the use of AT_CHECK follows
> > > the exact kernel semantic as for a real execution, user space gets the
> > > same error codes.
> > >
> > > An interesting point of using execveat(2) instead of openat2(2) is that
> > > it decouples the check from the enforcement. Indeed, the security check
> > > can be logged (e.g. with audit) without blocking an execution
> > > environment not yet ready to enforce a strict security policy.
> > >
> > > LSMs can control or log execution requests with
> > > security_bprm_creds_for_exec(). However, to enforce a consistent and
> > > complete access control (e.g. on binary's dependencies) LSMs should
> > > restrict file executability, or mesure executed files, with
> > > security_file_open() by checking file->f_flags & __FMODE_EXEC.
> > >
> > > Because AT_CHECK is dedicated to user space interpreters, it doesn't
> > > make sense for the kernel to parse the checked files, look for
> > > interpreters known to the kernel (e.g. ELF, shebang), and return ENOEXEC
> > > if the format is unknown. Because of that, security_bprm_check() is
> > > never called when AT_CHECK is used.
> > >
> > > It should be noted that script interpreters cannot directly use
> > > execveat(2) (without this new AT_CHECK flag) because this could lead to
> > > unexpected behaviors e.g., `python script.sh` could lead to Bash being
> > > executed to interpret the script. Unlike the kernel, script
> > > interpreters may just interpret the shebang as a simple comment, which
> > > should not change for backward compatibility reasons.
> > >
> > > Because scripts or libraries files might not currently have the
> > > executable permission set, or because we might want specific users to be
> > > allowed to run arbitrary scripts, the following patch provides a dynamic
> > > configuration mechanism with the SECBIT_EXEC_RESTRICT_FILE and
> > > SECBIT_EXEC_DENY_INTERACTIVE securebits.
> > >
> > > This is a redesign of the CLIP OS 4's O_MAYEXEC:
> > > https://github.com/clipos-archive/src_platform_clip-patches/blob/f5cb330d6b684752e403b4e41b39f7004d88e561/1901_open_mayexec.patch
> > > This patch has been used for more than a decade with customized script
> > > interpreters. Some examples can be found here:
> > > https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC
> > >
> > > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > > Cc: Christian Brauner <brauner@kernel.org>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Cc: Paul Moore <paul@paul-moore.com>
> > > Cc: Serge Hallyn <serge@hallyn.com>
> > > Link: https://docs.python.org/3/library/io.html#io.open_code [1]
> > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > Link: https://lore.kernel.org/r/20241011184422.977903-2-mic@digikod.net
> > > ---
> > >
> > > Changes since v19:
> > > * Remove mention of "role transition" as suggested by Andy.
> > > * Highlight the difference between security_bprm_creds_for_exec() and
> > > the __FMODE_EXEC check for LSMs (in commit message and LSM's hooks) as
> > > discussed with Jeff.
> > > * Improve documentation both in UAPI comments and kernel comments
> > > (requested by Kees).
> > >
> > > New design since v18:
> > > https://lore.kernel.org/r/20220104155024.48023-3-mic@digikod.net
> > > ---
> > > fs/exec.c | 18 ++++++++++++++++--
> > > include/linux/binfmts.h | 7 ++++++-
> > > include/uapi/linux/fcntl.h | 31 +++++++++++++++++++++++++++++++
> > > kernel/audit.h | 1 +
> > > kernel/auditsc.c | 1 +
> > > security/security.c | 10 ++++++++++
> > > 6 files changed, 65 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/fs/exec.c b/fs/exec.c
> > > index 6c53920795c2..163c659d9ae6 100644
> > > --- a/fs/exec.c
> > > +++ b/fs/exec.c
> > > @@ -891,7 +891,7 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
> > > .lookup_flags = LOOKUP_FOLLOW,
> > > };
> > >
> > > - if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
> > > + if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH | AT_CHECK)) != 0)
> > > return ERR_PTR(-EINVAL);
> > > if (flags & AT_SYMLINK_NOFOLLOW)
> > > open_exec_flags.lookup_flags &= ~LOOKUP_FOLLOW;
> > > @@ -1545,6 +1545,20 @@ static struct linux_binprm *alloc_bprm(int fd, struct filename *filename, int fl
> > > }
> > > bprm->interp = bprm->filename;
> > >
> > > + /*
> > > + * At this point, security_file_open() has already been called (with
> > > + * __FMODE_EXEC) and access control checks for AT_CHECK will stop just
> > > + * after the security_bprm_creds_for_exec() call in bprm_execve().
> > > + * Indeed, the kernel should not try to parse the content of the file
> > > + * with exec_binprm() nor change the calling thread, which means that
> > > + * the following security functions will be not called:
> > > + * - security_bprm_check()
> > > + * - security_bprm_creds_from_file()
> > > + * - security_bprm_committing_creds()
> > > + * - security_bprm_committed_creds()
> > > + */
> > > + bprm->is_check = !!(flags & AT_CHECK);
> > > +
> > > retval = bprm_mm_init(bprm);
> > > if (!retval)
> > > return bprm;
> > > @@ -1839,7 +1853,7 @@ static int bprm_execve(struct linux_binprm *bprm)
> > >
> > > /* Set the unchanging part of bprm->cred */
> > > retval = security_bprm_creds_for_exec(bprm);
> > > - if (retval)
> > > + if (retval || bprm->is_check)
> > > goto out;
> > >
> > > retval = exec_binprm(bprm);
> > > diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> > > index e6c00e860951..8ff0eb3644a1 100644
> > > --- a/include/linux/binfmts.h
> > > +++ b/include/linux/binfmts.h
> > > @@ -42,7 +42,12 @@ struct linux_binprm {
> > > * Set when errors can no longer be returned to the
> > > * original userspace.
> > > */
> > > - point_of_no_return:1;
> > > + point_of_no_return:1,
> > > + /*
> > > + * Set by user space to check executability according to the
> > > + * caller's environment.
> > > + */
> > > + is_check:1;
> > > struct file *executable; /* Executable to pass to the interpreter */
> > > struct file *interpreter;
> > > struct file *file;
> > > diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> > > index 87e2dec79fea..e606815b1c5a 100644
> > > --- a/include/uapi/linux/fcntl.h
> > > +++ b/include/uapi/linux/fcntl.h
> > > @@ -154,6 +154,37 @@
> > > usable with open_by_handle_at(2). */
> > > #define AT_HANDLE_MNT_ID_UNIQUE 0x001 /* Return the u64 unique mount ID. */
> > >
> > > +/*
> > > + * AT_CHECK only performs a check on a regular file and returns 0 if execution
> > > + * of this file would be allowed, ignoring the file format and then the related
> > > + * interpreter dependencies (e.g. ELF libraries, script's shebang).
> > > + *
> > > + * Programs should always perform this check to apply kernel-level checks
> > > + * against files that are not directly executed by the kernel but passed to a
> > > + * user space interpreter instead. All files that contain executable code,
> > > + * from the point of view of the interpreter, should be checked. However the
> > > + * result of this check should only be enforced according to
> > > + * SECBIT_EXEC_RESTRICT_FILE or SECBIT_EXEC_DENY_INTERACTIVE. See securebits.h
> > > + * documentation and the samples/check-exec/inc.c example.
> > > + *
> > > + * The main purpose of this flag is to improve the security and consistency of
> > > + * an execution environment to ensure that direct file execution (e.g.
> > > + * `./script.sh`) and indirect file execution (e.g. `sh script.sh`) lead to the
> > > + * same result. For instance, this can be used to check if a file is
> > > + * trustworthy according to the caller's environment.
> > > + *
> > > + * In a secure environment, libraries and any executable dependencies should
> > > + * also be checked. For instance, dynamic linking should make sure that all
> > > + * libraries are allowed for execution to avoid trivial bypass (e.g. using
> > > + * LD_PRELOAD). For such secure execution environment to make sense, only
> > > + * trusted code should be executable, which also requires integrity guarantees.
> > > + *
> > > + * To avoid race conditions leading to time-of-check to time-of-use issues,
> > > + * AT_CHECK should be used with AT_EMPTY_PATH to check against a file
> > > + * descriptor instead of a path.
> > > + */
> >
> > If you ask me, the very elaborate comment above belongs to execveat(2)
> > man page and is way too verbose for a uapi header.
>
> OK, but since this new flags raised a lot of questions, I guess a
> dedicated Documentation/userspace-api/check-exec.rst file with thit
> AT*_CHECK and the related securebits would be useful instead of the
> related inlined documentation.
>
> >
> > > +#define AT_CHECK 0x10000
> >
> > Please see the comment "Per-syscall flags for the *at(2) family of syscalls."
> > above. If this is a per-syscall flag please use one of the per-syscall
> > flags, e.g.:
> >
> > /* Flags for execveat2(2) */
> > #define AT_EXECVE_CHECK 0x0001 /* Only perform a check if
> > execution would be allowed */
>
> I missed this part, this prefix makes sense, thanks.
>
Not only the prefix, also the overloaded value.
Thanks,
Amir.
^ permalink raw reply
* Re: [PATCH v20 2/6] security: Add EXEC_RESTRICT_FILE and EXEC_DENY_INTERACTIVE securebits
From: Mickaël Salaün @ 2024-10-14 7:40 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Theodore Ts'o, Adhemerval Zanella Netto, Alejandro Colomar,
Aleksa Sarai, Andrew Morton, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Christian Heimes, Dmitry Vyukov, Elliott Hughes,
Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module, Andy Lutomirski
In-Reply-To: <20241013025150.GA1056399@mail.hallyn.com>
On Sat, Oct 12, 2024 at 09:51:50PM -0500, Serge E. Hallyn wrote:
> On Fri, Oct 11, 2024 at 08:44:18PM +0200, Mickaël Salaün wrote:
> > The new SECBIT_EXEC_RESTRICT_FILE, SECBIT_EXEC_DENY_INTERACTIVE, and
> > their *_LOCKED counterparts are designed to be set by processes setting
> > up an execution environment, such as a user session, a container, or a
> > security sandbox. Unlike other securebits, these ones can be set by
> > unprivileged processes. Like seccomp filters or Landlock domains, the
> > securebits are inherited across processes.
> >
> > When SECBIT_EXEC_RESTRICT_FILE is set, programs interpreting code should
> > control executable resources according to execveat(2) + AT_CHECK (see
> > previous commit).
> >
> > When SECBIT_EXEC_DENY_INTERACTIVE is set, a process should deny
> > execution of user interactive commands (which excludes executable
> > regular files).
> >
> > Being able to configure each of these securebits enables system
> > administrators or owner of image containers to gradually validate the
> > related changes and to identify potential issues (e.g. with interpreter
> > or audit logs).
> >
> > It should be noted that unlike other security bits, the
> > SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE bits are
> > dedicated to user space willing to restrict itself. Because of that,
> > they only make sense in the context of a trusted environment (e.g.
> > sandbox, container, user session, full system) where the process
> > changing its behavior (according to these bits) and all its parent
> > processes are trusted. Otherwise, any parent process could just execute
> > its own malicious code (interpreting a script or not), or even enforce a
> > seccomp filter to mask these bits.
> >
> > Such a secure environment can be achieved with an appropriate access
> > control (e.g. mount's noexec option, file access rights, LSM policy) and
> > an enlighten ld.so checking that libraries are allowed for execution
> > e.g., to protect against illegitimate use of LD_PRELOAD.
> >
> > Ptrace restrictions according to these securebits would not make sense
> > because of the processes' trust assumption.
> >
> > Scripts may need some changes to deal with untrusted data (e.g. stdin,
> > environment variables), but that is outside the scope of the kernel.
> >
> > See chromeOS's documentation about script execution control and the
> > related threat model:
> > https://www.chromium.org/chromium-os/developer-library/guides/security/noexec-shell-scripts/
> >
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > Cc: Andy Lutomirski <luto@amacapital.net>
> > Cc: Christian Brauner <brauner@kernel.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Paul Moore <paul@paul-moore.com>
> > Cc: Serge Hallyn <serge@hallyn.com>
> > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > Link: https://lore.kernel.org/r/20241011184422.977903-3-mic@digikod.net
> > ---
> >
> > Changes since v19:
> > * Replace SECBIT_SHOULD_EXEC_CHECK and SECBIT_SHOULD_EXEC_RESTRICT with
> > SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE:
> > https://lore.kernel.org/all/20240710.eiKohpa4Phai@digikod.net/
> > * Remove the ptrace restrictions, suggested by Andy.
> > * Improve documentation according to the discussion with Jeff.
> >
> > New design since v18:
> > https://lore.kernel.org/r/20220104155024.48023-3-mic@digikod.net
> > ---
> > include/uapi/linux/securebits.h | 113 +++++++++++++++++++++++++++++++-
> > security/commoncap.c | 29 ++++++--
> > 2 files changed, 135 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/uapi/linux/securebits.h b/include/uapi/linux/securebits.h
> > index d6d98877ff1a..351b6ecefc76 100644
> > --- a/include/uapi/linux/securebits.h
> > +++ b/include/uapi/linux/securebits.h
> > @@ -52,10 +52,121 @@
> > #define SECBIT_NO_CAP_AMBIENT_RAISE_LOCKED \
> > (issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE_LOCKED))
> >
> > +/*
> > + * The SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE securebits
> > + * are intended for script interpreters and dynamic linkers to enforce a
> > + * consistent execution security policy handled by the kernel.
> > + *
> > + * Whether an interpreter should check these securebits or not depends on the
> > + * security risk of running malicious scripts with respect to the execution
> > + * environment, and whether the kernel can check if a script is trustworthy or
> > + * not. For instance, Python scripts running on a server can use arbitrary
> > + * syscalls and access arbitrary files. Such interpreters should then be
> > + * enlighten to use these securebits and let users define their security
> > + * policy. However, a JavaScript engine running in a web browser should
> > + * already be sandboxed and then should not be able to harm the user's
> > + * environment.
> > + *
> > + * When SECBIT_EXEC_RESTRICT_FILE is set, a process should only interpret or
> > + * execute a file if a call to execveat(2) with the related file descriptor and
> > + * the AT_CHECK flag succeed.
> > + *
> > + * This secure bit may be set by user session managers, service managers,
> > + * container runtimes, sandboxer tools... Except for test environments, the
> > + * related SECBIT_EXEC_RESTRICT_FILE_LOCKED bit should also be set.
> > + *
> > + * Programs should only enforce consistent restrictions according to the
> > + * securebits but without relying on any other user-controlled configuration.
> > + * Indeed, the use case for these securebits is to only trust executable code
> > + * vetted by the system configuration (through the kernel), so we should be
> > + * careful to not let untrusted users control this configuration.
> > + *
> > + * However, script interpreters may still use user configuration such as
> > + * environment variables as long as it is not a way to disable the securebits
> > + * checks. For instance, the PATH and LD_PRELOAD variables can be set by a
> > + * script's caller. Changing these variables may lead to unintended code
> > + * executions, but only from vetted executable programs, which is OK. For this
> > + * to make sense, the system should provide a consistent security policy to
> > + * avoid arbitrary code execution e.g., by enforcing a write xor execute
> > + * policy.
> > + *
> > + * SECBIT_EXEC_RESTRICT_FILE is complementary and should also be checked.
> > + */
> > +#define SECURE_EXEC_RESTRICT_FILE 8
> > +#define SECURE_EXEC_RESTRICT_FILE_LOCKED 9 /* make bit-8 immutable */
> > +
> > +#define SECBIT_EXEC_RESTRICT_FILE (issecure_mask(SECURE_EXEC_RESTRICT_FILE))
> > +#define SECBIT_EXEC_RESTRICT_FILE_LOCKED \
> > + (issecure_mask(SECURE_EXEC_RESTRICT_FILE_LOCKED))
> > +
> > +/*
> > + * When SECBIT_EXEC_DENY_INTERACTIVE is set, a process should never interpret
> > + * interactive user commands (e.g. scripts). However, if such commands are
> > + * passed through a file descriptor (e.g. stdin), its content should be
> > + * interpreted if a call to execveat(2) with the related file descriptor and
> > + * the AT_CHECK flag succeed.
> > + *
> > + * For instance, script interpreters called with a script snippet as argument
> > + * should always deny such execution if SECBIT_EXEC_DENY_INTERACTIVE is set.
> > + *
> > + * This secure bit may be set by user session managers, service managers,
> > + * container runtimes, sandboxer tools... Except for test environments, the
> > + * related SECBIT_EXEC_DENY_INTERACTIVE_LOCKED bit should also be set.
> > + *
> > + * See the SECBIT_EXEC_RESTRICT_FILE documentation.
> > + *
> > + * Here is the expected behavior for a script interpreter according to
> > + * combination of any exec securebits:
> > + *
> > + * 1. SECURE_EXEC_RESTRICT_FILE=0 SECURE_EXEC_DENY_INTERACTIVE=0 (default)
> > + * Always interpret scripts, and allow arbitrary user commands.
> > + * => No threat, everyone and everything is trusted, but we can get ahead of
> > + * potential issues thanks to the call to execveat with AT_CHECK which
> > + * should always be performed but ignored by the script interpreter.
> > + * Indeed, this check is still important to enable systems administrators
> > + * to verify requests (e.g. with audit) and prepare for migration to a
> > + * secure mode.
> > + *
> > + * 2. SECURE_EXEC_RESTRICT_FILE=1 SECURE_EXEC_DENY_INTERACTIVE=0
> > + * Deny script interpretation if they are not executable, but allow
> > + * arbitrary user commands.
> > + * => The threat is (potential) malicious scripts run by trusted (and not
> > + * fooled) users. That can protect against unintended script executions
> > + * (e.g. sh /tmp/*.sh). This makes sense for (semi-restricted) user
> > + * sessions.
> > + *
> > + * 3. SECURE_EXEC_RESTRICT_FILE=0 SECURE_EXEC_DENY_INTERACTIVE=1
> > + * Always interpret scripts, but deny arbitrary user commands.
> > + * => This use case may be useful for secure services (i.e. without
> > + * interactive user session) where scripts' integrity is verified (e.g.
> > + * with IMA/EVM or dm-verity/IPE) but where access rights might not be
> > + * ready yet. Indeed, arbitrary interactive commands would be much more
> > + * difficult to check.
> > + *
> > + * 4. SECURE_EXEC_RESTRICT_FILE=1 SECURE_EXEC_DENY_INTERACTIVE=1
> > + * Deny script interpretation if they are not executable, and also deny
> > + * any arbitrary user commands.
> > + * => The threat is malicious scripts run by untrusted users (but trusted
> > + * code). This makes sense for system services that may only execute
> > + * trusted scripts.
> > + */
> > +#define SECURE_EXEC_DENY_INTERACTIVE 10
> > +#define SECURE_EXEC_DENY_INTERACTIVE_LOCKED 11 /* make bit-10 immutable */
> > +
> > +#define SECBIT_EXEC_DENY_INTERACTIVE \
> > + (issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> > +#define SECBIT_EXEC_DENY_INTERACTIVE_LOCKED \
> > + (issecure_mask(SECURE_EXEC_DENY_INTERACTIVE_LOCKED))
> > +
> > #define SECURE_ALL_BITS (issecure_mask(SECURE_NOROOT) | \
> > issecure_mask(SECURE_NO_SETUID_FIXUP) | \
> > issecure_mask(SECURE_KEEP_CAPS) | \
> > - issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE))
> > + issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE) | \
> > + issecure_mask(SECURE_EXEC_RESTRICT_FILE) | \
> > + issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> > #define SECURE_ALL_LOCKS (SECURE_ALL_BITS << 1)
> >
> > +#define SECURE_ALL_UNPRIVILEGED (issecure_mask(SECURE_EXEC_RESTRICT_FILE) | \
> > + issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> > +
> > #endif /* _UAPI_LINUX_SECUREBITS_H */
> > diff --git a/security/commoncap.c b/security/commoncap.c
> > index cefad323a0b1..52ea01acb453 100644
> > --- a/security/commoncap.c
> > +++ b/security/commoncap.c
> > @@ -1302,21 +1302,38 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
> > & (old->securebits ^ arg2)) /*[1]*/
> > || ((old->securebits & SECURE_ALL_LOCKS & ~arg2)) /*[2]*/
> > || (arg2 & ~(SECURE_ALL_LOCKS | SECURE_ALL_BITS)) /*[3]*/
> > - || (cap_capable(current_cred(),
> > - current_cred()->user_ns,
> > - CAP_SETPCAP,
> > - CAP_OPT_NONE) != 0) /*[4]*/
> > /*
> > * [1] no changing of bits that are locked
> > * [2] no unlocking of locks
> > * [3] no setting of unsupported bits
> > - * [4] doing anything requires privilege (go read about
> > - * the "sendmail capabilities bug")
> > */
> > )
> > /* cannot change a locked bit */
> > return -EPERM;
> >
> > + /*
> > + * Doing anything requires privilege (go read about the
> > + * "sendmail capabilities bug"), except for unprivileged bits.
> > + * Indeed, the SECURE_ALL_UNPRIVILEGED bits are not
> > + * restrictions enforced by the kernel but by user space on
> > + * itself.
> > + */
> > + if (cap_capable(current_cred(), current_cred()->user_ns,
> > + CAP_SETPCAP, CAP_OPT_NONE) != 0) {
> > + const unsigned long unpriv_and_locks =
> > + SECURE_ALL_UNPRIVILEGED |
> > + SECURE_ALL_UNPRIVILEGED << 1;
> > + const unsigned long changed = old->securebits ^ arg2;
> > +
> > + /* For legacy reason, denies non-change. */
> > + if (!changed)
> > + return -EPERM;
>
> This is odd to me. You say for legacy reasons, but, currently, calling
> PR_SET_SECUREBITS with no changes returns 0. So you may be breaking
> a lot of programs here, unless I'm mistaken.
When we call PR_SET_SECUREBITS with 0 (and if it was 0 too), it
currently goes through the capability check and return -EPERM if the
caller doesn't have CAP_SETCAP. This is tested with
TEST_F(secbits, legacy) in tools/testing/selftests/exec/check-exec.c
(patch 3/6).
>
> > +
> > + /* Denies privileged changes. */
> > + if (changed & ~unpriv_and_locks)
> > + return -EPERM;
> > + }
> > +
> > new = prepare_creds();
> > if (!new)
> > return -ENOMEM;
> > --
> > 2.46.1
>
^ permalink raw reply
* Re: [PATCH v20 1/6] exec: Add a new AT_CHECK flag to execveat(2)
From: Mickaël Salaün @ 2024-10-14 7:39 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Theodore Ts'o, Adhemerval Zanella Netto, Alejandro Colomar,
Aleksa Sarai, Andrew Morton, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Christian Heimes, Dmitry Vyukov, Elliott Hughes,
Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20241013030416.GA1056921@mail.hallyn.com>
On Sat, Oct 12, 2024 at 10:04:16PM -0500, Serge E. Hallyn wrote:
> On Fri, Oct 11, 2024 at 08:44:17PM +0200, Mickaël Salaün wrote:
> > Add a new AT_CHECK flag to execveat(2) to check if a file would be
>
> Apologies for both bikeshedding and missing earlier discussions.
>
> But AT_CHECK sounds quite generic. How about AT_EXEC_CHECK, or
> AT_CHECK_EXEC_CREDS? (I would suggest just AT_CHECK_CREDS since
> it's for use in execveat(2), but as it's an AT_ flag, it's
> probably worth being more precise).
As Amir pointed out, we need at least to use the AT_EXECVE_CHECK_
prefix, and I agree with the AT_EXECVE_CHECK name because it's about
checking the whole execve request, not sepcifically a "creds" part.
^ permalink raw reply
* Re: [PATCH v20 1/6] exec: Add a new AT_CHECK flag to execveat(2)
From: Mickaël Salaün @ 2024-10-14 7:39 UTC (permalink / raw)
To: Amir Goldstein
Cc: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Serge Hallyn, Theodore Ts'o, Adhemerval Zanella Netto,
Alejandro Colomar, Aleksa Sarai, Andrew Morton, Andy Lutomirski,
Arnd Bergmann, Casey Schaufler, Christian Heimes, Dmitry Vyukov,
Elliott Hughes, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <CAOQ4uxi502PNn8eyKt9BExXVhbpx04f0XTeLg771i6wPOrLadA@mail.gmail.com>
On Sun, Oct 13, 2024 at 11:25:11AM +0200, Amir Goldstein wrote:
> On Fri, Oct 11, 2024 at 8:45 PM Mickaël Salaün <mic@digikod.net> wrote:
> >
> > Add a new AT_CHECK flag to execveat(2) to check if a file would be
> > allowed for execution. The main use case is for script interpreters and
> > dynamic linkers to check execution permission according to the kernel's
> > security policy. Another use case is to add context to access logs e.g.,
> > which script (instead of interpreter) accessed a file. As any
> > executable code, scripts could also use this check [1].
> >
> > This is different from faccessat(2) + X_OK which only checks a subset of
> > access rights (i.e. inode permission and mount options for regular
> > files), but not the full context (e.g. all LSM access checks). The main
> > use case for access(2) is for SUID processes to (partially) check access
> > on behalf of their caller. The main use case for execveat(2) + AT_CHECK
> > is to check if a script execution would be allowed, according to all the
> > different restrictions in place. Because the use of AT_CHECK follows
> > the exact kernel semantic as for a real execution, user space gets the
> > same error codes.
> >
> > An interesting point of using execveat(2) instead of openat2(2) is that
> > it decouples the check from the enforcement. Indeed, the security check
> > can be logged (e.g. with audit) without blocking an execution
> > environment not yet ready to enforce a strict security policy.
> >
> > LSMs can control or log execution requests with
> > security_bprm_creds_for_exec(). However, to enforce a consistent and
> > complete access control (e.g. on binary's dependencies) LSMs should
> > restrict file executability, or mesure executed files, with
> > security_file_open() by checking file->f_flags & __FMODE_EXEC.
> >
> > Because AT_CHECK is dedicated to user space interpreters, it doesn't
> > make sense for the kernel to parse the checked files, look for
> > interpreters known to the kernel (e.g. ELF, shebang), and return ENOEXEC
> > if the format is unknown. Because of that, security_bprm_check() is
> > never called when AT_CHECK is used.
> >
> > It should be noted that script interpreters cannot directly use
> > execveat(2) (without this new AT_CHECK flag) because this could lead to
> > unexpected behaviors e.g., `python script.sh` could lead to Bash being
> > executed to interpret the script. Unlike the kernel, script
> > interpreters may just interpret the shebang as a simple comment, which
> > should not change for backward compatibility reasons.
> >
> > Because scripts or libraries files might not currently have the
> > executable permission set, or because we might want specific users to be
> > allowed to run arbitrary scripts, the following patch provides a dynamic
> > configuration mechanism with the SECBIT_EXEC_RESTRICT_FILE and
> > SECBIT_EXEC_DENY_INTERACTIVE securebits.
> >
> > This is a redesign of the CLIP OS 4's O_MAYEXEC:
> > https://github.com/clipos-archive/src_platform_clip-patches/blob/f5cb330d6b684752e403b4e41b39f7004d88e561/1901_open_mayexec.patch
> > This patch has been used for more than a decade with customized script
> > interpreters. Some examples can be found here:
> > https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC
> >
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > Cc: Christian Brauner <brauner@kernel.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Paul Moore <paul@paul-moore.com>
> > Cc: Serge Hallyn <serge@hallyn.com>
> > Link: https://docs.python.org/3/library/io.html#io.open_code [1]
> > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > Link: https://lore.kernel.org/r/20241011184422.977903-2-mic@digikod.net
> > ---
> >
> > Changes since v19:
> > * Remove mention of "role transition" as suggested by Andy.
> > * Highlight the difference between security_bprm_creds_for_exec() and
> > the __FMODE_EXEC check for LSMs (in commit message and LSM's hooks) as
> > discussed with Jeff.
> > * Improve documentation both in UAPI comments and kernel comments
> > (requested by Kees).
> >
> > New design since v18:
> > https://lore.kernel.org/r/20220104155024.48023-3-mic@digikod.net
> > ---
> > fs/exec.c | 18 ++++++++++++++++--
> > include/linux/binfmts.h | 7 ++++++-
> > include/uapi/linux/fcntl.h | 31 +++++++++++++++++++++++++++++++
> > kernel/audit.h | 1 +
> > kernel/auditsc.c | 1 +
> > security/security.c | 10 ++++++++++
> > 6 files changed, 65 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/exec.c b/fs/exec.c
> > index 6c53920795c2..163c659d9ae6 100644
> > --- a/fs/exec.c
> > +++ b/fs/exec.c
> > @@ -891,7 +891,7 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
> > .lookup_flags = LOOKUP_FOLLOW,
> > };
> >
> > - if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
> > + if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH | AT_CHECK)) != 0)
> > return ERR_PTR(-EINVAL);
> > if (flags & AT_SYMLINK_NOFOLLOW)
> > open_exec_flags.lookup_flags &= ~LOOKUP_FOLLOW;
> > @@ -1545,6 +1545,20 @@ static struct linux_binprm *alloc_bprm(int fd, struct filename *filename, int fl
> > }
> > bprm->interp = bprm->filename;
> >
> > + /*
> > + * At this point, security_file_open() has already been called (with
> > + * __FMODE_EXEC) and access control checks for AT_CHECK will stop just
> > + * after the security_bprm_creds_for_exec() call in bprm_execve().
> > + * Indeed, the kernel should not try to parse the content of the file
> > + * with exec_binprm() nor change the calling thread, which means that
> > + * the following security functions will be not called:
> > + * - security_bprm_check()
> > + * - security_bprm_creds_from_file()
> > + * - security_bprm_committing_creds()
> > + * - security_bprm_committed_creds()
> > + */
> > + bprm->is_check = !!(flags & AT_CHECK);
> > +
> > retval = bprm_mm_init(bprm);
> > if (!retval)
> > return bprm;
> > @@ -1839,7 +1853,7 @@ static int bprm_execve(struct linux_binprm *bprm)
> >
> > /* Set the unchanging part of bprm->cred */
> > retval = security_bprm_creds_for_exec(bprm);
> > - if (retval)
> > + if (retval || bprm->is_check)
> > goto out;
> >
> > retval = exec_binprm(bprm);
> > diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> > index e6c00e860951..8ff0eb3644a1 100644
> > --- a/include/linux/binfmts.h
> > +++ b/include/linux/binfmts.h
> > @@ -42,7 +42,12 @@ struct linux_binprm {
> > * Set when errors can no longer be returned to the
> > * original userspace.
> > */
> > - point_of_no_return:1;
> > + point_of_no_return:1,
> > + /*
> > + * Set by user space to check executability according to the
> > + * caller's environment.
> > + */
> > + is_check:1;
> > struct file *executable; /* Executable to pass to the interpreter */
> > struct file *interpreter;
> > struct file *file;
> > diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> > index 87e2dec79fea..e606815b1c5a 100644
> > --- a/include/uapi/linux/fcntl.h
> > +++ b/include/uapi/linux/fcntl.h
> > @@ -154,6 +154,37 @@
> > usable with open_by_handle_at(2). */
> > #define AT_HANDLE_MNT_ID_UNIQUE 0x001 /* Return the u64 unique mount ID. */
> >
> > +/*
> > + * AT_CHECK only performs a check on a regular file and returns 0 if execution
> > + * of this file would be allowed, ignoring the file format and then the related
> > + * interpreter dependencies (e.g. ELF libraries, script's shebang).
> > + *
> > + * Programs should always perform this check to apply kernel-level checks
> > + * against files that are not directly executed by the kernel but passed to a
> > + * user space interpreter instead. All files that contain executable code,
> > + * from the point of view of the interpreter, should be checked. However the
> > + * result of this check should only be enforced according to
> > + * SECBIT_EXEC_RESTRICT_FILE or SECBIT_EXEC_DENY_INTERACTIVE. See securebits.h
> > + * documentation and the samples/check-exec/inc.c example.
> > + *
> > + * The main purpose of this flag is to improve the security and consistency of
> > + * an execution environment to ensure that direct file execution (e.g.
> > + * `./script.sh`) and indirect file execution (e.g. `sh script.sh`) lead to the
> > + * same result. For instance, this can be used to check if a file is
> > + * trustworthy according to the caller's environment.
> > + *
> > + * In a secure environment, libraries and any executable dependencies should
> > + * also be checked. For instance, dynamic linking should make sure that all
> > + * libraries are allowed for execution to avoid trivial bypass (e.g. using
> > + * LD_PRELOAD). For such secure execution environment to make sense, only
> > + * trusted code should be executable, which also requires integrity guarantees.
> > + *
> > + * To avoid race conditions leading to time-of-check to time-of-use issues,
> > + * AT_CHECK should be used with AT_EMPTY_PATH to check against a file
> > + * descriptor instead of a path.
> > + */
>
> If you ask me, the very elaborate comment above belongs to execveat(2)
> man page and is way too verbose for a uapi header.
OK, but since this new flags raised a lot of questions, I guess a
dedicated Documentation/userspace-api/check-exec.rst file with thit
AT*_CHECK and the related securebits would be useful instead of the
related inlined documentation.
>
> > +#define AT_CHECK 0x10000
>
> Please see the comment "Per-syscall flags for the *at(2) family of syscalls."
> above. If this is a per-syscall flag please use one of the per-syscall
> flags, e.g.:
>
> /* Flags for execveat2(2) */
> #define AT_EXECVE_CHECK 0x0001 /* Only perform a check if
> execution would be allowed */
I missed this part, this prefix makes sense, thanks.
>
>
> Thanks,
> Amir.
^ permalink raw reply
* Re: [PATCH v20 1/6] exec: Add a new AT_CHECK flag to execveat(2)
From: Amir Goldstein @ 2024-10-13 9:25 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Serge Hallyn, Theodore Ts'o, Adhemerval Zanella Netto,
Alejandro Colomar, Aleksa Sarai, Andrew Morton, Andy Lutomirski,
Arnd Bergmann, Casey Schaufler, Christian Heimes, Dmitry Vyukov,
Elliott Hughes, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20241011184422.977903-2-mic@digikod.net>
On Fri, Oct 11, 2024 at 8:45 PM Mickaël Salaün <mic@digikod.net> wrote:
>
> Add a new AT_CHECK flag to execveat(2) to check if a file would be
> allowed for execution. The main use case is for script interpreters and
> dynamic linkers to check execution permission according to the kernel's
> security policy. Another use case is to add context to access logs e.g.,
> which script (instead of interpreter) accessed a file. As any
> executable code, scripts could also use this check [1].
>
> This is different from faccessat(2) + X_OK which only checks a subset of
> access rights (i.e. inode permission and mount options for regular
> files), but not the full context (e.g. all LSM access checks). The main
> use case for access(2) is for SUID processes to (partially) check access
> on behalf of their caller. The main use case for execveat(2) + AT_CHECK
> is to check if a script execution would be allowed, according to all the
> different restrictions in place. Because the use of AT_CHECK follows
> the exact kernel semantic as for a real execution, user space gets the
> same error codes.
>
> An interesting point of using execveat(2) instead of openat2(2) is that
> it decouples the check from the enforcement. Indeed, the security check
> can be logged (e.g. with audit) without blocking an execution
> environment not yet ready to enforce a strict security policy.
>
> LSMs can control or log execution requests with
> security_bprm_creds_for_exec(). However, to enforce a consistent and
> complete access control (e.g. on binary's dependencies) LSMs should
> restrict file executability, or mesure executed files, with
> security_file_open() by checking file->f_flags & __FMODE_EXEC.
>
> Because AT_CHECK is dedicated to user space interpreters, it doesn't
> make sense for the kernel to parse the checked files, look for
> interpreters known to the kernel (e.g. ELF, shebang), and return ENOEXEC
> if the format is unknown. Because of that, security_bprm_check() is
> never called when AT_CHECK is used.
>
> It should be noted that script interpreters cannot directly use
> execveat(2) (without this new AT_CHECK flag) because this could lead to
> unexpected behaviors e.g., `python script.sh` could lead to Bash being
> executed to interpret the script. Unlike the kernel, script
> interpreters may just interpret the shebang as a simple comment, which
> should not change for backward compatibility reasons.
>
> Because scripts or libraries files might not currently have the
> executable permission set, or because we might want specific users to be
> allowed to run arbitrary scripts, the following patch provides a dynamic
> configuration mechanism with the SECBIT_EXEC_RESTRICT_FILE and
> SECBIT_EXEC_DENY_INTERACTIVE securebits.
>
> This is a redesign of the CLIP OS 4's O_MAYEXEC:
> https://github.com/clipos-archive/src_platform_clip-patches/blob/f5cb330d6b684752e403b4e41b39f7004d88e561/1901_open_mayexec.patch
> This patch has been used for more than a decade with customized script
> interpreters. Some examples can be found here:
> https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC
>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Serge Hallyn <serge@hallyn.com>
> Link: https://docs.python.org/3/library/io.html#io.open_code [1]
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Link: https://lore.kernel.org/r/20241011184422.977903-2-mic@digikod.net
> ---
>
> Changes since v19:
> * Remove mention of "role transition" as suggested by Andy.
> * Highlight the difference between security_bprm_creds_for_exec() and
> the __FMODE_EXEC check for LSMs (in commit message and LSM's hooks) as
> discussed with Jeff.
> * Improve documentation both in UAPI comments and kernel comments
> (requested by Kees).
>
> New design since v18:
> https://lore.kernel.org/r/20220104155024.48023-3-mic@digikod.net
> ---
> fs/exec.c | 18 ++++++++++++++++--
> include/linux/binfmts.h | 7 ++++++-
> include/uapi/linux/fcntl.h | 31 +++++++++++++++++++++++++++++++
> kernel/audit.h | 1 +
> kernel/auditsc.c | 1 +
> security/security.c | 10 ++++++++++
> 6 files changed, 65 insertions(+), 3 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 6c53920795c2..163c659d9ae6 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -891,7 +891,7 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
> .lookup_flags = LOOKUP_FOLLOW,
> };
>
> - if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
> + if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH | AT_CHECK)) != 0)
> return ERR_PTR(-EINVAL);
> if (flags & AT_SYMLINK_NOFOLLOW)
> open_exec_flags.lookup_flags &= ~LOOKUP_FOLLOW;
> @@ -1545,6 +1545,20 @@ static struct linux_binprm *alloc_bprm(int fd, struct filename *filename, int fl
> }
> bprm->interp = bprm->filename;
>
> + /*
> + * At this point, security_file_open() has already been called (with
> + * __FMODE_EXEC) and access control checks for AT_CHECK will stop just
> + * after the security_bprm_creds_for_exec() call in bprm_execve().
> + * Indeed, the kernel should not try to parse the content of the file
> + * with exec_binprm() nor change the calling thread, which means that
> + * the following security functions will be not called:
> + * - security_bprm_check()
> + * - security_bprm_creds_from_file()
> + * - security_bprm_committing_creds()
> + * - security_bprm_committed_creds()
> + */
> + bprm->is_check = !!(flags & AT_CHECK);
> +
> retval = bprm_mm_init(bprm);
> if (!retval)
> return bprm;
> @@ -1839,7 +1853,7 @@ static int bprm_execve(struct linux_binprm *bprm)
>
> /* Set the unchanging part of bprm->cred */
> retval = security_bprm_creds_for_exec(bprm);
> - if (retval)
> + if (retval || bprm->is_check)
> goto out;
>
> retval = exec_binprm(bprm);
> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> index e6c00e860951..8ff0eb3644a1 100644
> --- a/include/linux/binfmts.h
> +++ b/include/linux/binfmts.h
> @@ -42,7 +42,12 @@ struct linux_binprm {
> * Set when errors can no longer be returned to the
> * original userspace.
> */
> - point_of_no_return:1;
> + point_of_no_return:1,
> + /*
> + * Set by user space to check executability according to the
> + * caller's environment.
> + */
> + is_check:1;
> struct file *executable; /* Executable to pass to the interpreter */
> struct file *interpreter;
> struct file *file;
> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> index 87e2dec79fea..e606815b1c5a 100644
> --- a/include/uapi/linux/fcntl.h
> +++ b/include/uapi/linux/fcntl.h
> @@ -154,6 +154,37 @@
> usable with open_by_handle_at(2). */
> #define AT_HANDLE_MNT_ID_UNIQUE 0x001 /* Return the u64 unique mount ID. */
>
> +/*
> + * AT_CHECK only performs a check on a regular file and returns 0 if execution
> + * of this file would be allowed, ignoring the file format and then the related
> + * interpreter dependencies (e.g. ELF libraries, script's shebang).
> + *
> + * Programs should always perform this check to apply kernel-level checks
> + * against files that are not directly executed by the kernel but passed to a
> + * user space interpreter instead. All files that contain executable code,
> + * from the point of view of the interpreter, should be checked. However the
> + * result of this check should only be enforced according to
> + * SECBIT_EXEC_RESTRICT_FILE or SECBIT_EXEC_DENY_INTERACTIVE. See securebits.h
> + * documentation and the samples/check-exec/inc.c example.
> + *
> + * The main purpose of this flag is to improve the security and consistency of
> + * an execution environment to ensure that direct file execution (e.g.
> + * `./script.sh`) and indirect file execution (e.g. `sh script.sh`) lead to the
> + * same result. For instance, this can be used to check if a file is
> + * trustworthy according to the caller's environment.
> + *
> + * In a secure environment, libraries and any executable dependencies should
> + * also be checked. For instance, dynamic linking should make sure that all
> + * libraries are allowed for execution to avoid trivial bypass (e.g. using
> + * LD_PRELOAD). For such secure execution environment to make sense, only
> + * trusted code should be executable, which also requires integrity guarantees.
> + *
> + * To avoid race conditions leading to time-of-check to time-of-use issues,
> + * AT_CHECK should be used with AT_EMPTY_PATH to check against a file
> + * descriptor instead of a path.
> + */
If you ask me, the very elaborate comment above belongs to execveat(2)
man page and is way too verbose for a uapi header.
> +#define AT_CHECK 0x10000
Please see the comment "Per-syscall flags for the *at(2) family of syscalls."
above. If this is a per-syscall flag please use one of the per-syscall
flags, e.g.:
/* Flags for execveat2(2) */
#define AT_EXECVE_CHECK 0x0001 /* Only perform a check if
execution would be allowed */
Thanks,
Amir.
^ permalink raw reply
* Re: [PATCH v20 1/6] exec: Add a new AT_CHECK flag to execveat(2)
From: Serge E. Hallyn @ 2024-10-13 3:04 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Serge Hallyn, Theodore Ts'o, Adhemerval Zanella Netto,
Alejandro Colomar, Aleksa Sarai, Andrew Morton, Andy Lutomirski,
Arnd Bergmann, Casey Schaufler, Christian Heimes, Dmitry Vyukov,
Elliott Hughes, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20241011184422.977903-2-mic@digikod.net>
On Fri, Oct 11, 2024 at 08:44:17PM +0200, Mickaël Salaün wrote:
> Add a new AT_CHECK flag to execveat(2) to check if a file would be
Apologies for both bikeshedding and missing earlier discussions.
But AT_CHECK sounds quite generic. How about AT_EXEC_CHECK, or
AT_CHECK_EXEC_CREDS? (I would suggest just AT_CHECK_CREDS since
it's for use in execveat(2), but as it's an AT_ flag, it's
probably worth being more precise).
^ permalink raw reply
* Re: [PATCH v20 2/6] security: Add EXEC_RESTRICT_FILE and EXEC_DENY_INTERACTIVE securebits
From: Serge E. Hallyn @ 2024-10-13 2:51 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Serge Hallyn, Theodore Ts'o, Adhemerval Zanella Netto,
Alejandro Colomar, Aleksa Sarai, Andrew Morton, Andy Lutomirski,
Arnd Bergmann, Casey Schaufler, Christian Heimes, Dmitry Vyukov,
Elliott Hughes, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module, Andy Lutomirski
In-Reply-To: <20241011184422.977903-3-mic@digikod.net>
On Fri, Oct 11, 2024 at 08:44:18PM +0200, Mickaël Salaün wrote:
> The new SECBIT_EXEC_RESTRICT_FILE, SECBIT_EXEC_DENY_INTERACTIVE, and
> their *_LOCKED counterparts are designed to be set by processes setting
> up an execution environment, such as a user session, a container, or a
> security sandbox. Unlike other securebits, these ones can be set by
> unprivileged processes. Like seccomp filters or Landlock domains, the
> securebits are inherited across processes.
>
> When SECBIT_EXEC_RESTRICT_FILE is set, programs interpreting code should
> control executable resources according to execveat(2) + AT_CHECK (see
> previous commit).
>
> When SECBIT_EXEC_DENY_INTERACTIVE is set, a process should deny
> execution of user interactive commands (which excludes executable
> regular files).
>
> Being able to configure each of these securebits enables system
> administrators or owner of image containers to gradually validate the
> related changes and to identify potential issues (e.g. with interpreter
> or audit logs).
>
> It should be noted that unlike other security bits, the
> SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE bits are
> dedicated to user space willing to restrict itself. Because of that,
> they only make sense in the context of a trusted environment (e.g.
> sandbox, container, user session, full system) where the process
> changing its behavior (according to these bits) and all its parent
> processes are trusted. Otherwise, any parent process could just execute
> its own malicious code (interpreting a script or not), or even enforce a
> seccomp filter to mask these bits.
>
> Such a secure environment can be achieved with an appropriate access
> control (e.g. mount's noexec option, file access rights, LSM policy) and
> an enlighten ld.so checking that libraries are allowed for execution
> e.g., to protect against illegitimate use of LD_PRELOAD.
>
> Ptrace restrictions according to these securebits would not make sense
> because of the processes' trust assumption.
>
> Scripts may need some changes to deal with untrusted data (e.g. stdin,
> environment variables), but that is outside the scope of the kernel.
>
> See chromeOS's documentation about script execution control and the
> related threat model:
> https://www.chromium.org/chromium-os/developer-library/guides/security/noexec-shell-scripts/
>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Serge Hallyn <serge@hallyn.com>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Link: https://lore.kernel.org/r/20241011184422.977903-3-mic@digikod.net
> ---
>
> Changes since v19:
> * Replace SECBIT_SHOULD_EXEC_CHECK and SECBIT_SHOULD_EXEC_RESTRICT with
> SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE:
> https://lore.kernel.org/all/20240710.eiKohpa4Phai@digikod.net/
> * Remove the ptrace restrictions, suggested by Andy.
> * Improve documentation according to the discussion with Jeff.
>
> New design since v18:
> https://lore.kernel.org/r/20220104155024.48023-3-mic@digikod.net
> ---
> include/uapi/linux/securebits.h | 113 +++++++++++++++++++++++++++++++-
> security/commoncap.c | 29 ++++++--
> 2 files changed, 135 insertions(+), 7 deletions(-)
>
> diff --git a/include/uapi/linux/securebits.h b/include/uapi/linux/securebits.h
> index d6d98877ff1a..351b6ecefc76 100644
> --- a/include/uapi/linux/securebits.h
> +++ b/include/uapi/linux/securebits.h
> @@ -52,10 +52,121 @@
> #define SECBIT_NO_CAP_AMBIENT_RAISE_LOCKED \
> (issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE_LOCKED))
>
> +/*
> + * The SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE securebits
> + * are intended for script interpreters and dynamic linkers to enforce a
> + * consistent execution security policy handled by the kernel.
> + *
> + * Whether an interpreter should check these securebits or not depends on the
> + * security risk of running malicious scripts with respect to the execution
> + * environment, and whether the kernel can check if a script is trustworthy or
> + * not. For instance, Python scripts running on a server can use arbitrary
> + * syscalls and access arbitrary files. Such interpreters should then be
> + * enlighten to use these securebits and let users define their security
> + * policy. However, a JavaScript engine running in a web browser should
> + * already be sandboxed and then should not be able to harm the user's
> + * environment.
> + *
> + * When SECBIT_EXEC_RESTRICT_FILE is set, a process should only interpret or
> + * execute a file if a call to execveat(2) with the related file descriptor and
> + * the AT_CHECK flag succeed.
> + *
> + * This secure bit may be set by user session managers, service managers,
> + * container runtimes, sandboxer tools... Except for test environments, the
> + * related SECBIT_EXEC_RESTRICT_FILE_LOCKED bit should also be set.
> + *
> + * Programs should only enforce consistent restrictions according to the
> + * securebits but without relying on any other user-controlled configuration.
> + * Indeed, the use case for these securebits is to only trust executable code
> + * vetted by the system configuration (through the kernel), so we should be
> + * careful to not let untrusted users control this configuration.
> + *
> + * However, script interpreters may still use user configuration such as
> + * environment variables as long as it is not a way to disable the securebits
> + * checks. For instance, the PATH and LD_PRELOAD variables can be set by a
> + * script's caller. Changing these variables may lead to unintended code
> + * executions, but only from vetted executable programs, which is OK. For this
> + * to make sense, the system should provide a consistent security policy to
> + * avoid arbitrary code execution e.g., by enforcing a write xor execute
> + * policy.
> + *
> + * SECBIT_EXEC_RESTRICT_FILE is complementary and should also be checked.
> + */
> +#define SECURE_EXEC_RESTRICT_FILE 8
> +#define SECURE_EXEC_RESTRICT_FILE_LOCKED 9 /* make bit-8 immutable */
> +
> +#define SECBIT_EXEC_RESTRICT_FILE (issecure_mask(SECURE_EXEC_RESTRICT_FILE))
> +#define SECBIT_EXEC_RESTRICT_FILE_LOCKED \
> + (issecure_mask(SECURE_EXEC_RESTRICT_FILE_LOCKED))
> +
> +/*
> + * When SECBIT_EXEC_DENY_INTERACTIVE is set, a process should never interpret
> + * interactive user commands (e.g. scripts). However, if such commands are
> + * passed through a file descriptor (e.g. stdin), its content should be
> + * interpreted if a call to execveat(2) with the related file descriptor and
> + * the AT_CHECK flag succeed.
> + *
> + * For instance, script interpreters called with a script snippet as argument
> + * should always deny such execution if SECBIT_EXEC_DENY_INTERACTIVE is set.
> + *
> + * This secure bit may be set by user session managers, service managers,
> + * container runtimes, sandboxer tools... Except for test environments, the
> + * related SECBIT_EXEC_DENY_INTERACTIVE_LOCKED bit should also be set.
> + *
> + * See the SECBIT_EXEC_RESTRICT_FILE documentation.
> + *
> + * Here is the expected behavior for a script interpreter according to
> + * combination of any exec securebits:
> + *
> + * 1. SECURE_EXEC_RESTRICT_FILE=0 SECURE_EXEC_DENY_INTERACTIVE=0 (default)
> + * Always interpret scripts, and allow arbitrary user commands.
> + * => No threat, everyone and everything is trusted, but we can get ahead of
> + * potential issues thanks to the call to execveat with AT_CHECK which
> + * should always be performed but ignored by the script interpreter.
> + * Indeed, this check is still important to enable systems administrators
> + * to verify requests (e.g. with audit) and prepare for migration to a
> + * secure mode.
> + *
> + * 2. SECURE_EXEC_RESTRICT_FILE=1 SECURE_EXEC_DENY_INTERACTIVE=0
> + * Deny script interpretation if they are not executable, but allow
> + * arbitrary user commands.
> + * => The threat is (potential) malicious scripts run by trusted (and not
> + * fooled) users. That can protect against unintended script executions
> + * (e.g. sh /tmp/*.sh). This makes sense for (semi-restricted) user
> + * sessions.
> + *
> + * 3. SECURE_EXEC_RESTRICT_FILE=0 SECURE_EXEC_DENY_INTERACTIVE=1
> + * Always interpret scripts, but deny arbitrary user commands.
> + * => This use case may be useful for secure services (i.e. without
> + * interactive user session) where scripts' integrity is verified (e.g.
> + * with IMA/EVM or dm-verity/IPE) but where access rights might not be
> + * ready yet. Indeed, arbitrary interactive commands would be much more
> + * difficult to check.
> + *
> + * 4. SECURE_EXEC_RESTRICT_FILE=1 SECURE_EXEC_DENY_INTERACTIVE=1
> + * Deny script interpretation if they are not executable, and also deny
> + * any arbitrary user commands.
> + * => The threat is malicious scripts run by untrusted users (but trusted
> + * code). This makes sense for system services that may only execute
> + * trusted scripts.
> + */
> +#define SECURE_EXEC_DENY_INTERACTIVE 10
> +#define SECURE_EXEC_DENY_INTERACTIVE_LOCKED 11 /* make bit-10 immutable */
> +
> +#define SECBIT_EXEC_DENY_INTERACTIVE \
> + (issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> +#define SECBIT_EXEC_DENY_INTERACTIVE_LOCKED \
> + (issecure_mask(SECURE_EXEC_DENY_INTERACTIVE_LOCKED))
> +
> #define SECURE_ALL_BITS (issecure_mask(SECURE_NOROOT) | \
> issecure_mask(SECURE_NO_SETUID_FIXUP) | \
> issecure_mask(SECURE_KEEP_CAPS) | \
> - issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE))
> + issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE) | \
> + issecure_mask(SECURE_EXEC_RESTRICT_FILE) | \
> + issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> #define SECURE_ALL_LOCKS (SECURE_ALL_BITS << 1)
>
> +#define SECURE_ALL_UNPRIVILEGED (issecure_mask(SECURE_EXEC_RESTRICT_FILE) | \
> + issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> +
> #endif /* _UAPI_LINUX_SECUREBITS_H */
> diff --git a/security/commoncap.c b/security/commoncap.c
> index cefad323a0b1..52ea01acb453 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -1302,21 +1302,38 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
> & (old->securebits ^ arg2)) /*[1]*/
> || ((old->securebits & SECURE_ALL_LOCKS & ~arg2)) /*[2]*/
> || (arg2 & ~(SECURE_ALL_LOCKS | SECURE_ALL_BITS)) /*[3]*/
> - || (cap_capable(current_cred(),
> - current_cred()->user_ns,
> - CAP_SETPCAP,
> - CAP_OPT_NONE) != 0) /*[4]*/
> /*
> * [1] no changing of bits that are locked
> * [2] no unlocking of locks
> * [3] no setting of unsupported bits
> - * [4] doing anything requires privilege (go read about
> - * the "sendmail capabilities bug")
> */
> )
> /* cannot change a locked bit */
> return -EPERM;
>
> + /*
> + * Doing anything requires privilege (go read about the
> + * "sendmail capabilities bug"), except for unprivileged bits.
> + * Indeed, the SECURE_ALL_UNPRIVILEGED bits are not
> + * restrictions enforced by the kernel but by user space on
> + * itself.
> + */
> + if (cap_capable(current_cred(), current_cred()->user_ns,
> + CAP_SETPCAP, CAP_OPT_NONE) != 0) {
> + const unsigned long unpriv_and_locks =
> + SECURE_ALL_UNPRIVILEGED |
> + SECURE_ALL_UNPRIVILEGED << 1;
> + const unsigned long changed = old->securebits ^ arg2;
> +
> + /* For legacy reason, denies non-change. */
> + if (!changed)
> + return -EPERM;
This is odd to me. You say for legacy reasons, but, currently, calling
PR_SET_SECUREBITS with no changes returns 0. So you may be breaking
a lot of programs here, unless I'm mistaken.
> +
> + /* Denies privileged changes. */
> + if (changed & ~unpriv_and_locks)
> + return -EPERM;
> + }
> +
> new = prepare_creds();
> if (!new)
> return -ENOMEM;
> --
> 2.46.1
^ permalink raw reply
* [PATCH v20 6/6] samples/check-exec: Add an enlighten "inc" interpreter and 28 tests
From: Mickaël Salaün @ 2024-10-11 18:44 UTC (permalink / raw)
To: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Serge Hallyn, Theodore Ts'o
Cc: Mickaël Salaün, Adhemerval Zanella Netto,
Alejandro Colomar, Aleksa Sarai, Andrew Morton, Andy Lutomirski,
Arnd Bergmann, Casey Schaufler, Christian Heimes, Dmitry Vyukov,
Elliott Hughes, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20241011184422.977903-1-mic@digikod.net>
Add a very simple script interpreter called "inc" that can evaluate two
different commands (one per line):
- "?" to initialize a counter from user's input;
- "+" to increment the counter (which is set to 0 by default).
It is enlighten to only interpret executable files according to AT_CHECK
and the related securebits:
# Executing a script with RESTRICT_FILE is only allowed if the script
# is exectuable:
./set-exec -f -- ./inc script-exec.inc # Allowed
./set-exec -f -- ./inc script-noexec.inc # Denied
# Executing stdin with DENY_INTERACTIVE is only allowed if stdin is an
# executable regular file:
./set-exec -i -- ./inc -i < script-exec.inc # Allowed
./set-exec -i -- ./inc -i < script-noexec.inc # Denied
# However, a pipe is not executable and it is then denied:
cat script-noexec.inc | ./set-exec -i -- ./inc -i # Denied
# Executing raw data (e.g. command argument) with DENY_INTERACTIVE is
# always denied.
./set-exec -i -- ./inc -c "+" # Denied
./inc -c "$(<script-ask.inc)" # Allowed
# To directly execute a script, we can update $PATH (used by `env`):
PATH="${PATH}:." ./script-exec.inc
# To execute several commands passed as argument:
Add a complete test suite to check the script interpreter against all
possible execution cases:
make TARGETS=exec kselftest-install
./tools/testing/selftests/kselftest_install/run_kselftest.sh
Fix ktap_helpers.sh to gracefully ignore optional argument.
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20241011184422.977903-7-mic@digikod.net
---
Changes since v19:
* New patch.
---
samples/Kconfig | 3 +-
samples/check-exec/.gitignore | 1 +
samples/check-exec/Makefile | 1 +
samples/check-exec/inc.c | 204 +++++++++++++++++
samples/check-exec/run-script-ask.inc | 8 +
samples/check-exec/script-ask.inc | 4 +
samples/check-exec/script-exec.inc | 3 +
samples/check-exec/script-noexec.inc | 3 +
tools/testing/selftests/exec/.gitignore | 2 +
tools/testing/selftests/exec/Makefile | 14 +-
.../selftests/exec/check-exec-tests.sh | 205 ++++++++++++++++++
.../selftests/kselftest/ktap_helpers.sh | 2 +-
12 files changed, 446 insertions(+), 4 deletions(-)
create mode 100644 samples/check-exec/inc.c
create mode 100755 samples/check-exec/run-script-ask.inc
create mode 100755 samples/check-exec/script-ask.inc
create mode 100755 samples/check-exec/script-exec.inc
create mode 100644 samples/check-exec/script-noexec.inc
create mode 100755 tools/testing/selftests/exec/check-exec-tests.sh
diff --git a/samples/Kconfig b/samples/Kconfig
index efa28ceadc42..0128cc68deed 100644
--- a/samples/Kconfig
+++ b/samples/Kconfig
@@ -296,7 +296,8 @@ config SAMPLE_CHECK_EXEC
depends on CC_CAN_LINK && HEADERS_INSTALL
help
Build a tool to easily configure SECBIT_EXEC_RESTRICT_FILE and
- SECBIT_EXEC_DENY_INTERACTIVE.
+ SECBIT_EXEC_DENY_INTERACTIVE, and a simple script interpreter to
+ demonstrate how they should be used with execveat(2) + AT_CHECK.
source "samples/rust/Kconfig"
diff --git a/samples/check-exec/.gitignore b/samples/check-exec/.gitignore
index 3f8119112ccf..cd759a19dacd 100644
--- a/samples/check-exec/.gitignore
+++ b/samples/check-exec/.gitignore
@@ -1 +1,2 @@
+/inc
/set-exec
diff --git a/samples/check-exec/Makefile b/samples/check-exec/Makefile
index d9f976e3ff98..c4f08ad0f8e3 100644
--- a/samples/check-exec/Makefile
+++ b/samples/check-exec/Makefile
@@ -1,6 +1,7 @@
# SPDX-License-Identifier: BSD-3-Clause
userprogs-always-y := \
+ inc \
set-exec
userccflags += -I usr/include
diff --git a/samples/check-exec/inc.c b/samples/check-exec/inc.c
new file mode 100644
index 000000000000..0710a860f360
--- /dev/null
+++ b/samples/check-exec/inc.c
@@ -0,0 +1,204 @@
+// SPDX-License-Identifier: BSD-3-Clause
+/*
+ * Very simple script interpreter that can evaluate two different commands (one
+ * per line):
+ * - "?" to initialize a counter from user's input;
+ * - "+" to increment the counter (which is set to 0 by default).
+ *
+ * See tools/testing/selftests/exec/check-exec-tests.sh
+ *
+ * Copyright © 2024 Microsoft Corporation
+ */
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <linux/fcntl.h>
+#include <linux/prctl.h>
+#include <linux/securebits.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/prctl.h>
+#include <unistd.h>
+
+/* Returns 1 on error, 0 otherwise. */
+static int interpret_buffer(char *buffer, size_t buffer_size)
+{
+ char *line, *saveptr = NULL;
+ long long number = 0;
+
+ /* Each command is the first character of a line. */
+ saveptr = NULL;
+ line = strtok_r(buffer, "\n", &saveptr);
+ while (line) {
+ if (*line != '#' && strlen(line) != 1) {
+ fprintf(stderr, "# ERROR: Unknown string\n");
+ return 1;
+ }
+ switch (*line) {
+ case '#':
+ /* Skips shebang and comments. */
+ break;
+ case '+':
+ /* Increments and prints the number. */
+ number++;
+ printf("%lld\n", number);
+ break;
+ case '?':
+ /* Reads integer from stdin. */
+ fprintf(stderr, "> Enter new number: \n");
+ if (scanf("%lld", &number) != 1) {
+ fprintf(stderr,
+ "# WARNING: Failed to read number from stdin\n");
+ }
+ break;
+ default:
+ fprintf(stderr, "# ERROR: Unknown character '%c'\n",
+ *line);
+ return 1;
+ }
+ line = strtok_r(NULL, "\n", &saveptr);
+ }
+ return 0;
+}
+
+/* Returns 1 on error, 0 otherwise. */
+static int interpret_stream(FILE *script, char *const script_name,
+ char *const *const envp, const bool restrict_stream)
+{
+ int err;
+ char *const script_argv[] = { script_name, NULL };
+ char buf[128] = {};
+ size_t buf_size = sizeof(buf);
+
+ /*
+ * We pass a valid argv and envp to the kernel to emulate a native
+ * script execution. We must use the script file descriptor instead of
+ * the script path name to avoid race conditions.
+ */
+ err = execveat(fileno(script), "", script_argv, envp,
+ AT_EMPTY_PATH | AT_CHECK);
+ if (err && restrict_stream) {
+ perror("ERROR: Script execution check");
+ return 1;
+ }
+
+ /* Reads script. */
+ buf_size = fread(buf, 1, buf_size - 1, script);
+ return interpret_buffer(buf, buf_size);
+}
+
+static void print_usage(const char *argv0)
+{
+ fprintf(stderr, "usage: %s <script.inc> | -i | -c <command>\n\n",
+ argv0);
+ fprintf(stderr, "Example:\n");
+ fprintf(stderr, " ./set-exec -fi -- ./inc -i < script-exec.inc\n");
+}
+
+int main(const int argc, char *const argv[], char *const *const envp)
+{
+ int opt;
+ char *cmd = NULL;
+ char *script_name = NULL;
+ bool interpret_stdin = false;
+ FILE *script_file = NULL;
+ int secbits;
+ bool deny_interactive, restrict_file;
+ size_t arg_nb;
+
+ secbits = prctl(PR_GET_SECUREBITS);
+ if (secbits == -1) {
+ /*
+ * This should never happen, except with a buggy seccomp
+ * filter.
+ */
+ perror("ERROR: Failed to get securebits");
+ return 1;
+ }
+
+ deny_interactive = !!(secbits & SECBIT_EXEC_DENY_INTERACTIVE);
+ restrict_file = !!(secbits & SECBIT_EXEC_RESTRICT_FILE);
+
+ while ((opt = getopt(argc, argv, "c:i")) != -1) {
+ switch (opt) {
+ case 'c':
+ if (cmd) {
+ fprintf(stderr, "ERROR: Command already set");
+ return 1;
+ }
+ cmd = optarg;
+ break;
+ case 'i':
+ interpret_stdin = true;
+ break;
+ default:
+ print_usage(argv[0]);
+ return 1;
+ }
+ }
+
+ /* Checks that only one argument is used, or read stdin. */
+ arg_nb = !!cmd + !!interpret_stdin;
+ if (arg_nb == 0 && argc == 2) {
+ script_name = argv[1];
+ } else if (arg_nb != 1) {
+ print_usage(argv[0]);
+ return 1;
+ }
+
+ if (cmd) {
+ /*
+ * Other kind of interactive interpretations should be denied
+ * as well (e.g. CLI arguments passing script snippets,
+ * environment variables interpreted as script). However, any
+ * way to pass script files should only be restricted according
+ * to restrict_file.
+ */
+ if (deny_interactive) {
+ fprintf(stderr,
+ "ERROR: Interactive interpretation denied.\n");
+ return 1;
+ }
+
+ return interpret_buffer(cmd, strlen(cmd));
+ }
+
+ if (interpret_stdin && !script_name) {
+ script_file = stdin;
+ /*
+ * As for any execve(2) call, this path may be logged by the
+ * kernel.
+ */
+ script_name = "/proc/self/fd/0";
+ /*
+ * When stdin is used, it can point to a regular file or a
+ * pipe. Restrict stdin execution according to
+ * SECBIT_EXEC_DENY_INTERACTIVE but always allow executable
+ * files (which are not considered as interactive inputs).
+ */
+ return interpret_stream(script_file, script_name, envp,
+ deny_interactive);
+ } else if (script_name && !interpret_stdin) {
+ /*
+ * In this sample, we don't pass any argument to scripts, but
+ * otherwise we would have to forge an argv with such
+ * arguments.
+ */
+ script_file = fopen(script_name, "r");
+ if (!script_file) {
+ perror("ERROR: Failed to open script");
+ return 1;
+ }
+ /*
+ * Restricts file execution according to
+ * SECBIT_EXEC_RESTRICT_FILE.
+ */
+ return interpret_stream(script_file, script_name, envp,
+ restrict_file);
+ }
+
+ print_usage(argv[0]);
+ return 1;
+}
diff --git a/samples/check-exec/run-script-ask.inc b/samples/check-exec/run-script-ask.inc
new file mode 100755
index 000000000000..3ea3e15fbd5a
--- /dev/null
+++ b/samples/check-exec/run-script-ask.inc
@@ -0,0 +1,8 @@
+#!/usr/bin/env sh
+
+DIR="$(dirname -- "$0")"
+
+PATH="${PATH}:${DIR}"
+
+set -x
+"${DIR}/script-ask.inc"
diff --git a/samples/check-exec/script-ask.inc b/samples/check-exec/script-ask.inc
new file mode 100755
index 000000000000..f48252ab07c1
--- /dev/null
+++ b/samples/check-exec/script-ask.inc
@@ -0,0 +1,4 @@
+#!/usr/bin/env inc
+
+?
++
diff --git a/samples/check-exec/script-exec.inc b/samples/check-exec/script-exec.inc
new file mode 100755
index 000000000000..525e958e1c20
--- /dev/null
+++ b/samples/check-exec/script-exec.inc
@@ -0,0 +1,3 @@
+#!/usr/bin/env inc
+
++
diff --git a/samples/check-exec/script-noexec.inc b/samples/check-exec/script-noexec.inc
new file mode 100644
index 000000000000..525e958e1c20
--- /dev/null
+++ b/samples/check-exec/script-noexec.inc
@@ -0,0 +1,3 @@
+#!/usr/bin/env inc
+
++
diff --git a/tools/testing/selftests/exec/.gitignore b/tools/testing/selftests/exec/.gitignore
index a32c63bb4df1..7f3d1ae762ec 100644
--- a/tools/testing/selftests/exec/.gitignore
+++ b/tools/testing/selftests/exec/.gitignore
@@ -11,9 +11,11 @@ non-regular
null-argv
/check-exec
/false
+/inc
/load_address.*
!load_address.c
/recursion-depth
+/set-exec
xxxxxxxx*
pipe
S_I*.test
diff --git a/tools/testing/selftests/exec/Makefile b/tools/testing/selftests/exec/Makefile
index 8713d1c862ae..45a3cfc435cf 100644
--- a/tools/testing/selftests/exec/Makefile
+++ b/tools/testing/selftests/exec/Makefile
@@ -10,9 +10,9 @@ ALIGN_PIES := $(patsubst %,load_address.%,$(ALIGNS))
ALIGN_STATIC_PIES := $(patsubst %,load_address.static.%,$(ALIGNS))
ALIGNMENT_TESTS := $(ALIGN_PIES) $(ALIGN_STATIC_PIES)
-TEST_PROGS := binfmt_script.py
+TEST_PROGS := binfmt_script.py check-exec-tests.sh
TEST_GEN_PROGS := execveat non-regular $(ALIGNMENT_TESTS)
-TEST_GEN_PROGS_EXTENDED := false
+TEST_GEN_PROGS_EXTENDED := false inc set-exec script-exec.inc script-noexec.inc
TEST_GEN_FILES := execveat.symlink execveat.denatured script subdir
# Makefile is a run-time dependency, since it's accessed by the execveat test
TEST_FILES := Makefile
@@ -26,6 +26,8 @@ EXTRA_CLEAN := $(OUTPUT)/subdir.moved $(OUTPUT)/execveat.moved $(OUTPUT)/xxxxx*
include ../lib.mk
+CHECK_EXEC_SAMPLES := $(top_srcdir)/samples/check-exec
+
$(OUTPUT)/subdir:
mkdir -p $@
$(OUTPUT)/script: Makefile
@@ -45,3 +47,11 @@ $(OUTPUT)/load_address.static.0x%: load_address.c
-fPIE -static-pie $< -o $@
$(OUTPUT)/false: false.c
$(CC) $(CFLAGS) $(LDFLAGS) -static $< -o $@
+$(OUTPUT)/inc: $(CHECK_EXEC_SAMPLES)/inc.c
+ $(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
+$(OUTPUT)/set-exec: $(CHECK_EXEC_SAMPLES)/set-exec.c
+ $(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
+$(OUTPUT)/script-exec.inc: $(CHECK_EXEC_SAMPLES)/script-exec.inc
+ cp $< $@
+$(OUTPUT)/script-noexec.inc: $(CHECK_EXEC_SAMPLES)/script-noexec.inc
+ cp $< $@
diff --git a/tools/testing/selftests/exec/check-exec-tests.sh b/tools/testing/selftests/exec/check-exec-tests.sh
new file mode 100755
index 000000000000..87102906ae3c
--- /dev/null
+++ b/tools/testing/selftests/exec/check-exec-tests.sh
@@ -0,0 +1,205 @@
+#!/usr/bin/env bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# Test the "inc" interpreter.
+#
+# See include/uapi/linux/securebits.h, include/uapi/linux/fcntl.h and
+# samples/check-exec/inc.c
+#
+# Copyright © 2024 Microsoft Corporation
+
+set -u -e -o pipefail
+
+EXPECTED_OUTPUT="1"
+exec 2>/dev/null
+
+DIR="$(dirname $(readlink -f "$0"))"
+source "${DIR}"/../kselftest/ktap_helpers.sh
+
+exec_direct() {
+ local expect="$1"
+ local script="$2"
+ shift 2
+ local ret=0
+ local out
+
+ # Updates PATH for `env` to execute the `inc` interpreter.
+ out="$(PATH="." "$@" "${script}")" || ret=$?
+
+ if [[ ${ret} -ne ${expect} ]]; then
+ echo "ERROR: Wrong expectation for direct file execution: ${ret}"
+ return 1
+ fi
+ if [[ ${ret} -eq 0 && "${out}" != "${EXPECTED_OUTPUT}" ]]; then
+ echo "ERROR: Wrong output for direct file execution: ${out}"
+ return 1
+ fi
+}
+
+exec_indirect() {
+ local expect="$1"
+ local script="$2"
+ shift 2
+ local ret=0
+ local out
+
+ # Script passed as argument.
+ out="$("$@" ./inc "${script}")" || ret=$?
+
+ if [[ ${ret} -ne ${expect} ]]; then
+ echo "ERROR: Wrong expectation for indirect file execution: ${ret}"
+ return 1
+ fi
+ if [[ ${ret} -eq 0 && "${out}" != "${EXPECTED_OUTPUT}" ]]; then
+ echo "ERROR: Wrong output for indirect file execution: ${out}"
+ return 1
+ fi
+}
+
+exec_stdin_reg() {
+ local expect="$1"
+ local script="$2"
+ shift 2
+ local ret=0
+ local out
+
+ # Executing stdin must be allowed if the related file is executable.
+ out="$("$@" ./inc -i < "${script}")" || ret=$?
+
+ if [[ ${ret} -ne ${expect} ]]; then
+ echo "ERROR: Wrong expectation for stdin regular file execution: ${ret}"
+ return 1
+ fi
+ if [[ ${ret} -eq 0 && "${out}" != "${EXPECTED_OUTPUT}" ]]; then
+ echo "ERROR: Wrong output for stdin regular file execution: ${out}"
+ return 1
+ fi
+}
+
+exec_stdin_pipe() {
+ local expect="$1"
+ shift
+ local ret=0
+ local out
+
+ # A pipe is not executable.
+ out="$(cat script-exec.inc | "$@" ./inc -i)" || ret=$?
+
+ if [[ ${ret} -ne ${expect} ]]; then
+ echo "ERROR: Wrong expectation for stdin pipe execution: ${ret}"
+ return 1
+ fi
+}
+
+exec_argument() {
+ local expect="$1"
+ local ret=0
+ shift
+ local out
+
+ # Script not coming from a file must not be executed.
+ out="$("$@" ./inc -c "$(< script-exec.inc)")" || ret=$?
+
+ if [[ ${ret} -ne ${expect} ]]; then
+ echo "ERROR: Wrong expectation for arbitrary argument execution: ${ret}"
+ return 1
+ fi
+ if [[ ${ret} -eq 0 && "${out}" != "${EXPECTED_OUTPUT}" ]]; then
+ echo "ERROR: Wrong output for arbitrary argument execution: ${out}"
+ return 1
+ fi
+}
+
+exec_interactive() {
+ exec_stdin_pipe "$@"
+ exec_argument "$@"
+}
+
+ktap_test() {
+ ktap_test_result "$*" "$@"
+}
+
+ktap_print_header
+ktap_set_plan 28
+
+# Without secbit configuration, nothing is changed.
+
+ktap_print_msg "By default, executable scripts are allowed to be interpreted and executed."
+ktap_test exec_direct 0 script-exec.inc
+ktap_test exec_indirect 0 script-exec.inc
+
+ktap_print_msg "By default, executable stdin is allowed to be interpreted."
+ktap_test exec_stdin_reg 0 script-exec.inc
+
+ktap_print_msg "By default, non-executable scripts are allowed to be interpreted, but not directly executed."
+# We get 126 because of direct execution by Bash.
+ktap_test exec_direct 126 script-noexec.inc
+ktap_test exec_indirect 0 script-noexec.inc
+
+ktap_print_msg "By default, non-executable stdin is allowed to be interpreted."
+ktap_test exec_stdin_reg 0 script-noexec.inc
+
+ktap_print_msg "By default, interactive commands are allowed to be interpreted."
+ktap_test exec_interactive 0
+
+# With only file restriction: protect non-malicious users from inadvertent errors (e.g. python ~/Downloads/*.py).
+
+ktap_print_msg "With -f, executable scripts are allowed to be interpreted and executed."
+ktap_test exec_direct 0 script-exec.inc ./set-exec -f --
+ktap_test exec_indirect 0 script-exec.inc ./set-exec -f --
+
+ktap_print_msg "With -f, executable stdin is allowed to be interpreted."
+ktap_test exec_stdin_reg 0 script-exec.inc ./set-exec -f --
+
+ktap_print_msg "With -f, non-executable scripts are not allowed to be executed nor interpreted."
+# Direct execution of non-executable script is alwayse denied by the kernel.
+ktap_test exec_direct 1 script-noexec.inc ./set-exec -f --
+ktap_test exec_indirect 1 script-noexec.inc ./set-exec -f --
+
+ktap_print_msg "With -f, non-executable stdin is allowed to be interpreted."
+ktap_test exec_stdin_reg 0 script-noexec.inc ./set-exec -f --
+
+ktap_print_msg "With -f, interactive commands are allowed to be interpreted."
+ktap_test exec_interactive 0 ./set-exec -f --
+
+# With only denied interactive commands: check or monitor script content (e.g. with LSM).
+
+ktap_print_msg "With -i, executable scripts are allowed to be interpreted and executed."
+ktap_test exec_direct 0 script-exec.inc ./set-exec -i --
+ktap_test exec_indirect 0 script-exec.inc ./set-exec -i --
+
+ktap_print_msg "With -i, executable stdin is allowed to be interpreted."
+ktap_test exec_stdin_reg 0 script-exec.inc ./set-exec -i --
+
+ktap_print_msg "With -i, non-executable scripts are allowed to be interpreted, but not directly executed."
+# Direct execution of non-executable script is alwayse denied by the kernel.
+ktap_test exec_direct 1 script-noexec.inc ./set-exec -i --
+ktap_test exec_indirect 0 script-noexec.inc ./set-exec -i --
+
+ktap_print_msg "With -i, non-executable stdin is not allowed to be interpreted."
+ktap_test exec_stdin_reg 1 script-noexec.inc ./set-exec -i --
+
+ktap_print_msg "With -i, interactive commands are not allowed to be interpreted."
+ktap_test exec_interactive 1 ./set-exec -i --
+
+# With both file restriction and denied interactive commands: only allow executable scripts.
+
+ktap_print_msg "With -fi, executable scripts are allowed to be interpreted and executed."
+ktap_test exec_direct 0 script-exec.inc ./set-exec -fi --
+ktap_test exec_indirect 0 script-exec.inc ./set-exec -fi --
+
+ktap_print_msg "With -fi, executable stdin is allowed to be interpreted."
+ktap_test exec_stdin_reg 0 script-exec.inc ./set-exec -fi --
+
+ktap_print_msg "With -fi, non-executable scripts are not allowed to be interpreted nor executed."
+# Direct execution of non-executable script is alwayse denied by the kernel.
+ktap_test exec_direct 1 script-noexec.inc ./set-exec -fi --
+ktap_test exec_indirect 1 script-noexec.inc ./set-exec -fi --
+
+ktap_print_msg "With -fi, non-executable stdin is not allowed to be interpreted."
+ktap_test exec_stdin_reg 1 script-noexec.inc ./set-exec -fi --
+
+ktap_print_msg "With -fi, interactive commands are not allowed to be interpreted."
+ktap_test exec_interactive 1 ./set-exec -fi --
+
+ktap_finished
diff --git a/tools/testing/selftests/kselftest/ktap_helpers.sh b/tools/testing/selftests/kselftest/ktap_helpers.sh
index 79a125eb24c2..14e7f3ec3f84 100644
--- a/tools/testing/selftests/kselftest/ktap_helpers.sh
+++ b/tools/testing/selftests/kselftest/ktap_helpers.sh
@@ -40,7 +40,7 @@ ktap_skip_all() {
__ktap_test() {
result="$1"
description="$2"
- directive="$3" # optional
+ directive="${3:-}" # optional
local directive_str=
[ ! -z "$directive" ] && directive_str="# $directive"
--
2.46.1
^ permalink raw reply related
* [PATCH v20 3/6] selftests/exec: Add 32 tests for AT_CHECK and exec securebits
From: Mickaël Salaün @ 2024-10-11 18:44 UTC (permalink / raw)
To: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Serge Hallyn, Theodore Ts'o
Cc: Mickaël Salaün, Adhemerval Zanella Netto,
Alejandro Colomar, Aleksa Sarai, Andrew Morton, Andy Lutomirski,
Arnd Bergmann, Casey Schaufler, Christian Heimes, Dmitry Vyukov,
Elliott Hughes, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20241011184422.977903-1-mic@digikod.net>
Test that checks performed by execveat(..., AT_CHECK) are consistent
with noexec mount points and file execute permissions.
Test that SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE are
inherited by child processes and that they can be pinned with the
appropriate SECBIT_EXEC_RESTRICT_FILE_LOCKED and
SECBIT_EXEC_DENY_INTERACTIVE_LOCKED bits.
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20241011184422.977903-4-mic@digikod.net
---
Changes since v19:
* Rename securebits.
* Rename test file.
Changes since v18:
* Rewrite tests with the new design: execveat/AT_CHECK and securebits.
* Simplify the capability dropping and improve it with the NOROOT
securebits.
* Replace most ASSERT with EXPECT.
* Fix NULL execve's argv to avoid kernel warning.
* Move tests to exec/
* Build a "false" static binary to test full execution path.
Changes since v14:
* Add Reviewed-by Kees Cook.
Changes since v13:
* Move -I to CFLAGS (suggested by Kees Cook).
* Update sysctl name.
Changes since v12:
* Fix Makefile's license.
Changes since v10:
* Update selftest Makefile.
Changes since v9:
* Rename the syscall and the sysctl.
* Update tests for enum trusted_for_usage
Changes since v8:
* Update with the dedicated syscall introspect_access(2) and the renamed
fs.introspection_policy sysctl.
* Remove check symlink which can't be use as is anymore.
* Use socketpair(2) to test UNIX socket.
Changes since v7:
* Update tests with faccessat2/AT_INTERPRETED, including new ones to
check that setting R_OK or W_OK returns EINVAL.
* Add tests for memfd, pipefs and nsfs.
* Rename and move back tests to a standalone directory.
Changes since v6:
* Add full combination tests for all file types, including block
devices, character devices, fifos, sockets and symlinks.
* Properly save and restore initial sysctl value for all tests.
Changes since v5:
* Refactor with FIXTURE_VARIANT, which make the tests much more easy to
read and maintain.
* Save and restore initial sysctl value (suggested by Kees Cook).
* Test with a sysctl value of 0.
* Check errno in sysctl_access_write test.
* Update tests for the CAP_SYS_ADMIN switch.
* Update tests to check -EISDIR (replacing -EACCES).
* Replace FIXTURE_DATA() with FIXTURE() (spotted by Kees Cook).
* Use global const strings.
Changes since v3:
* Replace RESOLVE_MAYEXEC with O_MAYEXEC.
* Add tests to check that O_MAYEXEC is ignored by open(2) and openat(2).
Changes since v2:
* Move tests from exec/ to openat2/ .
* Replace O_MAYEXEC with RESOLVE_MAYEXEC from openat2(2).
* Cleanup tests.
Changes since v1:
* Move tests from yama/ to exec/ .
* Fix _GNU_SOURCE in kselftest_harness.h .
* Add a new test sysctl_access_write to check if CAP_MAC_ADMIN is taken
into account.
* Test directory execution which is always forbidden since commit
73601ea5b7b1 ("fs/open.c: allow opening only regular files during
execve()"), and also check that even the root user can not bypass file
execution checks.
* Make sure delete_workspace() always as enough right to succeed.
* Cosmetic cleanup.
---
tools/testing/selftests/exec/.gitignore | 2 +
tools/testing/selftests/exec/Makefile | 7 +
tools/testing/selftests/exec/check-exec.c | 446 ++++++++++++++++++++++
tools/testing/selftests/exec/config | 2 +
tools/testing/selftests/exec/false.c | 5 +
5 files changed, 462 insertions(+)
create mode 100644 tools/testing/selftests/exec/check-exec.c
create mode 100644 tools/testing/selftests/exec/config
create mode 100644 tools/testing/selftests/exec/false.c
diff --git a/tools/testing/selftests/exec/.gitignore b/tools/testing/selftests/exec/.gitignore
index a0dc5d4bf733..a32c63bb4df1 100644
--- a/tools/testing/selftests/exec/.gitignore
+++ b/tools/testing/selftests/exec/.gitignore
@@ -9,6 +9,8 @@ execveat.ephemeral
execveat.denatured
non-regular
null-argv
+/check-exec
+/false
/load_address.*
!load_address.c
/recursion-depth
diff --git a/tools/testing/selftests/exec/Makefile b/tools/testing/selftests/exec/Makefile
index ba012bc5aab9..8713d1c862ae 100644
--- a/tools/testing/selftests/exec/Makefile
+++ b/tools/testing/selftests/exec/Makefile
@@ -1,6 +1,9 @@
# SPDX-License-Identifier: GPL-2.0
CFLAGS = -Wall
CFLAGS += -Wno-nonnull
+CFLAGS += $(KHDR_INCLUDES)
+
+LDLIBS += -lcap
ALIGNS := 0x1000 0x200000 0x1000000
ALIGN_PIES := $(patsubst %,load_address.%,$(ALIGNS))
@@ -9,12 +12,14 @@ ALIGNMENT_TESTS := $(ALIGN_PIES) $(ALIGN_STATIC_PIES)
TEST_PROGS := binfmt_script.py
TEST_GEN_PROGS := execveat non-regular $(ALIGNMENT_TESTS)
+TEST_GEN_PROGS_EXTENDED := false
TEST_GEN_FILES := execveat.symlink execveat.denatured script subdir
# Makefile is a run-time dependency, since it's accessed by the execveat test
TEST_FILES := Makefile
TEST_GEN_PROGS += recursion-depth
TEST_GEN_PROGS += null-argv
+TEST_GEN_PROGS += check-exec
EXTRA_CLEAN := $(OUTPUT)/subdir.moved $(OUTPUT)/execveat.moved $(OUTPUT)/xxxxx* \
$(OUTPUT)/S_I*.test
@@ -38,3 +43,5 @@ $(OUTPUT)/load_address.0x%: load_address.c
$(OUTPUT)/load_address.static.0x%: load_address.c
$(CC) $(CFLAGS) $(LDFLAGS) -Wl,-z,max-page-size=$(lastword $(subst ., ,$@)) \
-fPIE -static-pie $< -o $@
+$(OUTPUT)/false: false.c
+ $(CC) $(CFLAGS) $(LDFLAGS) -static $< -o $@
diff --git a/tools/testing/selftests/exec/check-exec.c b/tools/testing/selftests/exec/check-exec.c
new file mode 100644
index 000000000000..084f5e90f45a
--- /dev/null
+++ b/tools/testing/selftests/exec/check-exec.c
@@ -0,0 +1,446 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Test execveat(2) with AT_CHECK, and prctl(2) with SECBIT_EXEC_RESTRICT_FILE,
+ * SECBIT_EXEC_DENY_INTERACTIVE, and their locked counterparts.
+ *
+ * Copyright © 2018-2020 ANSSI
+ * Copyright © 2024 Microsoft Corporation
+ *
+ * Author: Mickaël Salaün <mic@digikod.net>
+ */
+
+#include <asm-generic/unistd.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <linux/prctl.h>
+#include <linux/securebits.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/capability.h>
+#include <sys/mount.h>
+#include <sys/prctl.h>
+#include <sys/socket.h>
+#include <sys/stat.h>
+#include <sys/sysmacros.h>
+#include <unistd.h>
+
+/* Defines AT_CHECK without type conflicts. */
+#define _ASM_GENERIC_FCNTL_H
+#include <linux/fcntl.h>
+
+#include "../kselftest_harness.h"
+
+static void drop_privileges(struct __test_metadata *const _metadata)
+{
+ const unsigned int noroot = SECBIT_NOROOT | SECBIT_NOROOT_LOCKED;
+ cap_t cap_p;
+
+ if ((cap_get_secbits() & noroot) != noroot)
+ EXPECT_EQ(0, cap_set_secbits(noroot));
+
+ cap_p = cap_get_proc();
+ EXPECT_NE(NULL, cap_p);
+ EXPECT_NE(-1, cap_clear(cap_p));
+
+ /*
+ * Drops everything, especially CAP_SETPCAP, CAP_DAC_OVERRIDE, and
+ * CAP_DAC_READ_SEARCH.
+ */
+ EXPECT_NE(-1, cap_set_proc(cap_p));
+ EXPECT_NE(-1, cap_free(cap_p));
+}
+
+static int test_secbits_set(const unsigned int secbits)
+{
+ int err;
+
+ err = prctl(PR_SET_SECUREBITS, secbits);
+ if (err)
+ return errno;
+ return 0;
+}
+
+FIXTURE(access)
+{
+ int memfd, pipefd;
+ int pipe_fds[2], socket_fds[2];
+};
+
+FIXTURE_VARIANT(access)
+{
+ const bool mount_exec;
+ const bool file_exec;
+};
+
+FIXTURE_VARIANT_ADD(access, mount_exec_file_exec){
+ .mount_exec = true,
+ .file_exec = true,
+};
+
+FIXTURE_VARIANT_ADD(access, mount_exec_file_noexec){
+ .mount_exec = true,
+ .file_exec = false,
+};
+
+FIXTURE_VARIANT_ADD(access, mount_noexec_file_exec){
+ .mount_exec = false,
+ .file_exec = true,
+};
+
+FIXTURE_VARIANT_ADD(access, mount_noexec_file_noexec){
+ .mount_exec = false,
+ .file_exec = false,
+};
+
+static const char binary_path[] = "./false";
+static const char workdir_path[] = "./test-mount";
+static const char reg_file_path[] = "./test-mount/regular_file";
+static const char dir_path[] = "./test-mount/directory";
+static const char block_dev_path[] = "./test-mount/block_device";
+static const char char_dev_path[] = "./test-mount/character_device";
+static const char fifo_path[] = "./test-mount/fifo";
+
+FIXTURE_SETUP(access)
+{
+ int procfd_path_size;
+ static const char path_template[] = "/proc/self/fd/%d";
+ char procfd_path[sizeof(path_template) + 10];
+
+ /* Makes sure we are not already restricted nor locked. */
+ EXPECT_EQ(0, test_secbits_set(0));
+
+ /*
+ * Cleans previous workspace if any error previously happened (don't
+ * check errors).
+ */
+ umount(workdir_path);
+ rmdir(workdir_path);
+
+ /* Creates a clean mount point. */
+ ASSERT_EQ(0, mkdir(workdir_path, 00700));
+ ASSERT_EQ(0, mount("test", workdir_path, "tmpfs",
+ MS_MGC_VAL | (variant->mount_exec ? 0 : MS_NOEXEC),
+ "mode=0700,size=9m"));
+
+ /* Creates a regular file. */
+ ASSERT_EQ(0, mknod(reg_file_path,
+ S_IFREG | (variant->file_exec ? 0700 : 0600), 0));
+ /* Creates a directory. */
+ ASSERT_EQ(0, mkdir(dir_path, variant->file_exec ? 0700 : 0600));
+ /* Creates a character device: /dev/null. */
+ ASSERT_EQ(0, mknod(char_dev_path, S_IFCHR | 0400, makedev(1, 3)));
+ /* Creates a block device: /dev/loop0 */
+ ASSERT_EQ(0, mknod(block_dev_path, S_IFBLK | 0400, makedev(7, 0)));
+ /* Creates a fifo. */
+ ASSERT_EQ(0, mknod(fifo_path, S_IFIFO | 0600, 0));
+
+ /* Creates a regular file without user mount point. */
+ self->memfd = memfd_create("test-exec-probe", MFD_CLOEXEC);
+ ASSERT_LE(0, self->memfd);
+ /* Sets mode, which must be ignored by the exec check. */
+ ASSERT_EQ(0, fchmod(self->memfd, variant->file_exec ? 0700 : 0600));
+
+ /* Creates a pipefs file descriptor. */
+ ASSERT_EQ(0, pipe(self->pipe_fds));
+ procfd_path_size = snprintf(procfd_path, sizeof(procfd_path),
+ path_template, self->pipe_fds[0]);
+ ASSERT_LT(procfd_path_size, sizeof(procfd_path));
+ self->pipefd = open(procfd_path, O_RDWR | O_CLOEXEC);
+ ASSERT_LE(0, self->pipefd);
+ ASSERT_EQ(0, fchmod(self->pipefd, variant->file_exec ? 0700 : 0600));
+
+ /* Creates a socket file descriptor. */
+ ASSERT_EQ(0, socketpair(AF_UNIX, SOCK_DGRAM | SOCK_CLOEXEC, 0,
+ self->socket_fds));
+}
+
+FIXTURE_TEARDOWN_PARENT(access)
+{
+ /* There is no need to unlink the test files. */
+ EXPECT_EQ(0, umount(workdir_path));
+ EXPECT_EQ(0, rmdir(workdir_path));
+}
+
+static void fill_exec_fd(struct __test_metadata *_metadata, const int fd_out)
+{
+ char buf[1024];
+ size_t len;
+ int fd_in;
+
+ fd_in = open(binary_path, O_CLOEXEC | O_RDONLY);
+ ASSERT_LE(0, fd_in);
+ /* Cannot use copy_file_range(2) because of EXDEV. */
+ len = read(fd_in, buf, sizeof(buf));
+ EXPECT_LE(0, len);
+ while (len > 0) {
+ EXPECT_EQ(len, write(fd_out, buf, len))
+ {
+ TH_LOG("Failed to write: %s (%d)", strerror(errno),
+ errno);
+ }
+ len = read(fd_in, buf, sizeof(buf));
+ EXPECT_LE(0, len);
+ }
+ EXPECT_EQ(0, close(fd_in));
+}
+
+static void fill_exec_path(struct __test_metadata *_metadata,
+ const char *const path)
+{
+ int fd_out;
+
+ fd_out = open(path, O_CLOEXEC | O_WRONLY);
+ ASSERT_LE(0, fd_out)
+ {
+ TH_LOG("Failed to open %s: %s", path, strerror(errno));
+ }
+ fill_exec_fd(_metadata, fd_out);
+ EXPECT_EQ(0, close(fd_out));
+}
+
+static void test_exec_fd(struct __test_metadata *_metadata, const int fd,
+ const int err_code)
+{
+ char *const argv[] = { "", NULL };
+ int access_ret, access_errno;
+
+ /*
+ * If we really execute fd, filled with the "false" binary, the current
+ * thread will exits with an error, which will be interpreted by the
+ * test framework as an error. With AT_CHECK, we only check a
+ * potential successful execution.
+ */
+ access_ret = execveat(fd, "", argv, NULL, AT_EMPTY_PATH | AT_CHECK);
+ access_errno = errno;
+ if (err_code) {
+ EXPECT_EQ(-1, access_ret);
+ EXPECT_EQ(err_code, access_errno)
+ {
+ TH_LOG("Wrong error for execveat(2): %s (%d)",
+ strerror(access_errno), errno);
+ }
+ } else {
+ EXPECT_EQ(0, access_ret)
+ {
+ TH_LOG("Access denied: %s", strerror(access_errno));
+ }
+ }
+}
+
+static void test_exec_path(struct __test_metadata *_metadata,
+ const char *const path, const int err_code)
+{
+ int flags = O_CLOEXEC;
+ int fd;
+
+ /* Do not block on pipes. */
+ if (path == fifo_path)
+ flags |= O_NONBLOCK;
+
+ fd = open(path, flags | O_RDONLY);
+ ASSERT_LE(0, fd)
+ {
+ TH_LOG("Failed to open %s: %s", path, strerror(errno));
+ }
+ test_exec_fd(_metadata, fd, err_code);
+ EXPECT_EQ(0, close(fd));
+}
+
+/* Tests that we don't get ENOEXEC. */
+TEST_F(access, regular_file_empty)
+{
+ const int exec = variant->mount_exec && variant->file_exec;
+
+ test_exec_path(_metadata, reg_file_path, exec ? 0 : EACCES);
+
+ drop_privileges(_metadata);
+ test_exec_path(_metadata, reg_file_path, exec ? 0 : EACCES);
+}
+
+TEST_F(access, regular_file_elf)
+{
+ const int exec = variant->mount_exec && variant->file_exec;
+
+ fill_exec_path(_metadata, reg_file_path);
+
+ test_exec_path(_metadata, reg_file_path, exec ? 0 : EACCES);
+
+ drop_privileges(_metadata);
+ test_exec_path(_metadata, reg_file_path, exec ? 0 : EACCES);
+}
+
+/* Tests that we don't get ENOEXEC. */
+TEST_F(access, memfd_empty)
+{
+ const int exec = variant->file_exec;
+
+ test_exec_fd(_metadata, self->memfd, exec ? 0 : EACCES);
+
+ drop_privileges(_metadata);
+ test_exec_fd(_metadata, self->memfd, exec ? 0 : EACCES);
+}
+
+TEST_F(access, memfd_elf)
+{
+ const int exec = variant->file_exec;
+
+ fill_exec_fd(_metadata, self->memfd);
+
+ test_exec_fd(_metadata, self->memfd, exec ? 0 : EACCES);
+
+ drop_privileges(_metadata);
+ test_exec_fd(_metadata, self->memfd, exec ? 0 : EACCES);
+}
+
+TEST_F(access, non_regular_files)
+{
+ test_exec_path(_metadata, dir_path, EACCES);
+ test_exec_path(_metadata, block_dev_path, EACCES);
+ test_exec_path(_metadata, char_dev_path, EACCES);
+ test_exec_path(_metadata, fifo_path, EACCES);
+ test_exec_fd(_metadata, self->socket_fds[0], EACCES);
+ test_exec_fd(_metadata, self->pipefd, EACCES);
+}
+
+/* clang-format off */
+FIXTURE(secbits) {};
+/* clang-format on */
+
+FIXTURE_VARIANT(secbits)
+{
+ const bool is_privileged;
+ const int error;
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(secbits, priv) {
+ /* clang-format on */
+ .is_privileged = true,
+ .error = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(secbits, unpriv) {
+ /* clang-format on */
+ .is_privileged = false,
+ .error = EPERM,
+};
+
+FIXTURE_SETUP(secbits)
+{
+ /* Makes sure no exec bits are set. */
+ EXPECT_EQ(0, test_secbits_set(0));
+ EXPECT_EQ(0, prctl(PR_GET_SECUREBITS));
+
+ if (!variant->is_privileged)
+ drop_privileges(_metadata);
+}
+
+FIXTURE_TEARDOWN(secbits)
+{
+}
+
+TEST_F(secbits, legacy)
+{
+ EXPECT_EQ(variant->error, test_secbits_set(0));
+}
+
+#define CHILD(...) \
+ do { \
+ pid_t child = vfork(); \
+ EXPECT_LE(0, child); \
+ if (child == 0) { \
+ __VA_ARGS__; \
+ _exit(0); \
+ } \
+ } while (0)
+
+TEST_F(secbits, exec)
+{
+ unsigned int secbits = prctl(PR_GET_SECUREBITS);
+
+ secbits |= SECBIT_EXEC_RESTRICT_FILE;
+ EXPECT_EQ(0, test_secbits_set(secbits));
+ EXPECT_EQ(secbits, prctl(PR_GET_SECUREBITS));
+ CHILD(EXPECT_EQ(secbits, prctl(PR_GET_SECUREBITS)));
+
+ secbits |= SECBIT_EXEC_DENY_INTERACTIVE;
+ EXPECT_EQ(0, test_secbits_set(secbits));
+ EXPECT_EQ(secbits, prctl(PR_GET_SECUREBITS));
+ CHILD(EXPECT_EQ(secbits, prctl(PR_GET_SECUREBITS)));
+
+ secbits &= ~(SECBIT_EXEC_RESTRICT_FILE | SECBIT_EXEC_DENY_INTERACTIVE);
+ EXPECT_EQ(0, test_secbits_set(secbits));
+ EXPECT_EQ(secbits, prctl(PR_GET_SECUREBITS));
+ CHILD(EXPECT_EQ(secbits, prctl(PR_GET_SECUREBITS)));
+}
+
+TEST_F(secbits, check_locked_set)
+{
+ unsigned int secbits = prctl(PR_GET_SECUREBITS);
+
+ secbits |= SECBIT_EXEC_RESTRICT_FILE;
+ EXPECT_EQ(0, test_secbits_set(secbits));
+ secbits |= SECBIT_EXEC_RESTRICT_FILE_LOCKED;
+ EXPECT_EQ(0, test_secbits_set(secbits));
+
+ /* Checks lock set but unchanged. */
+ EXPECT_EQ(variant->error, test_secbits_set(secbits));
+ CHILD(EXPECT_EQ(variant->error, test_secbits_set(secbits)));
+
+ secbits &= ~SECBIT_EXEC_RESTRICT_FILE;
+ EXPECT_EQ(EPERM, test_secbits_set(0));
+ CHILD(EXPECT_EQ(EPERM, test_secbits_set(0)));
+}
+
+TEST_F(secbits, check_locked_unset)
+{
+ unsigned int secbits = prctl(PR_GET_SECUREBITS);
+
+ secbits |= SECBIT_EXEC_RESTRICT_FILE_LOCKED;
+ EXPECT_EQ(0, test_secbits_set(secbits));
+
+ /* Checks lock unset but unchanged. */
+ EXPECT_EQ(variant->error, test_secbits_set(secbits));
+ CHILD(EXPECT_EQ(variant->error, test_secbits_set(secbits)));
+
+ secbits &= ~SECBIT_EXEC_RESTRICT_FILE;
+ EXPECT_EQ(EPERM, test_secbits_set(0));
+ CHILD(EXPECT_EQ(EPERM, test_secbits_set(0)));
+}
+
+TEST_F(secbits, restrict_locked_set)
+{
+ unsigned int secbits = prctl(PR_GET_SECUREBITS);
+
+ secbits |= SECBIT_EXEC_DENY_INTERACTIVE;
+ EXPECT_EQ(0, test_secbits_set(secbits));
+ secbits |= SECBIT_EXEC_DENY_INTERACTIVE_LOCKED;
+ EXPECT_EQ(0, test_secbits_set(secbits));
+
+ /* Checks lock set but unchanged. */
+ EXPECT_EQ(variant->error, test_secbits_set(secbits));
+ CHILD(EXPECT_EQ(variant->error, test_secbits_set(secbits)));
+
+ secbits &= ~SECBIT_EXEC_DENY_INTERACTIVE;
+ EXPECT_EQ(EPERM, test_secbits_set(0));
+ CHILD(EXPECT_EQ(EPERM, test_secbits_set(0)));
+}
+
+TEST_F(secbits, restrict_locked_unset)
+{
+ unsigned int secbits = prctl(PR_GET_SECUREBITS);
+
+ secbits |= SECBIT_EXEC_DENY_INTERACTIVE_LOCKED;
+ EXPECT_EQ(0, test_secbits_set(secbits));
+
+ /* Checks lock unset but unchanged. */
+ EXPECT_EQ(variant->error, test_secbits_set(secbits));
+ CHILD(EXPECT_EQ(variant->error, test_secbits_set(secbits)));
+
+ secbits &= ~SECBIT_EXEC_DENY_INTERACTIVE;
+ EXPECT_EQ(EPERM, test_secbits_set(0));
+ CHILD(EXPECT_EQ(EPERM, test_secbits_set(0)));
+}
+
+TEST_HARNESS_MAIN
diff --git a/tools/testing/selftests/exec/config b/tools/testing/selftests/exec/config
new file mode 100644
index 000000000000..c308079867b3
--- /dev/null
+++ b/tools/testing/selftests/exec/config
@@ -0,0 +1,2 @@
+CONFIG_BLK_DEV=y
+CONFIG_BLK_DEV_LOOP=y
diff --git a/tools/testing/selftests/exec/false.c b/tools/testing/selftests/exec/false.c
new file mode 100644
index 000000000000..104383ec3a79
--- /dev/null
+++ b/tools/testing/selftests/exec/false.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0
+int main(void)
+{
+ return 1;
+}
--
2.46.1
^ permalink raw reply related
* [PATCH v20 5/6] samples/check-exec: Add set-exec
From: Mickaël Salaün @ 2024-10-11 18:44 UTC (permalink / raw)
To: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Serge Hallyn, Theodore Ts'o
Cc: Mickaël Salaün, Adhemerval Zanella Netto,
Alejandro Colomar, Aleksa Sarai, Andrew Morton, Andy Lutomirski,
Arnd Bergmann, Casey Schaufler, Christian Heimes, Dmitry Vyukov,
Elliott Hughes, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20241011184422.977903-1-mic@digikod.net>
Add a simple tool to set SECBIT_EXEC_RESTRICT_FILE or
SECBIT_EXEC_DENY_INTERACTIVE before executing a command. This is useful
to easily test against enlighten script interpreters.
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20241011184422.977903-6-mic@digikod.net
---
Changes since v19:
* Rename file and directory.
* Update securebits and related arguments.
* Remove useless call to prctl() when securebits are unchanged.
---
samples/Kconfig | 7 +++
samples/Makefile | 1 +
samples/check-exec/.gitignore | 1 +
samples/check-exec/Makefile | 14 ++++++
samples/check-exec/set-exec.c | 85 +++++++++++++++++++++++++++++++++++
5 files changed, 108 insertions(+)
create mode 100644 samples/check-exec/.gitignore
create mode 100644 samples/check-exec/Makefile
create mode 100644 samples/check-exec/set-exec.c
diff --git a/samples/Kconfig b/samples/Kconfig
index b288d9991d27..efa28ceadc42 100644
--- a/samples/Kconfig
+++ b/samples/Kconfig
@@ -291,6 +291,13 @@ config SAMPLE_CGROUP
help
Build samples that demonstrate the usage of the cgroup API.
+config SAMPLE_CHECK_EXEC
+ bool "Exec secure bits examples"
+ depends on CC_CAN_LINK && HEADERS_INSTALL
+ help
+ Build a tool to easily configure SECBIT_EXEC_RESTRICT_FILE and
+ SECBIT_EXEC_DENY_INTERACTIVE.
+
source "samples/rust/Kconfig"
endif # SAMPLES
diff --git a/samples/Makefile b/samples/Makefile
index b85fa64390c5..f988202f3a30 100644
--- a/samples/Makefile
+++ b/samples/Makefile
@@ -3,6 +3,7 @@
subdir-$(CONFIG_SAMPLE_AUXDISPLAY) += auxdisplay
subdir-$(CONFIG_SAMPLE_ANDROID_BINDERFS) += binderfs
+subdir-$(CONFIG_SAMPLE_CHECK_EXEC) += check-exec
subdir-$(CONFIG_SAMPLE_CGROUP) += cgroup
obj-$(CONFIG_SAMPLE_CONFIGFS) += configfs/
obj-$(CONFIG_SAMPLE_CONNECTOR) += connector/
diff --git a/samples/check-exec/.gitignore b/samples/check-exec/.gitignore
new file mode 100644
index 000000000000..3f8119112ccf
--- /dev/null
+++ b/samples/check-exec/.gitignore
@@ -0,0 +1 @@
+/set-exec
diff --git a/samples/check-exec/Makefile b/samples/check-exec/Makefile
new file mode 100644
index 000000000000..d9f976e3ff98
--- /dev/null
+++ b/samples/check-exec/Makefile
@@ -0,0 +1,14 @@
+# SPDX-License-Identifier: BSD-3-Clause
+
+userprogs-always-y := \
+ set-exec
+
+userccflags += -I usr/include
+
+.PHONY: all clean
+
+all:
+ $(MAKE) -C ../.. samples/check-exec/
+
+clean:
+ $(MAKE) -C ../.. M=samples/check-exec/ clean
diff --git a/samples/check-exec/set-exec.c b/samples/check-exec/set-exec.c
new file mode 100644
index 000000000000..ba86a60a20dd
--- /dev/null
+++ b/samples/check-exec/set-exec.c
@@ -0,0 +1,85 @@
+// SPDX-License-Identifier: BSD-3-Clause
+/*
+ * Simple tool to set SECBIT_EXEC_RESTRICT_FILE, SECBIT_EXEC_DENY_INTERACTIVE,
+ * before executing a command.
+ *
+ * Copyright © 2024 Microsoft Corporation
+ */
+
+#define _GNU_SOURCE
+#define __SANE_USERSPACE_TYPES__
+#include <errno.h>
+#include <linux/prctl.h>
+#include <linux/securebits.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/prctl.h>
+#include <unistd.h>
+
+static void print_usage(const char *argv0)
+{
+ fprintf(stderr, "usage: %s -f|-i -- <cmd> [args]...\n\n", argv0);
+ fprintf(stderr, "Execute a command with\n");
+ fprintf(stderr, "- SECBIT_EXEC_RESTRICT_FILE set: -f\n");
+ fprintf(stderr, "- SECBIT_EXEC_DENY_INTERACTIVE set: -i\n");
+}
+
+int main(const int argc, char *const argv[], char *const *const envp)
+{
+ const char *cmd_path;
+ char *const *cmd_argv;
+ int opt, secbits_cur, secbits_new;
+ bool has_policy = false;
+
+ secbits_cur = prctl(PR_GET_SECUREBITS);
+ if (secbits_cur == -1) {
+ /*
+ * This should never happen, except with a buggy seccomp
+ * filter.
+ */
+ perror("ERROR: Failed to get securebits");
+ return 1;
+ }
+
+ secbits_new = secbits_cur;
+ while ((opt = getopt(argc, argv, "fi")) != -1) {
+ switch (opt) {
+ case 'f':
+ secbits_new |= SECBIT_EXEC_RESTRICT_FILE |
+ SECBIT_EXEC_RESTRICT_FILE_LOCKED;
+ has_policy = true;
+ break;
+ case 'i':
+ secbits_new |= SECBIT_EXEC_DENY_INTERACTIVE |
+ SECBIT_EXEC_DENY_INTERACTIVE_LOCKED;
+ has_policy = true;
+ break;
+ default:
+ print_usage(argv[0]);
+ return 1;
+ }
+ }
+
+ if (!argv[optind] || !has_policy) {
+ print_usage(argv[0]);
+ return 1;
+ }
+
+ if (secbits_cur != secbits_new &&
+ prctl(PR_SET_SECUREBITS, secbits_new)) {
+ perror("Failed to set secure bit(s).");
+ fprintf(stderr,
+ "Hint: The running kernel may not support this feature.\n");
+ return 1;
+ }
+
+ cmd_path = argv[optind];
+ cmd_argv = argv + optind;
+ fprintf(stderr, "Executing command...\n");
+ execvpe(cmd_path, cmd_argv, envp);
+ fprintf(stderr, "Failed to execute \"%s\": %s\n", cmd_path,
+ strerror(errno));
+ return 1;
+}
--
2.46.1
^ permalink raw reply related
* [PATCH v20 4/6] selftests/landlock: Add tests for execveat + AT_CHECK
From: Mickaël Salaün @ 2024-10-11 18:44 UTC (permalink / raw)
To: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Serge Hallyn, Theodore Ts'o
Cc: Mickaël Salaün, Adhemerval Zanella Netto,
Alejandro Colomar, Aleksa Sarai, Andrew Morton, Andy Lutomirski,
Arnd Bergmann, Casey Schaufler, Christian Heimes, Dmitry Vyukov,
Elliott Hughes, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module, Günther Noack
In-Reply-To: <20241011184422.977903-1-mic@digikod.net>
Extend layout1.execute with the new AT_CHECK flag. The semantic with
AT_CHECK is the same as with a simple execve(2),
LANDLOCK_ACCESS_FS_EXECUTE is enforced the same way.
Cc: Günther Noack <gnoack@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Paul Moore <paul@paul-moore.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20241011184422.977903-5-mic@digikod.net
---
tools/testing/selftests/landlock/fs_test.c | 26 ++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 6788762188fe..413be4fea21e 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -37,6 +37,10 @@
#include <linux/fs.h>
#include <linux/mount.h>
+/* Defines AT_CHECK without type conflicts. */
+#define _ASM_GENERIC_FCNTL_H
+#include <linux/fcntl.h>
+
#include "common.h"
#ifndef renameat2
@@ -2008,6 +2012,21 @@ static void test_execute(struct __test_metadata *const _metadata, const int err,
};
}
+static void test_check_exec(struct __test_metadata *const _metadata,
+ const int err, const char *const path)
+{
+ int ret;
+ char *const argv[] = { (char *)path, NULL };
+
+ ret = execveat(AT_FDCWD, path, argv, NULL, AT_EMPTY_PATH | AT_CHECK);
+ if (err) {
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(errno, err);
+ } else {
+ EXPECT_EQ(0, ret);
+ }
+}
+
TEST_F_FORK(layout1, execute)
{
const struct rule rules[] = {
@@ -2025,20 +2044,27 @@ TEST_F_FORK(layout1, execute)
copy_binary(_metadata, file1_s1d2);
copy_binary(_metadata, file1_s1d3);
+ /* Checks before file1_s1d1 being denied. */
+ test_execute(_metadata, 0, file1_s1d1);
+ test_check_exec(_metadata, 0, file1_s1d1);
+
enforce_ruleset(_metadata, ruleset_fd);
ASSERT_EQ(0, close(ruleset_fd));
ASSERT_EQ(0, test_open(dir_s1d1, O_RDONLY));
ASSERT_EQ(0, test_open(file1_s1d1, O_RDONLY));
test_execute(_metadata, EACCES, file1_s1d1);
+ test_check_exec(_metadata, EACCES, file1_s1d1);
ASSERT_EQ(0, test_open(dir_s1d2, O_RDONLY));
ASSERT_EQ(0, test_open(file1_s1d2, O_RDONLY));
test_execute(_metadata, 0, file1_s1d2);
+ test_check_exec(_metadata, 0, file1_s1d2);
ASSERT_EQ(0, test_open(dir_s1d3, O_RDONLY));
ASSERT_EQ(0, test_open(file1_s1d3, O_RDONLY));
test_execute(_metadata, 0, file1_s1d3);
+ test_check_exec(_metadata, 0, file1_s1d3);
}
TEST_F_FORK(layout1, link)
--
2.46.1
^ permalink raw reply related
* [PATCH v20 2/6] security: Add EXEC_RESTRICT_FILE and EXEC_DENY_INTERACTIVE securebits
From: Mickaël Salaün @ 2024-10-11 18:44 UTC (permalink / raw)
To: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Serge Hallyn, Theodore Ts'o
Cc: Mickaël Salaün, Adhemerval Zanella Netto,
Alejandro Colomar, Aleksa Sarai, Andrew Morton, Andy Lutomirski,
Arnd Bergmann, Casey Schaufler, Christian Heimes, Dmitry Vyukov,
Elliott Hughes, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module, Andy Lutomirski
In-Reply-To: <20241011184422.977903-1-mic@digikod.net>
The new SECBIT_EXEC_RESTRICT_FILE, SECBIT_EXEC_DENY_INTERACTIVE, and
their *_LOCKED counterparts are designed to be set by processes setting
up an execution environment, such as a user session, a container, or a
security sandbox. Unlike other securebits, these ones can be set by
unprivileged processes. Like seccomp filters or Landlock domains, the
securebits are inherited across processes.
When SECBIT_EXEC_RESTRICT_FILE is set, programs interpreting code should
control executable resources according to execveat(2) + AT_CHECK (see
previous commit).
When SECBIT_EXEC_DENY_INTERACTIVE is set, a process should deny
execution of user interactive commands (which excludes executable
regular files).
Being able to configure each of these securebits enables system
administrators or owner of image containers to gradually validate the
related changes and to identify potential issues (e.g. with interpreter
or audit logs).
It should be noted that unlike other security bits, the
SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE bits are
dedicated to user space willing to restrict itself. Because of that,
they only make sense in the context of a trusted environment (e.g.
sandbox, container, user session, full system) where the process
changing its behavior (according to these bits) and all its parent
processes are trusted. Otherwise, any parent process could just execute
its own malicious code (interpreting a script or not), or even enforce a
seccomp filter to mask these bits.
Such a secure environment can be achieved with an appropriate access
control (e.g. mount's noexec option, file access rights, LSM policy) and
an enlighten ld.so checking that libraries are allowed for execution
e.g., to protect against illegitimate use of LD_PRELOAD.
Ptrace restrictions according to these securebits would not make sense
because of the processes' trust assumption.
Scripts may need some changes to deal with untrusted data (e.g. stdin,
environment variables), but that is outside the scope of the kernel.
See chromeOS's documentation about script execution control and the
related threat model:
https://www.chromium.org/chromium-os/developer-library/guides/security/noexec-shell-scripts/
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20241011184422.977903-3-mic@digikod.net
---
Changes since v19:
* Replace SECBIT_SHOULD_EXEC_CHECK and SECBIT_SHOULD_EXEC_RESTRICT with
SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE:
https://lore.kernel.org/all/20240710.eiKohpa4Phai@digikod.net/
* Remove the ptrace restrictions, suggested by Andy.
* Improve documentation according to the discussion with Jeff.
New design since v18:
https://lore.kernel.org/r/20220104155024.48023-3-mic@digikod.net
---
include/uapi/linux/securebits.h | 113 +++++++++++++++++++++++++++++++-
security/commoncap.c | 29 ++++++--
2 files changed, 135 insertions(+), 7 deletions(-)
diff --git a/include/uapi/linux/securebits.h b/include/uapi/linux/securebits.h
index d6d98877ff1a..351b6ecefc76 100644
--- a/include/uapi/linux/securebits.h
+++ b/include/uapi/linux/securebits.h
@@ -52,10 +52,121 @@
#define SECBIT_NO_CAP_AMBIENT_RAISE_LOCKED \
(issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE_LOCKED))
+/*
+ * The SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE securebits
+ * are intended for script interpreters and dynamic linkers to enforce a
+ * consistent execution security policy handled by the kernel.
+ *
+ * Whether an interpreter should check these securebits or not depends on the
+ * security risk of running malicious scripts with respect to the execution
+ * environment, and whether the kernel can check if a script is trustworthy or
+ * not. For instance, Python scripts running on a server can use arbitrary
+ * syscalls and access arbitrary files. Such interpreters should then be
+ * enlighten to use these securebits and let users define their security
+ * policy. However, a JavaScript engine running in a web browser should
+ * already be sandboxed and then should not be able to harm the user's
+ * environment.
+ *
+ * When SECBIT_EXEC_RESTRICT_FILE is set, a process should only interpret or
+ * execute a file if a call to execveat(2) with the related file descriptor and
+ * the AT_CHECK flag succeed.
+ *
+ * This secure bit may be set by user session managers, service managers,
+ * container runtimes, sandboxer tools... Except for test environments, the
+ * related SECBIT_EXEC_RESTRICT_FILE_LOCKED bit should also be set.
+ *
+ * Programs should only enforce consistent restrictions according to the
+ * securebits but without relying on any other user-controlled configuration.
+ * Indeed, the use case for these securebits is to only trust executable code
+ * vetted by the system configuration (through the kernel), so we should be
+ * careful to not let untrusted users control this configuration.
+ *
+ * However, script interpreters may still use user configuration such as
+ * environment variables as long as it is not a way to disable the securebits
+ * checks. For instance, the PATH and LD_PRELOAD variables can be set by a
+ * script's caller. Changing these variables may lead to unintended code
+ * executions, but only from vetted executable programs, which is OK. For this
+ * to make sense, the system should provide a consistent security policy to
+ * avoid arbitrary code execution e.g., by enforcing a write xor execute
+ * policy.
+ *
+ * SECBIT_EXEC_RESTRICT_FILE is complementary and should also be checked.
+ */
+#define SECURE_EXEC_RESTRICT_FILE 8
+#define SECURE_EXEC_RESTRICT_FILE_LOCKED 9 /* make bit-8 immutable */
+
+#define SECBIT_EXEC_RESTRICT_FILE (issecure_mask(SECURE_EXEC_RESTRICT_FILE))
+#define SECBIT_EXEC_RESTRICT_FILE_LOCKED \
+ (issecure_mask(SECURE_EXEC_RESTRICT_FILE_LOCKED))
+
+/*
+ * When SECBIT_EXEC_DENY_INTERACTIVE is set, a process should never interpret
+ * interactive user commands (e.g. scripts). However, if such commands are
+ * passed through a file descriptor (e.g. stdin), its content should be
+ * interpreted if a call to execveat(2) with the related file descriptor and
+ * the AT_CHECK flag succeed.
+ *
+ * For instance, script interpreters called with a script snippet as argument
+ * should always deny such execution if SECBIT_EXEC_DENY_INTERACTIVE is set.
+ *
+ * This secure bit may be set by user session managers, service managers,
+ * container runtimes, sandboxer tools... Except for test environments, the
+ * related SECBIT_EXEC_DENY_INTERACTIVE_LOCKED bit should also be set.
+ *
+ * See the SECBIT_EXEC_RESTRICT_FILE documentation.
+ *
+ * Here is the expected behavior for a script interpreter according to
+ * combination of any exec securebits:
+ *
+ * 1. SECURE_EXEC_RESTRICT_FILE=0 SECURE_EXEC_DENY_INTERACTIVE=0 (default)
+ * Always interpret scripts, and allow arbitrary user commands.
+ * => No threat, everyone and everything is trusted, but we can get ahead of
+ * potential issues thanks to the call to execveat with AT_CHECK which
+ * should always be performed but ignored by the script interpreter.
+ * Indeed, this check is still important to enable systems administrators
+ * to verify requests (e.g. with audit) and prepare for migration to a
+ * secure mode.
+ *
+ * 2. SECURE_EXEC_RESTRICT_FILE=1 SECURE_EXEC_DENY_INTERACTIVE=0
+ * Deny script interpretation if they are not executable, but allow
+ * arbitrary user commands.
+ * => The threat is (potential) malicious scripts run by trusted (and not
+ * fooled) users. That can protect against unintended script executions
+ * (e.g. sh /tmp/*.sh). This makes sense for (semi-restricted) user
+ * sessions.
+ *
+ * 3. SECURE_EXEC_RESTRICT_FILE=0 SECURE_EXEC_DENY_INTERACTIVE=1
+ * Always interpret scripts, but deny arbitrary user commands.
+ * => This use case may be useful for secure services (i.e. without
+ * interactive user session) where scripts' integrity is verified (e.g.
+ * with IMA/EVM or dm-verity/IPE) but where access rights might not be
+ * ready yet. Indeed, arbitrary interactive commands would be much more
+ * difficult to check.
+ *
+ * 4. SECURE_EXEC_RESTRICT_FILE=1 SECURE_EXEC_DENY_INTERACTIVE=1
+ * Deny script interpretation if they are not executable, and also deny
+ * any arbitrary user commands.
+ * => The threat is malicious scripts run by untrusted users (but trusted
+ * code). This makes sense for system services that may only execute
+ * trusted scripts.
+ */
+#define SECURE_EXEC_DENY_INTERACTIVE 10
+#define SECURE_EXEC_DENY_INTERACTIVE_LOCKED 11 /* make bit-10 immutable */
+
+#define SECBIT_EXEC_DENY_INTERACTIVE \
+ (issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
+#define SECBIT_EXEC_DENY_INTERACTIVE_LOCKED \
+ (issecure_mask(SECURE_EXEC_DENY_INTERACTIVE_LOCKED))
+
#define SECURE_ALL_BITS (issecure_mask(SECURE_NOROOT) | \
issecure_mask(SECURE_NO_SETUID_FIXUP) | \
issecure_mask(SECURE_KEEP_CAPS) | \
- issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE))
+ issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE) | \
+ issecure_mask(SECURE_EXEC_RESTRICT_FILE) | \
+ issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
#define SECURE_ALL_LOCKS (SECURE_ALL_BITS << 1)
+#define SECURE_ALL_UNPRIVILEGED (issecure_mask(SECURE_EXEC_RESTRICT_FILE) | \
+ issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
+
#endif /* _UAPI_LINUX_SECUREBITS_H */
diff --git a/security/commoncap.c b/security/commoncap.c
index cefad323a0b1..52ea01acb453 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -1302,21 +1302,38 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
& (old->securebits ^ arg2)) /*[1]*/
|| ((old->securebits & SECURE_ALL_LOCKS & ~arg2)) /*[2]*/
|| (arg2 & ~(SECURE_ALL_LOCKS | SECURE_ALL_BITS)) /*[3]*/
- || (cap_capable(current_cred(),
- current_cred()->user_ns,
- CAP_SETPCAP,
- CAP_OPT_NONE) != 0) /*[4]*/
/*
* [1] no changing of bits that are locked
* [2] no unlocking of locks
* [3] no setting of unsupported bits
- * [4] doing anything requires privilege (go read about
- * the "sendmail capabilities bug")
*/
)
/* cannot change a locked bit */
return -EPERM;
+ /*
+ * Doing anything requires privilege (go read about the
+ * "sendmail capabilities bug"), except for unprivileged bits.
+ * Indeed, the SECURE_ALL_UNPRIVILEGED bits are not
+ * restrictions enforced by the kernel but by user space on
+ * itself.
+ */
+ if (cap_capable(current_cred(), current_cred()->user_ns,
+ CAP_SETPCAP, CAP_OPT_NONE) != 0) {
+ const unsigned long unpriv_and_locks =
+ SECURE_ALL_UNPRIVILEGED |
+ SECURE_ALL_UNPRIVILEGED << 1;
+ const unsigned long changed = old->securebits ^ arg2;
+
+ /* For legacy reason, denies non-change. */
+ if (!changed)
+ return -EPERM;
+
+ /* Denies privileged changes. */
+ if (changed & ~unpriv_and_locks)
+ return -EPERM;
+ }
+
new = prepare_creds();
if (!new)
return -ENOMEM;
--
2.46.1
^ permalink raw reply related
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