Changeset View
Standalone View
sys/kern/kern_resource.c
| Show First 20 Lines • Show All 665 Lines • ▼ Show 20 Lines | kern_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; | |||||
kib: This needs to be clamped against hard limit. | |||||
Not Done Inline ActionsYou 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… | |||||
Done Inline ActionsYeah, 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… | |||||
| 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; | ||||
Not Done Inline ActionsIf 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… | |||||
Not Done Inline ActionsThe 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… | |||||
Not Done Inline ActionsWe 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 | |||||
This needs to be clamped against hard limit.