Received-SPF: pass (google.com: domain of linux-kernel+bounces-178105-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: Jiri Olsa <olsajiri@gmail.com>
Date: Mon, 13 May 2024 15:23:42 -0600
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
Message-ID: <ZkKE3qT1X_Jirb92@krava>
References: <20240507105321.71524-1-jolsa@kernel.org>
 <20240507105321.71524-7-jolsa@kernel.org>
 <a08a955c74682e9dc6eb6d49b91c6968c9b62f75.camel@intel.com>
 <ZjyJsl_u_FmYHrki@krava>
 <a8b7be15e6dbb1e8f2acaee7dae21fec7775194c.camel@intel.com>
 <Zj_enIB_J6pGJ6Nu@krava>
 <20240513185040.416d62bc4a71e79367c1cd9c@kernel.org>
 <c56ae75e9cf0878ac46185a14a18f6ff7e8f891a.camel@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
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.
> 
>