Page MenuHomeFreeBSD
Differential D31516 Diff 94939 sys/kern/kern_resource.c

Changeset View

Standalone View

View Options

sys/kern/kern_resource.c

Show First 20 Lines • Show All 665 Lines • ▼ Show 20 Lineskern_proc_setrlimit(struct thread *td, struct proc *p, u_int which,
/* /*
* Preserve historical bugs by treating negative limits as unsigned. * Preserve historical bugs by treating negative limits as unsigned.
*/ */
if (limp->rlim_cur < 0) if (limp->rlim_cur < 0)
limp->rlim_cur = RLIM_INFINITY; limp->rlim_cur = RLIM_INFINITY;
if (limp->rlim_max < 0) if (limp->rlim_max < 0)
limp->rlim_max = RLIM_INFINITY; limp->rlim_max = RLIM_INFINITY;
if (which == RLIMIT_STACK && limp->rlim_cur != RLIM_INFINITY) {
limp->rlim_cur += p->p_vmspace->vm_stkgap;
kibUnsubmitted
Not Done
Inline Actions

This needs to be clamped against hard limit.

kib: This needs to be clamped against hard limit.
if (limp->rlim_cur > limp->rlim_max)
kibUnsubmitted
Not Done
Inline Actions

You are clamping against the hard limit passed by userspace? This is wrong. You need to clamp it against process rlim_max.

Hmm, I think this is already done at line 692. I am sorry for not noting it initially.

kib: You are clamping against the hard limit passed by userspace? This is wrong. You need to clamp…
dgr_semihalf.comAuthorUnsubmitted
Done
Inline Actions

Yeah, looking at this more closely, this is already being done for every resource limit slightly later in this function, so we don't need it here.

dgr_semihalf.com: Yeah, looking at this more closely, this is already being done for every resource limit…
limp->rlim_cur = limp->rlim_max;
}
oldssiz.rlim_cur = 0; oldssiz.rlim_cur = 0;
newlim = lim_alloc(); newlim = lim_alloc();
PROC_LOCK(p); PROC_LOCK(p);
oldlim = p->p_limit; oldlim = p->p_limit;
alimp = &oldlim->pl_rlimit[which]; alimp = &oldlim->pl_rlimit[which];
if (limp->rlim_cur > alimp->rlim_max || if (limp->rlim_cur > alimp->rlim_max ||
limp->rlim_max > alimp->rlim_max) limp->rlim_max > alimp->rlim_max)
if ((error = priv_check(td, PRIV_PROC_SETRLIMIT))) { if ((error = priv_check(td, PRIV_PROC_SETRLIMIT))) {
PROC_UNLOCK(p); PROC_UNLOCK(p);
lim_free(newlim); lim_free(newlim);
return (error); return (error);
} }
if (limp->rlim_cur > limp->rlim_max) if (limp->rlim_cur > limp->rlim_max)
limp->rlim_cur = limp->rlim_max; limp->rlim_cur = limp->rlim_max;
markjUnsubmitted
Not Done
Inline Actions

If userspace does rlim_cur = rlim_max = <value>, which IMHO is reasonable, then the stack gap is not taken into account, and the vm_map_protect() call below can clobber the existing stack if the gap is big enough. Why don't we adjust the hard limit by the gap as well?

markj: If userspace does `rlim_cur = rlim_max = <value>`, which IMHO is reasonable, then the stack gap…
dgr_semihalf.comAuthorUnsubmitted
Not Done
Inline Actions

The original patch did something similar (although it didn't adjust the rlimit structure, so it was invisible for the user). I adjusted this to be less intrusive according to kib's suggestion.

dgr_semihalf.com: The original patch did something similar (although it didn't adjust the rlimit structure, so it…
kibUnsubmitted
Not Done
Inline Actions

We must not allow user to increase hard limit, unless user has appropriate privilege. Then, this automatic tweaking should not depend on the user uid.

If there is an issue with vm_map_protect() over-extending its region, it should be clamped accordingly.

kib: We must not allow user to increase hard limit, unless user has appropriate privilege. Then…
lim_copy(newlim, oldlim); lim_copy(newlim, oldlim);
alimp = &newlim->pl_rlimit[which]; alimp = &newlim->pl_rlimit[which];
switch (which) { switch (which) {
case RLIMIT_CPU: case RLIMIT_CPU:
if (limp->rlim_cur != RLIM_INFINITY && if (limp->rlim_cur != RLIM_INFINITY &&
p->p_cpulimit == RLIM_INFINITY) p->p_cpulimit == RLIM_INFINITY)
callout_reset_sbt(&p->p_limco, SBT_1S, 0, callout_reset_sbt(&p->p_limco, SBT_1S, 0,
▲ Show 20 Lines • Show All 866 Lines • Show Last 20 Lines