| Message ID | 20181005081333.15018-1-ard.biesheuvel@linaro.org |
|---|---|
| Headers | show |
| Series | patchable function pointers for pluggable crypto routines | expand |
Hi Ard, On Fri, Oct 5, 2018 at 10:13 AM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > At the moment, the Zinc library [1] is being proposed as a solution for that, > and while it does address the usability problems, it does a lot more than > that, and what we end up with is a lot less flexible than what we have now. > > Currently, arch specific implementations (based on SIMD or optimized > assembler) live in arch/*/crypto, where each sub-community is in charge > of its own specialized versions of the various primitives (although still > under the purview of the crypto maintainer). Any proposal to change this > model should be judged on its own merit, and not blindly accepted as the > fallout of cleaning up some library code. > > Also, Zinc removes the possibility to plug different versions of a routine, > and instead, keeps all versions in the same module. Currently, the kernel's > module support permits user land to take charge of the policies that decide > which version to use in which context (by loading or blacklisting modules). I think this explanation misunderstands many of the design goals of Zinc and also points to why going your direction instead is a bad idea that will cause even more problems down the road. Zinc does several important things: - Introduces direct C function calls throughout, as a central way of being implemented and as a central way of being used by consumers of the API. This has various obvious benefits for the consumers of the API, but it also has big benefits for the developers of the library as well. Namely, it keeps the relationship between different parts extremely clear and direct. It's an explicit choice for simplicity. And by being the simpler and more direct solution, it also gives gcc an important opportunity to optimize and inline. - Reorganizes crypto routines so that they're grouped together by primitive. This again leads to a much simpler design and layout, making it more obvious what's happening, and making things generally cleaner. It is not only useful and clearer for developers, but it also makes contributors and auditors more easily aware of what implementations are available. - Has higher standards for code and implementations that are introduced. Zinc prefers code that has been formally verified, that has been in widespread usage and has received many eyeballs and fuzzing hours, that has been fuzzed extensively, that is simple in design, that comes from well-known high-quality authors -- in roughly but not precisely that order of preference. That's a bit different from the current practices of the existing crypto API. - Has a simpler mechanism that is just as effective for choosing available implementations. This is, again, more obvious and direct than the present crypto API's module approach, leads to smaller code size, and has the potential of being just as flexible with the inevitable desire for nobs, adjustable from userspace, from kernelspace, or from elsewhere. - Is designed to promote collaboration with the larger cryptography community and with academia, which will yield better implementations and for assurance. - Can easily be extracted to userspace libraries (perhaps a future libzinc could be easily based on it), which makes testing and fuzzing using tools like libfuzzer and afl more accessible. - Has faster implementations than the current crypto API. - Has, again, a very strong focus on being simple and minimal, as opposed to bloated and complicated, so that it's actually possible to understand and audit the library. Therefore, I think this patch goes in exactly the wrong direction. I mean, if you want to introduce dynamic patching as a means for making the crypto API's dynamic dispatch stuff not as slow in a post-spectre world, sure, go for it; that may very well be a good idea. But presenting it as an alternative to Zinc very widely misses the point and serves to prolong a series of bad design choices, which are now able to be rectified by putting energy into Zinc instead. Jason
On Fri, Oct 05, 2018 at 10:13:25AM +0200, Ard Biesheuvel wrote: > Add a function pointer abstraction that can be implemented by the arch > in a manner that avoids the downsides of function pointers, i.e., the > fact that they are typically located in a writable data section, and > their vulnerability to Spectre like defects. > > The FFP (or fast function pointer) is callable as a function, since > the generic incarnation is simply that. However, due to the fact that > C does not distinguish between functions and function pointers at the > call site, the architecture can instead emit it as a patchable sequence > of instructions consisting of ordinary branches. This is basically a static_key, except for indirection function calls? So why not call the thing static_func or static_call or something like that?
On 5 October 2018 at 15:57, Peter Zijlstra <peterz@infradead.org> wrote: > On Fri, Oct 05, 2018 at 10:13:25AM +0200, Ard Biesheuvel wrote: >> Add a function pointer abstraction that can be implemented by the arch >> in a manner that avoids the downsides of function pointers, i.e., the >> fact that they are typically located in a writable data section, and >> their vulnerability to Spectre like defects. >> >> The FFP (or fast function pointer) is callable as a function, since >> the generic incarnation is simply that. However, due to the fact that >> C does not distinguish between functions and function pointers at the >> call site, the architecture can instead emit it as a patchable sequence >> of instructions consisting of ordinary branches. > > This is basically a static_key, except for indirection function calls? Yes, that is why I put you on cc :-) > So why not call the thing static_func or static_call or something like > that? Yep that sounds better.
On Fri, Oct 05, 2018 at 10:13:25AM +0200, Ard Biesheuvel wrote: > diff --git a/include/linux/ffp.h b/include/linux/ffp.h > new file mode 100644 > index 000000000000..8fc3b4c9b38f > --- /dev/null > +++ b/include/linux/ffp.h > @@ -0,0 +1,43 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +#ifndef __LINUX_FFP_H > +#define __LINUX_FFP_H > + > +#include <linux/types.h> > +#include <linux/compiler.h> > + > +#ifdef CONFIG_HAVE_ARCH_FFP > +#include <asm/ffp.h> > +#else > + > +struct ffp { > + void (**fn)(void); > + void (*default_fn)(void); > +}; > + > +#define DECLARE_FFP(_fn, _def) \ > + extern typeof(_def) *_fn; \ > + extern struct ffp const __ffp_ ## _fn > + > +#define DEFINE_FFP(_fn, _def) \ > + typeof(_def) *_fn = &_def; \ > + struct ffp const __ffp_ ## _fn \ > + = { (void(**)(void))&_fn, (void(*)(void))&_def }; \ > + EXPORT_SYMBOL(__ffp_ ## _fn) > + > +static inline void ffp_set_target(const struct ffp *m, void *new_fn) > +{ > + WRITE_ONCE(*m->fn, new_fn); > +} > + > +static inline void ffp_reset_target(const struct ffp *m) > +{ > + WRITE_ONCE(*m->fn, m->default_fn); > +} > + > +#endif > + > +#define SET_FFP(_fn, _new) ffp_set_target(&__ffp_ ## _fn, _new) > +#define RESET_FFP(_fn) ffp_reset_target(&__ffp_ ## _fn) > + > +#endif I don't understand this interface. There is no wrapper for the call site, so how are we going to patch all call-sites when you update the target?
On 5 October 2018 at 16:14, Peter Zijlstra <peterz@infradead.org> wrote: > On Fri, Oct 05, 2018 at 10:13:25AM +0200, Ard Biesheuvel wrote: >> diff --git a/include/linux/ffp.h b/include/linux/ffp.h >> new file mode 100644 >> index 000000000000..8fc3b4c9b38f >> --- /dev/null >> +++ b/include/linux/ffp.h >> @@ -0,0 +1,43 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> + >> +#ifndef __LINUX_FFP_H >> +#define __LINUX_FFP_H >> + >> +#include <linux/types.h> >> +#include <linux/compiler.h> >> + >> +#ifdef CONFIG_HAVE_ARCH_FFP >> +#include <asm/ffp.h> >> +#else >> + >> +struct ffp { >> + void (**fn)(void); >> + void (*default_fn)(void); >> +}; >> + >> +#define DECLARE_FFP(_fn, _def) \ >> + extern typeof(_def) *_fn; \ >> + extern struct ffp const __ffp_ ## _fn >> + >> +#define DEFINE_FFP(_fn, _def) \ >> + typeof(_def) *_fn = &_def; \ >> + struct ffp const __ffp_ ## _fn \ >> + = { (void(**)(void))&_fn, (void(*)(void))&_def }; \ >> + EXPORT_SYMBOL(__ffp_ ## _fn) >> + >> +static inline void ffp_set_target(const struct ffp *m, void *new_fn) >> +{ >> + WRITE_ONCE(*m->fn, new_fn); >> +} >> + >> +static inline void ffp_reset_target(const struct ffp *m) >> +{ >> + WRITE_ONCE(*m->fn, m->default_fn); >> +} >> + >> +#endif >> + >> +#define SET_FFP(_fn, _new) ffp_set_target(&__ffp_ ## _fn, _new) >> +#define RESET_FFP(_fn) ffp_reset_target(&__ffp_ ## _fn) >> + >> +#endif > > I don't understand this interface. There is no wrapper for the call > site, so how are we going to patch all call-sites when you update the > target? This is the generic implementation, and in this case, it is just a function pointer which gets dereferenced and called indirectly at each call site. In the arch specific implementations (for ones where it matters, see 2/9), it is more like a PLT entry, which gets called from each call site, and which can be patched to point to another function. So we replace a single indirect call with a direct call plus a direct jump.
> On Oct 5, 2018, at 7:14 AM, Peter Zijlstra <peterz@infradead.org> wrote: > >> On Fri, Oct 05, 2018 at 10:13:25AM +0200, Ard Biesheuvel wrote: >> diff --git a/include/linux/ffp.h b/include/linux/ffp.h >> new file mode 100644 >> index 000000000000..8fc3b4c9b38f >> --- /dev/null >> +++ b/include/linux/ffp.h >> @@ -0,0 +1,43 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> + >> +#ifndef __LINUX_FFP_H >> +#define __LINUX_FFP_H >> + >> +#include <linux/types.h> >> +#include <linux/compiler.h> >> + >> +#ifdef CONFIG_HAVE_ARCH_FFP >> +#include <asm/ffp.h> >> +#else >> + >> +struct ffp { >> + void (**fn)(void); >> + void (*default_fn)(void); >> +}; >> + >> +#define DECLARE_FFP(_fn, _def) \ >> + extern typeof(_def) *_fn; \ >> + extern struct ffp const __ffp_ ## _fn >> + >> +#define DEFINE_FFP(_fn, _def) \ >> + typeof(_def) *_fn = &_def; \ >> + struct ffp const __ffp_ ## _fn \ >> + = { (void(**)(void))&_fn, (void(*)(void))&_def }; \ >> + EXPORT_SYMBOL(__ffp_ ## _fn) >> + >> +static inline void ffp_set_target(const struct ffp *m, void *new_fn) >> +{ >> + WRITE_ONCE(*m->fn, new_fn); >> +} >> + >> +static inline void ffp_reset_target(const struct ffp *m) >> +{ >> + WRITE_ONCE(*m->fn, m->default_fn); >> +} >> + >> +#endif >> + >> +#define SET_FFP(_fn, _new) ffp_set_target(&__ffp_ ## _fn, _new) >> +#define RESET_FFP(_fn) ffp_reset_target(&__ffp_ ## _fn) >> + >> +#endif > > I don't understand this interface. There is no wrapper for the call > site, so how are we going to patch all call-sites when you update the > target? I’m also confused. Anyway, we have patchable functions on x86. They’re called PVOPs, and they’re way overcomplicated. I’ve proposed a better way that should generate better code, be more portable, and be more maintainable. It goes like this. To call the function, you literally just call the default implementation. It *might* be necessary to call a nonexistent wrapper to avoid annoying optimizations. At build time, the kernel is built with relocations, so the object files contain relocation entries for the call. We collect these entries into a table. If we’re using the “nonexistent wrapper” approach, we can link in a .S or linker script to alias them to the default implementation. To patch them, we just patch them. It can’t necessarily be done concurrently because nothing forces the right alignment. But we can do it at boot time and module load time. (Maybe we can patch at runtime on architectures with appropriate instruction alignment. Or we ask gcc for an extension to align calls to a function.) Most of the machinery already exists: this is roughly how the module loader resolves calls outside of a module.
On 5 October 2018 at 17:08, Andy Lutomirski <luto@amacapital.net> wrote: > > >> On Oct 5, 2018, at 7:14 AM, Peter Zijlstra <peterz@infradead.org> wrote: >> >>> On Fri, Oct 05, 2018 at 10:13:25AM +0200, Ard Biesheuvel wrote: >>> diff --git a/include/linux/ffp.h b/include/linux/ffp.h >>> new file mode 100644 >>> index 000000000000..8fc3b4c9b38f >>> --- /dev/null >>> +++ b/include/linux/ffp.h >>> @@ -0,0 +1,43 @@ >>> +/* SPDX-License-Identifier: GPL-2.0 */ >>> + >>> +#ifndef __LINUX_FFP_H >>> +#define __LINUX_FFP_H >>> + >>> +#include <linux/types.h> >>> +#include <linux/compiler.h> >>> + >>> +#ifdef CONFIG_HAVE_ARCH_FFP >>> +#include <asm/ffp.h> >>> +#else >>> + >>> +struct ffp { >>> + void (**fn)(void); >>> + void (*default_fn)(void); >>> +}; >>> + >>> +#define DECLARE_FFP(_fn, _def) \ >>> + extern typeof(_def) *_fn; \ >>> + extern struct ffp const __ffp_ ## _fn >>> + >>> +#define DEFINE_FFP(_fn, _def) \ >>> + typeof(_def) *_fn = &_def; \ >>> + struct ffp const __ffp_ ## _fn \ >>> + = { (void(**)(void))&_fn, (void(*)(void))&_def }; \ >>> + EXPORT_SYMBOL(__ffp_ ## _fn) >>> + >>> +static inline void ffp_set_target(const struct ffp *m, void *new_fn) >>> +{ >>> + WRITE_ONCE(*m->fn, new_fn); >>> +} >>> + >>> +static inline void ffp_reset_target(const struct ffp *m) >>> +{ >>> + WRITE_ONCE(*m->fn, m->default_fn); >>> +} >>> + >>> +#endif >>> + >>> +#define SET_FFP(_fn, _new) ffp_set_target(&__ffp_ ## _fn, _new) >>> +#define RESET_FFP(_fn) ffp_reset_target(&__ffp_ ## _fn) >>> + >>> +#endif >> >> I don't understand this interface. There is no wrapper for the call >> site, so how are we going to patch all call-sites when you update the >> target? > > I’m also confused. > > Anyway, we have patchable functions on x86. They’re called PVOPs, and they’re way overcomplicated. > > I’ve proposed a better way that should generate better code, be more portable, and be more maintainable. It goes like this. > > To call the function, you literally just call the default implementation. It *might* be necessary to call a nonexistent wrapper to avoid annoying optimizations. At build time, the kernel is built with relocations, so the object files contain relocation entries for the call. We collect these entries into a table. If we’re using the “nonexistent wrapper” approach, we can link in a .S or linker script to alias them to the default implementation. > > To patch them, we just patch them. It can’t necessarily be done concurrently because nothing forces the right alignment. But we can do it at boot time and module load time. (Maybe we can patch at runtime on architectures with appropriate instruction alignment. Or we ask gcc for an extension to align calls to a function.) > > Most of the machinery already exists: this is roughly how the module loader resolves calls outside of a module. Yeah nothing is ever simple on x86 :-( So are you saying the approach i use in patch #2 (which would translate to emitting a jmpq instruction pointing to the default implementation, and patching it at runtime to point elsewhere) would not fly on x86?
On 5 October 2018 at 18:58, Andy Lutomirski <luto@kernel.org> wrote: > On Fri, Oct 5, 2018 at 8:24 AM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> >> On 5 October 2018 at 17:08, Andy Lutomirski <luto@amacapital.net> wrote: >> > >> > >> >> On Oct 5, 2018, at 7:14 AM, Peter Zijlstra <peterz@infradead.org> wrote: >> >> >> >>> On Fri, Oct 05, 2018 at 10:13:25AM +0200, Ard Biesheuvel wrote: >> >>> diff --git a/include/linux/ffp.h b/include/linux/ffp.h >> >>> new file mode 100644 >> >>> index 000000000000..8fc3b4c9b38f >> >>> --- /dev/null >> >>> +++ b/include/linux/ffp.h >> >>> @@ -0,0 +1,43 @@ >> >>> +/* SPDX-License-Identifier: GPL-2.0 */ >> >>> + >> >>> +#ifndef __LINUX_FFP_H >> >>> +#define __LINUX_FFP_H >> >>> + >> >>> +#include <linux/types.h> >> >>> +#include <linux/compiler.h> >> >>> + >> >>> +#ifdef CONFIG_HAVE_ARCH_FFP >> >>> +#include <asm/ffp.h> >> >>> +#else >> >>> + >> >>> +struct ffp { >> >>> + void (**fn)(void); >> >>> + void (*default_fn)(void); >> >>> +}; >> >>> + >> >>> +#define DECLARE_FFP(_fn, _def) \ >> >>> + extern typeof(_def) *_fn; \ >> >>> + extern struct ffp const __ffp_ ## _fn >> >>> + >> >>> +#define DEFINE_FFP(_fn, _def) \ >> >>> + typeof(_def) *_fn = &_def; \ >> >>> + struct ffp const __ffp_ ## _fn \ >> >>> + = { (void(**)(void))&_fn, (void(*)(void))&_def }; \ >> >>> + EXPORT_SYMBOL(__ffp_ ## _fn) >> >>> + >> >>> +static inline void ffp_set_target(const struct ffp *m, void *new_fn) >> >>> +{ >> >>> + WRITE_ONCE(*m->fn, new_fn); >> >>> +} >> >>> + >> >>> +static inline void ffp_reset_target(const struct ffp *m) >> >>> +{ >> >>> + WRITE_ONCE(*m->fn, m->default_fn); >> >>> +} >> >>> + >> >>> +#endif >> >>> + >> >>> +#define SET_FFP(_fn, _new) ffp_set_target(&__ffp_ ## _fn, _new) >> >>> +#define RESET_FFP(_fn) ffp_reset_target(&__ffp_ ## _fn) >> >>> + >> >>> +#endif >> >> >> >> I don't understand this interface. There is no wrapper for the call >> >> site, so how are we going to patch all call-sites when you update the >> >> target? >> > >> > I’m also confused. >> > >> > Anyway, we have patchable functions on x86. They’re called PVOPs, and they’re way overcomplicated. >> > >> > I’ve proposed a better way that should generate better code, be more portable, and be more maintainable. It goes like this. >> > >> > To call the function, you literally just call the default implementation. It *might* be necessary to call a nonexistent wrapper to avoid annoying optimizations. At build time, the kernel is built with relocations, so the object files contain relocation entries for the call. We collect these entries into a table. If we’re using the “nonexistent wrapper” approach, we can link in a .S or linker script to alias them to the default implementation. >> > >> > To patch them, we just patch them. It can’t necessarily be done concurrently because nothing forces the right alignment. But we can do it at boot time and module load time. (Maybe we can patch at runtime on architectures with appropriate instruction alignment. Or we ask gcc for an extension to align calls to a function.) >> > >> > Most of the machinery already exists: this is roughly how the module loader resolves calls outside of a module. >> >> Yeah nothing is ever simple on x86 :-( >> >> So are you saying the approach i use in patch #2 (which would >> translate to emitting a jmpq instruction pointing to the default >> implementation, and patching it at runtime to point elsewhere) would >> not fly on x86? > > After getting some more sleep, I'm obviously wrong. The > text_poke_bp() mechanism will work. It's just really slow. > OK > Let me try to summarize some of the issues. First, when emitting > jumps and calls from inline asm on x86, there are a few considerations > that are annoying: > > 1. Following the x86_64 ABI calling conventions is basically > impossible. x86_64 requires a 128-byte redzone and 16-byte stack > alignment. After much discussion a while back, we decided that it was > flat-out impossible on current gcc to get the stack pointer aligned in > a known manner in an inline asm statement. Instead, if we actually > need alignment, we need to align manually. Fortunately, the kernel is > built with an override that forces only 8-byte alignment (on *most* > GCC versions). But for crypto in particular, it sucks extra, since > the crypto code is basically the only thing in the kernel that > actually wants 16-byte alignment. I don't think this is a huge > problem in practice, but it's annoying. And the kernel is built > without a redzone. > > 2. On x86_64, depending on config, we either need frame pointers or > ORC. ORC is no big deal -- it Just Works (tm). Frame pointers need > extra asm hackery. It's doable, but it's still annoying. > > 3. Actually getting the asm constraints right to do what a C > programmer expects is distinctly nontrivial. I just fixed an > extremely longstanding bug in the vDSO code in which the asm > constraints for the syscall fallback were wrong in such a way that GCC > didn't notice that the fallback wrote to its output parameter. > Whoops. > OK, so the thing I am missing is why this all matters. Note that the compiler should take care of all of this. It emits a call a function with external linkage having prototype X, and all the inline asm does is emit a jmp to some function having that same prototype, either the default one or the one we patched in. Apologies if I am missing something obvious here: as you know, x86 is not my focus in general. So in the arm64 case, we have " .globl " #_fn " \n" \ #_fn " : \n" \ " b " #_def " \n" \ " nop \n" \ " nop \n" \ " nop \n" \ " nop \n" \ and all the patching does is replace the target of that branch (the NOPs are there for jumps that are more then 128 MB away, which require a indirect jump on arm64) > And having all this asm hackery per architecture is ugly and annoying. > True. But note that only the architectures that cannot tolerate the default instantiation using function pointers will require a special implementation. > So my suggestion is to do it like a regular relocation. Call a > function the normal way (make it literally be a C function and call > it), and rig up the noinline and noclone attributes and whatever else > is needed to make sure that it's a *relocatable* call. Then the > toolchain emits ELF relocations saying exactly what part of the text > needs patching, and we can patch it at runtime. On x86, this is a bit > extra annoying because we can't fully reliably parse backwards to find > the beginning of the instruction, but objtool could doit. > > And then we get something that is mostly arch-neutral! Because surely > ARM can also use a relocation-based mechanism. > Not really. We don't have any build time tooling for this, and CONFIG_RELOCATABLE only produces R_AARCH64_RELATIVE relocations for absolute quantities. So it would mean we'd have to start building vmlinux with --emit-relocs, add tooling to parse all of that etc etc. > I will generally object to x86 containing more than one > inline-asm-hackery-based patchable call mechanism, which your series > will add. I would *love* to see a non-inline-asm one, and then we > could move most of the x86 paravirt crap over to use it for a big win > in readability and maintainability. > Fair enough.
On 5 October 2018 at 19:26, Andy Lutomirski <luto@kernel.org> wrote: > On Fri, Oct 5, 2018 at 10:15 AM Ard Biesheuvel > <ard.biesheuvel@linaro.org> wrote: >> >> On 5 October 2018 at 15:37, Jason A. Donenfeld <Jason@zx2c4.com> wrote: >> ... >> > Therefore, I think this patch goes in exactly the wrong direction. I >> > mean, if you want to introduce dynamic patching as a means for making >> > the crypto API's dynamic dispatch stuff not as slow in a post-spectre >> > world, sure, go for it; that may very well be a good idea. But >> > presenting it as an alternative to Zinc very widely misses the point and >> > serves to prolong a series of bad design choices, which are now able to >> > be rectified by putting energy into Zinc instead. >> > >> >> This series has nothing to do with dynamic dispatch: the call sites >> call crypto functions using ordinary function calls (although my >> example uses CRC-T10DIF), and these calls are redirected via what is >> essentially a PLT entry, so that we can supsersede those routines at >> runtime. > > If you really want to do it PLT-style, then just do: > > extern void whatever_func(args); > > Call it like: > whatever_func(args here); > > And rig up something to emit asm like: > > GLOBAL(whatever_func) > jmpq default_whatever_func > ENDPROC(whatever_func) > > Architectures without support can instead do: > > void whatever_func(args) > { > READ_ONCE(patchable_function_struct_for_whatever_func->ptr)(args); > } > > and patch the asm function for basic support. It will be slower than > necessary, but maybe the relocation trick could be used on top of this > to redirect the call to whatever_func directly to the target for > architectures that want to squeeze out the last bit of performance. > This might actually be the best of all worlds: easy implementation on > all architectures, no inline asm, and the totally non-magical version > works with okay performance. > > (Is this what your code is doing? I admit I didn't follow all the way > through all the macros.) Basically
On Fri, Oct 5, 2018 at 7:29 PM Andy Lutomirski <luto@kernel.org> wrote: > (None of this is to say that I disagree with Jason, though -- I'm not > entirely convinced that this makes sense for Zinc. But maybe it can > be done in a way that makes everyone happy.) Zinc indeed will continue to push in the simpler and more minimal direction. Down the line I'm open to trying and benching a few different ways of going about it with dynamic patching -- something that will be pretty easy to experiment with given the lean structure of Zinc -- but for the initial merge I intend to do it the way it is, which is super fast and pretty straightforward to follow. Jason
Hey Andy, On Fri, Oct 5, 2018 at 7:44 PM Andy Lutomirski <luto@kernel.org> wrote: > I *think* the only change to Zinc per se would be that the calls like > chacha20_simd() would be static calls or patchable functions or > whatever we want to call them. And there could be a debugfs to > override the default selection. Yea, right, exactly. It turns out this is really easy to do with the way it's structured now. I'd actually experimented considerably with using the static keys a while back, but couldn't find any performance difference on any platform at all (four ARM microarchitectures, three MIPS, various random intel, an old powerpc), so went with the simplest solution. But we can certainly play with more elaborate patching mechanisms later on and see how those turn out. Also, even with the simple bools as we have now, it's quite easy to make all the parameters toggle-able. > Ard, I don't think that sticking this in udev rules makes sense. The > kernel has bascially complete information as to what the right choice > is, and that will change over time as the implementation gets tuned, > and the udev rules will never get updated in sync. Yes, I agree with this. Jason
> On 5 Oct 2018, at 20:28, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > Hey Andy, > >> On Fri, Oct 5, 2018 at 7:44 PM Andy Lutomirski <luto@kernel.org> wrote: >> I *think* the only change to Zinc per se would be that the calls like >> chacha20_simd() would be static calls or patchable functions or >> whatever we want to call them. And there could be a debugfs to >> override the default selection. > > Yea, right, exactly. It turns out this is really easy to do with the > way it's structured now. I'd actually experimented considerably with > using the static keys a while back, but couldn't find any performance > difference on any platform at all (four ARM microarchitectures, three > MIPS, various random intel, an old powerpc), so went with the simplest > solution. But we can certainly play with more elaborate patching > mechanisms later on and see how those turn out. Also, even with the > simple bools as we have now, it's quite easy to make all the > parameters toggle-able. > >> Ard, I don't think that sticking this in udev rules makes sense. The >> kernel has bascially complete information as to what the right choice >> is, and that will change over time as the implementation gets tuned, >> and the udev rules will never get updated in sync. > > Yes, I agree with this. > > I am not referring to udev rules. I just mean the current way that udev autoloads modules based on CPU features.
Linux's crypto API is widely regarded as being difficult to use for cases where support for asynchronous accelerators or runtime dispatch of algorithms (i.e., passed in as a string) are not needed. This leads to kludgy code and also to actual security issues [0], although arguably, using AES in the wrong mode can be done using any kind of crypto abstraction. At the moment, the Zinc library [1] is being proposed as a solution for that, and while it does address the usability problems, it does a lot more than that, and what we end up with is a lot less flexible than what we have now. Currently, arch specific implementations (based on SIMD or optimized assembler) live in arch/*/crypto, where each sub-community is in charge of its own specialized versions of the various primitives (although still under the purview of the crypto maintainer). Any proposal to change this model should be judged on its own merit, and not blindly accepted as the fallout of cleaning up some library code. Also, Zinc removes the possibility to plug different versions of a routine, and instead, keeps all versions in the same module. Currently, the kernel's module support permits user land to take charge of the policies that decide which version to use in which context (by loading or blacklisting modules). Instead, Zinc moves to a model where the policy is hardcoded into the module (e.g., ChaCha20 on ARM uses NEON unless on Cortex-A5 or A7). In the ARM community, we have always attempted to avoid hardcoding policy like that: the ARM SoC space is a very heteregeneous one, and putting policy like that into the code will lead to an endless stream of patches from silicon vendors that want tweaks for their cores into various parts of the kernel. Creating monolithic modules for algorithms also makes it very difficult to support driver based implementations. The kernel's driver model is heavily based on modules, using alias strings to match drivers against the hardware. As an example, there ARM ST platforms that support synchronous hardware based CRC32, and plugging that into the monolithic Zinc model is difficult if not impossible. The primary justification for moving away from the module system is that it depends heavily on function pointers, and in the post-Spectre world, those are vulnerable and costly. For this reason, this series proposes a patchable function pointer abstraction that, in its generic incarnation, consists simply of function pointers and some plumbing to set or reset them (patch #1) Patch #2 illustrates how architectures could optimize these abstractions by using code patching techniques to get rid of the indirect calls, and use fixed jumps instead. This has the side effect of making the function pointer const, making it more robust as well. The remainder of the series shows how we can use this abstraction to clean up the CRC-T10DIF handling in the kernel, which is a pretty depressing example of how cumbersome it is to expose crypto API algorithms as library routines. Note that the various arch specific implementations are kept in their original place as modules, which can be automatically dispatched by udev (as before) based on CPU feature bits. Cc: Jason A. Donenfeld <Jason@zx2c4.com> Cc: Eric Biggers <ebiggers@kernel.org> Cc: Samuel Neves <sneves@dei.uc.pt> Cc: Andy Lutomirski <luto@kernel.org> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Herbert Xu <herbert@gondor.apana.org.au> Cc: "David S. Miller" <davem@davemloft.net> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will.deacon@arm.com> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: Paul Mackerras <paulus@samba.org> Cc: Michael Ellerman <mpe@ellerman.id.au> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: Kees Cook <keescook@chromium.org> Cc: "Martin K. Petersen" <martin.petersen@oracle.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Richard Weinberger <richard@nod.at> Cc: Peter Zijlstra <peterz@infradead.org> Cc: linux-kernel@vger.kernel.org Cc: linux-crypto@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linuxppc-dev@lists.ozlabs.org [0] commit 428490e38b2e ("security/keys: rewrite all of big_key crypto") [1] https://lore.kernel.org/lkml/20180925145622.29959-3-Jason@zx2c4.com/ Ard Biesheuvel (9): kernel: add support for patchable function pointers arm64: kernel: add arch support for patchable function pointers crypto: crc-t10dif - make crc_t10dif a static inline crypto: crc-t10dif - use patchable function pointer for core update routine crypto: crc-t10dif/arm64 - move PMULL based code into core library crypto: crc-t10dif/arm - move PMULL based code into core library crypto: crct10dif/generic - switch crypto API driver to core library crypto: crc-t10dif/powerpc - move PMULL based code into core library crypto: crc-t10dif/x86 - move PMULL based code into core library arch/Kconfig | 3 + arch/arm/crypto/crct10dif-ce-glue.c | 78 +++------- arch/arm64/Kconfig | 1 + arch/arm64/crypto/crct10dif-ce-glue.c | 61 ++------ arch/arm64/include/asm/ffp.h | 35 +++++ arch/arm64/kernel/insn.c | 22 +++ arch/powerpc/crypto/crct10dif-vpmsum_glue.c | 55 +------- arch/x86/crypto/crct10dif-pclmul_glue.c | 98 ++----------- crypto/Kconfig | 1 + crypto/Makefile | 2 +- crypto/crct10dif_common.c | 82 ----------- crypto/crct10dif_generic.c | 4 +- include/linux/crc-t10dif.h | 24 +++- include/linux/ffp.h | 43 ++++++ lib/Kconfig | 2 - lib/crc-t10dif.c | 149 +++++++------------- 16 files changed, 235 insertions(+), 425 deletions(-) create mode 100644 arch/arm64/include/asm/ffp.h delete mode 100644 crypto/crct10dif_common.c create mode 100644 include/linux/ffp.h -- 2.11.0