The ntpd client code corrects both T1 and T4 with the current offset

returned by adjtime(2) from the kernel.  T1 is local time when the
NTP packet is sent and T4 when the response is received.  If between
these events a NTP reply from another server is received, it may
change the kernel offset with adjtime(2).  Then the calulation of
the client offset was done with different bases, the result was
wrong and the system time started moving around.
So instead of correcting T1 and T4 individually at different events,
correct their sum once.
Error handling was missing if there is no timestamp in the response.
As this should not happen in our kernel, fatal() is appropriate.
tested by weerd@; OK claudio@

diff --git a/usr.sbin/ntpd/client.c b/usr.sbin/ntpd/client.c
index f53d242..db10c8a 100644 (file)
--- a/usr.sbin/ntpd/client.c
+++ b/usr.sbin/ntpd/client.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: client.c,v 1.114 2020/09/11 07:09:41 otto Exp $ */
+/*     $OpenBSD: client.c,v 1.115 2021/03/18 11:06:41 bluhm Exp $ */
 
 /*
  * Copyright (c) 2003, 2004 Henning Brauer <henning@openbsd.org>
@@ -208,7 +208,7 @@ client_query(struct ntp_peer *p)
 
        p->query->msg.xmttime.int_partl = arc4random();
        p->query->msg.xmttime.fractionl = arc4random();
-       p->query->xmttime = gettime_corrected();
+       p->query->xmttime = gettime();
 
        if (ntp_sendmsg(p->query->fd, NULL, &p->query->msg) == -1) {
                p->senderrors++;
@@ -295,7 +295,6 @@ client_dispatch(struct ntp_peer *p, u_int8_t settime, u_int8_t automatic)
        somsg.msg_control = cmsgbuf.buf;
        somsg.msg_controllen = sizeof(cmsgbuf.buf);
 
-       T4 = getoffset();
        if ((size = recvmsg(p->query->fd, &somsg, 0)) == -1) {
                if (errno == EHOSTUNREACH || errno == EHOSTDOWN ||
                    errno == ENETUNREACH || errno == ENETDOWN ||
@@ -325,10 +324,12 @@ client_dispatch(struct ntp_peer *p, u_int8_t settime, u_int8_t automatic)
                if (cmsg->cmsg_level == SOL_SOCKET &&
                    cmsg->cmsg_type == SCM_TIMESTAMP) {
                        memcpy(&tv, CMSG_DATA(cmsg), sizeof(tv));
-                       T4 += gettime_from_timeval(&tv);
+                       T4 = gettime_from_timeval(&tv);
                        break;
                }
        }
+       if (cmsg == NULL)
+               fatal("SCM_TIMESTAMP");
 
        ntp_getmsg((struct sockaddr *)&p->addr->ss, buf, size, &msg);
 
@@ -384,7 +385,7 @@ client_dispatch(struct ntp_peer *p, u_int8_t settime, u_int8_t automatic)
                return (0);
        }
 
-       p->reply[p->shift].offset = ((T2 - T1) + (T3 - T4)) / 2;
+       p->reply[p->shift].offset = ((T2 - T1) + (T3 - T4)) / 2 - getoffset();
        p->reply[p->shift].delay = (T4 - T1) - (T3 - T2);
        p->reply[p->shift].status.stratum = msg.stratum;
        if (p->reply[p->shift].delay < 0) {