Hi,
sys_pledge() has races when concurrent threads calling it in parallel.
for now, the race isn't really triggerable, but when wlpaths will be
enabled it could be more simple, and will result in leaking memory
(malloc without associated free).
This diff is an attempt to reordering operations in sys_pledge() in
order to avoid these memory leaks. Concurrent threads could still enter
together in sys_pledge(), but the reordering ensures that allocated
memory is always freed, and one thread will see EPERM while another will
see 0 (and its pledging applied).
It is acheived by moving all checks on ps_pledge and ps_pledgepaths
values at end of the call, just before setting them. As these operations
(checks + setting) doesn't sleep (just comparing values or simples
operations) there could be considered as atomic (pledge(2) is under
KERN_LOCK).
The global view of sys_pledge become:
- parse `requests' (char *) to `flags' (uint64_t)
- parse `paths' (char *) to `wl' (struct whitepaths)
(once here: no more sleep)
- check `flags' and `wl' are setteable (allow only promises
reductions, and setting ps_pledgepaths only once) - free allocated
string on error
- set `flags' and `wl' to struct proc.
In order to avoid abuse of repeatively call pledge(NULL, BIGLIST) for
DoSing the kernel (1. doing parsing / 2. get error / 3. goto 1), I keep
a check at beginning of `paths' parsing, and commented it accordingly.
Comments ?
--
Sebastien Marie
Index: kern/kern_pledge.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_pledge.c,v
retrieving revision 1.164
diff -u -p -r1.164 kern_pledge.c
--- kern/kern_pledge.c 25 Apr 2016 10:01:23 -0000 1.164
+++ kern/kern_pledge.c 25 Apr 2016 11:34:08 -0000
@@ -400,6 +400,7 @@ sys_pledge(struct proc *p, void *v, regi
syscallarg(const char **)paths;
} */ *uap = v;
uint64_t flags = 0;
+ struct whitepaths *wl = NULL;
int error;
if (SCARG(uap, request)) {
@@ -433,15 +434,6 @@ sys_pledge(struct proc *p, void *v, regi
flags |= f;
}
free(rbuf, M_TEMP, MAXPATHLEN);
-
- /*
- * if we are already pledged, allow only promises reductions.
- * flags doesn't contain flags outside _USERSET: they will be
- * relearned.
- */
- if (ISSET(p->p_p->ps_flags, PS_PLEDGE) &&
- (((flags | p->p_p->ps_pledge) != p->p_p->ps_pledge)))
- return (EPERM);
}
if (SCARG(uap, paths)) {
@@ -449,13 +441,16 @@ sys_pledge(struct proc *p, void *v, regi
return (EINVAL);
#else
const char **u = SCARG(uap, paths), *sp;
- struct whitepaths *wl;
char *path, *rdir = NULL, *cwd = NULL;
size_t pathlen, rdirlen, cwdlen;
size_t maxargs = 0;
int i, error;
+ /*
+ * anticipated check. it doesn't protect from races, but will
+ * avoid abuse of calling repeatively pledge(2).
+ */
if (p->p_p->ps_pledgepaths)
return (EPERM);
@@ -513,14 +508,8 @@ sys_pledge(struct proc *p, void *v, regi
free(cwd, M_TEMP, cwdlen);
free(path, M_TEMP, MAXPATHLEN);
- if (error) {
- for (i = 0; i < wl->wl_count; i++)
- free(wl->wl_paths[i].name,
- M_TEMP, wl->wl_paths[i].len);
- free(wl, M_TEMP, wl->wl_size);
- return (error);
- }
- p->p_p->ps_pledgepaths = wl;
+ if (error)
+ goto error;
#ifdef DEBUG_PLEDGE
/* print paths registered as whilelisted (viewed as without
chroot) */
@@ -535,12 +524,56 @@ sys_pledge(struct proc *p, void *v, regi
#endif
}
+ /*
+ * WARNING: all the code below should be done without sleep.
+ * it ensures no race between threads.
+ *
+ * - check for permissions on requested parameters
+ * - apply ps_pledgepaths
+ * - apply ps_pledge, and set PS_PLEDGE flag
+ */
+
+ /*
+ * if we are already pledged, allow only promises reductions.
+ * flags doesn't contain flags outside _USERSET: they will be
+ * relearned.
+ */
+ if (ISSET(p->p_p->ps_flags, PS_PLEDGE) &&
+ (((flags | p->p_p->ps_pledge) != p->p_p->ps_pledge))) {
+ error = EPERM;
+ goto error;
+ }
+
+ /* we can set ps_pledgepaths only once */
+ if (wl && p->p_p->ps_pledgepaths) {
+ error = EPERM;
+ goto error;
+ }
+
+ /* apply ps_pledgepaths */
+ if (wl)
+ p->p_p->ps_pledgepaths = wl;
+
+ /* apply ps_pledge and PS_PLEDGE */
if (SCARG(uap, request)) {
p->p_p->ps_pledge = flags;
- p->p_p->ps_flags |= PS_PLEDGE;
+ SET(p->p_p->ps_flags, PS_PLEDGE);
}
return (0);
+
+error:
+ if (wl) {
+ int i;
+
+ for (i = 0; i < wl->wl_count; i++)
+ free(wl->wl_paths[i].name,
+ M_TEMP, wl->wl_paths[i].len);
+
+ free(wl, M_TEMP, wl->wl_size);
+ }
+
+ return (error);
}
int
