Changeset View
Standalone View
sys/sys/pax.h
- This file was added.
/*- | |||||
* Copyright (c) 2006 Elad Efrat <elad@NetBSD.org> | |||||
* Copyright (c) 2013-2015, by Oliver Pinter <oliver.pinter@hardenedbsd.org> | |||||
* Copyright (c) 2014-2015, by Shawn Webb <shawn.webb@hardenedbsd.org> | |||||
* 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$ | |||||
* | |||||
* HardenedBSD-version: 2fba75c32739bd7f7c80163ec88e3655c3130753 | |||||
* HardenedBSD-version: v16 | |||||
* | |||||
*/ | |||||
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. | |||||
#ifndef __SYS_PAX_H | |||||
#define __SYS_PAX_H | |||||
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… | |||||
#if defined(_KERNEL) || defined(_WANT_PRISON) | |||||
struct hardening_features { | |||||
int hr_pax_aslr_status; /* (p) PaX ASLR enabled */ | |||||
int hr_pax_aslr_mmap_len; /* (p) Number of bits randomized with mmap */ | |||||
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. | |||||
int hr_pax_aslr_stack_len; /* (p) Number of bits randomized with stack */ | |||||
int hr_pax_aslr_exec_len; /* (p) Number of bits randomized with the execbase */ | |||||
int hr_pax_aslr_compat_status; /* (p) PaX ASLR enabled (compat32) */ | |||||
int hr_pax_aslr_compat_mmap_len; /* (p) Number of bits randomized with mmap (compat32) */ | |||||
int hr_pax_aslr_compat_stack_len; /* (p) Number of bits randomized with stack (compat32) */ | |||||
int hr_pax_aslr_compat_exec_len; /* (p) Number of bits randomized with the execbase (compat32) */ | |||||
}; | |||||
#endif | |||||
#ifdef _KERNEL | |||||
struct image_params; | |||||
struct prison; | |||||
struct thread; | |||||
struct proc; | |||||
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. | |||||
struct vnode; | |||||
struct vm_offset_t; | |||||
/* | |||||
* used in sysctl handler | |||||
*/ | |||||
#define PAX_FEATURE_DISABLED 0 | |||||
Not Done Inline ActionsUse Title Case. rwatson: Use Title Case. | |||||
#define PAX_FEATURE_OPTIN 1 | |||||
#define PAX_FEATURE_OPTOUT 2 | |||||
#define PAX_FEATURE_FORCE_ENABLED 3 | |||||
#define PAX_FEATURE_UNKNOWN_STATUS 4 | |||||
extern const char *pax_status_str[]; | |||||
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… | |||||
#define PAX_FEATURE_SIMPLE_DISABLED 0 | |||||
#define PAX_FEATURE_SIMPLE_ENABLED 1 | |||||
extern const char *pax_status_simple_str[]; | |||||
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… | |||||
/* | |||||
* generic pax functions | |||||
*/ | |||||
int pax_elf(struct image_params *, uint32_t); | |||||
Not Done Inline ActionsUse Title Case. rwatson: Use Title Case.
PaX or PAX, depending on what you pick. | |||||
void pax_get_flags(struct proc *p, uint32_t *flags); | |||||
void pax_get_flags_td(struct thread *td, uint32_t *flags); | |||||
struct prison *pax_get_prison(struct proc *p); | |||||
struct prison *pax_get_prison_td(struct thread *td); | |||||
void pax_init_prison(struct prison *pr); | |||||
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… | |||||
* ASLR related functions | |||||
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… | |||||
*/ | |||||
int pax_aslr_active(struct proc *p); | |||||
void pax_aslr_init_vmspace(struct proc *p); | |||||
void pax_aslr_init_vmspace32(struct proc *p); | |||||
#ifdef PAX_ASLR | |||||
Not Done Inline ActionsRemove this blank line. rwatson: Remove this blank line. | |||||
void pax_aslr_init_prison(struct prison *pr); | |||||
void pax_aslr_init_prison32(struct prison *pr); | |||||
#else | |||||
#define pax_aslr_init_prison(pr) do {} while (0) | |||||
#define pax_aslr_init_prison32(pr) do {} while (0) | |||||
#endif | |||||
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! | |||||
void pax_aslr_init(struct image_params *imgp); | |||||
void pax_aslr_execbase(struct proc *p, u_long *et_dyn_addr); | |||||
void pax_aslr_mmap(struct proc *p, vm_offset_t *addr, | |||||
vm_offset_t orig_addr, int flags); | |||||
uint32_t pax_aslr_setup_flags(struct image_params *imgp, uint32_t mode); | |||||
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… | |||||
void pax_aslr_stack(struct proc *p, uintptr_t *addr); | |||||
void pax_aslr_stack_fixup(struct proc *p); | |||||
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 /* _KERNEL */ | |||||
/* | |||||
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. | |||||
* keep this values, to keep compatibility with HardenedBSD | |||||
*/ | |||||
#define PAX_NOTE_ASLR 0x00000040 | |||||
#define PAX_NOTE_NOASLR 0x00000080 | |||||
#define PAX_NOTE_ALL_ENABLED (PAX_NOTE_ASLR) | |||||
#define PAX_NOTE_ALL_DISABLED (PAX_NOTE_NOASLR) | |||||
#define PAX_NOTE_ALL (PAX_NOTE_ALL_ENABLED | PAX_NOTE_ALL_DISABLED) | |||||
#endif /* __SYS_PAX_H */ | |||||
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 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 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 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 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 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 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. | |||||
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 ActionsSure, I will add these checks. op: Sure, I will add these checks. | |||||
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 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… | |||||
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… | |||||
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… | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. | |||||
Not Done Inline ActionsRemove blank line. rwatson: Remove blank line. |
This whole file should be #ifndef __KERNEL since there's no user-space serviceable parts inside.