Changeset View
Standalone View
sys/sys/pax.h
- This file was added.
/*- | |||||
* Copyright (c) 2006 Elad Efrat <elad@NetBSD.org> | |||||
* Copyright (c) 2013-2014, by Oliver Pinter <oliver.pntr at gmail.com> | |||||
* Copyright (c) 2014, by Shawn Webb <lattera at gmail.com> | |||||
* All rights reserved. | |||||
* | |||||
* Redistribution and use in source and binary forms, with or without | |||||
* modification, are permitted provided that the following conditions | |||||
* are met: | |||||
* 1. Redistributions of source code must retain the above copyright | |||||
* notice, this list of conditions and the following disclaimer. | |||||
* 2. Redistributions in binary form must reproduce the above copyright | |||||
* notice, this list of conditions and the following disclaimer in the | |||||
* documentation and/or other materials provided with the distribution. | |||||
* 3. The name of the author may not be used to endorse or promote products | |||||
* derived from this software without specific prior written permission. | |||||
* | |||||
* THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR | |||||
* IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES | |||||
* OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. | |||||
* IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT, | |||||
* INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT | |||||
* NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, | |||||
* DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY | |||||
* THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT | |||||
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF | |||||
* THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. | |||||
* | |||||
* $FreeBSD$ | |||||
*/ | |||||
#ifndef __SYS_PAX_H | |||||
#define __SYS_PAX_H | |||||
#ifdef _KERNEL | |||||
imp: This whole file should be #ifndef __KERNEL since there's no user-space serviceable parts inside. | |||||
Not Done Inline ActionsWill do. lattera-gmail.com: Will do. | |||||
struct image_params; | |||||
Not Done Inline Actionsstyle(9) defines different syntax here -- in particular, no tab after #ifndef, only after #define. The include guard names here are not consistent with other headers, and should be prefixed with a single underscore, not a double underscore. rwatson: style(9) defines different syntax here -- in particular, no tab after #ifndef, only after… | |||||
struct prison; | |||||
struct thread; | |||||
struct vnode; | |||||
struct vmspace; | |||||
struct vm_offset_t; | |||||
Not Done Inline ActionsThe prefixes here should be hf_ not hr_, reflecting the data-structure name. rwatson: The prefixes here should be hf_ not hr_, reflecting the data-structure name. | |||||
/* | |||||
* used in sysctl handler | |||||
*/ | |||||
#define PAX_ASLR_DISABLED 0 | |||||
#define PAX_ASLR_OPTIN 1 | |||||
#define PAX_ASLR_OPTOUT 2 | |||||
#define PAX_ASLR_FORCE_ENABLED 3 | |||||
#ifndef PAX_ASLR_DELTA | |||||
#define PAX_ASLR_DELTA(delta, lsb, len) \ | |||||
(((delta) & ((1UL << (len)) - 1)) << (lsb)) | |||||
#endif /* PAX_ASLR_DELTA */ | |||||
Not Done Inline ActionsThis file seems to only be included when this is defined. Why test again here? But honestly, I'd rather you only test here and lose the other tests. imp: This file seems to only be included when this is defined. Why test again here? But honestly… | |||||
Not Done Inline ActionsWill do. lattera-gmail.com: Will do. | |||||
/* | |||||
* generic ASLR values | |||||
* | |||||
* MMAP | 32 bit | 64 bit | | |||||
* +-------+--------+--------+ | |||||
* | MIN | 8 bit | 16 bit | | |||||
* +-------+--------+--------+ | |||||
Not Done Inline ActionsUse Title Case. rwatson: Use Title Case. | |||||
* | DEF | 8 bit | 21 bit | | |||||
* +-------+--------+--------+ | |||||
* | MAX | 16 bit | 32 bit | | |||||
* +-------+--------+--------+ | |||||
* | |||||
* STACK | 32 bit | 64 bit | | |||||
Not Done Inline ActionsA little hard to say in phabricator, but it looks like there might be spaces rather than tabs between the constant names and their values. rwatson: A little hard to say in phabricator, but it looks like there might be spaces rather than tabs… | |||||
* +-------+--------+--------+ | |||||
* | MIN | 6 bit | 12 bit | | |||||
* +-------+--------+--------+ | |||||
* | DEF | 6 bit | 16 bit | | |||||
* +-------+--------+--------+ | |||||
Not Done Inline ActionsThere should be tabs after the #defines here. rwatson: There should be tabs after the #defines here.
There may be spaces instead of tabs between… | |||||
* | MAX | 10 bit | 21 bit | | |||||
* +-------+--------+--------+ | |||||
* | |||||
* EXEC | 32 bit | 64 bit | | |||||
* +-------+--------+--------+ | |||||
Not Done Inline ActionsUse Title Case. rwatson: Use Title Case.
PaX or PAX, depending on what you pick. | |||||
* | MIN | 6 bit | 12 bit | | |||||
* +-------+--------+--------+ | |||||
* | DEF | 6 bit | 21 bit | | |||||
* +-------+--------+--------+ | |||||
* | MAX | 10 bit | 21 bit | | |||||
* +-------+--------+--------+ | |||||
Not Done Inline ActionsThese functions are only available when PAX_ASLR is defined. Likely they should be under an ifdef. In fact, most of this file looks like it should be under that ifdef to prevent stray references. imp: These functions are only available when PAX_ASLR is defined. Likely they should be under an… | |||||
* | |||||
Not Done Inline ActionsThese prototypes inconsistently include argument variable names vs. plain types. In general, in system headers, it is a good idea to just use the types to avoid namespace collisions. rwatson: These prototypes inconsistently include argument variable names vs. plain types. In general, in… | |||||
*/ | |||||
Not Done Inline ActionsIn general I like variable names in headers, because they helps the development when you use an IDE or vim with clang_complete. op: In general I like variable names in headers, because they helps the development when you use an… | |||||
Not Done Inline ActionsI like them too -- but in large, complex programs made up of many parts maintained by different people and organisations -- and worse, as the OS vendor where your headers are included by many third parties -- the risk of a namespace collision is simply too great. For example, if future kernel or user code or another header #defines a colliding name. rwatson: I like them too -- but in large, complex programs made up of many parts maintained by different… | |||||
#ifndef PAX_ASLR_DELTA_MMAP_LSB | |||||
#define PAX_ASLR_DELTA_MMAP_LSB PAGE_SHIFT | |||||
#endif /* PAX_ASLR_DELTA_MMAP_LSB */ | |||||
#ifndef PAX_ASLR_DELTA_MMAP_MIN_LEN | |||||
Not Done Inline ActionsRemove this blank line. rwatson: Remove this blank line. | |||||
#define PAX_ASLR_DELTA_MMAP_MIN_LEN ((sizeof(void *) * NBBY) / 4) | |||||
#endif /* PAX_ASLR_DELTA_MMAP_MAX_LEN */ | |||||
#ifndef PAX_ASLR_DELTA_MMAP_MAX_LEN | |||||
#define PAX_ASLR_DELTA_MMAP_MAX_LEN ((sizeof(void *) * NBBY) / 2) | |||||
#endif /* PAX_ASLR_DELTA_MMAP_MAX_LEN */ | |||||
Not Done Inline ActionsI think this always makes these functions NULL. imp: I think this always makes these functions NULL.
The fix is to move the #endif down three lines. | |||||
Not Done Inline ActionsNice catch! op: Nice catch! | |||||
#ifndef PAX_ASLR_DELTA_STACK_LSB | |||||
#define PAX_ASLR_DELTA_STACK_LSB 3 | |||||
#endif /* PAX_ASLR_DELTA_STACK_LSB */ | |||||
Not Done Inline ActionsTo avoid namespace collisions, these variable names should likely be removed. rwatson: To avoid namespace collisions, these variable names should likely be removed.
Alphabetise the… | |||||
#ifndef PAX_ASLR_DELTA_STACK_MIN_LEN | |||||
#define PAX_ASLR_DELTA_STACK_MIN_LEN ((sizeof(void *) * NBBY) / 5) | |||||
Not Done Inline ActionsI don't think you need to do this. pax_aslr_init_prison is only called from kern_pax.c, which is only included with option PAX_ASLR. imp: I don't think you need to do this. pax_aslr_init_prison is only called from kern_pax.c, which… | |||||
Not Done Inline ActionsFor the current state, this is true, but if someone decide to upstream SegvGuard (danilo@freebsd.org working on this feature ...) which makes the ASLR much more efficient, then it's not true. op: For the current state, this is true, but if someone decide to upstream SegvGuard… | |||||
#endif /* PAX_ASLR_DELTA_STACK_MAX_LEN */ | |||||
#ifndef PAX_ASLR_DELTA_STACK_MAX_LEN | |||||
Not Done Inline ActionsThese spaces should be tabs both before and after the constant name. rwatson: These spaces should be tabs both before and after the constant name. | |||||
#define PAX_ASLR_DELTA_STACK_MAX_LEN ((sizeof(void *) * NBBY) / 3) | |||||
#endif /* PAX_ASLR_DELTA_STACK_MAX_LEN */ | |||||
#ifndef PAX_ASLR_DELTA_EXEC_LSB | |||||
#define PAX_ASLR_DELTA_EXEC_LSB PAGE_SHIFT | |||||
#endif /* PAX_ASLR_DELTA_EXEC_LSB */ | |||||
#ifndef PAX_ASLR_DELTA_EXEC_MIN_LEN | |||||
#define PAX_ASLR_DELTA_EXEC_MIN_LEN ((sizeof(void *) * NBBY) / 5) | |||||
#endif /* PAX_ASLR_DELTA_EXEC_MAX_LEN */ | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
#ifndef PAX_ASLR_DELTA_EXEC_MAX_LEN | |||||
#define PAX_ASLR_DELTA_EXEC_MAX_LEN ((sizeof(void *) * NBBY) / 3) | |||||
#endif /* PAX_ASLR_DELTA_EXEC_MAX_LEN */ | |||||
/* | |||||
* ASLR default values for native host | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
*/ | |||||
#ifdef __amd64__ | |||||
#ifndef PAX_ASLR_DELTA_MMAP_DEF_LEN | |||||
#define PAX_ASLR_DELTA_MMAP_DEF_LEN 21 | |||||
#endif /* PAX_ASLR_DELTA_MMAP_DEF_LEN */ | |||||
#ifndef PAX_ASLR_DELTA_STACK_DEF_LEN | |||||
#define PAX_ASLR_DELTA_STACK_DEF_LEN 16 | |||||
#endif /* PAX_ASLR_DELTA_STACK_DEF_LEN */ | |||||
#ifndef PAX_ASLR_DELTA_EXEC_DEF_LEN | |||||
#define PAX_ASLR_DELTA_EXEC_DEF_LEN 21 | |||||
#endif /* PAX_ASLR_DELTA_EXEC_DEF_LEN */ | |||||
#else | |||||
Not Done Inline ActionsWhy is 64-bit x86 special-cased here? If there is machine-dependent content here, shouldn't it be in machine/pax.h? rwatson: Why is 64-bit x86 special-cased here? If there is machine-dependent content here, shouldn't it… | |||||
Not Done Inline ActionsWe can abstract that out. That'll also help keep this header smaller. Will address in the next patch. lattera-gmail.com: We can abstract that out. That'll also help keep this header smaller. Will address in the next… | |||||
Not Done Inline ActionsThe question is not just one of abstraction, but also whether there needs to be machine-dependent behaviour. E.g., is this really a property of 64-bit pointers that applies also to other 64-bit architectures, not just amd64? rwatson: The question is not just one of abstraction, but also whether there needs to be machine… | |||||
Not Done Inline ActionsAt the beginning the patch was developed on amd64 system, this ifdef is a leftover. The proper statement is for checking against 64 bit (which is not specific to amd64) or separate this out to MD layer. In our development repo we already changed them to LP64: https://github.com/HardenedBSD/hardenedBSD/blame/hardened/current/aslr/sys/sys/pax.h op: At the beginning the patch was developed on amd64 system, this ifdef is a leftover. The proper… | |||||
Not Done Inline ActionsSounds good. This is especially important as we believe arm64 will become a widely used 64-bit architecture. rwatson: Sounds good. This is especially important as we believe arm64 will become a widely used 64-bit… | |||||
#ifndef PAX_ASLR_DELTA_MMAP_DEF_LEN | |||||
#define PAX_ASLR_DELTA_MMAP_DEF_LEN PAX_ASLR_DELTA_MMAP_MIN_LEN | |||||
#endif /* PAX_ASLR_DELTA_MMAP_DEF_LEN */ | |||||
#ifndef PAX_ASLR_DELTA_STACK_DEF_LEN | |||||
#define PAX_ASLR_DELTA_STACK_DEF_LEN PAX_ASLR_DELTA_STACK_MIN_LEN | |||||
#endif /* PAX_ASLR_DELTA_STACK_DEF_LEN */ | |||||
#ifndef PAX_ASLR_DELTA_EXEC_DEF_LEN | |||||
#define PAX_ASLR_DELTA_EXEC_DEF_LEN PAX_ASLR_DELTA_EXEC_MIN_LEN | |||||
#endif /* PAX_ASLR_DELTA_EXEC_DEF_LEN */ | |||||
#endif /* __amd64__ */ | |||||
/* | |||||
* ASLR values for COMPAT_FREEBSD32 and COMPAT_LINUX | |||||
*/ | |||||
#ifndef PAX_ASLR_COMPAT_DELTA_MMAP_LSB | |||||
#define PAX_ASLR_COMPAT_DELTA_MMAP_LSB PAGE_SHIFT | |||||
#endif /* PAX_ASLR_COMPAT_DELTA_MMAP_LSB */ | |||||
#ifndef PAX_ASLR_COMPAT_DELTA_MMAP_MIN_LEN | |||||
#define PAX_ASLR_COMPAT_DELTA_MMAP_MIN_LEN ((sizeof(int) * NBBY) / 4) | |||||
#endif /* PAX_ASLR_COMPAT_DELTA_MMAP_MAX_LEN */ | |||||
#ifndef PAX_ASLR_COMPAT_DELTA_MMAP_MAX_LEN | |||||
#define PAX_ASLR_COMPAT_DELTA_MMAP_MAX_LEN ((sizeof(int) * NBBY) / 2) | |||||
Not Done Inline ActionsShould these be ifdef COMPAT_FREEBSD32 / COMPAT_LINUX, then, to detect programmer errors? rwatson: Should these be ifdef COMPAT_FREEBSD32 / COMPAT_LINUX, then, to detect programmer errors? | |||||
Not Done Inline ActionsSure, I will add these checks. op: Sure, I will add these checks. | |||||
#endif /* PAX_ASLR_COMPAT_DELTA_MMAP_MAX_LEN */ | |||||
#ifndef PAX_ASLR_COMPAT_DELTA_STACK_LSB | |||||
#define PAX_ASLR_COMPAT_DELTA_STACK_LSB 3 | |||||
#endif /* PAX_ASLR_COMPAT_DELTA_STACK_LSB */ | |||||
Not Done Inline ActionsIs there a better type to use here than 'int'? E.g., a type that represents guest-ABI pointers? (Surely one should exist if it doesn't?) rwatson: Is there a better type to use here than 'int'? E.g., a type that represents guest-ABI pointers? | |||||
Not Done Inline ActionsIt's a good question. I choose this because int is 32 bit on all platform where FreeBSD runs. op: It's a good question. I choose this because int is 32 bit on all platform where FreeBSD runs. | |||||
Not Done Inline ActionsI'm sure it would be an 'int' underneath, but using another type name on top makes the code easier to read. I'm not sure if the compat32 or linux-compat layers already have a type for this .. if not, they probably should but fixing it here isn't necessary. On the other hand, if they do have a suitable type, I think you do want to use it here. (It could be that they use int32t or uint32t or such which might also be better.) rwatson: I'm sure it would be an 'int' underneath, but using another type name on top makes the code… | |||||
#ifndef PAX_ASLR_COMPAT_DELTA_STACK_MIN_LEN | |||||
#define PAX_ASLR_COMPAT_DELTA_STACK_MIN_LEN ((sizeof(int) * NBBY) / 5) | |||||
#endif /* PAX_ASLR_COMPAT_DELTA_STACK_MAX_LEN */ | |||||
#ifndef PAX_ASLR_COMPAT_DELTA_STACK_MAX_LEN | |||||
#define PAX_ASLR_COMPAT_DELTA_STACK_MAX_LEN ((sizeof(int) * NBBY) / 3) | |||||
#endif /* PAX_ASLR_COMPAT_DELTA_STACK_MAX_LEN */ | |||||
#ifndef PAX_ASLR_COMPAT_DELTA_EXEC_LSB | |||||
#define PAX_ASLR_COMPAT_DELTA_EXEC_LSB PAGE_SHIFT | |||||
#endif /* PAX_ASLR_COMPAT_DELTA_EXEC_LSB */ | |||||
#ifndef PAX_ASLR_COMPAT_DELTA_EXEC_MIN_LEN | |||||
#define PAX_ASLR_COMPAT_DELTA_EXEC_MIN_LEN ((sizeof(int) * NBBY) / 5) | |||||
#endif /* PAX_ASLR_COMPAT_DELTA_EXEC_MAX_LEN */ | |||||
#ifndef PAX_ASLR_COMPAT_DELTA_EXEC_MAX_LEN | |||||
#define PAX_ASLR_COMPAT_DELTA_EXEC_MAX_LEN ((sizeof(int) * NBBY) / 3) | |||||
#endif /* PAX_ASLR_COMPAT_DELTA_EXEC_MAX_LEN */ | |||||
extern int pax_aslr_status; | |||||
extern int pax_aslr_debug; | |||||
extern int pax_aslr_mmap_len; | |||||
extern int pax_aslr_stack_len; | |||||
extern int pax_aslr_exec_len; | |||||
#ifdef COMPAT_FREEBSD32 | |||||
extern int pax_aslr_compat_status; | |||||
extern int pax_aslr_compat_mmap_len; | |||||
extern int pax_aslr_compat_stack_len; | |||||
extern int pax_aslr_compat_exec_len; | |||||
#endif /* COMPAT_FREEBSD32 */ | |||||
#define ELF_NOTE_TYPE_PAX_TAG 3 | |||||
#define PAX_NOTE_MPROTECT 0x01 | |||||
#define PAX_NOTE_NOMPROTECT 0x02 | |||||
#define PAX_NOTE_GUARD 0x04 | |||||
#define PAX_NOTE_NOGUARD 0x08 | |||||
#define PAX_NOTE_ASLR 0x10 | |||||
#define PAX_NOTE_NOASLR 0x20 | |||||
void pax_init(void); | |||||
void pax_init_prison(struct prison *pr); | |||||
bool pax_aslr_active(struct proc *proc); | |||||
void _pax_aslr_init(struct vmspace *vm, struct proc *p); | |||||
void _pax_aslr_init32(struct vmspace *vm, struct proc *p); | |||||
void pax_aslr_init(struct image_params *imgp); | |||||
Not Done Inline ActionsThis ifdef seems not to be used anywhere else in the patch. Is this a mismerge? rwatson: This ifdef seems not to be used anywhere else in the patch. Is this a mismerge? | |||||
Not Done Inline ActionsYou are correct, it is a mismerge. I will remove this in the next patch. lattera-gmail.com: You are correct, it is a mismerge. I will remove this in the next patch. | |||||
void pax_aslr_mmap(struct proc *p, vm_offset_t *addr, | |||||
vm_offset_t orig_addr, int flags); | |||||
void pax_aslr_stack(struct thread *td, uintptr_t *addr); | |||||
struct prison *pax_get_prison(struct proc *proc); | |||||
void pax_elf(struct image_params *, uint32_t); | |||||
#endif /* _KERNEL */ | |||||
#endif /* __SYS_PAX_H */ | |||||
Not Done Inline ActionsThese function names should not begin with '_'. rwatson: These function names should not begin with '_'. | |||||
Not Done Inline ActionsThere seem to be a number of different names floating around here and elsewhere -- 'flags', 'mode', etc, and types -- u_int, uint32_t, and so on. Do check that they are all consistent with one another. rwatson: There seem to be a number of different names floating around here and elsewhere -- 'flags'… | |||||
Not Done Inline ActionsThe types in overall adapted it's context typing convention. These function applied in different area of the kernel and to different subsystems. op: The types in overall adapted it's context typing convention. These function applied in… |
This whole file should be #ifndef __KERNEL since there's no user-space serviceable parts inside.