| Message ID | 20231206-alice-file-v2-5-af617c0d9d94@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | File abstractions needed by Rust Binder | expand |
On Wed, Dec 06, 2023 at 11:59:50AM +0000, Alice Ryhl wrote: > diff --git a/rust/helpers.c b/rust/helpers.c > index fd633d9db79a..58e3a9dff349 100644 > --- a/rust/helpers.c > +++ b/rust/helpers.c > @@ -142,6 +142,51 @@ void rust_helper_put_task_struct(struct task_struct *t) > } > EXPORT_SYMBOL_GPL(rust_helper_put_task_struct); > > +kuid_t rust_helper_task_uid(struct task_struct *task) > +{ > + return task_uid(task); > +} > +EXPORT_SYMBOL_GPL(rust_helper_task_uid); > + > +kuid_t rust_helper_task_euid(struct task_struct *task) > +{ > + return task_euid(task); > +} > +EXPORT_SYMBOL_GPL(rust_helper_task_euid); So I still object to these on the ground that they're obvious and trivial speculation gadgets. We should not have (exported) functions that are basically a single dereference of a pointer argument. And I do not appreciate my feedback on the previous round being ignored.
On Wed, Dec 6, 2023 at 1:34 PM Peter Zijlstra <peterz@infradead.org> wrote: > > On Wed, Dec 06, 2023 at 11:59:50AM +0000, Alice Ryhl wrote: > > > diff --git a/rust/helpers.c b/rust/helpers.c > > index fd633d9db79a..58e3a9dff349 100644 > > --- a/rust/helpers.c > > +++ b/rust/helpers.c > > @@ -142,6 +142,51 @@ void rust_helper_put_task_struct(struct task_struct *t) > > } > > EXPORT_SYMBOL_GPL(rust_helper_put_task_struct); > > > > +kuid_t rust_helper_task_uid(struct task_struct *task) > > +{ > > + return task_uid(task); > > +} > > +EXPORT_SYMBOL_GPL(rust_helper_task_uid); > > + > > +kuid_t rust_helper_task_euid(struct task_struct *task) > > +{ > > + return task_euid(task); > > +} > > +EXPORT_SYMBOL_GPL(rust_helper_task_euid); > > So I still object to these on the ground that they're obvious and > trivial speculation gadgets. > > We should not have (exported) functions that are basically a single > dereference of a pointer argument. > > And I do not appreciate my feedback on the previous round being ignored. I'm sorry about that. I barely know what speculation gadgets are, so I didn't really know what to respond. But I should have responded by saying that. I can reimplement these specific functions as inline Rust functions, but I don't think I can give you a general solution to the rust_helper_* problem in this patch series. Alice
On Wed, Dec 06, 2023 at 01:57:52PM +0100, Alice Ryhl wrote: > On Wed, Dec 6, 2023 at 1:34 PM Peter Zijlstra <peterz@infradead.org> wrote: > > > > On Wed, Dec 06, 2023 at 11:59:50AM +0000, Alice Ryhl wrote: > > > > > diff --git a/rust/helpers.c b/rust/helpers.c > > > index fd633d9db79a..58e3a9dff349 100644 > > > --- a/rust/helpers.c > > > +++ b/rust/helpers.c > > > @@ -142,6 +142,51 @@ void rust_helper_put_task_struct(struct task_struct *t) > > > } > > > EXPORT_SYMBOL_GPL(rust_helper_put_task_struct); > > > > > > +kuid_t rust_helper_task_uid(struct task_struct *task) > > > +{ > > > + return task_uid(task); > > > +} > > > +EXPORT_SYMBOL_GPL(rust_helper_task_uid); > > > + > > > +kuid_t rust_helper_task_euid(struct task_struct *task) > > > +{ > > > + return task_euid(task); > > > +} > > > +EXPORT_SYMBOL_GPL(rust_helper_task_euid); > > > > So I still object to these on the ground that they're obvious and > > trivial speculation gadgets. > > > > We should not have (exported) functions that are basically a single > > dereference of a pointer argument. > > > > And I do not appreciate my feedback on the previous round being ignored. > > I'm sorry about that. I barely know what speculation gadgets are, so I > didn't really know what to respond. But I should have responded by > saying that. Sigh... how many years since Meltdown are we now? Oh well, the basic idea is to abuse the basic speculative execution of the CPU to make it disclose secrets. The way this is done is by training the various branch predictors such that you gain control over the speculative control flow. You then use this control to cause micro-architectural side-effects to observe state, eg. cache state. In the case above, if you train the indirect branch predictor to jump to the above functions after you've loaded a secret into %rdi (first argument) then it will dereference your pointer + offset. This causes the CPU to populate the cacheline, even if the actual execution path never does this. So by wiping the cache, doing your tricky thing, and then probing the cache to find out which line is present, you can infer the secret value. Anywhoo, the longer a function is, the harder it becomes, since you need to deal with everything a function does and consider the specuation window length. So trivial functions like the above that do an immediate dereference and are (and must be) a valid indirect target (because EXPORT) are ideal. > I can reimplement these specific functions as inline Rust functions, That would be good, but how are you going to do that without duplicating the horror that is struct task_struct ? > but I don't think I can give you a general solution to the > rust_helper_* problem in this patch series. Well, I really wish the Rust community would address the C interoperability in a hurry. Basically make it a requirement for in-kernel Rust. I mean, how hard can it be to have clang parse the C headers and inject them into the Rust IR as if they're external FFI things.
On Wed, Dec 6, 2023 at 2:40 PM Peter Zijlstra <peterz@infradead.org> wrote: > > I can reimplement these specific functions as inline Rust functions, > > That would be good, but how are you going to do that without duplicating > the horror that is struct task_struct ? That shouldn't be an issue. The bindgen tool takes care of generating a Rust version of struct task_struct for us at build time. The only thing it can't handle is inline functions and #defines. I'll respond to the other things later. But thank you for the thorough explanation! Alice
On Wed, Dec 06, 2023 at 02:40:41PM +0100, Peter Zijlstra wrote: > On Wed, Dec 06, 2023 at 01:57:52PM +0100, Alice Ryhl wrote: > > On Wed, Dec 6, 2023 at 1:34 PM Peter Zijlstra <peterz@infradead.org> wrote: > > I can reimplement these specific functions as inline Rust functions, > > That would be good, but how are you going to do that without duplicating > the horror that is struct task_struct ? > > > but I don't think I can give you a general solution to the > > rust_helper_* problem in this patch series. > > Well, I really wish the Rust community would address the C > interoperability in a hurry. Basically make it a requirement for > in-kernel Rust. > > I mean, how hard can it be to have clang parse the C headers and inject > them into the Rust IR as if they're external FFI things. That's pretty much how Swift and Carbon are doing C (and C++) interop. Carbon: https://youtu.be/1ZTJ9omXOQ0?si=yiuLHn6o8RMezEZj Swift: https://youtu.be/lgivCGdmFrw?si=-x9Uva-_Y2x-JNBe The swift talk doesn't allude much to the codegen interop they're doing (still an excellent talk), but the carbon folks have adopted a model from Swift of doing interop at the IR layer. Those compilers link against clang to provide IR interop. Rust's bindgen crate links against clang to generate Rust stubs. At some point, someone on the Rust side will notice what Swift and Carbon are up to, realize they're probably already linking against libclang for C/C++ interop, and try just linking libclang into rustc itself.
On Wed, Dec 6, 2023 at 2:41 PM Peter Zijlstra <peterz@infradead.org> wrote: > > Anywhoo, the longer a function is, the harder it becomes, since you need > to deal with everything a function does and consider the specuation > window length. So trivial functions like the above that do an immediate > dereference and are (and must be) a valid indirect target (because > EXPORT) are ideal. We discussed this in our weekly meeting, and we would like to ask a few questions: - Could you please describe an example attack that you are thinking of? (i.e. a "full" attack, rather than just Spectre itself). For instance, would it rely on other vulnerabilities? - Is this a kernel rule everybody should follow now? i.e. "no (new?) short, exported symbols that just dereference their pointer args". If so, could this please be documented? Or is it already somewhere? - Are we checking for this in some way already, e.g. via `objtool`? Especially if this is a rule, then it would be nice to have a way to double-check if we are getting rid of (most of) these "dangerous" symbols (or at least not introduce new ones, and not just in Rust but C too). Thanks Peter! > That would be good, but how are you going to do that without duplicating > the horror that is struct task_struct ? As Alice pointed out, `bindgen` "solves" that, but it is nevertheless extra maintenance effort. > Well, I really wish the Rust community would address the C > interoperability in a hurry. Basically make it a requirement for > in-kernel Rust. Yeah, some of us have advocated for more integrated C support within Rust (or within `rustc` at least). > I mean, how hard can it be to have clang parse the C headers and inject > them into the Rust IR as if they're external FFI things. That is what `bindgen` does (it uses Clang as a library), except it does not create Rust IR, it outputs normal Rust code, i.e. similar to C declarations. But note that using Clang does not solve the issue of `#define`s in the general case. That is why we would still need "helpers" like these so that the compiler knows how to expand the macro in a C context, which then can be inlined as LLVM IR or similar (which is what I suspect you were actually thinking about, rather than "Rust IR"?). That "mix the LLVM IRs from Clang and `rustc`" ("local LTO hack") approach is something we have been discussing in the past for performance reasons (i.e. to inline these small C functions that Rust needs, cross-language, even in non-LTO builds). And if it helps to avoid certain attacks around speculation, then even better. So if the LLVM folks do not have any major concerns about it, then I think we should go ahead with that (please see also my reply to comex). GCC is still a question mark, though. Cheers, Miguel
On 12/6/23 12:59, Alice Ryhl wrote: > + /// Returns the given task's pid in the current pid namespace. > + pub fn pid_in_current_ns(&self) -> Pid { > + // SAFETY: Calling `task_active_pid_ns` with the current task is always safe. > + let namespace = unsafe { bindings::task_active_pid_ns(bindings::get_current()) }; Why not create a safe wrapper for `bindings::get_current()`? This patch series has three occurrences of `get_current`, so I think it should be ok to add a wrapper. I would also prefer to move the call to `bindings::get_current()` out of the `unsafe` block. > + // SAFETY: We know that `self.0.get()` is valid by the type invariant. What about `namespace`?
On Fri, Dec 08, 2023 at 04:40:09PM +0000, Benno Lossin wrote: > On 12/6/23 12:59, Alice Ryhl wrote: > > + /// Returns the given task's pid in the current pid namespace. > > + pub fn pid_in_current_ns(&self) -> Pid { > > + // SAFETY: Calling `task_active_pid_ns` with the current task is always safe. > > + let namespace = unsafe { bindings::task_active_pid_ns(bindings::get_current()) }; > > Why not create a safe wrapper for `bindings::get_current()`? > This patch series has three occurrences of `get_current`, so I think it > should be ok to add a wrapper. > I would also prefer to move the call to `bindings::get_current()` out of > the `unsafe` block. FWIW, we have a current!() macro, we should use it here. Regards, Boqun > > > + // SAFETY: We know that `self.0.get()` is valid by the type invariant. > > What about `namespace`? > > -- > Cheers, > Benno > > > + unsafe { bindings::task_tgid_nr_ns(self.0.get(), namespace) } > > + }
On Fri, Dec 08, 2023 at 05:31:59PM +0100, Miguel Ojeda wrote: > On Wed, Dec 6, 2023 at 2:41 PM Peter Zijlstra <peterz@infradead.org> wrote: > > > > Anywhoo, the longer a function is, the harder it becomes, since you need > > to deal with everything a function does and consider the specuation > > window length. So trivial functions like the above that do an immediate > > dereference and are (and must be) a valid indirect target (because > > EXPORT) are ideal. > > We discussed this in our weekly meeting, and we would like to ask a > few questions: > > - Could you please describe an example attack that you are thinking > of? (i.e. a "full" attack, rather than just Spectre itself). For > instance, would it rely on other vulnerabilities? There's a fairly large amount of that on github, google spectre poc and stuff like that. > - Is this a kernel rule everybody should follow now? i.e. "no (new?) > short, exported symbols that just dereference their pointer args". If > so, could this please be documented? Or is it already somewhere? Gadget scanners are ever evolving and I think there's a bunch of groups running them against Linux, but most of us don't have spare time beyond trying to plug the latest hole. > - Are we checking for this in some way already, e.g. via `objtool`? > Especially if this is a rule, then it would be nice to have a way to > double-check if we are getting rid of (most of) these "dangerous" > symbols (or at least not introduce new ones, and not just in Rust but > C too). Objtool does not look for these (gadget scanners are quite complicated by now and I don't think I want to go there with objtool). At the moment I'm simply fixing whatever gets reported from 3rd parties when and where time permits. The thing at hand was just me eyeballing it. > > That would be good, but how are you going to do that without duplicating > > the horror that is struct task_struct ? > > As Alice pointed out, `bindgen` "solves" that, but it is nevertheless > extra maintenance effort. > > > Well, I really wish the Rust community would address the C > > interoperability in a hurry. Basically make it a requirement for > > in-kernel Rust. > > Yeah, some of us have advocated for more integrated C support within > Rust (or within `rustc` at least). \o/ > > I mean, how hard can it be to have clang parse the C headers and inject > > them into the Rust IR as if they're external FFI things. > > That is what `bindgen` does (it uses Clang as a library), except it > does not create Rust IR, it outputs normal Rust code, i.e. similar to > C declarations. Right, but then you get into the problem that Rust simply cannot express a fair amount of the things we already do, like asm-goto, or even simple asm with memops apparently. > But note that using Clang does not solve the issue of `#define`s in > the general case. That is why we would still need "helpers" like these > so that the compiler knows how to expand the macro in a C context, > which then can be inlined as LLVM IR or similar (which is what I > suspect you were actually thinking about, rather than "Rust IR"?). Yeah, LLVM-IR. And urgh yeah, CPP, this is another down-side of Rust not being in the C language family, you can't sanely run CPP on it. Someone really should do a Rust like language in the C family, then perhaps it will stop looking like line noise to me :-) I suppose converting things to enum and inline functions where possible might help a bit with that, but things like tracepoints, which are built from a giant pile of CPP are just not going to be happy :/ Anyway, I think it would be a giant step forwards from where we are today. > That "mix the LLVM IRs from Clang and `rustc`" ("local LTO hack") > approach is something we have been discussing in the past for > performance reasons (i.e. to inline these small C functions that Rust > needs, cross-language, even in non-LTO builds). And if it helps to > avoid certain attacks around speculation, then even better. So if the > LLVM folks do not have any major concerns about it, then I think we > should go ahead with that (please see also my reply to comex). But does LTO make any guarantees about inlining? The thing is, with actual LLVM-IR you can express the __always_inline attribute and inlining becomes guaranteed, I don't think you can rely on LTO for the same level of guarantees. And you still need to create these C functions by hand in this local-LTO scenario, which is less than ideal.
On Fri, Dec 08, 2023 at 05:57:02PM +0100, Peter Zijlstra wrote: > On Fri, Dec 08, 2023 at 05:31:59PM +0100, Miguel Ojeda wrote: > > On Wed, Dec 6, 2023 at 2:41 PM Peter Zijlstra <peterz@infradead.org> wrote: > > > > > > Anywhoo, the longer a function is, the harder it becomes, since you need > > > to deal with everything a function does and consider the specuation > > > window length. So trivial functions like the above that do an immediate > > > dereference and are (and must be) a valid indirect target (because > > > EXPORT) are ideal. > > > > We discussed this in our weekly meeting, and we would like to ask a > > few questions: > > > > - Could you please describe an example attack that you are thinking > > of? (i.e. a "full" attack, rather than just Spectre itself). For > > instance, would it rely on other vulnerabilities? > > There's a fairly large amount of that on github, google spectre poc and > stuff like that. tl;dr: I don't think the introduction of speculation gadgets is a sufficient reason to block Rust interfaces like this. Long version: I think the question here is "what is the threat model?" If I break down the objection, I understand it as: 1) The trivial wrappers-of-inlines are speculation gadgets. 2) They're exported, so callable by anything. If the threat model is "something can call these to trigger speculation", I think this is pretty strongly mitigated already; 1) These aren't syscall definitions, so their "reachability" is pretty limited. In fact, they're already going to be used in places, logically, where the inline would be used, so the speculation window is going to be same (or longer, given the addition of the direct call and return). 2) If an attacker is in a position to directly call these helpers, they're not going to use them: if an attacker already has arbitrary execution, they're not going to bother with speculation. Fundamentally I don't see added risk here. From the security hardening perspective we have two goals: kill bug classes and block exploitation techniques, and the former is a much more powerful defensive strategy since without the bugs, there's no chance to perform an exploit. In general, I think we should prioritize bug class elimination over exploit technique foiling. In this case, we're adding a potential weakness to the image of the kernel of fairly limited scope in support of stronger bug elimination goals. Even if we look at the prerequisites for mounting an attack here, we've already got things in place to help mitigate arbitrary code execution (KCFI, BTI, etc). Nothing is perfect, but speculation gadgets are pretty far down on the list of concerns, IMO. We have no real x86 ROP defense right now in the kernel, so that's a much lower hanging fruit for attackers. As another comparison, on x86 there are so many direct execution gadgets present in middle-of-instruction code patterns that worrying about a speculation gadget seems silly to me. > [...] > The thing at hand was just me eyeballing it. I can understand the point you and Greg have both expressed here: "this is known to be an anti-pattern, we need to do something else". I generally agree with this, but in this case, I don't think it's the right call. This is an area we'll naturally see improvement from on the Rust side since these calls are a _performance_ concern too, so it's not like this will be "forgotten" about. But blocking it until there is a complete and perfect solution feels like we're letting perfect be the enemy of good. All of our development is evolutionary, so I think we'd be in a much better position to take these (currently ugly) work-arounds (visible only in Rust builds) so that we gain the ability to evolve towards more memory safe code. -Kees
On Fri, Dec 08, 2023 at 10:18:47AM -0800, Kees Cook wrote: > Even if we look at the prerequisites for mounting an attack here, we've > already got things in place to help mitigate arbitrary code execution > (KCFI, BTI, etc). Nothing is perfect, but speculation gadgets are > pretty far down on the list of concerns, IMO. We have no real x86 ROP > defense right now in the kernel, so that's a much lower hanging fruit > for attackers. Supervisor shadow stacks, as they exist today, just can't work on Linux. Should get fixed with FRED, but yeah, this is all somewhat unfortunate. > As another comparison, on x86 there are so many direct execution gadgets > present in middle-of-instruction code patterns that worrying about a > speculation gadget seems silly to me. FineIBT (or even IBT) limits the middle of function gadgets significantly.
On Fri, Dec 08, 2023 at 09:45:01PM +0100, Peter Zijlstra wrote: > On Fri, Dec 08, 2023 at 10:18:47AM -0800, Kees Cook wrote: > > > Even if we look at the prerequisites for mounting an attack here, we've > > already got things in place to help mitigate arbitrary code execution > > (KCFI, BTI, etc). Nothing is perfect, but speculation gadgets are > > pretty far down on the list of concerns, IMO. We have no real x86 ROP > > defense right now in the kernel, so that's a much lower hanging fruit > > for attackers. > > Supervisor shadow stacks, as they exist today, just can't work on Linux. Yeah, totally agreed. I still wonder if we can extend KCFI to cover return paths (i.e. emitting cookies for return destinations and doing pre-return cookie checking for return destinations). > Should get fixed with FRED, but yeah, this is all somewhat unfortunate. Agreed. > > As another comparison, on x86 there are so many direct execution gadgets > > present in middle-of-instruction code patterns that worrying about a > > speculation gadget seems silly to me. > > FineIBT (or even IBT) limits the middle of function gadgets > significantly. Right -- for indirect calls we are at least able to restrict to same-prototype (KCFI) or "actual function" (IBT). Regardless, for the case at hand, it seems like the Rust wrappers are still not "reachable" since we do BTB stuffing to defang these kinds of speculation gadgets. -Kees
Benno Lossin <benno.lossin@proton.me> writes: > On 12/6/23 12:59, Alice Ryhl wrote: > > + /// Returns the given task's pid in the current pid namespace. > > + pub fn pid_in_current_ns(&self) -> Pid { > > + // SAFETY: Calling `task_active_pid_ns` with the current task is always safe. > > + let namespace = unsafe { bindings::task_active_pid_ns(bindings::get_current()) }; > > Why not create a safe wrapper for `bindings::get_current()`? > This patch series has three occurrences of `get_current`, so I think it > should be ok to add a wrapper. > I would also prefer to move the call to `bindings::get_current()` out of > the `unsafe` block. See thread on patch 6. > > + // SAFETY: We know that `self.0.get()` is valid by the type invariant. > > What about `namespace`? I'll update the comment. Alice
On Fri, Dec 08, 2023 at 08:43:19AM -0800, Boqun Feng wrote: > On Fri, Dec 08, 2023 at 04:40:09PM +0000, Benno Lossin wrote: > > On 12/6/23 12:59, Alice Ryhl wrote: > > > + /// Returns the given task's pid in the current pid namespace. > > > + pub fn pid_in_current_ns(&self) -> Pid { > > > + // SAFETY: Calling `task_active_pid_ns` with the current task is always safe. > > > + let namespace = unsafe { bindings::task_active_pid_ns(bindings::get_current()) }; > > > > Why not create a safe wrapper for `bindings::get_current()`? > > This patch series has three occurrences of `get_current`, so I think it > > should be ok to add a wrapper. > > I would also prefer to move the call to `bindings::get_current()` out of > > the `unsafe` block. > > FWIW, we have a current!() macro, we should use it here. Why does it need to be a macro?
On 12/11/23 16:58, Kent Overstreet wrote: > On Fri, Dec 08, 2023 at 08:43:19AM -0800, Boqun Feng wrote: >> On Fri, Dec 08, 2023 at 04:40:09PM +0000, Benno Lossin wrote: >>> On 12/6/23 12:59, Alice Ryhl wrote: >>>> + /// Returns the given task's pid in the current pid namespace. >>>> + pub fn pid_in_current_ns(&self) -> Pid { >>>> + // SAFETY: Calling `task_active_pid_ns` with the current task is always safe. >>>> + let namespace = unsafe { bindings::task_active_pid_ns(bindings::get_current()) }; >>> >>> Why not create a safe wrapper for `bindings::get_current()`? >>> This patch series has three occurrences of `get_current`, so I think it >>> should be ok to add a wrapper. >>> I would also prefer to move the call to `bindings::get_current()` out of >>> the `unsafe` block. >> >> FWIW, we have a current!() macro, we should use it here. > > Why does it need to be a macro? This is a very interesting question. A `Task` is `AlwaysRefCounted`, so if you have a `&'a Task`, someone above you owns a refcount on that task. But the `current` task will never go away as long as you stay in the same task. So you actually do not need to own a refcount as long as there are no context switches. We use this to our advantage and the `current!()` macro returns something that acts like `&'a Task` but additionally is `!Send` (so it cannot be sent over to a different task). This means that we do not need to take a refcount on the current task to get a reference to it. But just having a function that returns a `&'a Task` like thing that is `!Send` is not enough, since there are no constraints on 'a. This is because the function `Task::current` would take no arguments and there is nothing the lifetime could even bind to. Since there are no constraints, you could just choose 'a = 'static which is obviously wrong, since there are tasks that end. For this reason the `Task::current` function is `unsafe` and the macro `current!` ensures that the lifetime 'a ends early enough. It is implemented like this: macro_rules! current { () => { // SAFETY: Deref + addr-of below create a temporary `TaskRef` that cannot outlive the // caller. unsafe { &*$crate::task::Task::current() } }; } Note the `&*`. This ensures that the thing returned by `Task::current` is temporary and cannot outlive the current function thus preventing the creation of static references to it. If you want to read more, see here for Gary's original discovery of the issue: https://lore.kernel.org/rust-for-linux/20230331034701.0657d5f2.gary@garyguo.net/
On Fri, Dec 08, 2023 at 10:18:47AM -0800, Kees Cook wrote: > On Fri, Dec 08, 2023 at 05:57:02PM +0100, Peter Zijlstra wrote: > > On Fri, Dec 08, 2023 at 05:31:59PM +0100, Miguel Ojeda wrote: > > > On Wed, Dec 6, 2023 at 2:41 PM Peter Zijlstra <peterz@infradead.org> wrote: > > > > > > > > Anywhoo, the longer a function is, the harder it becomes, since you need > > > > to deal with everything a function does and consider the specuation > > > > window length. So trivial functions like the above that do an immediate > > > > dereference and are (and must be) a valid indirect target (because > > > > EXPORT) are ideal. > > > > > > We discussed this in our weekly meeting, and we would like to ask a > > > few questions: > > > > > > - Could you please describe an example attack that you are thinking > > > of? (i.e. a "full" attack, rather than just Spectre itself). For > > > instance, would it rely on other vulnerabilities? > > > > There's a fairly large amount of that on github, google spectre poc and > > stuff like that. > > tl;dr: I don't think the introduction of speculation gadgets is a > sufficient reason to block Rust interfaces like this. > > Long version: > > I think the question here is "what is the threat model?" If I break down > the objection, I understand it as: > > 1) The trivial wrappers-of-inlines are speculation gadgets. > 2) They're exported, so callable by anything. > > If the threat model is "something can call these to trigger > speculation", I think this is pretty strongly mitigated already; > > 1) These aren't syscall definitions, so their "reachability" is pretty > limited. In fact, they're already going to be used in places, logically, > where the inline would be used, so the speculation window is going to be > same (or longer, given the addition of the direct call and return). > > 2) If an attacker is in a position to directly call these helpers, > they're not going to use them: if an attacker already has arbitrary > execution, they're not going to bother with speculation. > > Fundamentally I don't see added risk here. From the security hardening > perspective we have two goals: kill bug classes and block exploitation > techniques, and the former is a much more powerful defensive strategy > since without the bugs, there's no chance to perform an exploit. > > In general, I think we should prioritize bug class elimination over > exploit technique foiling. In this case, we're adding a potential weakness > to the image of the kernel of fairly limited scope in support of stronger > bug elimination goals. > > Even if we look at the prerequisites for mounting an attack here, we've > already got things in place to help mitigate arbitrary code execution > (KCFI, BTI, etc). Nothing is perfect, but speculation gadgets are > pretty far down on the list of concerns, IMO. We have no real x86 ROP > defense right now in the kernel, so that's a much lower hanging fruit > for attackers. > > As another comparison, on x86 there are so many direct execution gadgets > present in middle-of-instruction code patterns that worrying about a > speculation gadget seems silly to me. > > > [...] > > The thing at hand was just me eyeballing it. > > I can understand the point you and Greg have both expressed here: "this > is known to be an anti-pattern, we need to do something else". I generally > agree with this, but in this case, I don't think it's the right call. This > is an area we'll naturally see improvement from on the Rust side since > these calls are a _performance_ concern too, so it's not like this will be > "forgotten" about. But blocking it until there is a complete and perfect > solution feels like we're letting perfect be the enemy of good. > > All of our development is evolutionary, so I think we'd be in a much > better position to take these (currently ugly) work-arounds (visible > only in Rust builds) so that we gain the ability to evolve towards more > memory safe code. Well said. More than that, I think this is a situation where we really need to consider what our priorities are. Where are most of our real world vulnerabilities coming from? Are they spectre issues, or are they memory safety issues, and the kinds of logic/resource issues we know Rust's type system is going to help with? I think we all know the answer to that question. More than that, as kernel programmers, we need to be focusing primarily on the issues that are our responsibility and under our control. At the point when the inability of the CPU manufacturers to get their shit together is causing us to have concerns over _generated code for little helper functions_, the world has gone crazy. I remember when the spectre stuff first hit, and everyone was saying "this is just a temporary workaround; CPU manufacturers are going to sort this out in a couple releases". They haven't. That's batshit. And I doubt they ever will if we keep pretending we can just work around everything in software.
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index 81b13a953eae..700f01840188 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -11,6 +11,7 @@ #include <linux/errname.h> #include <linux/file.h> #include <linux/fs.h> +#include <linux/pid_namespace.h> #include <linux/security.h> #include <linux/slab.h> #include <linux/refcount.h> diff --git a/rust/helpers.c b/rust/helpers.c index fd633d9db79a..58e3a9dff349 100644 --- a/rust/helpers.c +++ b/rust/helpers.c @@ -142,6 +142,51 @@ void rust_helper_put_task_struct(struct task_struct *t) } EXPORT_SYMBOL_GPL(rust_helper_put_task_struct); +kuid_t rust_helper_task_uid(struct task_struct *task) +{ + return task_uid(task); +} +EXPORT_SYMBOL_GPL(rust_helper_task_uid); + +kuid_t rust_helper_task_euid(struct task_struct *task) +{ + return task_euid(task); +} +EXPORT_SYMBOL_GPL(rust_helper_task_euid); + +#ifndef CONFIG_USER_NS +uid_t rust_helper_from_kuid(struct user_namespace *to, kuid_t uid) +{ + return from_kuid(to, uid); +} +EXPORT_SYMBOL_GPL(rust_helper_from_kuid); +#endif /* CONFIG_USER_NS */ + +bool rust_helper_uid_eq(kuid_t left, kuid_t right) +{ + return uid_eq(left, right); +} +EXPORT_SYMBOL_GPL(rust_helper_uid_eq); + +kuid_t rust_helper_current_euid(void) +{ + return current_euid(); +} +EXPORT_SYMBOL_GPL(rust_helper_current_euid); + +struct user_namespace *rust_helper_current_user_ns(void) +{ + return current_user_ns(); +} +EXPORT_SYMBOL_GPL(rust_helper_current_user_ns); + +pid_t rust_helper_task_tgid_nr_ns(struct task_struct *tsk, + struct pid_namespace *ns) +{ + return task_tgid_nr_ns(tsk, ns); +} +EXPORT_SYMBOL_GPL(rust_helper_task_tgid_nr_ns); + struct kunit *rust_helper_kunit_get_current_test(void) { return kunit_get_current_test(); diff --git a/rust/kernel/cred.rs b/rust/kernel/cred.rs index 3794937b5294..fbc749788bfa 100644 --- a/rust/kernel/cred.rs +++ b/rust/kernel/cred.rs @@ -8,6 +8,7 @@ use crate::{ bindings, + task::Kuid, types::{AlwaysRefCounted, Opaque}, }; @@ -52,9 +53,9 @@ pub fn get_secid(&self) -> u32 { } /// Returns the effective UID of the given credential. - pub fn euid(&self) -> bindings::kuid_t { + pub fn euid(&self) -> Kuid { // SAFETY: By the type invariant, we know that `self.0` is valid. - unsafe { (*self.0.get()).euid } + Kuid::from_raw(unsafe { (*self.0.get()).euid }) } } diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs index b2299bc7ac1f..7a3a07660af7 100644 --- a/rust/kernel/task.rs +++ b/rust/kernel/task.rs @@ -5,7 +5,12 @@ //! C header: [`include/linux/sched.h`](../../../../include/linux/sched.h). use crate::{bindings, types::Opaque}; -use core::{marker::PhantomData, ops::Deref, ptr}; +use core::{ + cmp::{Eq, PartialEq}, + marker::PhantomData, + ops::Deref, + ptr, +}; /// Returns the currently running task. #[macro_export] @@ -78,6 +83,12 @@ unsafe impl Sync for Task {} /// The type of process identifiers (PIDs). type Pid = bindings::pid_t; +/// The type of user identifiers (UIDs). +#[derive(Copy, Clone)] +pub struct Kuid { + kuid: bindings::kuid_t, +} + impl Task { /// Returns a task reference for the currently executing task/thread. /// @@ -132,12 +143,32 @@ pub fn pid(&self) -> Pid { unsafe { *ptr::addr_of!((*self.0.get()).pid) } } + /// Returns the UID of the given task. + pub fn uid(&self) -> Kuid { + // SAFETY: By the type invariant, we know that `self.0` is valid. + Kuid::from_raw(unsafe { bindings::task_uid(self.0.get()) }) + } + + /// Returns the effective UID of the given task. + pub fn euid(&self) -> Kuid { + // SAFETY: By the type invariant, we know that `self.0` is valid. + Kuid::from_raw(unsafe { bindings::task_euid(self.0.get()) }) + } + /// Determines whether the given task has pending signals. pub fn signal_pending(&self) -> bool { // SAFETY: By the type invariant, we know that `self.0` is valid. unsafe { bindings::signal_pending(self.0.get()) != 0 } } + /// Returns the given task's pid in the current pid namespace. + pub fn pid_in_current_ns(&self) -> Pid { + // SAFETY: Calling `task_active_pid_ns` with the current task is always safe. + let namespace = unsafe { bindings::task_active_pid_ns(bindings::get_current()) }; + // SAFETY: We know that `self.0.get()` is valid by the type invariant. + unsafe { bindings::task_tgid_nr_ns(self.0.get(), namespace) } + } + /// Wakes up the task. pub fn wake_up(&self) { // SAFETY: By the type invariant, we know that `self.0.get()` is non-null and valid. @@ -147,6 +178,41 @@ pub fn wake_up(&self) { } } +impl Kuid { + /// Get the current euid. + pub fn current_euid() -> Kuid { + // SAFETY: Just an FFI call. + Self::from_raw(unsafe { bindings::current_euid() }) + } + + /// Create a `Kuid` given the raw C type. + pub fn from_raw(kuid: bindings::kuid_t) -> Self { + Self { kuid } + } + + /// Turn this kuid into the raw C type. + pub fn into_raw(self) -> bindings::kuid_t { + self.kuid + } + + /// Converts this kernel UID into a userspace UID. + /// + /// Uses the namespace of the current task. + pub fn into_uid_in_current_ns(self) -> bindings::uid_t { + // SAFETY: Just an FFI call. + unsafe { bindings::from_kuid(bindings::current_user_ns(), self.kuid) } + } +} + +impl PartialEq for Kuid { + fn eq(&self, other: &Kuid) -> bool { + // SAFETY: Just an FFI call. + unsafe { bindings::uid_eq(self.kuid, other.kuid) } + } +} + +impl Eq for Kuid {} + // SAFETY: The type invariants guarantee that `Task` is always ref-counted. unsafe impl crate::types::AlwaysRefCounted for Task { fn inc_ref(&self) {
Adds a wrapper around `kuid_t` called `Kuid`. This allows us to define various operations on kuids such as equality and current_euid. It also lets us provide conversions from kuid into userspace values. Rust Binder needs these operations because it needs to compare kuids for equality, and it needs to tell userspace about the pid and uid of incoming transactions. Signed-off-by: Alice Ryhl <aliceryhl@google.com> --- rust/bindings/bindings_helper.h | 1 + rust/helpers.c | 45 +++++++++++++++++++++++++++ rust/kernel/cred.rs | 5 +-- rust/kernel/task.rs | 68 ++++++++++++++++++++++++++++++++++++++++- 4 files changed, 116 insertions(+), 3 deletions(-)