* Re: [PATCH v4 00/30] NT synchronization primitive driver
From: Elizabeth Figura @ 2024-05-15 23:32 UTC (permalink / raw)
To: Peter Zijlstra, wine-devel
Cc: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan,
linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Andy Lutomirski, linux-doc, linux-kselftest,
Randy Dunlap, Ingo Molnar, Will Deacon, Waiman Long, Boqun Feng,
Elizabeth Figura
In-Reply-To: <1790266.VLH7GnMWUR@camazotz>
On Wednesday, April 17, 2024 3:02:13 PM CDT Elizabeth Figura wrote:
> > > Except for the "unowned" semantics of zero, the actual value of the
> > > owner identifier is not interpreted by the ntsync driver at all. The
> > > intended use is to store a thread identifier; however, the ntsync
> > > driver does not actually validate that a calling thread provides
> > > consistent or unique identifiers.
> >
> > Why not verify it? Seems simple enough to put in a TID check, esp. if NT
> > mandates the same.
>
> I mostly figured it'd be simplest to leave the driver completely
> agnostic, but I don't think there's any reason we can't use the real
> TID for most calls.
While trying to implement this I did realize a reason: if a Linux thread dies
and a new Wine thread is created which happens to have the same Linux TID
*before* Wine notices the thread death, that thread's TID will be conflated
with the thread that died. I don't think we can guarantee that we notice
thread death before we notice a request to create a new Wine thread.
Using Wine-managed TIDs avoids this by virtue of ensuring that a Wine TID is
not reused until the associated Wine thread has been cleaned up.
^ permalink raw reply
* Re: [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support
From: Oleg Nesterov @ 2024-05-15 15:42 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: olsajiri@gmail.com, songliubraving@fb.com, luto@kernel.org,
mhiramat@kernel.org, andrii@kernel.org, debug@rivosinc.com,
john.fastabend@gmail.com, linux-api@vger.kernel.org,
linux-kernel@vger.kernel.org, mingo@redhat.com,
rostedt@goodmis.org, ast@kernel.org, tglx@linutronix.de,
linux-man@vger.kernel.org, yhs@fb.com, daniel@iogearbox.net,
peterz@infradead.org, linux-trace-kernel@vger.kernel.org,
bp@alien8.de, bpf@vger.kernel.org, x86@kernel.org
In-Reply-To: <0fa9634e9ac0d30d513eefe6099f5d8d354d93c1.camel@intel.com>
On 05/15, Edgecombe, Rick P wrote:
>
> On Wed, 2024-05-15 at 13:35 +0200, Oleg Nesterov wrote:
> >
> > > I'm ok with not using optimized uretprobe when shadow stack is detected
> > > as enabled and we go with current uretprobe in that case
> >
> > But how can we detect it? Again, suppose userspace does
>
> the rdssp instruction returns the value of the shadow stack pointer. On non-
> shadow stack it is a nop. So you could check if the SSP is non-zero to find if
> shadow stack is enabled.
But again, the ret-probed function can enable it before it returns? And we
need to check if it is enabled on the function entry if we want to avoid
sys_uretprobe() in this case. Although I don't understand why we want to
avoid it.
> This would catch most cases, but I guess there is the
> possibility of it getting enabled in a signal that hit between checking and the
> rest of operation.
Or from signal handler.
> Is this uretprobe stuff signal safe in general?
In what sense?
I forgot everything about this code but I can't recall any problem with signals.
Except it doesn't support sigaltstack() + siglongjmp().
Oleg.
^ permalink raw reply
* Re: [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support
From: Edgecombe, Rick P @ 2024-05-15 15:31 UTC (permalink / raw)
To: olsajiri@gmail.com, oleg@redhat.com
Cc: songliubraving@fb.com, luto@kernel.org, mhiramat@kernel.org,
andrii@kernel.org, debug@rivosinc.com, john.fastabend@gmail.com,
linux-api@vger.kernel.org, linux-kernel@vger.kernel.org,
mingo@redhat.com, rostedt@goodmis.org, ast@kernel.org,
tglx@linutronix.de, linux-man@vger.kernel.org, yhs@fb.com,
daniel@iogearbox.net, peterz@infradead.org,
linux-trace-kernel@vger.kernel.org, bp@alien8.de,
bpf@vger.kernel.org, x86@kernel.org
In-Reply-To: <20240515152609.GD6821@redhat.com>
On Wed, 2024-05-15 at 17:26 +0200, Oleg Nesterov wrote:
> > I think it will crash, there's explanation in the comment in
> > tools/testing/selftests/x86/test_shadow_stack.c test
>
> OK, thanks...
>
> But test_shadow_stack.c doesn't do ARCH_PRCTL(ARCH_SHSTK_DISABLE) if
> all the tests succeed ? Confused but nevermind.
The last test disables shadow stack as part of the test. So if it succeeds it
doesn't need to disable the shadow stack to prevent underflow.
^ permalink raw reply
* Re: [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support
From: Oleg Nesterov @ 2024-05-15 15:26 UTC (permalink / raw)
To: Jiri Olsa
Cc: Deepak Gupta, Edgecombe, Rick P, songliubraving@fb.com,
luto@kernel.org, mhiramat@kernel.org, andrii@kernel.org,
john.fastabend@gmail.com, linux-api@vger.kernel.org,
linux-kernel@vger.kernel.org, mingo@redhat.com,
rostedt@goodmis.org, ast@kernel.org, tglx@linutronix.de,
linux-man@vger.kernel.org, yhs@fb.com, daniel@iogearbox.net,
peterz@infradead.org, linux-trace-kernel@vger.kernel.org,
bp@alien8.de, bpf@vger.kernel.org, x86@kernel.org
In-Reply-To: <ZkTIU1QUAJF0f0KK@krava>
On 05/15, Jiri Olsa wrote:
>
> On Wed, May 15, 2024 at 01:19:20PM +0200, Oleg Nesterov wrote:
> > Let me ask a couple of really stupid questions. What if the shadow stack
> > is "shorter" than the normal stack? I mean,
> >
> > enable_shstk()
> > {
> > prctl(ARCH_SHSTK_SHSTK);
I meant ARCH_SHSTK_ENABLE, of course
> > }
> >
> > what happens when enable_shstk() returns?
>
> I think it will crash, there's explanation in the comment in
> tools/testing/selftests/x86/test_shadow_stack.c test
OK, thanks...
But test_shadow_stack.c doesn't do ARCH_PRCTL(ARCH_SHSTK_DISABLE) if
all the tests succeed ? Confused but nevermind.
> > And what is the purpose of fpregs_lock_and_load() ? Why do we need to
> > fpregs_restore_userregs() in shstk_setup() and other places?
> >
> > Oleg.
> >
>
^ permalink raw reply
* Re: [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support
From: Edgecombe, Rick P @ 2024-05-15 15:18 UTC (permalink / raw)
To: olsajiri@gmail.com, oleg@redhat.com
Cc: songliubraving@fb.com, luto@kernel.org, mhiramat@kernel.org,
andrii@kernel.org, debug@rivosinc.com, john.fastabend@gmail.com,
linux-api@vger.kernel.org, linux-kernel@vger.kernel.org,
mingo@redhat.com, rostedt@goodmis.org, ast@kernel.org,
tglx@linutronix.de, linux-man@vger.kernel.org, yhs@fb.com,
daniel@iogearbox.net, peterz@infradead.org,
linux-trace-kernel@vger.kernel.org, bp@alien8.de,
bpf@vger.kernel.org, x86@kernel.org
In-Reply-To: <ZkTIU1QUAJF0f0KK@krava>
On Wed, 2024-05-15 at 08:36 -0600, Jiri Olsa wrote:
> >
> > Let me ask a couple of really stupid questions. What if the shadow stack
> > is "shorter" than the normal stack? I mean,
The shadow stack could overflow if it is not big enough. However since the
normal stack has return addresses and data, and shadow stack has the smaller
amount data of only return addresses, we can mostly avoid this by picking a
large size for the shadow stack.
For underflow, you can't return from the point where you enable shadow stack.
Almost all uses will enable it very early. Glibc loader does it before main is
reached, for example. The shadow stack selftest is not a typical usage in this
respect.
^ permalink raw reply
* Re: [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support
From: Edgecombe, Rick P @ 2024-05-15 15:13 UTC (permalink / raw)
To: olsajiri@gmail.com, oleg@redhat.com
Cc: songliubraving@fb.com, luto@kernel.org, mhiramat@kernel.org,
andrii@kernel.org, debug@rivosinc.com, john.fastabend@gmail.com,
linux-api@vger.kernel.org, linux-kernel@vger.kernel.org,
mingo@redhat.com, rostedt@goodmis.org, ast@kernel.org,
tglx@linutronix.de, linux-man@vger.kernel.org, yhs@fb.com,
daniel@iogearbox.net, peterz@infradead.org,
linux-trace-kernel@vger.kernel.org, bp@alien8.de,
bpf@vger.kernel.org, x86@kernel.org
In-Reply-To: <20240515113525.GB6821@redhat.com>
On Wed, 2024-05-15 at 13:35 +0200, Oleg Nesterov wrote:
> Let me repeat I know nothing about shadow stacks, only tried to
> read Documentation/arch/x86/shstk.rst few minutes ago ;)
>
> On 05/13, Jiri Olsa wrote:
> >
> > 1) current uretprobe which are not working at the moment and we change
> > the top value of shadow stack with shstk_push_frame
> > 2) optimized uretprobe which needs to push new frame on shadow stack
> > with shstk_update_last_frame
> >
> > I think we should do 1) and have current uretprobe working with shadow
> > stack, which is broken at the moment
>
> Agreed,
>
> > I'm ok with not using optimized uretprobe when shadow stack is detected
> > as enabled and we go with current uretprobe in that case
>
> But how can we detect it? Again, suppose userspace does
the rdssp instruction returns the value of the shadow stack pointer. On non-
shadow stack it is a nop. So you could check if the SSP is non-zero to find if
shadow stack is enabled. This would catch most cases, but I guess there is the
possibility of it getting enabled in a signal that hit between checking and the
rest of operation. Is this uretprobe stuff signal safe in general?
^ permalink raw reply
* Re: [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support
From: Jiri Olsa @ 2024-05-15 14:36 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Deepak Gupta, Edgecombe, Rick P, olsajiri@gmail.com,
songliubraving@fb.com, luto@kernel.org, mhiramat@kernel.org,
andrii@kernel.org, john.fastabend@gmail.com,
linux-api@vger.kernel.org, linux-kernel@vger.kernel.org,
mingo@redhat.com, rostedt@goodmis.org, ast@kernel.org,
tglx@linutronix.de, linux-man@vger.kernel.org, yhs@fb.com,
daniel@iogearbox.net, peterz@infradead.org,
linux-trace-kernel@vger.kernel.org, bp@alien8.de,
bpf@vger.kernel.org, x86@kernel.org
In-Reply-To: <20240515111919.GA6821@redhat.com>
On Wed, May 15, 2024 at 01:19:20PM +0200, Oleg Nesterov wrote:
> Sorry for the late reply, I was on PTO.
>
> On 05/14, Deepak Gupta wrote:
> >
> > Question,
> >
> > Is it kernel who is maintaining all return probes, meaning original return addresses
> > are saved in kernel data structures on per task basis.
>
> Yes. task_struct->utask->return_instances
>
> See prepare_uretprobe() which inserts the new return_instance with
> ->orig_ret_vaddr = original return addresses
> when the tracee enters the ret-probed function.
>
> > Once uretprobe did its job then
> > its kernel who is ensuring return to original return address ?
>
> Yes. See instruction_pointer_set(regs, ri->orig_ret_vaddr) in
> handle_trampoline().
>
>
>
> I know absolutely nothing about the shadow stacks, trying to read
> Documentation/arch/x86/shstk.rst but it doesn't tell me too much...
> Where can I find more documentation? I didn't try to google yet.
>
> Upon function return, the processor pops the shadow stack copy
> and compares it to the normal stack copy. If the two differ, the
> processor raises a control-protection fault.
>
> grep-grep-grep... exc_control_protection I guess.
>
> Let me ask a couple of really stupid questions. What if the shadow stack
> is "shorter" than the normal stack? I mean,
>
> enable_shstk()
> {
> prctl(ARCH_SHSTK_SHSTK);
> }
>
> what happens when enable_shstk() returns?
I think it will crash, there's explanation in the comment in
tools/testing/selftests/x86/test_shadow_stack.c test
that's why ARCH_PRCTL is using syscall instruction directly and
not calling syscall function
jirka
> And what is the purpose of fpregs_lock_and_load() ? Why do we need to
> fpregs_restore_userregs() in shstk_setup() and other places?
>
> Oleg.
>
^ permalink raw reply
* Re: [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support
From: Oleg Nesterov @ 2024-05-15 11:35 UTC (permalink / raw)
To: Jiri Olsa
Cc: Edgecombe, Rick P, mhiramat@kernel.org, songliubraving@fb.com,
luto@kernel.org, andrii@kernel.org, debug@rivosinc.com,
john.fastabend@gmail.com, linux-api@vger.kernel.org,
linux-kernel@vger.kernel.org, mingo@redhat.com,
rostedt@goodmis.org, ast@kernel.org, tglx@linutronix.de,
yhs@fb.com, linux-man@vger.kernel.org, daniel@iogearbox.net,
peterz@infradead.org, linux-trace-kernel@vger.kernel.org,
bp@alien8.de, bpf@vger.kernel.org, x86@kernel.org
In-Reply-To: <ZkKE3qT1X_Jirb92@krava>
Let me repeat I know nothing about shadow stacks, only tried to
read Documentation/arch/x86/shstk.rst few minutes ago ;)
On 05/13, Jiri Olsa wrote:
>
> 1) current uretprobe which are not working at the moment and we change
> the top value of shadow stack with shstk_push_frame
> 2) optimized uretprobe which needs to push new frame on shadow stack
> with shstk_update_last_frame
>
> I think we should do 1) and have current uretprobe working with shadow
> stack, which is broken at the moment
Agreed,
> I'm ok with not using optimized uretprobe when shadow stack is detected
> as enabled and we go with current uretprobe in that case
But how can we detect it? Again, suppose userspace does
enable_shstk()
{
prctl(ARCH_SHSTK_SHSTK);
}
what if enable_shstk() is ret-probed ?
Oleg.
^ permalink raw reply
* Re: [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support
From: Oleg Nesterov @ 2024-05-15 11:19 UTC (permalink / raw)
To: Deepak Gupta
Cc: Edgecombe, Rick P, olsajiri@gmail.com, songliubraving@fb.com,
luto@kernel.org, mhiramat@kernel.org, andrii@kernel.org,
john.fastabend@gmail.com, linux-api@vger.kernel.org,
linux-kernel@vger.kernel.org, mingo@redhat.com,
rostedt@goodmis.org, ast@kernel.org, tglx@linutronix.de,
linux-man@vger.kernel.org, yhs@fb.com, daniel@iogearbox.net,
peterz@infradead.org, linux-trace-kernel@vger.kernel.org,
bp@alien8.de, bpf@vger.kernel.org, x86@kernel.org
In-Reply-To: <ZkQTgQ3aKU4MAjPu@debug.ba.rivosinc.com>
Sorry for the late reply, I was on PTO.
On 05/14, Deepak Gupta wrote:
>
> Question,
>
> Is it kernel who is maintaining all return probes, meaning original return addresses
> are saved in kernel data structures on per task basis.
Yes. task_struct->utask->return_instances
See prepare_uretprobe() which inserts the new return_instance with
->orig_ret_vaddr = original return addresses
when the tracee enters the ret-probed function.
> Once uretprobe did its job then
> its kernel who is ensuring return to original return address ?
Yes. See instruction_pointer_set(regs, ri->orig_ret_vaddr) in
handle_trampoline().
I know absolutely nothing about the shadow stacks, trying to read
Documentation/arch/x86/shstk.rst but it doesn't tell me too much...
Where can I find more documentation? I didn't try to google yet.
Upon function return, the processor pops the shadow stack copy
and compares it to the normal stack copy. If the two differ, the
processor raises a control-protection fault.
grep-grep-grep... exc_control_protection I guess.
Let me ask a couple of really stupid questions. What if the shadow stack
is "shorter" than the normal stack? I mean,
enable_shstk()
{
prctl(ARCH_SHSTK_SHSTK);
}
what happens when enable_shstk() returns?
And what is the purpose of fpregs_lock_and_load() ? Why do we need to
fpregs_restore_userregs() in shstk_setup() and other places?
Oleg.
^ permalink raw reply
* Re: [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support
From: Deepak Gupta @ 2024-05-15 1:44 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: olsajiri@gmail.com, songliubraving@fb.com, luto@kernel.org,
mhiramat@kernel.org, andrii@kernel.org, john.fastabend@gmail.com,
linux-api@vger.kernel.org, linux-kernel@vger.kernel.org,
mingo@redhat.com, rostedt@goodmis.org, ast@kernel.org,
tglx@linutronix.de, linux-man@vger.kernel.org, oleg@redhat.com,
yhs@fb.com, daniel@iogearbox.net, peterz@infradead.org,
linux-trace-kernel@vger.kernel.org, bp@alien8.de,
bpf@vger.kernel.org, x86@kernel.org
In-Reply-To: <3e15152888d543d2ee4e5a1d75298c80aa946659.camel@intel.com>
On Wed, May 15, 2024 at 01:10:03AM +0000, Edgecombe, Rick P wrote:
>On Mon, 2024-05-13 at 15:23 -0600, Jiri Olsa wrote:
>> so at the moment the patch 6 changes shadow stack for
>>
>> 1) current uretprobe which are not working at the moment and we change
>> the top value of shadow stack with shstk_push_frame
>> 2) optimized uretprobe which needs to push new frame on shadow stack
>> with shstk_update_last_frame
>>
>> I think we should do 1) and have current uretprobe working with shadow
>> stack, which is broken at the moment
>>
>> I'm ok with not using optimized uretprobe when shadow stack is detected
>> as enabled and we go with current uretprobe in that case
>>
>> would this work for you?
>
>Sorry for the delay. It seems reasonable to me due to 1 being at a fixed address
>where 2 was arbitrary address. But Peterz might have felt the opposite earlier.
>Not sure.
>
>I'd also love to get some second opinions from broonie (arm shadow stack) and
>Deepak (riscv shadow stack).
>
>Deepak, even if riscv has a special instruction that pushes to the shadow stack,
>will it be ok if there is a callable operation that does the same thing? Like,
>aren't you relying on endbranches or the compiler or something such that
>arbitrary data can't be pushed via that instruction?
Instruction is `sspush x1/ra`. It pushes contents of register return address (ra
also called x1) onto shadow stack. `ra` is like arm's equivalent of link register.
Prologue of function is supposed to have `sspush x1` to save it away.
ISA doesn't allow encodings with register in risc-v GPRs (except register x5
because some embedded riscv space toolchains have used x5 as ra too).
On question of callable operation, I think still need to fully understand who manages
the probe and forward progress.
Question,
Is it kernel who is maintaining all return probes, meaning original return addresses
are saved in kernel data structures on per task basis. Once uretprobe did its job then
its kernel who is ensuring return to original return address ?
>
>BTW Jiri, thanks for considering shadow stack in your work.
^ permalink raw reply
* Re: [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support
From: Edgecombe, Rick P @ 2024-05-15 1:10 UTC (permalink / raw)
To: olsajiri@gmail.com
Cc: songliubraving@fb.com, luto@kernel.org, mhiramat@kernel.org,
andrii@kernel.org, debug@rivosinc.com, john.fastabend@gmail.com,
linux-api@vger.kernel.org, linux-kernel@vger.kernel.org,
mingo@redhat.com, rostedt@goodmis.org, ast@kernel.org,
tglx@linutronix.de, linux-man@vger.kernel.org, oleg@redhat.com,
yhs@fb.com, daniel@iogearbox.net, peterz@infradead.org,
linux-trace-kernel@vger.kernel.org, bp@alien8.de,
bpf@vger.kernel.org, x86@kernel.org
In-Reply-To: <ZkKE3qT1X_Jirb92@krava>
On Mon, 2024-05-13 at 15:23 -0600, Jiri Olsa wrote:
> so at the moment the patch 6 changes shadow stack for
>
> 1) current uretprobe which are not working at the moment and we change
> the top value of shadow stack with shstk_push_frame
> 2) optimized uretprobe which needs to push new frame on shadow stack
> with shstk_update_last_frame
>
> I think we should do 1) and have current uretprobe working with shadow
> stack, which is broken at the moment
>
> I'm ok with not using optimized uretprobe when shadow stack is detected
> as enabled and we go with current uretprobe in that case
>
> would this work for you?
Sorry for the delay. It seems reasonable to me due to 1 being at a fixed address
where 2 was arbitrary address. But Peterz might have felt the opposite earlier.
Not sure.
I'd also love to get some second opinions from broonie (arm shadow stack) and
Deepak (riscv shadow stack).
Deepak, even if riscv has a special instruction that pushes to the shadow stack,
will it be ok if there is a callable operation that does the same thing? Like,
aren't you relying on endbranches or the compiler or something such that
arbitrary data can't be pushed via that instruction?
BTW Jiri, thanks for considering shadow stack in your work.
^ permalink raw reply
* Re: [PATCH v4 02/27] ntsync: Introduce NTSYNC_IOC_WAIT_ALL.
From: Elizabeth Figura @ 2024-05-14 4:15 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan,
linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Andy Lutomirski, linux-doc,
linux-kselftest, Randy Dunlap, Ingo Molnar, Will Deacon,
Waiman Long, Boqun Feng
In-Reply-To: <20240419162814.GA39162@noisy.programming.kicks-ass.net>
On Friday, April 19, 2024 11:28:14 AM CDT Peter Zijlstra wrote:
> On Thu, Apr 18, 2024 at 11:35:11AM +0200, Peter Zijlstra wrote:
> > On Wed, Apr 17, 2024 at 03:03:05PM -0500, Elizabeth Figura wrote:
> > > Ach. I wrote this with the idea that the race isn't meaningful, but
> > > looking at it again you're right—there is a harmful race here.
> > >
> > > I think it should be fixable by moving the atomic_read inside the lock,
> > > though.
> >
> > Right, I've ended up with the (as yet untested) below. I'll see if I can
> > find time later to actually test things.
>
> Latest hackery... I tried testing this but I'm not having luck using the
> patched wine as per the other email.
>
I converted the rest of the direct uses of spin_lock() using the below patch
and tested it myself, and it passes Wine tests. As far as I can tell the logic
is correct, too; I couldn't find any races.
I'll incorporate these changes into the next revision, unless there's a good
reason not to.
---
--- a/drivers/misc/ntsync.c
+++ b/drivers/misc/ntsync.c
@@ -569,17 +569,19 @@ static int ntsync_event_set(struct ntsync_obj *event,
void __user *argp, bool pu
static int ntsync_event_reset(struct ntsync_obj *event, void __user *argp)
{
+ struct ntsync_device *dev = event->dev;
__u32 prev_state;
+ bool all;
if (event->type != NTSYNC_TYPE_EVENT)
return -EINVAL;
- spin_lock(&event->lock);
+ all = ntsync_lock_obj(dev, event);
prev_state = event->u.event.signaled;
event->u.event.signaled = false;
- spin_unlock(&event->lock);
+ ntsync_unlock_obj(dev, event, all);
if (put_user(prev_state, (__u32 __user *)argp))
return -EFAULT;
@@ -590,16 +592,21 @@ static int ntsync_event_reset(struct ntsync_obj *event,
void __user *argp)
static int ntsync_sem_read(struct ntsync_obj *sem, void __user *argp)
{
struct ntsync_sem_args __user *user_args = argp;
+ struct ntsync_device *dev = sem->dev;
struct ntsync_sem_args args;
+ bool all;
if (sem->type != NTSYNC_TYPE_SEM)
return -EINVAL;
args.sem = 0;
- spin_lock(&sem->lock);
+
+ all = ntsync_lock_obj(dev, sem);
+
args.count = sem->u.sem.count;
args.max = sem->u.sem.max;
- spin_unlock(&sem->lock);
+
+ ntsync_unlock_obj(dev, sem, all);
if (copy_to_user(user_args, &args, sizeof(args)))
return -EFAULT;
@@ -609,18 +616,23 @@ static int ntsync_sem_read(struct ntsync_obj *sem, void
__user *argp)
static int ntsync_mutex_read(struct ntsync_obj *mutex, void __user *argp)
{
struct ntsync_mutex_args __user *user_args = argp;
+ struct ntsync_device *dev = mutex->dev;
struct ntsync_mutex_args args;
+ bool all;
int ret;
if (mutex->type != NTSYNC_TYPE_MUTEX)
return -EINVAL;
args.mutex = 0;
- spin_lock(&mutex->lock);
+
+ all = ntsync_lock_obj(dev, mutex);
+
args.count = mutex->u.mutex.count;
args.owner = mutex->u.mutex.owner;
ret = mutex->u.mutex.ownerdead ? -EOWNERDEAD : 0;
- spin_unlock(&mutex->lock);
+
+ ntsync_unlock_obj(dev, mutex, all);
if (copy_to_user(user_args, &args, sizeof(args)))
return -EFAULT;
@@ -630,16 +642,21 @@ static int ntsync_mutex_read(struct ntsync_obj *mutex,
void __user *argp)
static int ntsync_event_read(struct ntsync_obj *event, void __user *argp)
{
struct ntsync_event_args __user *user_args = argp;
+ struct ntsync_device *dev = event->dev;
struct ntsync_event_args args;
+ bool all;
if (event->type != NTSYNC_TYPE_EVENT)
return -EINVAL;
args.event = 0;
- spin_lock(&event->lock);
+
+ all = ntsync_lock_obj(dev, event);
+
args.manual = event->u.event.manual;
args.signaled = event->u.event.signaled;
- spin_unlock(&event->lock);
+
+ ntsync_unlock_obj(dev, event, all);
if (copy_to_user(user_args, &args, sizeof(args)))
return -EFAULT;
@@ -962,6 +979,7 @@ static int ntsync_wait_any(struct ntsync_device *dev, void
__user *argp)
__u32 i, total_count;
struct ntsync_q *q;
int signaled;
+ bool all;
int ret;
if (copy_from_user(&args, argp, sizeof(args)))
@@ -981,9 +999,9 @@ static int ntsync_wait_any(struct ntsync_device *dev, void
__user *argp)
struct ntsync_q_entry *entry = &q->entries[i];
struct ntsync_obj *obj = entry->obj;
- spin_lock(&obj->lock);
+ all = ntsync_lock_obj(dev, obj);
list_add_tail(&entry->node, &obj->any_waiters);
- spin_unlock(&obj->lock);
+ ntsync_unlock_obj(dev, obj, all);
}
/*
@@ -1000,9 +1018,9 @@ static int ntsync_wait_any(struct ntsync_device *dev,
void __user *argp)
if (atomic_read(&q->signaled) != -1)
break;
- spin_lock(&obj->lock);
+ all = ntsync_lock_obj(dev, obj);
try_wake_any_obj(obj);
- spin_unlock(&obj->lock);
+ ntsync_unlock_obj(dev, obj, all);
}
/* sleep */
@@ -1015,9 +1033,9 @@ static int ntsync_wait_any(struct ntsync_device *dev,
void __user *argp)
struct ntsync_q_entry *entry = &q->entries[i];
struct ntsync_obj *obj = entry->obj;
- spin_lock(&obj->lock);
+ all = ntsync_lock_obj(dev, obj);
list_del(&entry->node);
- spin_unlock(&obj->lock);
+ ntsync_unlock_obj(dev, obj, all);
put_obj(obj);
}
@@ -1075,9 +1093,9 @@ static int ntsync_wait_all(struct ntsync_device *dev,
void __user *argp)
struct ntsync_q_entry *entry = &q->entries[args.count];
struct ntsync_obj *obj = entry->obj;
- spin_lock_nest_lock(&obj->lock, &dev->wait_all_lock);
+ dev_lock_obj(dev, obj);
list_add_tail(&entry->node, &obj->any_waiters);
- spin_unlock(&obj->lock);
+ dev_unlock_obj(dev, obj);
}
/* check if we are already signaled */
@@ -1095,9 +1113,9 @@ static int ntsync_wait_all(struct ntsync_device *dev,
void __user *argp)
struct ntsync_obj *obj = q->entries[args.count].obj;
if (atomic_read(&q->signaled) == -1) {
- spin_lock(&obj->lock);
+ dev_lock_obj(dev, obj);
try_wake_any_obj(obj);
- spin_unlock(&obj->lock);
+ dev_unlock_obj(dev, obj);
}
}
@@ -1127,9 +1145,9 @@ static int ntsync_wait_all(struct ntsync_device *dev,
void __user *argp)
struct ntsync_q_entry *entry = &q->entries[args.count];
struct ntsync_obj *obj = entry->obj;
- spin_lock_nest_lock(&obj->lock, &dev->wait_all_lock);
+ dev_lock_obj(dev, obj);
list_del(&entry->node);
- spin_unlock(&obj->lock);
+ dev_unlock_obj(dev, obj);
put_obj(obj);
}
^ permalink raw reply
* Re: [PATCHv5 bpf-next 7/8] selftests/x86: Add return uprobe shadow stack test
From: Jiri Olsa @ 2024-05-13 21:28 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Steven Rostedt, Oleg Nesterov, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, linux-kernel,
linux-trace-kernel, linux-api, linux-man, x86, bpf, Song Liu,
Yonghong Song, John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov (AMD), Ingo Molnar, Andy Lutomirski,
Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240513184507.215ec89dea4790243d17a52c@kernel.org>
On Mon, May 13, 2024 at 06:45:07PM +0900, Masami Hiramatsu wrote:
> On Tue, 7 May 2024 12:53:20 +0200
> Jiri Olsa <jolsa@kernel.org> wrote:
>
> > Adding return uprobe test for shadow stack and making sure it's
> > working properly. Borrowed some of the code from bpf selftests.
>
> Hi Jiri,
>
> I can not find "SKIP" result in this change. If CONFIG_UPROBES=n,
> this should skip uprobe test.
ah it should be detected by parse_uint_from_file returning ENOENT
or something like that.. will add that
thanks,
jirka
>
> Thank you,
>
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > .../testing/selftests/x86/test_shadow_stack.c | 142 ++++++++++++++++++
> > 1 file changed, 142 insertions(+)
> >
> > diff --git a/tools/testing/selftests/x86/test_shadow_stack.c b/tools/testing/selftests/x86/test_shadow_stack.c
> > index 757e6527f67e..1b919baa999b 100644
> > --- a/tools/testing/selftests/x86/test_shadow_stack.c
> > +++ b/tools/testing/selftests/x86/test_shadow_stack.c
> > @@ -34,6 +34,7 @@
> > #include <sys/ptrace.h>
> > #include <sys/signal.h>
> > #include <linux/elf.h>
> > +#include <linux/perf_event.h>
> >
> > /*
> > * Define the ABI defines if needed, so people can run the tests
> > @@ -681,6 +682,141 @@ int test_32bit(void)
> > return !segv_triggered;
> > }
> >
> > +static int parse_uint_from_file(const char *file, const char *fmt)
> > +{
> > + int err, ret;
> > + FILE *f;
> > +
> > + f = fopen(file, "re");
> > + if (!f) {
> > + err = -errno;
> > + printf("failed to open '%s': %d\n", file, err);
> > + return err;
> > + }
> > + err = fscanf(f, fmt, &ret);
> > + if (err != 1) {
> > + err = err == EOF ? -EIO : -errno;
> > + printf("failed to parse '%s': %d\n", file, err);
> > + fclose(f);
> > + return err;
> > + }
> > + fclose(f);
> > + return ret;
> > +}
> > +
> > +static int determine_uprobe_perf_type(void)
> > +{
> > + const char *file = "/sys/bus/event_source/devices/uprobe/type";
> > +
> > + return parse_uint_from_file(file, "%d\n");
> > +}
> > +
> > +static int determine_uprobe_retprobe_bit(void)
> > +{
> > + const char *file = "/sys/bus/event_source/devices/uprobe/format/retprobe";
> > +
> > + return parse_uint_from_file(file, "config:%d\n");
> > +}
> > +
> > +static ssize_t get_uprobe_offset(const void *addr)
> > +{
> > + size_t start, end, base;
> > + char buf[256];
> > + bool found = false;
> > + FILE *f;
> > +
> > + f = fopen("/proc/self/maps", "r");
> > + if (!f)
> > + return -errno;
> > +
> > + while (fscanf(f, "%zx-%zx %s %zx %*[^\n]\n", &start, &end, buf, &base) == 4) {
> > + if (buf[2] == 'x' && (uintptr_t)addr >= start && (uintptr_t)addr < end) {
> > + found = true;
> > + break;
> > + }
> > + }
> > +
> > + fclose(f);
> > +
> > + if (!found)
> > + return -ESRCH;
> > +
> > + return (uintptr_t)addr - start + base;
> > +}
> > +
> > +static __attribute__((noinline)) void uretprobe_trigger(void)
> > +{
> > + asm volatile ("");
> > +}
> > +
> > +/*
> > + * This test setups return uprobe, which is sensitive to shadow stack
> > + * (crashes without extra fix). After executing the uretprobe we fail
> > + * the test if we receive SIGSEGV, no crash means we're good.
> > + *
> > + * Helper functions above borrowed from bpf selftests.
> > + */
> > +static int test_uretprobe(void)
> > +{
> > + const size_t attr_sz = sizeof(struct perf_event_attr);
> > + const char *file = "/proc/self/exe";
> > + int bit, fd = 0, type, err = 1;
> > + struct perf_event_attr attr;
> > + struct sigaction sa = {};
> > + ssize_t offset;
> > +
> > + type = determine_uprobe_perf_type();
> > + if (type < 0)
> > + return 1;
> > +
> > + offset = get_uprobe_offset(uretprobe_trigger);
> > + if (offset < 0)
> > + return 1;
> > +
> > + bit = determine_uprobe_retprobe_bit();
> > + if (bit < 0)
> > + return 1;
> > +
> > + sa.sa_sigaction = segv_gp_handler;
> > + sa.sa_flags = SA_SIGINFO;
> > + if (sigaction(SIGSEGV, &sa, NULL))
> > + return 1;
> > +
> > + /* Setup return uprobe through perf event interface. */
> > + memset(&attr, 0, attr_sz);
> > + attr.size = attr_sz;
> > + attr.type = type;
> > + attr.config = 1 << bit;
> > + attr.config1 = (__u64) (unsigned long) file;
> > + attr.config2 = offset;
> > +
> > + fd = syscall(__NR_perf_event_open, &attr, 0 /* pid */, -1 /* cpu */,
> > + -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
> > + if (fd < 0)
> > + goto out;
> > +
> > + if (sigsetjmp(jmp_buffer, 1))
> > + goto out;
> > +
> > + ARCH_PRCTL(ARCH_SHSTK_ENABLE, ARCH_SHSTK_SHSTK);
> > +
> > + /*
> > + * This either segfaults and goes through sigsetjmp above
> > + * or succeeds and we're good.
> > + */
> > + uretprobe_trigger();
> > +
> > + printf("[OK]\tUretprobe test\n");
> > + err = 0;
> > +
> > +out:
> > + ARCH_PRCTL(ARCH_SHSTK_DISABLE, ARCH_SHSTK_SHSTK);
> > + signal(SIGSEGV, SIG_DFL);
> > + if (fd)
> > + close(fd);
> > + return err;
> > +}
> > +
> > void segv_handler_ptrace(int signum, siginfo_t *si, void *uc)
> > {
> > /* The SSP adjustment caused a segfault. */
> > @@ -867,6 +1003,12 @@ int main(int argc, char *argv[])
> > goto out;
> > }
> >
> > + if (test_uretprobe()) {
> > + ret = 1;
> > + printf("[FAIL]\turetprobe test\n");
> > + goto out;
> > + }
> > +
> > return ret;
> >
> > out:
> > --
> > 2.44.0
> >
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
* Re: [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support
From: Jiri Olsa @ 2024-05-13 21:23 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: olsajiri@gmail.com, mhiramat@kernel.org, songliubraving@fb.com,
luto@kernel.org, andrii@kernel.org, debug@rivosinc.com,
john.fastabend@gmail.com, linux-api@vger.kernel.org,
linux-kernel@vger.kernel.org, mingo@redhat.com,
rostedt@goodmis.org, ast@kernel.org, tglx@linutronix.de,
yhs@fb.com, oleg@redhat.com, linux-man@vger.kernel.org,
daniel@iogearbox.net, peterz@infradead.org,
linux-trace-kernel@vger.kernel.org, bp@alien8.de,
bpf@vger.kernel.org, x86@kernel.org
In-Reply-To: <c56ae75e9cf0878ac46185a14a18f6ff7e8f891a.camel@intel.com>
On Mon, May 13, 2024 at 05:12:31PM +0000, Edgecombe, Rick P wrote:
> On Mon, 2024-05-13 at 18:50 +0900, Masami Hiramatsu wrote:
> > > I guess it's doable, we'd need to keep both trampolines around, because
> > > shadow stack is enabled by app dynamically and use one based on the
> > > state of shadow stack when uretprobe is installed
> > >
> > > so you're worried the optimized syscall path could be somehow exploited
> > > to add data on shadow stack?
>
> Shadow stack allows for modification to the shadow stack only through a few
> limited ways (call, ret, etc). The kernel has the ability to write through
> shadow stack protections (for example when pushing and popping signal frames),
> but the ways in which it does this are limited in order to try to prevent
> providing extra capabilities to attackers wanting to craft their own shadow
> stacks.
>
> But the HW features have optional abilities to allow extra patterns of shadow
> stack modification for userspace as well. This can facilitate unusual patterns
> of stack modification (like in this series). For, x86 there is the ability to
> allow an instruction (called WRSS) such that userspace can also write arbitrary
> data to the shadow stack. Arm has something likes that, plus an instruction to
> push to the shadow stack.
>
> There was some debate about whether to use these features, as glibc could not
> perfectly match compatibility for features that play with the stack like
> longjmp(). As in, without using those extra HW capabilities, some apps would
> require modifications to work with shadow stack.
>
> There has been a lot of design tension between security, performance and
> compatibility in figuring out how to fit this feature into existing software. In
> the end the consensus was to not use these extra HW capabilities, and lean
> towards security in the implementation. To try to summarize the debate, this was
> because we could get pretty close to compatibility without enabling these extra
> features.
>
> So since this solution does something like enabling these extra capabilities in
> software that were purposely disabled in HW, it raises eyebrows. Glibc has some
> operations that now have extra steps because of shadow stack. So if we could do
> something that was still functional, but slower and more secure, then it seems
> roughly in line with the tradeoffs we have gone with so far.
so at the moment the patch 6 changes shadow stack for
1) current uretprobe which are not working at the moment and we change
the top value of shadow stack with shstk_push_frame
2) optimized uretprobe which needs to push new frame on shadow stack
with shstk_update_last_frame
I think we should do 1) and have current uretprobe working with shadow
stack, which is broken at the moment
I'm ok with not using optimized uretprobe when shadow stack is detected
as enabled and we go with current uretprobe in that case
would this work for you?
thanks,
jirka
>
> But shadow stack is not in widespread use yet, so whether we have the final
> tradeoffs settled is still open I think. For example, other libcs have expressed
> interest in using WRSS.
>
> I'm also not clear on the typical use of uretprobes (debugging vs production).
> And whether shadow stack + debugging + production will happen seems pretty
> unknown.
>
> >
> > Good point. For the security concerning (e.g. leaking sensitive information
> > from secure process which uses shadow stack), we need another limitation
> > which prohibits probing such process even for debugging. But I think that
> > needs another series of patches. We also need to discuss when it should be
> > prohibited and how (e.g. audit interface? SELinux?).
> > But I think this series is just optimizing currently available uprobes with
> > a new syscall. I don't think it changes such security concerning.
>
> Patch 6 adds support for shadow stack for uretprobes. Currently there is no
> support.
>
> Peterz had asked that the new solution consider shadow stack support, so I think
> that is how this series grew kind of two goals: new faster uretprobes and
> initial shadow stack support.
>
>
^ permalink raw reply
* Re: [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support
From: Edgecombe, Rick P @ 2024-05-13 17:12 UTC (permalink / raw)
To: olsajiri@gmail.com, mhiramat@kernel.org
Cc: songliubraving@fb.com, luto@kernel.org, andrii@kernel.org,
debug@rivosinc.com, john.fastabend@gmail.com,
linux-api@vger.kernel.org, linux-kernel@vger.kernel.org,
mingo@redhat.com, rostedt@goodmis.org, ast@kernel.org,
tglx@linutronix.de, yhs@fb.com, oleg@redhat.com,
linux-man@vger.kernel.org, daniel@iogearbox.net,
peterz@infradead.org, linux-trace-kernel@vger.kernel.org,
bp@alien8.de, bpf@vger.kernel.org, x86@kernel.org
In-Reply-To: <20240513185040.416d62bc4a71e79367c1cd9c@kernel.org>
On Mon, 2024-05-13 at 18:50 +0900, Masami Hiramatsu wrote:
> > I guess it's doable, we'd need to keep both trampolines around, because
> > shadow stack is enabled by app dynamically and use one based on the
> > state of shadow stack when uretprobe is installed
> >
> > so you're worried the optimized syscall path could be somehow exploited
> > to add data on shadow stack?
Shadow stack allows for modification to the shadow stack only through a few
limited ways (call, ret, etc). The kernel has the ability to write through
shadow stack protections (for example when pushing and popping signal frames),
but the ways in which it does this are limited in order to try to prevent
providing extra capabilities to attackers wanting to craft their own shadow
stacks.
But the HW features have optional abilities to allow extra patterns of shadow
stack modification for userspace as well. This can facilitate unusual patterns
of stack modification (like in this series). For, x86 there is the ability to
allow an instruction (called WRSS) such that userspace can also write arbitrary
data to the shadow stack. Arm has something likes that, plus an instruction to
push to the shadow stack.
There was some debate about whether to use these features, as glibc could not
perfectly match compatibility for features that play with the stack like
longjmp(). As in, without using those extra HW capabilities, some apps would
require modifications to work with shadow stack.
There has been a lot of design tension between security, performance and
compatibility in figuring out how to fit this feature into existing software. In
the end the consensus was to not use these extra HW capabilities, and lean
towards security in the implementation. To try to summarize the debate, this was
because we could get pretty close to compatibility without enabling these extra
features.
So since this solution does something like enabling these extra capabilities in
software that were purposely disabled in HW, it raises eyebrows. Glibc has some
operations that now have extra steps because of shadow stack. So if we could do
something that was still functional, but slower and more secure, then it seems
roughly in line with the tradeoffs we have gone with so far.
But shadow stack is not in widespread use yet, so whether we have the final
tradeoffs settled is still open I think. For example, other libcs have expressed
interest in using WRSS.
I'm also not clear on the typical use of uretprobes (debugging vs production).
And whether shadow stack + debugging + production will happen seems pretty
unknown.
>
> Good point. For the security concerning (e.g. leaking sensitive information
> from secure process which uses shadow stack), we need another limitation
> which prohibits probing such process even for debugging. But I think that
> needs another series of patches. We also need to discuss when it should be
> prohibited and how (e.g. audit interface? SELinux?).
> But I think this series is just optimizing currently available uprobes with
> a new syscall. I don't think it changes such security concerning.
Patch 6 adds support for shadow stack for uretprobes. Currently there is no
support.
Peterz had asked that the new solution consider shadow stack support, so I think
that is how this series grew kind of two goals: new faster uretprobes and
initial shadow stack support.
^ permalink raw reply
* [RFC PATCH] LSM, net: Add SO_PEERCONTEXT for peer LSM data
From: Casey Schaufler @ 2024-05-13 17:03 UTC (permalink / raw)
To: LSM List, netdev, linux-api, Linux kernel mailing list
In-Reply-To: <763db426-6f60-4d36-b3f9-b316008889f7.ref@schaufler-ca.com>
We recently introduced system calls to access process attributes that
are used by Linux Security Modules (LSM). An important aspect of these
system calls is that they provide the LSM attribute data in a format
that identifies the LSM to which the data applies. Another aspect is that
it can be used to provide multiple instances of the attribute for the
case where more than one LSM supplies the attribute.
We wish to take advantage of this format for data about network peers.
The existing mechanism, SO_PEERSEC, provides peer security data as a
text string. This is sufficient when the LSM providing the information
is known by the user of SO_PEERSEC, and there is only one LSM providing
the information. It fails, however, if the user does not know which
LSM is providing the information.
Discussions about extending SO_PEERSEC to accomodate either the new
format or some other encoding scheme invariably lead to the conclusion
that doing so would lead to tears. Hence, we introduce SO_PEERCONTEXT
which uses the same API data as the LSM system calls.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
arch/alpha/include/uapi/asm/socket.h | 1 +
arch/mips/include/uapi/asm/socket.h | 1 +
arch/parisc/include/uapi/asm/socket.h | 1 +
arch/sparc/include/uapi/asm/socket.h | 1 +
include/linux/lsm_hook_defs.h | 2 +
include/linux/security.h | 18 ++++++++
include/uapi/asm-generic/socket.h | 1 +
net/core/sock.c | 4 ++
security/apparmor/lsm.c | 39 ++++++++++++++++
security/security.c | 86 +++++++++++++++++++++++++++++++++++
security/selinux/hooks.c | 35 ++++++++++++++
security/smack/smack_lsm.c | 25 ++++++++++
12 files changed, 214 insertions(+)
diff --git a/arch/alpha/include/uapi/asm/socket.h b/arch/alpha/include/uapi/asm/socket.h
index e94f621903fe..e49ed0a765ce 100644
--- a/arch/alpha/include/uapi/asm/socket.h
+++ b/arch/alpha/include/uapi/asm/socket.h
@@ -139,6 +139,7 @@
#define SO_PASSPIDFD 76
#define SO_PEERPIDFD 77
+#define SO_PEERCONTEXT 78
#if !defined(__KERNEL__)
diff --git a/arch/mips/include/uapi/asm/socket.h b/arch/mips/include/uapi/asm/socket.h
index 60ebaed28a4c..e647f065b3c9 100644
--- a/arch/mips/include/uapi/asm/socket.h
+++ b/arch/mips/include/uapi/asm/socket.h
@@ -150,6 +150,7 @@
#define SO_PASSPIDFD 76
#define SO_PEERPIDFD 77
+#define SO_PEERCONTEXT 78
#if !defined(__KERNEL__)
diff --git a/arch/parisc/include/uapi/asm/socket.h b/arch/parisc/include/uapi/asm/socket.h
index be264c2b1a11..4f48bef863bc 100644
--- a/arch/parisc/include/uapi/asm/socket.h
+++ b/arch/parisc/include/uapi/asm/socket.h
@@ -131,6 +131,7 @@
#define SO_PASSPIDFD 0x404A
#define SO_PEERPIDFD 0x404B
+#define SO_PEERCONTEXT 0x404C
#if !defined(__KERNEL__)
diff --git a/arch/sparc/include/uapi/asm/socket.h b/arch/sparc/include/uapi/asm/socket.h
index 682da3714686..b54c56056323 100644
--- a/arch/sparc/include/uapi/asm/socket.h
+++ b/arch/sparc/include/uapi/asm/socket.h
@@ -132,6 +132,7 @@
#define SO_PASSPIDFD 0x0055
#define SO_PEERPIDFD 0x0056
+#define SO_PEERCONTEXT 0x0057
#if !defined(__KERNEL__)
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 27e6384ec779..d923acb40c20 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -336,6 +336,8 @@ LSM_HOOK(int, 0, socket_shutdown, struct socket *sock, int how)
LSM_HOOK(int, 0, socket_sock_rcv_skb, struct sock *sk, struct sk_buff *skb)
LSM_HOOK(int, -ENOPROTOOPT, socket_getpeersec_stream, struct socket *sock,
sockptr_t optval, sockptr_t optlen, unsigned int len)
+LSM_HOOK(int, -ENOPROTOOPT, socket_getpeerctx_stream, struct socket *sock,
+ sockptr_t optval, sockptr_t optlen, unsigned int len)
LSM_HOOK(int, -ENOPROTOOPT, socket_getpeersec_dgram, struct socket *sock,
struct sk_buff *skb, u32 *secid)
LSM_HOOK(int, 0, sk_alloc_security, struct sock *sk, int family, gfp_t priority)
diff --git a/include/linux/security.h b/include/linux/security.h
index a54543ded9fc..d4f449b7deea 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -563,6 +563,8 @@ int security_inode_getsecctx(struct inode *inode, struct lsmcontext *cp);
int security_locked_down(enum lockdown_reason what);
int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len,
void *val, size_t val_len, u64 id, u64 flags);
+int lsm_fill_socket_ctx(sockptr_t optval, sockptr_t optlen, void *val,
+ size_t val_len, u64 id, u64 flags);
#else /* CONFIG_SECURITY */
static inline int call_blocking_lsm_notifier(enum lsm_event event, void *data)
@@ -1552,6 +1554,12 @@ static inline int lsm_fill_user_ctx(struct lsm_ctx __user *uctx,
{
return -EOPNOTSUPP;
}
+static inline int lsm_fill_socket_ctx(sockptr_t optval, sockptr_t optlen,
+ void *val, size_t val_len, u64 id,
+ u64 flags)
+{
+ return -EOPNOTSUPP;
+}
#endif /* CONFIG_SECURITY */
#if defined(CONFIG_SECURITY) && defined(CONFIG_WATCH_QUEUE)
@@ -1599,6 +1607,8 @@ int security_socket_shutdown(struct socket *sock, int how);
int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb);
int security_socket_getpeersec_stream(struct socket *sock, sockptr_t optval,
sockptr_t optlen, unsigned int len);
+int security_socket_getpeerctx_stream(struct socket *sock, sockptr_t optval,
+ sockptr_t optlen, unsigned int len);
int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid);
int security_sk_alloc(struct sock *sk, int family, gfp_t priority);
void security_sk_free(struct sock *sk);
@@ -1744,6 +1754,14 @@ static inline int security_socket_getpeersec_stream(struct socket *sock,
return -ENOPROTOOPT;
}
+static inline int security_socket_getpeerctx_stream(struct socket *sock,
+ sockptr_t optval,
+ sockptr_t optlen,
+ unsigned int len)
+{
+ return -ENOPROTOOPT;
+}
+
static inline int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid)
{
return -ENOPROTOOPT;
diff --git a/include/uapi/asm-generic/socket.h b/include/uapi/asm-generic/socket.h
index 8ce8a39a1e5f..e0166ff53670 100644
--- a/include/uapi/asm-generic/socket.h
+++ b/include/uapi/asm-generic/socket.h
@@ -134,6 +134,7 @@
#define SO_PASSPIDFD 76
#define SO_PEERPIDFD 77
+#define SO_PEERCONTEXT 78
#if !defined(__KERNEL__)
diff --git a/net/core/sock.c b/net/core/sock.c
index 0963689a5950..251346eccfa5 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1849,6 +1849,10 @@ int sk_getsockopt(struct sock *sk, int level, int optname,
return security_socket_getpeersec_stream(sock,
optval, optlen, len);
+ case SO_PEERCONTEXT:
+ return security_socket_getpeerctx_stream(sock,
+ optval, optlen, len);
+
case SO_MARK:
v.val = READ_ONCE(sk->sk_mark);
break;
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index c478493f3eaf..66d4ecd6afcb 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -1357,6 +1357,43 @@ static int apparmor_socket_getpeersec_stream(struct socket *sock,
return error;
}
+/**
+ * apparmor_socket_getpeerctx_stream - get security context of peer
+ * @sock: socket that we are trying to get the peer context of
+ * @optval: output - buffer to copy peer name to
+ * @optlen: output - size of copied name in @optval
+ * @len: size of @optval buffer
+ * Returns: 0 on success, -errno of failure
+ *
+ * Note: for tcp only valid if using ipsec or cipso on lan
+ */
+static int apparmor_socket_getpeerctx_stream(struct socket *sock,
+ sockptr_t optval, sockptr_t optlen,
+ unsigned int len)
+{
+ char *name = NULL;
+ int slen, error = 0;
+ struct aa_label *label;
+ struct aa_label *peer;
+
+ label = begin_current_label_crit_section();
+ peer = sk_peer_label(sock->sk);
+ if (IS_ERR(peer)) {
+ error = PTR_ERR(peer);
+ goto done;
+ }
+ slen = aa_label_asxprint(&name, labels_ns(label), peer,
+ FLAG_SHOW_MODE | FLAG_VIEW_SUBNS |
+ FLAG_HIDDEN_UNCONFINED, GFP_KERNEL);
+
+ error = lsm_fill_socket_ctx(optval, optlen, name, slen,
+ LSM_ID_APPARMOR, 0);
+done:
+ end_current_label_crit_section(label);
+ kfree(name);
+ return error;
+}
+
/**
* apparmor_sock_graft - Initialize newly created socket
* @sk: child sock
@@ -1466,6 +1503,8 @@ static struct security_hook_list apparmor_hooks[] __ro_after_init = {
#endif
LSM_HOOK_INIT(socket_getpeersec_stream,
apparmor_socket_getpeersec_stream),
+ LSM_HOOK_INIT(socket_getpeerctx_stream,
+ apparmor_socket_getpeerctx_stream),
LSM_HOOK_INIT(sock_graft, apparmor_sock_graft),
#ifdef CONFIG_NETWORK_SECMARK
LSM_HOOK_INIT(inet_conn_request, apparmor_inet_conn_request),
diff --git a/security/security.c b/security/security.c
index e387614cb054..fd4919c28e8f 100644
--- a/security/security.c
+++ b/security/security.c
@@ -874,6 +874,64 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len,
return rc;
}
+/**
+ * lsm_fill_socket_ctx - Fill a socket lsm_ctx structure
+ * @optval: a socket LSM context to be filled
+ * @optlen: uctx size
+ * @val: the new LSM context value
+ * @val_len: the size of the new LSM context value
+ * @id: LSM id
+ * @flags: LSM defined flags
+ *
+ * Fill all of the fields in a lsm_ctx structure. If @optval is NULL
+ * simply calculate the required size to output via @optlen and return
+ * success.
+ *
+ * Returns 0 on success, -E2BIG if userspace buffer is not large enough,
+ * -EFAULT on a copyout error, -ENOMEM if memory can't be allocated.
+ */
+int lsm_fill_socket_ctx(sockptr_t optval, sockptr_t optlen, void *val,
+ size_t val_len, u64 id, u64 flags)
+{
+ struct lsm_ctx *nctx = NULL;
+ unsigned int nctx_len;
+ int loptlen;
+ int rc = 0;
+
+ if (copy_from_sockptr(&loptlen, optlen, sizeof(int)))
+ return -EFAULT;
+
+ nctx_len = ALIGN(struct_size(nctx, ctx, val_len), sizeof(void *));
+ if (nctx_len > loptlen && !sockptr_is_null(optval))
+ rc = -E2BIG;
+
+ /* no buffer - return success/0 and set @uctx_len to the req size */
+ if (sockptr_is_null(optval) || rc)
+ goto out;
+
+ nctx = kzalloc(nctx_len, GFP_KERNEL);
+ if (!nctx) {
+ rc = -ENOMEM;
+ goto out;
+ }
+ nctx->id = id;
+ nctx->flags = flags;
+ nctx->len = nctx_len;
+ nctx->ctx_len = val_len;
+ memcpy(nctx->ctx, val, val_len);
+
+ if (copy_to_sockptr(optval, nctx, nctx_len))
+ rc = -EFAULT;
+
+ kfree(nctx);
+out:
+ if (copy_to_sockptr(optlen, &nctx_len, sizeof(int)))
+ rc = -EFAULT;
+
+ return rc;
+}
+
+
/*
* The default value of the LSM hook is defined in linux/lsm_hook_defs.h and
* can be accessed with:
@@ -4743,6 +4801,34 @@ int security_socket_getpeersec_stream(struct socket *sock, sockptr_t optval,
return LSM_RET_DEFAULT(socket_getpeersec_stream);
}
+/**
+ * security_socket_getpeerctx_stream() - Get the remote peer label
+ * @sock: socket
+ * @optval: destination buffer
+ * @optlen: size of peer label copied into the buffer
+ * @len: maximum size of the destination buffer
+ *
+ * This hook allows the security module to provide peer socket security state
+ * for unix or connected tcp sockets to userspace via getsockopt
+ * SO_GETPEERCONTEXT. For tcp sockets this can be meaningful if the socket
+ * is associated with an ipsec SA.
+ *
+ * Return: Returns 0 if all is well, otherwise, typical getsockopt return
+ * values.
+ */
+int security_socket_getpeerctx_stream(struct socket *sock, sockptr_t optval,
+ sockptr_t optlen, unsigned int len)
+{
+ struct security_hook_list *hp;
+
+ hlist_for_each_entry(hp, &security_hook_heads.socket_getpeerctx_stream,
+ list)
+ return hp->hook.socket_getpeerctx_stream(sock, optval, optlen,
+ len);
+
+ return LSM_RET_DEFAULT(socket_getpeerctx_stream);
+}
+
/**
* security_socket_getpeersec_dgram() - Get the remote peer label
* @sock: socket
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 83d30579e568..3c34cbc2b197 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5221,6 +5221,39 @@ static int selinux_socket_getpeersec_stream(struct socket *sock,
return err;
}
+static int selinux_socket_getpeerctx_stream(struct socket *sock,
+ sockptr_t optval, sockptr_t optlen,
+ unsigned int len)
+{
+ int err = 0;
+ char *scontext = NULL;
+ u32 scontext_len;
+ struct sk_security_struct *sksec = selinux_sock(sock->sk);
+ u32 peer_sid = SECSID_NULL;
+
+ if (sksec->sclass == SECCLASS_UNIX_STREAM_SOCKET ||
+ sksec->sclass == SECCLASS_TCP_SOCKET ||
+ sksec->sclass == SECCLASS_SCTP_SOCKET)
+ peer_sid = sksec->peer_sid;
+ if (peer_sid == SECSID_NULL)
+ return -ENOPROTOOPT;
+
+ err = security_sid_to_context(peer_sid, &scontext,
+ &scontext_len);
+ if (err)
+ return err;
+ if (scontext_len > len) {
+ err = -ERANGE;
+ goto out_len;
+ }
+
+ err = lsm_fill_socket_ctx(optval, optlen, scontext, scontext_len,
+ LSM_ID_SELINUX, 0);
+out_len:
+ kfree(scontext);
+ return err;
+}
+
static int selinux_socket_getpeersec_dgram(struct socket *sock,
struct sk_buff *skb, u32 *secid)
{
@@ -7345,6 +7378,8 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
LSM_HOOK_INIT(socket_sock_rcv_skb, selinux_socket_sock_rcv_skb),
LSM_HOOK_INIT(socket_getpeersec_stream,
selinux_socket_getpeersec_stream),
+ LSM_HOOK_INIT(socket_getpeerctx_stream,
+ selinux_socket_getpeerctx_stream),
LSM_HOOK_INIT(socket_getpeersec_dgram, selinux_socket_getpeersec_dgram),
LSM_HOOK_INIT(sk_free_security, selinux_sk_free_security),
LSM_HOOK_INIT(sk_clone_security, selinux_sk_clone_security),
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 3380a9e91fb8..dd0ea4da7eeb 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4297,6 +4297,30 @@ static int smack_socket_getpeersec_stream(struct socket *sock,
return rc;
}
+/**
+ * smack_socket_getpeerctx_stream - pull in packet label
+ * @sock: the socket
+ * @optval: user's destination
+ * @optlen: size thereof
+ * @len: max thereof
+ *
+ * returns zero on success, an error code otherwise
+ */
+static int smack_socket_getpeerctx_stream(struct socket *sock,
+ sockptr_t optval, sockptr_t optlen,
+ unsigned int len)
+{
+ struct socket_smack *ssp;
+ size_t slen;
+ char *rcp = "";
+
+ ssp = smack_sock(sock->sk);
+ if (ssp->smk_packet)
+ rcp = ssp->smk_packet->smk_known;
+ slen = strlen(rcp) + 1;
+
+ return lsm_fill_socket_ctx(optval, optlen, rcp, slen, LSM_ID_SMACK, 0);
+}
/**
* smack_socket_getpeersec_dgram - pull in packet label
@@ -5206,6 +5230,7 @@ static struct security_hook_list smack_hooks[] __ro_after_init = {
LSM_HOOK_INIT(socket_sendmsg, smack_socket_sendmsg),
LSM_HOOK_INIT(socket_sock_rcv_skb, smack_socket_sock_rcv_skb),
LSM_HOOK_INIT(socket_getpeersec_stream, smack_socket_getpeersec_stream),
+ LSM_HOOK_INIT(socket_getpeerctx_stream, smack_socket_getpeerctx_stream),
LSM_HOOK_INIT(socket_getpeersec_dgram, smack_socket_getpeersec_dgram),
LSM_HOOK_INIT(sk_alloc_security, smack_sk_alloc_security),
#ifdef SMACK_IPV6_PORT_LABELING
^ permalink raw reply related
* Re: [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support
From: Masami Hiramatsu @ 2024-05-13 9:50 UTC (permalink / raw)
To: Jiri Olsa
Cc: Edgecombe, Rick P, songliubraving@fb.com, luto@kernel.org,
mhiramat@kernel.org, andrii@kernel.org, debug@rivosinc.com,
john.fastabend@gmail.com, linux-api@vger.kernel.org,
linux-kernel@vger.kernel.org, mingo@redhat.com,
rostedt@goodmis.org, ast@kernel.org, tglx@linutronix.de,
yhs@fb.com, oleg@redhat.com, peterz@infradead.org,
linux-man@vger.kernel.org, daniel@iogearbox.net,
linux-trace-kernel@vger.kernel.org, bp@alien8.de,
bpf@vger.kernel.org, x86@kernel.org
In-Reply-To: <Zj_enIB_J6pGJ6Nu@krava>
On Sat, 11 May 2024 15:09:48 -0600
Jiri Olsa <olsajiri@gmail.com> wrote:
> On Thu, May 09, 2024 at 04:24:37PM +0000, Edgecombe, Rick P wrote:
> > On Thu, 2024-05-09 at 10:30 +0200, Jiri Olsa wrote:
> > > > Per the earlier discussion, this cannot be reached unless uretprobes are in
> > > > use,
> > > > which cannot happen without something with privileges taking an action. But
> > > > are
> > > > uretprobes ever used for monitoring applications where security is
> > > > important? Or
> > > > is it strictly a debug-time thing?
> > >
> > > sorry, I don't have that level of detail, but we do have customers
> > > that use uprobes in general or want to use it and complain about
> > > the speed
> > >
> > > there are several tools in bcc [1] that use uretprobes in scripts,
> > > like:
> > > memleak, sslsniff, trace, bashreadline, gethostlatency, argdist,
> > > funclatency
> >
> > Is it possible to have shadow stack only use the non-syscall solution? It seems
> > it exposes a more limited compatibility in that it only allows writing the
> > specific trampoline address. (IIRC) Then shadow stack users could still use
> > uretprobes, but just not the new optimized solution. There are already
> > operations that are slower with shadow stack, like longjmp(), so this could be
> > ok maybe.
>
> I guess it's doable, we'd need to keep both trampolines around, because
> shadow stack is enabled by app dynamically and use one based on the
> state of shadow stack when uretprobe is installed
>
> so you're worried the optimized syscall path could be somehow exploited
> to add data on shadow stack?
Good point. For the security concerning (e.g. leaking sensitive information
from secure process which uses shadow stack), we need another limitation
which prohibits probing such process even for debugging. But I think that
needs another series of patches. We also need to discuss when it should be
prohibited and how (e.g. audit interface? SELinux?).
But I think this series is just optimizing currently available uprobes with
a new syscall. I don't think it changes such security concerning.
Thank you,
>
> jirka
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
* Re: [PATCHv5 bpf-next 7/8] selftests/x86: Add return uprobe shadow stack test
From: Masami Hiramatsu @ 2024-05-13 9:45 UTC (permalink / raw)
To: Jiri Olsa
Cc: Steven Rostedt, Oleg Nesterov, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, linux-kernel,
linux-trace-kernel, linux-api, linux-man, x86, bpf, Song Liu,
Yonghong Song, John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov (AMD), Ingo Molnar, Andy Lutomirski,
Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240507105321.71524-8-jolsa@kernel.org>
On Tue, 7 May 2024 12:53:20 +0200
Jiri Olsa <jolsa@kernel.org> wrote:
> Adding return uprobe test for shadow stack and making sure it's
> working properly. Borrowed some of the code from bpf selftests.
Hi Jiri,
I can not find "SKIP" result in this change. If CONFIG_UPROBES=n,
this should skip uprobe test.
Thank you,
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> .../testing/selftests/x86/test_shadow_stack.c | 142 ++++++++++++++++++
> 1 file changed, 142 insertions(+)
>
> diff --git a/tools/testing/selftests/x86/test_shadow_stack.c b/tools/testing/selftests/x86/test_shadow_stack.c
> index 757e6527f67e..1b919baa999b 100644
> --- a/tools/testing/selftests/x86/test_shadow_stack.c
> +++ b/tools/testing/selftests/x86/test_shadow_stack.c
> @@ -34,6 +34,7 @@
> #include <sys/ptrace.h>
> #include <sys/signal.h>
> #include <linux/elf.h>
> +#include <linux/perf_event.h>
>
> /*
> * Define the ABI defines if needed, so people can run the tests
> @@ -681,6 +682,141 @@ int test_32bit(void)
> return !segv_triggered;
> }
>
> +static int parse_uint_from_file(const char *file, const char *fmt)
> +{
> + int err, ret;
> + FILE *f;
> +
> + f = fopen(file, "re");
> + if (!f) {
> + err = -errno;
> + printf("failed to open '%s': %d\n", file, err);
> + return err;
> + }
> + err = fscanf(f, fmt, &ret);
> + if (err != 1) {
> + err = err == EOF ? -EIO : -errno;
> + printf("failed to parse '%s': %d\n", file, err);
> + fclose(f);
> + return err;
> + }
> + fclose(f);
> + return ret;
> +}
> +
> +static int determine_uprobe_perf_type(void)
> +{
> + const char *file = "/sys/bus/event_source/devices/uprobe/type";
> +
> + return parse_uint_from_file(file, "%d\n");
> +}
> +
> +static int determine_uprobe_retprobe_bit(void)
> +{
> + const char *file = "/sys/bus/event_source/devices/uprobe/format/retprobe";
> +
> + return parse_uint_from_file(file, "config:%d\n");
> +}
> +
> +static ssize_t get_uprobe_offset(const void *addr)
> +{
> + size_t start, end, base;
> + char buf[256];
> + bool found = false;
> + FILE *f;
> +
> + f = fopen("/proc/self/maps", "r");
> + if (!f)
> + return -errno;
> +
> + while (fscanf(f, "%zx-%zx %s %zx %*[^\n]\n", &start, &end, buf, &base) == 4) {
> + if (buf[2] == 'x' && (uintptr_t)addr >= start && (uintptr_t)addr < end) {
> + found = true;
> + break;
> + }
> + }
> +
> + fclose(f);
> +
> + if (!found)
> + return -ESRCH;
> +
> + return (uintptr_t)addr - start + base;
> +}
> +
> +static __attribute__((noinline)) void uretprobe_trigger(void)
> +{
> + asm volatile ("");
> +}
> +
> +/*
> + * This test setups return uprobe, which is sensitive to shadow stack
> + * (crashes without extra fix). After executing the uretprobe we fail
> + * the test if we receive SIGSEGV, no crash means we're good.
> + *
> + * Helper functions above borrowed from bpf selftests.
> + */
> +static int test_uretprobe(void)
> +{
> + const size_t attr_sz = sizeof(struct perf_event_attr);
> + const char *file = "/proc/self/exe";
> + int bit, fd = 0, type, err = 1;
> + struct perf_event_attr attr;
> + struct sigaction sa = {};
> + ssize_t offset;
> +
> + type = determine_uprobe_perf_type();
> + if (type < 0)
> + return 1;
> +
> + offset = get_uprobe_offset(uretprobe_trigger);
> + if (offset < 0)
> + return 1;
> +
> + bit = determine_uprobe_retprobe_bit();
> + if (bit < 0)
> + return 1;
> +
> + sa.sa_sigaction = segv_gp_handler;
> + sa.sa_flags = SA_SIGINFO;
> + if (sigaction(SIGSEGV, &sa, NULL))
> + return 1;
> +
> + /* Setup return uprobe through perf event interface. */
> + memset(&attr, 0, attr_sz);
> + attr.size = attr_sz;
> + attr.type = type;
> + attr.config = 1 << bit;
> + attr.config1 = (__u64) (unsigned long) file;
> + attr.config2 = offset;
> +
> + fd = syscall(__NR_perf_event_open, &attr, 0 /* pid */, -1 /* cpu */,
> + -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
> + if (fd < 0)
> + goto out;
> +
> + if (sigsetjmp(jmp_buffer, 1))
> + goto out;
> +
> + ARCH_PRCTL(ARCH_SHSTK_ENABLE, ARCH_SHSTK_SHSTK);
> +
> + /*
> + * This either segfaults and goes through sigsetjmp above
> + * or succeeds and we're good.
> + */
> + uretprobe_trigger();
> +
> + printf("[OK]\tUretprobe test\n");
> + err = 0;
> +
> +out:
> + ARCH_PRCTL(ARCH_SHSTK_DISABLE, ARCH_SHSTK_SHSTK);
> + signal(SIGSEGV, SIG_DFL);
> + if (fd)
> + close(fd);
> + return err;
> +}
> +
> void segv_handler_ptrace(int signum, siginfo_t *si, void *uc)
> {
> /* The SSP adjustment caused a segfault. */
> @@ -867,6 +1003,12 @@ int main(int argc, char *argv[])
> goto out;
> }
>
> + if (test_uretprobe()) {
> + ret = 1;
> + printf("[FAIL]\turetprobe test\n");
> + goto out;
> + }
> +
> return ret;
>
> out:
> --
> 2.44.0
>
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
* Re: [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support
From: Jiri Olsa @ 2024-05-11 21:09 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: olsajiri@gmail.com, songliubraving@fb.com, luto@kernel.org,
mhiramat@kernel.org, andrii@kernel.org, debug@rivosinc.com,
john.fastabend@gmail.com, linux-api@vger.kernel.org,
linux-kernel@vger.kernel.org, mingo@redhat.com,
rostedt@goodmis.org, ast@kernel.org, tglx@linutronix.de,
yhs@fb.com, oleg@redhat.com, peterz@infradead.org,
linux-man@vger.kernel.org, daniel@iogearbox.net,
linux-trace-kernel@vger.kernel.org, bp@alien8.de,
bpf@vger.kernel.org, x86@kernel.org
In-Reply-To: <a8b7be15e6dbb1e8f2acaee7dae21fec7775194c.camel@intel.com>
On Thu, May 09, 2024 at 04:24:37PM +0000, Edgecombe, Rick P wrote:
> On Thu, 2024-05-09 at 10:30 +0200, Jiri Olsa wrote:
> > > Per the earlier discussion, this cannot be reached unless uretprobes are in
> > > use,
> > > which cannot happen without something with privileges taking an action. But
> > > are
> > > uretprobes ever used for monitoring applications where security is
> > > important? Or
> > > is it strictly a debug-time thing?
> >
> > sorry, I don't have that level of detail, but we do have customers
> > that use uprobes in general or want to use it and complain about
> > the speed
> >
> > there are several tools in bcc [1] that use uretprobes in scripts,
> > like:
> > memleak, sslsniff, trace, bashreadline, gethostlatency, argdist,
> > funclatency
>
> Is it possible to have shadow stack only use the non-syscall solution? It seems
> it exposes a more limited compatibility in that it only allows writing the
> specific trampoline address. (IIRC) Then shadow stack users could still use
> uretprobes, but just not the new optimized solution. There are already
> operations that are slower with shadow stack, like longjmp(), so this could be
> ok maybe.
I guess it's doable, we'd need to keep both trampolines around, because
shadow stack is enabled by app dynamically and use one based on the
state of shadow stack when uretprobe is installed
so you're worried the optimized syscall path could be somehow exploited
to add data on shadow stack?
jirka
^ permalink raw reply
* Re: [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support
From: Edgecombe, Rick P @ 2024-05-09 16:24 UTC (permalink / raw)
To: olsajiri@gmail.com
Cc: songliubraving@fb.com, luto@kernel.org, mhiramat@kernel.org,
andrii@kernel.org, debug@rivosinc.com, john.fastabend@gmail.com,
linux-api@vger.kernel.org, linux-kernel@vger.kernel.org,
mingo@redhat.com, rostedt@goodmis.org, ast@kernel.org,
tglx@linutronix.de, yhs@fb.com, oleg@redhat.com,
peterz@infradead.org, linux-man@vger.kernel.org,
daniel@iogearbox.net, linux-trace-kernel@vger.kernel.org,
bp@alien8.de, bpf@vger.kernel.org, x86@kernel.org
In-Reply-To: <ZjyJsl_u_FmYHrki@krava>
On Thu, 2024-05-09 at 10:30 +0200, Jiri Olsa wrote:
> > Per the earlier discussion, this cannot be reached unless uretprobes are in
> > use,
> > which cannot happen without something with privileges taking an action. But
> > are
> > uretprobes ever used for monitoring applications where security is
> > important? Or
> > is it strictly a debug-time thing?
>
> sorry, I don't have that level of detail, but we do have customers
> that use uprobes in general or want to use it and complain about
> the speed
>
> there are several tools in bcc [1] that use uretprobes in scripts,
> like:
> memleak, sslsniff, trace, bashreadline, gethostlatency, argdist,
> funclatency
Is it possible to have shadow stack only use the non-syscall solution? It seems
it exposes a more limited compatibility in that it only allows writing the
specific trampoline address. (IIRC) Then shadow stack users could still use
uretprobes, but just not the new optimized solution. There are already
operations that are slower with shadow stack, like longjmp(), so this could be
ok maybe.
^ permalink raw reply
* Re: [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support
From: Jiri Olsa @ 2024-05-09 8:30 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: mhiramat@kernel.org, rostedt@goodmis.org, ast@kernel.org,
andrii@kernel.org, oleg@redhat.com, daniel@iogearbox.net,
debug@rivosinc.com, luto@kernel.org, bp@alien8.de, yhs@fb.com,
songliubraving@fb.com, linux-api@vger.kernel.org, x86@kernel.org,
linux-kernel@vger.kernel.org, john.fastabend@gmail.com,
tglx@linutronix.de, mingo@redhat.com,
linux-trace-kernel@vger.kernel.org, bpf@vger.kernel.org,
linux-man@vger.kernel.org, peterz@infradead.org
In-Reply-To: <a08a955c74682e9dc6eb6d49b91c6968c9b62f75.camel@intel.com>
On Tue, May 07, 2024 at 05:35:54PM +0000, Edgecombe, Rick P wrote:
> On Tue, 2024-05-07 at 12:53 +0200, Jiri Olsa wrote:
> > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> > index 81e6ee95784d..ae6c3458a675 100644
> > --- a/arch/x86/kernel/uprobes.c
> > +++ b/arch/x86/kernel/uprobes.c
> > @@ -406,6 +406,11 @@ SYSCALL_DEFINE0(uretprobe)
> > * trampoline's ret instruction
> > */
> > r11_cx_ax[2] = regs->ip;
> > +
> > + /* make the shadow stack follow that */
> > + if (shstk_push_frame(regs->ip))
> > + goto sigill;
> > +
> > regs->ip = ip;
> >
>
> Per the earlier discussion, this cannot be reached unless uretprobes are in use,
> which cannot happen without something with privileges taking an action. But are
> uretprobes ever used for monitoring applications where security is important? Or
> is it strictly a debug-time thing?
sorry, I don't have that level of detail, but we do have customers
that use uprobes in general or want to use it and complain about
the speed
there are several tools in bcc [1] that use uretprobes in scripts,
like:
memleak, sslsniff, trace, bashreadline, gethostlatency, argdist,
funclatency
jirka
[1] https://github.com/iovisor/bcc
^ permalink raw reply
* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: Andy Lutomirski @ 2024-05-08 17:30 UTC (permalink / raw)
To: Christian Brauner
Cc: Aleksa Sarai, Stas Sergeev, Serge E. Hallyn, linux-kernel,
Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <20240508-flugverbindung-sonnig-dcfa4971152e@brauner>
On Wed, May 8, 2024 at 12:32 AM Christian Brauner <brauner@kernel.org> wrote:
>
> On Tue, May 07, 2024 at 01:38:42PM -0700, Andy Lutomirski wrote:
> > On Tue, May 7, 2024 at 12:42 AM Christian Brauner <brauner@kernel.org> wrote:
> > >
> > > > With my kernel hat on, maybe I agree. But with my *user* hat on, I
> > > > think I pretty strongly disagree. Look, idmapis lousy for
> > > > unprivileged use:
> > > >
> > > > $ install -m 0700 -d test_directory
> > > > $ echo 'hi there' >test_directory/file
> > > > $ podman run -it --rm
> > > > --mount=type=bind,src=test_directory,dst=/tmp,idmap [debian-slim]
> > >
> > > $ podman run -it --rm --mount=type=bind,src=test_directory,dst=/tmp,idmap [debian-slim]
> > >
> > > as an unprivileged user doesn't use idmapped mounts at all. So I'm not
> > > sure what this is showing. I suppose you're talking about idmaps in
> > > general.
> >
> > Meh, fair enough. But I don't think this would have worked any better
> > with privilege.
> >
> > Can idmaps be programmed by an otherwise unprivileged owner of a
> > userns and a mountns inside?
>
> Yes, but only for userns mountable filesystems that support idmapped
> mounts. IOW, you need privilege over the superblock and the idmapping
> you're trying to use.
Hmm. Is there a good reason to require privilege over the superblock?
Obviously creating an idmap that allows one to impersonate someone
else seems like a problem, but if an unprivileged task already "owns"
(see below) a UID or GID, then effectively delegating that UID or GID
is would need caution but is not fundamentally terrible.
So, if I'm 1000:1000, then creating an idmap that makes some other
task (that isn't 1000:1000) get to act as 1000:1000 doesn't grant new
powers. But maybe something even more general could be done (although
I'm not sure this is worthwhile): if I own a userns and that userns
has an outside UID 1001 mapped (via newuidmap, for example), then
perhaps letting me configure an idmap that grants UID 1001 seems not
especially dangerous. But maybe that particular job should also be
delegated to newuidmap.
Out of an abundance of caution, maybe this whole thing should be
opt-in. For example, there could be a new CAP_DELEGATE that allows
delegation of one's own uid and gid. The idea is that it should be
safe to grant regular users CAP_DELEGATE as an ambient capability.
--Andy
^ permalink raw reply
* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: Christian Brauner @ 2024-05-08 7:32 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Aleksa Sarai, Stas Sergeev, Serge E. Hallyn, linux-kernel,
Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <CALCETrWuVQ-ggnak40AX16PUnM43zhogceFN-3c_YAKZGvs5Og@mail.gmail.com>
On Tue, May 07, 2024 at 01:38:42PM -0700, Andy Lutomirski wrote:
> On Tue, May 7, 2024 at 12:42 AM Christian Brauner <brauner@kernel.org> wrote:
> >
> > > With my kernel hat on, maybe I agree. But with my *user* hat on, I
> > > think I pretty strongly disagree. Look, idmapis lousy for
> > > unprivileged use:
> > >
> > > $ install -m 0700 -d test_directory
> > > $ echo 'hi there' >test_directory/file
> > > $ podman run -it --rm
> > > --mount=type=bind,src=test_directory,dst=/tmp,idmap [debian-slim]
> >
> > $ podman run -it --rm --mount=type=bind,src=test_directory,dst=/tmp,idmap [debian-slim]
> >
> > as an unprivileged user doesn't use idmapped mounts at all. So I'm not
> > sure what this is showing. I suppose you're talking about idmaps in
> > general.
>
> Meh, fair enough. But I don't think this would have worked any better
> with privilege.
>
> Can idmaps be programmed by an otherwise unprivileged owner of a
> userns and a mountns inside?
Yes, but only for userns mountable filesystems that support idmapped
mounts. IOW, you need privilege over the superblock and the idmapping
you're trying to use.
>
> > Many idmappings to one is in principle possible and I've noted that idea
> > down as a possible extension at
> > https://github.com/uapi-group/kernel-features quite a while (2 years?) ago.
> >
> > > I haven't looked at the idmap implementation nearly enough to have any
> > > opinion as to whether squashing UID is practical or whether there's
> >
> > It's doable. The interesting bit to me was that if we want to allow
> > writes we'd need a way to determine what the uid/gid would be to write
> > down. Imho, that's not super difficult to solve though. The most obvious
> > one is that userspace can just determine it when creating the idmapped
> > mount.
>
> Seems reasonable to me. If this is set up by someone unprivileged, it
> would need to be that uid/gid.
^ permalink raw reply
* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: Andy Lutomirski @ 2024-05-07 20:38 UTC (permalink / raw)
To: Christian Brauner
Cc: Aleksa Sarai, Stas Sergeev, Serge E. Hallyn, linux-kernel,
Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <20240507-verpennen-defekt-b6f2c9a46916@brauner>
On Tue, May 7, 2024 at 12:42 AM Christian Brauner <brauner@kernel.org> wrote:
>
> > With my kernel hat on, maybe I agree. But with my *user* hat on, I
> > think I pretty strongly disagree. Look, idmapis lousy for
> > unprivileged use:
> >
> > $ install -m 0700 -d test_directory
> > $ echo 'hi there' >test_directory/file
> > $ podman run -it --rm
> > --mount=type=bind,src=test_directory,dst=/tmp,idmap [debian-slim]
>
> $ podman run -it --rm --mount=type=bind,src=test_directory,dst=/tmp,idmap [debian-slim]
>
> as an unprivileged user doesn't use idmapped mounts at all. So I'm not
> sure what this is showing. I suppose you're talking about idmaps in
> general.
Meh, fair enough. But I don't think this would have worked any better
with privilege.
Can idmaps be programmed by an otherwise unprivileged owner of a
userns and a mountns inside?
> Many idmappings to one is in principle possible and I've noted that idea
> down as a possible extension at
> https://github.com/uapi-group/kernel-features quite a while (2 years?) ago.
>
> > I haven't looked at the idmap implementation nearly enough to have any
> > opinion as to whether squashing UID is practical or whether there's
>
> It's doable. The interesting bit to me was that if we want to allow
> writes we'd need a way to determine what the uid/gid would be to write
> down. Imho, that's not super difficult to solve though. The most obvious
> one is that userspace can just determine it when creating the idmapped
> mount.
Seems reasonable to me. If this is set up by someone unprivileged, it
would need to be that uid/gid.
^ permalink raw reply
* Re: [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support
From: Edgecombe, Rick P @ 2024-05-07 17:35 UTC (permalink / raw)
To: jolsa@kernel.org, mhiramat@kernel.org, rostedt@goodmis.org,
ast@kernel.org, andrii@kernel.org, oleg@redhat.com,
daniel@iogearbox.net
Cc: debug@rivosinc.com, luto@kernel.org, bp@alien8.de, yhs@fb.com,
songliubraving@fb.com, linux-api@vger.kernel.org, x86@kernel.org,
linux-kernel@vger.kernel.org, john.fastabend@gmail.com,
tglx@linutronix.de, mingo@redhat.com,
linux-trace-kernel@vger.kernel.org, bpf@vger.kernel.org,
linux-man@vger.kernel.org, peterz@infradead.org
In-Reply-To: <20240507105321.71524-7-jolsa@kernel.org>
On Tue, 2024-05-07 at 12:53 +0200, Jiri Olsa wrote:
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 81e6ee95784d..ae6c3458a675 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -406,6 +406,11 @@ SYSCALL_DEFINE0(uretprobe)
> * trampoline's ret instruction
> */
> r11_cx_ax[2] = regs->ip;
> +
> + /* make the shadow stack follow that */
> + if (shstk_push_frame(regs->ip))
> + goto sigill;
> +
> regs->ip = ip;
>
Per the earlier discussion, this cannot be reached unless uretprobes are in use,
which cannot happen without something with privileges taking an action. But are
uretprobes ever used for monitoring applications where security is important? Or
is it strictly a debug-time thing?
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox