On Wed, May 27, 2020 at 09:07:55AM +0200, Greg KH wrote: > On Tue, May 26, 2020 at 07:24:03PM +0200, Wojtek Porczyk wrote: > > On Tue, May 26, 2020 at 06:32:35PM +0200, Greg KH wrote: > > > On Tue, May 26, 2020 at 08:48:35AM -0700, Andi Kleen wrote: > > > > On Tue, May 26, 2020 at 08:56:18AM +0200, Greg KH wrote: > > > > > On Mon, May 25, 2020 at 10:28:48PM -0700, Andi Kleen wrote: > > > > > > From: Andi Kleen <ak@xxxxxxxxxxxxxxx> > > > > > > > > > > > > Since there seem to be kernel modules floating around that set > > > > > > FSGSBASE incorrectly, prevent this in the CR4 pinning. Currently > > > > > > CR4 pinning just checks that bits are set, this also checks > > > > > > that the FSGSBASE bit is not set, and if it is clears it again. > > > > > > > > > > So we are trying to "protect" ourselves from broken out-of-tree kernel > > > > > modules now? > > > > > > > > Well it's a specific case where we know they're opening a root hole > > > > unintentionally. This is just an pragmatic attempt to protect the users in the > > > > short term. > > > > > > Can't you just go and fix those out-of-tree kernel modules instead? > > > What's keeping you all from just doing that instead of trying to force > > > the kernel to play traffic cop? > > > > We'd very much welcome any help really, but we're under impression that this > > couldn't be done correctly in a module, so this hack occured. > > Really? How is this hack anything other than a "prevent a kernel module > from doing something foolish" change? By "this hack" I meant our module [1], which just enables FSGSBASE bit without doing everything else that Sasha's patchset does, and that "everything else" is there to prevent a gaping root hoole. Original author wanted module for the reason snipped below, but Sasha's patchset can't be packaged into module. I'll be happy to be corrected on this point, I personally have next to no kernel programming experience. This kernel change I think is correct, because if kernel assumes some invariants, it's a good idea to actually enforce them, isn't it? So we don't have anything against this kernel change. We'll have to live with it, with our hand forced. > Why can't you just change the kernel module's code to not do this? What > prevents that from happening right now which would prevent the need to > change a core api from being abused in such a way? [snip] > I'm sorry, but I still do not understand. Your kernel module calls the > core with this bit being set, and this new kernel patch is there to > prevent the bit from being set and will WARN_ON() if it happens. Why > can't you just change your module code to not set the bit? Because we need userspace access to wrfsbase, which this bit enables: https://github.com/oscarlab/graphene/blob/58c53ad747579225bf29e3506d883586ff4b8eee/Pal/src/host/Linux-SGX/sgx_api.h#L94-L98 https://github.com/oscarlab/graphene/blob/0dd84ddf943d256e5494f07cb41b1d0ece847c1a/Pal/src/host/Linux-SGX/enclave_entry.S#L675 https://github.com/oscarlab/graphene/blob/e4e16aa10e3c2221170aee7da66370507cc52428/Pal/src/host/Linux-SGX/db_misc.c#L69 > Do you have a pointer to the kernel module code that does this operation > which this core kernel change will try to prevent from happening? Sure: https://github.com/oscarlab/graphene-sgx-driver/blob/a73de5fed96fc330fc0417d262ba5e87fea128c2/gsgx.c#L32-L39 The whole module is 86 sloc, and the sole purpose is to set this bit on load and clear on unload. [1] ^ -- pozdrawiam / best regards Wojtek Porczyk Graphene / Invisible Things Lab I do not fear computers, I fear lack of them. -- Isaac Asimov
Attachment:
signature.asc
Description: PGP signature
