Linux-man Archive mirror
 help / color / mirror / Atom feed
From: Jiri Olsa <olsajiri@gmail.com>
To: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>
Cc: "olsajiri@gmail.com" <olsajiri@gmail.com>,
	"mhiramat@kernel.org" <mhiramat@kernel.org>,
	"songliubraving@fb.com" <songliubraving@fb.com>,
	"luto@kernel.org" <luto@kernel.org>,
	"andrii@kernel.org" <andrii@kernel.org>,
	"debug@rivosinc.com" <debug@rivosinc.com>,
	"john.fastabend@gmail.com" <john.fastabend@gmail.com>,
	"linux-api@vger.kernel.org" <linux-api@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"rostedt@goodmis.org" <rostedt@goodmis.org>,
	"ast@kernel.org" <ast@kernel.org>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"yhs@fb.com" <yhs@fb.com>, "oleg@redhat.com" <oleg@redhat.com>,
	"linux-man@vger.kernel.org" <linux-man@vger.kernel.org>,
	"daniel@iogearbox.net" <daniel@iogearbox.net>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"linux-trace-kernel@vger.kernel.org"
	<linux-trace-kernel@vger.kernel.org>,
	"bp@alien8.de" <bp@alien8.de>,
	"bpf@vger.kernel.org" <bpf@vger.kernel.org>,
	"x86@kernel.org" <x86@kernel.org>
Subject: Re: [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support
Date: Mon, 13 May 2024 15:23:42 -0600	[thread overview]
Message-ID: <ZkKE3qT1X_Jirb92@krava> (raw)
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.
> 
> 

  reply	other threads:[~2024-05-13 21:23 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-05-07 10:53 [PATCHv5 bpf-next 0/8] uprobe: uretprobe speed up Jiri Olsa
2024-05-07 10:53 ` [PATCHv5 bpf-next 1/8] uprobe: Wire up uretprobe system call Jiri Olsa
2024-05-07 10:53 ` [PATCHv5 bpf-next 2/8] uprobe: Add uretprobe syscall to speed up return probe Jiri Olsa
2024-05-07 10:53 ` [PATCHv5 bpf-next 3/8] selftests/bpf: Add uretprobe syscall test for regs integrity Jiri Olsa
2024-05-07 10:53 ` [PATCHv5 bpf-next 4/8] selftests/bpf: Add uretprobe syscall test for regs changes Jiri Olsa
2024-05-07 10:53 ` [PATCHv5 bpf-next 5/8] selftests/bpf: Add uretprobe syscall call from user space test Jiri Olsa
2024-05-07 16:57   ` Andrii Nakryiko
2024-05-07 10:53 ` [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support Jiri Olsa
2024-05-07 17:35   ` Edgecombe, Rick P
2024-05-09  8:30     ` Jiri Olsa
2024-05-09 16:24       ` Edgecombe, Rick P
2024-05-11 21:09         ` Jiri Olsa
2024-05-13  9:50           ` Masami Hiramatsu
2024-05-13 17:12             ` Edgecombe, Rick P
2024-05-13 21:23               ` Jiri Olsa [this message]
2024-05-15  1:10                 ` Edgecombe, Rick P
2024-05-15  1:44                   ` Deepak Gupta
2024-05-15 11:19                     ` Oleg Nesterov
2024-05-15 14:36                       ` Jiri Olsa
2024-05-15 15:18                         ` Edgecombe, Rick P
2024-05-15 15:26                         ` Oleg Nesterov
2024-05-15 15:31                           ` Edgecombe, Rick P
2024-05-15 11:35                 ` Oleg Nesterov
2024-05-15 15:13                   ` Edgecombe, Rick P
2024-05-15 15:42                     ` Oleg Nesterov
2024-05-19 22:18                       ` Jiri Olsa
2024-05-21  1:31                         ` Edgecombe, Rick P
2024-05-21 10:11                           ` Jiri Olsa
2024-05-07 10:53 ` [PATCHv5 bpf-next 7/8] selftests/x86: Add return uprobe shadow stack test Jiri Olsa
2024-05-13  9:45   ` Masami Hiramatsu
2024-05-13 21:28     ` Jiri Olsa
2024-05-07 10:53 ` [PATCHv5 8/8] man2: Add uretprobe syscall page Jiri Olsa
2024-05-07 11:13   ` Dmitry V. Levin
2024-05-07 11:49     ` Jiri Olsa
2024-05-07 13:44       ` Alejandro Colomar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZkKE3qT1X_Jirb92@krava \
    --to=olsajiri@gmail.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bp@alien8.de \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=debug@rivosinc.com \
    --cc=john.fastabend@gmail.com \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-man@vger.kernel.org \
    --cc=linux-trace-kernel@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=mhiramat@kernel.org \
    --cc=mingo@redhat.com \
    --cc=oleg@redhat.com \
    --cc=peterz@infradead.org \
    --cc=rick.p.edgecombe@intel.com \
    --cc=rostedt@goodmis.org \
    --cc=songliubraving@fb.com \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    --cc=yhs@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).