Changeset View
Standalone View
sys/kern/imgact_elf.c
Context not available. | |||||
#include "opt_capsicum.h" | #include "opt_capsicum.h" | ||||
#include "opt_compat.h" | #include "opt_compat.h" | ||||
#include "opt_core.h" | #include "opt_core.h" | ||||
#include "opt_pax.h" | |||||
#include <sys/param.h> | #include <sys/param.h> | ||||
#include <sys/capsicum.h> | #include <sys/capsicum.h> | ||||
Context not available. | |||||
#include <sys/mman.h> | #include <sys/mman.h> | ||||
#include <sys/namei.h> | #include <sys/namei.h> | ||||
#include <sys/pioctl.h> | #include <sys/pioctl.h> | ||||
#include <sys/jail.h> | |||||
#include <sys/pax.h> | |||||
#include <sys/proc.h> | #include <sys/proc.h> | ||||
#include <sys/procfs.h> | #include <sys/procfs.h> | ||||
#include <sys/racct.h> | #include <sys/racct.h> | ||||
imp: This change is gratuitous and should be removed since it also doesn't follow style(9). | |||||
Not Done Inline ActionsThis should be done seprately. imp: This should be done seprately. | |||||
Not Done Inline ActionsLeftover cruft. Can certainly remove. lattera-gmail.com: Leftover cruft. Can certainly remove. | |||||
Not Done Inline ActionsAgreed. lattera-gmail.com: Agreed. | |||||
Context not available. | |||||
if (hdr->e_type == ET_DYN) { | if (hdr->e_type == ET_DYN) { | ||||
if ((brand_info->flags & BI_CAN_EXEC_DYN) == 0) | if ((brand_info->flags & BI_CAN_EXEC_DYN) == 0) | ||||
return (ENOEXEC); | return (ENOEXEC); | ||||
/* | } | ||||
* Honour the base load address from the dso if it is | |||||
* non-zero for some reason. | |||||
*/ | |||||
if (baddr == 0) | |||||
et_dyn_addr = ET_DYN_LOAD_ADDR; | |||||
else | |||||
et_dyn_addr = 0; | |||||
} else | |||||
et_dyn_addr = 0; | |||||
sv = brand_info->sysvec; | sv = brand_info->sysvec; | ||||
if (interp != NULL && brand_info->interp_newpath != NULL) | if (interp != NULL && brand_info->interp_newpath != NULL) | ||||
newinterp = brand_info->interp_newpath; | newinterp = brand_info->interp_newpath; | ||||
Context not available. | |||||
error = exec_new_vmspace(imgp, sv); | error = exec_new_vmspace(imgp, sv); | ||||
imgp->proc->p_sysent = sv; | imgp->proc->p_sysent = sv; | ||||
et_dyn_addr = 0; | |||||
if (hdr->e_type == ET_DYN) { | |||||
/* | |||||
* Honour the base load address from the dso if it is | |||||
* non-zero for some reason. | |||||
*/ | |||||
if (baddr == 0) { | |||||
et_dyn_addr = ET_DYN_LOAD_ADDR; | |||||
#ifdef PAX_ASLR | |||||
Not Done Inline ActionsThe original version of this code avoided assigning more than once, placing this assignment in two 'else' clauses. I'd rather you maintained the original structure, minimising differences. rwatson: The original version of this code avoided assigning more than once, placing this assignment in… | |||||
Not Done Inline ActionsThough this adds to the difference, it simplifies the logic. Previously, you had et_dyn_addr being set in multiple conditionals. This now only has a single conditional for modifying it. I'd rather keep it this way for simplicity sake. lattera-gmail.com: Though this adds to the difference, it simplifies the logic. Previously, you had et_dyn_addr… | |||||
if (pax_aslr_active(NULL, imgp->proc)) | |||||
et_dyn_addr += imgp->proc->p_vmspace->vm_aslr_delta_exec; | |||||
#endif | |||||
} | |||||
} | |||||
Not Done Inline ActionsIs the move of this block of code a bug fix? The diffs will be smaller if we move it in -current and then apply the modification. Not a big deal either way... I like to commit subsets of patches that are bug fixes when I'm doing other work to help keep patch-sets manageable. imp: Is the move of this block of code a bug fix? The diffs will be smaller if we move it in… | |||||
Not Done Inline ActionsIt's a code move and a simplification of code. You'll see a few lines above the removal of the et_dyn_addr variable initialization. Our code just simplifies what we moved a bit and applies ASLR to it if ASLR is active. lattera-gmail.com: It's a code move and a simplification of code. You'll see a few lines above the removal of the… | |||||
Not Done Inline ActionsIs referring to the binary as a 'dso' really accurate here? I wonder if 'binary' would be more appropriate? Also, 'for some reason' might just be because it needs to be mapped there due to its linkage -- so possibly we should just drop 'for some reason' as there are entirely legitimate reasons reasons, not vague 'some reasons'? rwatson: Is referring to the binary as a 'dso' really accurate here? I wonder if 'binary' would be more… | |||||
Not Done Inline ActionsThis comment was come from kib@: op@opn hardenedBSD.git> git blame freebsd/master -- sys/kern/kern_exec.c [...] edf781a81 (kib 2009-10-10 15:33:01 +0000 801) if ((brand_info->flags & BI_CAN_EXEC_DYN) == 0) edf781a81 (kib 2009-10-10 15:33:01 +0000 802) return (ENOEXEC); 2eb5677d2 (kib 2009-10-18 12:57:48 +0000 803) /* 2eb5677d2 (kib 2009-10-18 12:57:48 +0000 804) * Honour the base load address from the dso if it is 2eb5677d2 (kib 2009-10-18 12:57:48 +0000 805) * non-zero for some reason. 2eb5677d2 (kib 2009-10-18 12:57:48 +0000 806) */ 2eb5677d2 (kib 2009-10-18 12:57:48 +0000 807) if (baddr == 0) 2eb5677d2 (kib 2009-10-18 12:57:48 +0000 808) et_dyn_addr = ET_DYN_LOAD_ADDR; 2eb5677d2 (kib 2009-10-18 12:57:48 +0000 809) else 2eb5677d2 (kib 2009-10-18 12:57:48 +0000 810) et_dyn_addr = 0; [...] op: This comment was come from kib@:
op@opn hardenedBSD.git> git blame freebsd/master… | |||||
vn_lock(imgp->vp, LK_EXCLUSIVE | LK_RETRY); | vn_lock(imgp->vp, LK_EXCLUSIVE | LK_RETRY); | ||||
if (error) | if (error) | ||||
return (error); | return (error); | ||||
Context not available. | |||||
Not Done Inline ActionsThe next update will issue the missing rtld randomization here. op: The next update will issue the missing rtld randomization here. |
This change is gratuitous and should be removed since it also doesn't follow style(9).