On Wed, Aug 12, 2020 at 01:58:08PM -0500, Scott Cheloha wrote:
>
> [...]
>
> There is one behavior change: in the setitimer(2) swap case it is now
> possible to EFAULT on copyout(9) *after* you have written the new
> timer value and (possibly) started the ITIMER_REAL timeout.
>
> For example, the following code now yields EFAULT even though a new
> oneshot timer has been started successfully.
>
> [...]
>
> I don't think there is a way to avoid this without introducing a bunch
> of extra complexity. [...]
It has occurred to me that we could do a trial copyout(9) in
sys_setitimer() before entering the critical section. This is a *bit*
wasteful, but is relatively inexpensive and narrows the behavior
change I mentioned down to truly improbable cases involving multiple
threads and munmap(2).
Updated patch below.
Index: kern_time.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_time.c,v
retrieving revision 1.140
diff -u -p -r1.140 kern_time.c
--- kern_time.c 12 Aug 2020 15:31:27 -0000 1.140
+++ kern_time.c 14 Aug 2020 23:59:23 -0000
@@ -491,7 +491,7 @@ out:
struct mutex itimer_mtx = MUTEX_INITIALIZER(IPL_CLOCK);
/*
- * Get value of an interval timer. The process virtual and
+ * Get and/or set value of an interval timer. The process virtual and
* profiling virtual time timers are kept internally in the
* way they are specified externally: in time until they expire.
*
@@ -509,6 +509,63 @@ struct mutex itimer_mtx = MUTEX_INITIALI
* real time timers .it_interval. Rather, we compute the next time in
* absolute time the timer should go off.
*/
+void
+setitimer(int which, struct itimerval *itv, struct itimerval *olditv)
+{
+ struct itimerspec its, oldits;
+ struct itimerspec *itimer;
+ struct process *pr;
+ int timo;
+
+ KASSERT(which >= ITIMER_REAL && which <= ITIMER_PROF);
+
+ pr = curproc->p_p;
+ itimer = &pr->ps_timer[which];
+
+ if (itv != NULL) {
+ TIMEVAL_TO_TIMESPEC(&itv->it_value, &its.it_value);
+ TIMEVAL_TO_TIMESPEC(&itv->it_interval, &its.it_interval);
+ }
+
+ if (which != ITIMER_REAL)
+ mtx_enter(&itimer_mtx);
+
+ if (olditv != NULL)
+ oldits = *itimer;
+ if (itv != NULL) {
+ if (which == ITIMER_REAL) {
+ struct timespec cts;
+ getnanouptime(&cts);
+ if (timespecisset(&its.it_value)) {
+ timo = tstohz(&its.it_value);
+ timeout_add(&pr->ps_realit_to, timo);
+ timespecadd(&its.it_value, &cts, &its.it_value);
+ } else
+ timeout_del(&pr->ps_realit_to);
+ }
+ *itimer = its;
+ }
+
+ if (which != ITIMER_REAL)
+ mtx_leave(&itimer_mtx);
+
+ if (olditv != NULL) {
+ if (which == ITIMER_REAL) {
+ struct timespec now;
+ getnanouptime(&now);
+ if (timespecisset(&oldits.it_value)) {
+ if (timespeccmp(&oldits.it_value, &now, <))
+ timespecclear(&oldits.it_value);
+ else
+ timespecsub(&oldits.it_value, &now,
+ &oldits.it_value);
+ }
+ }
+ TIMESPEC_TO_TIMEVAL(&olditv->it_value, &oldits.it_value);
+ TIMESPEC_TO_TIMEVAL(&olditv->it_interval, &oldits.it_interval);
+ }
+}
+
int
sys_getitimer(struct proc *p, void *v, register_t *retval)
{
@@ -516,44 +573,16 @@ sys_getitimer(struct proc *p, void *v, r
syscallarg(int) which;
syscallarg(struct itimerval *) itv;
} */ *uap = v;
- struct itimerspec its;
struct itimerval aitv;
- struct itimerspec *itimer;
int which;
which = SCARG(uap, which);
if (which < ITIMER_REAL || which > ITIMER_PROF)
return (EINVAL);
- itimer = &p->p_p->ps_timer[which];
memset(&aitv, 0, sizeof(aitv));
- if (which != ITIMER_REAL)
- mtx_enter(&itimer_mtx);
- its = *itimer;
- if (which != ITIMER_REAL)
- mtx_leave(&itimer_mtx);
-
- if (which == ITIMER_REAL) {
- struct timespec now;
-
- getnanouptime(&now);
- /*
- * Convert from absolute to relative time in .it_value
- * part of real time timer. If time for real time timer
- * has passed return 0, else return difference between
- * current time and time for the timer to go off.
- */
- if (timespecisset(&its.it_value)) {
- if (timespeccmp(&its.it_value, &now, <))
- timespecclear(&its.it_value);
- else
- timespecsub(&its.it_value, &now,
- &its.it_value);
- }
- }
- TIMESPEC_TO_TIMEVAL(&aitv.it_value, &its.it_value);
- TIMESPEC_TO_TIMEVAL(&aitv.it_interval, &its.it_interval);
+ setitimer(which, NULL, &aitv);
return (copyout(&aitv, SCARG(uap, itv), sizeof (struct itimerval)));
}
@@ -566,14 +595,12 @@ sys_setitimer(struct proc *p, void *v, r
syscallarg(const struct itimerval *) itv;
syscallarg(struct itimerval *) oitv;
} */ *uap = v;
- struct sys_getitimer_args getargs;
- struct itimerspec aits;
struct itimerval aitv;
+ struct itimerval olditv;
const struct itimerval *itvp;
struct itimerval *oitv;
- struct process *pr = p->p_p;
+ struct itimerval *newitvp = NULL, *olditvp = NULL;
int error;
- int timo;
int which;
which = SCARG(uap, which);
@@ -588,36 +615,22 @@ sys_setitimer(struct proc *p, void *v, r
return (error);
if (itimerfix(&aitv.it_value) || itimerfix(&aitv.it_interval))
return (EINVAL);
- TIMEVAL_TO_TIMESPEC(&aitv.it_value, &aits.it_value);
- TIMEVAL_TO_TIMESPEC(&aitv.it_interval, &aits.it_interval);
+ newitvp = &aitv;
}
if (oitv != NULL) {
- SCARG(&getargs, which) = which;
- SCARG(&getargs, itv) = oitv;
- if ((error = sys_getitimer(p, &getargs, retval)))
- return (error);
+ memset(&olditv, 0, sizeof(olditv));
+ error = copyout(&olditv, SCARG(uap, oitv), sizeof(olditv));
+ if (error)
+ return error;
+ olditvp = &olditv;
}
- if (itvp == 0)
+ if (newitvp == NULL && olditvp == NULL)
return (0);
- if (which != ITIMER_REAL)
- mtx_enter(&itimer_mtx);
-
- if (which == ITIMER_REAL) {
- struct timespec cts;
-
- getnanouptime(&cts);
- if (timespecisset(&aits.it_value)) {
- timo = tstohz(&aits.it_value);
- timeout_add(&pr->ps_realit_to, timo);
- timespecadd(&aits.it_value, &cts, &aits.it_value);
- } else
- timeout_del(&pr->ps_realit_to);
- }
- pr->ps_timer[which] = aits;
+ setitimer(which, newitvp, olditvp);
- if (which != ITIMER_REAL)
- mtx_leave(&itimer_mtx);
+ if (SCARG(uap, oitv) != NULL)
+ return copyout(&olditv, SCARG(uap, oitv), sizeof(olditv));
return (0);
}
