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);
 }

Reply via email to