Re: [PATCH] seccomp: Add pkru into seccomp_data
On Thu, Oct 25, 2018 at 5:49 PM, Andy Lutomirski wrote: >> On Oct 25, 2018, at 5:35 PM, Kees Cook wrote: >> >>> On Fri, Oct 26, 2018 at 12:00 AM, Andy Lutomirski >>> wrote: >>> You could bite the bullet and add seccomp eBPF support :) >> >> I'm not convinced this is a good enough reason for gaining the eBPF >> attack surface yet. > > Is it an interesting attack surface? It’s certainly scarier if you’re > worried about attacks from the sandbox creator, but the security inside the > sandbox should be more or less equivalent, no? Yeah, I'm more worried about the creator: I don't want kernel exploits via seccomp APIs. :P There have been kind of a long list of eBPF-related flaws, so I'd really rather not tie seccomp to it. As for sandbox escapes, I don't think there's too much concern, but I do worry about "unexpected" situations exposed by eBPF (i.e. maps or functions not doing what was expected, or allowing map manipulation from outside). seccomp cBPF is pretty self-contained right now, so I'd like a strong reason to justify the increase in code complexity and related attack surface. -- Kees Cook
Re: [PATCH] seccomp: Add pkru into seccomp_data
On Thu, Oct 25, 2018 at 11:12:25AM +0200, Florian Weimer wrote: > * Michael Sammler: > > > Thank you for the pointer about the POWER implementation. I am not > > familiar with POWER in general and its protection key feature at > > all. Would the AMR register be the correct register to expose here? > > Yes, according to my notes, the register is called AMR (special purpose > register 13). Yes. it is AMR register. RP
Re: [PATCH] seccomp: Add pkru into seccomp_data
> On Oct 25, 2018, at 5:35 PM, Kees Cook wrote: > >> On Fri, Oct 26, 2018 at 12:00 AM, Andy Lutomirski >> wrote: >> You could bite the bullet and add seccomp eBPF support :) > > I'm not convinced this is a good enough reason for gaining the eBPF > attack surface yet. > > Is it an interesting attack surface? It’s certainly scarier if you’re worried about attacks from the sandbox creator, but the security inside the sandbox should be more or less equivalent, no?
Re: [PATCH] seccomp: Add pkru into seccomp_data
On Fri, Oct 26, 2018 at 12:00 AM, Andy Lutomirski wrote: > You could bite the bullet and add seccomp eBPF support :) I'm not convinced this is a good enough reason for gaining the eBPF attack surface yet. -Kees -- Kees Cook
Re: [PATCH] seccomp: Add pkru into seccomp_data
On Thu, Oct 25, 2018 at 9:42 AM Michael Sammler wrote: > > On 10/25/2018 11:12 AM, Florian Weimer wrote: > >> I understand your concern about exposing the number of protection keys > >> in the ABI. One idea would be to state, that the pkru field (which > >> should probably be renamed) contains an architecture specific value, > >> which could then be the PKRU on x86 and AMR (or another register) on > >> POWER. This new field should probably be extended to __u64 and the > >> reserved field removed. > > POWER also has proper read/write bit separation, not PKEY_DISABLE_ACCESS > > (disable read and write) and PKEY_DISABLE_WRITE like Intel. It's > > currently translated by the kernel, but I really need a > > PKEY_DISABLE_READ bit in glibc to implement pkey_get in case the memory > > is write-only. > The idea here would be to simply provide the raw value of the register > (PKRU on x86, AMR on POWER) to the BPF program and let the BPF program > (or maybe a higher level library like libseccomp) deal with the > complications of interpreting this architecture specific value (similar > how the BPF program currently already has to deal with architecture > specific system call numbers). If an architecture were to support more > protection keys than fit into the field, the architecture specific value > stored in the field might simply be the first protection keys. If there > was interest, it would be possible to add more architecture specific > fields to seccomp_data. > >> Another idea would be to not add a field in the seccomp_data > >> structure, but instead provide a new BPF instruction, which reads the > >> value of a specified protection key. > > I would prefer that if it's possible. We should make sure that the bits > > are the same as those returned from pkey_get. I have an implementation > > on POWER, but have yet to figure out the implications for 32-bit because > > I do not know the AMR register size there. > > > > Thanks, > > Florian > I have had a look at how BPF is implemented and it does not seem to be > easy to just add an BPF instruction for seccomp since (as far as I > understand) the code of the classical BPF (as used by seccomp) is shared > with the code of eBPF, which is used in many parts of the kernel and > there is at least one interpreter and one JIT compiler for BPF. But > maybe someone with more experience than me can comment on how hard it > would be to add an instruction to BPF. > You could bite the bullet and add seccomp eBPF support :)
Re: [PATCH] seccomp: Add pkru into seccomp_data
On 10/25/2018 11:12 AM, Florian Weimer wrote: I understand your concern about exposing the number of protection keys in the ABI. One idea would be to state, that the pkru field (which should probably be renamed) contains an architecture specific value, which could then be the PKRU on x86 and AMR (or another register) on POWER. This new field should probably be extended to __u64 and the reserved field removed. POWER also has proper read/write bit separation, not PKEY_DISABLE_ACCESS (disable read and write) and PKEY_DISABLE_WRITE like Intel. It's currently translated by the kernel, but I really need a PKEY_DISABLE_READ bit in glibc to implement pkey_get in case the memory is write-only. The idea here would be to simply provide the raw value of the register (PKRU on x86, AMR on POWER) to the BPF program and let the BPF program (or maybe a higher level library like libseccomp) deal with the complications of interpreting this architecture specific value (similar how the BPF program currently already has to deal with architecture specific system call numbers). If an architecture were to support more protection keys than fit into the field, the architecture specific value stored in the field might simply be the first protection keys. If there was interest, it would be possible to add more architecture specific fields to seccomp_data. Another idea would be to not add a field in the seccomp_data structure, but instead provide a new BPF instruction, which reads the value of a specified protection key. I would prefer that if it's possible. We should make sure that the bits are the same as those returned from pkey_get. I have an implementation on POWER, but have yet to figure out the implications for 32-bit because I do not know the AMR register size there. Thanks, Florian I have had a look at how BPF is implemented and it does not seem to be easy to just add an BPF instruction for seccomp since (as far as I understand) the code of the classical BPF (as used by seccomp) is shared with the code of eBPF, which is used in many parts of the kernel and there is at least one interpreter and one JIT compiler for BPF. But maybe someone with more experience than me can comment on how hard it would be to add an instruction to BPF. - Michael
Re: [PATCH] seccomp: Add pkru into seccomp_data
On 10/24/2018 08:06 PM, Florian Weimer wrote:
* Michael Sammler:
Add the current value of the PKRU register to data available for
seccomp-bpf programs to work on. This allows filters based on the
currently enabled protection keys.
diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
index 9efc0e73..e8b9ecfc 100644
--- a/include/uapi/linux/seccomp.h
+++ b/include/uapi/linux/seccomp.h
@@ -52,12 +52,16 @@
* @instruction_pointer: at the time of the system call.
* @args: up to 6 system call arguments always stored as 64-bit values
*regardless of the architecture.
+ * @pkru: value of the pkru register
+ * @reserved: pad the structure to a multiple of eight bytes
*/
struct seccomp_data {
int nr;
__u32 arch;
__u64 instruction_pointer;
__u64 args[6];
+ __u32 pkru;
+ __u32 reserved;
};
This doesn't cover the POWER implementation. Adding Cc:s.
And I think the kernel shouldn't expose the number of protection keys in
the ABI.
Thanks,
Florian
Thank you for the pointer about the POWER implementation. I am not
familiar with POWER in general and its protection key feature at all.
Would the AMR register be the correct register to expose here?
I understand your concern about exposing the number of protection keys
in the ABI. One idea would be to state, that the pkru field (which
should probably be renamed) contains an architecture specific value,
which could then be the PKRU on x86 and AMR (or another register) on
POWER. This new field should probably be extended to __u64 and the
reserved field removed.
Another idea would be to not add a field in the seccomp_data structure,
but instead provide a new BPF instruction, which reads the value of a
specified protection key.
- Michael
Re: [PATCH] seccomp: Add pkru into seccomp_data
* Michael Sammler: > Thank you for the pointer about the POWER implementation. I am not > familiar with POWER in general and its protection key feature at > all. Would the AMR register be the correct register to expose here? Yes, according to my notes, the register is called AMR (special purpose register 13). > I understand your concern about exposing the number of protection keys > in the ABI. One idea would be to state, that the pkru field (which > should probably be renamed) contains an architecture specific value, > which could then be the PKRU on x86 and AMR (or another register) on > POWER. This new field should probably be extended to __u64 and the > reserved field removed. POWER also has proper read/write bit separation, not PKEY_DISABLE_ACCESS (disable read and write) and PKEY_DISABLE_WRITE like Intel. It's currently translated by the kernel, but I really need a PKEY_DISABLE_READ bit in glibc to implement pkey_get in case the memory is write-only. > Another idea would be to not add a field in the seccomp_data > structure, but instead provide a new BPF instruction, which reads the > value of a specified protection key. I would prefer that if it's possible. We should make sure that the bits are the same as those returned from pkey_get. I have an implementation on POWER, but have yet to figure out the implications for 32-bit because I do not know the AMR register size there. Thanks, Florian
Re: [PATCH] seccomp: Add pkru into seccomp_data
* Michael Sammler:
> Add the current value of the PKRU register to data available for
> seccomp-bpf programs to work on. This allows filters based on the
> currently enabled protection keys.
> diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
> index 9efc0e73..e8b9ecfc 100644
> --- a/include/uapi/linux/seccomp.h
> +++ b/include/uapi/linux/seccomp.h
> @@ -52,12 +52,16 @@
> * @instruction_pointer: at the time of the system call.
> * @args: up to 6 system call arguments always stored as 64-bit values
> *regardless of the architecture.
> + * @pkru: value of the pkru register
> + * @reserved: pad the structure to a multiple of eight bytes
> */
> struct seccomp_data {
> int nr;
> __u32 arch;
> __u64 instruction_pointer;
> __u64 args[6];
> + __u32 pkru;
> + __u32 reserved;
> };
This doesn't cover the POWER implementation. Adding Cc:s.
And I think the kernel shouldn't expose the number of protection keys in
the ABI.
Thanks,
Florian
