[Linuxptp-devel] [PATCH RFC 0/2] delay request interval fixes

[Richard Cochran]

While testing ptp4l with another open source PTP stack (who shall
remain nameless), I found that the foreign master would set the
logMessageInterval field to 0x7F (or 2^127 seconds!) in some
circumstances. Up until now we have trusted this input, but I think
it wiser to enforce some input checking, in order to handle wacky
foreign masters.


Richard Cochran (2):
Fix integer overflow in the foreign master bookkeeping code.
port: constrain the master's logMinDelayReqInterval.

port.c | 25 ++++++++++++++++++-------
1 file changed, 18 insertions(+), 7 deletions(-)

[Richard Cochran]

The logMessageInterval field has an improbable range from 2^-128 to 2^127
seconds. The extreme ends cause an integer overflow in the calculation
of the "foreign master time window". Buggy or mis-configured foreign
masters advertising extreme values will cause incorrect announce message
aging.

This patch fixes the issue by adding thresholds for the bogus extremes.

Signed-off-by: Richard Cochran <***@gmail.com>
---
port.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/port.c b/port.c
index 3984b78..ef2686d 100644
--- a/port.c
+++ b/port.c
@@ -163,10 +163,15 @@ static int msg_current(struct ptp_message *m, struct timespec now)
t1 = m->ts.host.tv_sec * NSEC2SEC + m->ts.host.tv_nsec;
t2 = now.tv_sec * NSEC2SEC + now.tv_nsec;

- if (m->header.logMessageInterval < 0)
+ if (m->header.logMessageInterval < -63) {
+ tmo = 0;
+ } else if (m->header.logMessageInterval > 31) {
+ tmo = INT64_MAX;
+ } else if (m->header.logMessageInterval < 0) {
tmo = 4LL * NSEC2SEC / (1 << -m->header.logMessageInterval);
- else
+ } else {
tmo = 4LL * (1 << m->header.logMessageInterval) * NSEC2SEC;
+ }

return t2 - t1 < tmo;
}

[Richard Cochran]

Buggy or mis-configured masters can place bogus logMessageInterval values
in their delay response messages. This patch places reasonable limits on
the range of values that we will accept.

Signed-off-by: Richard Cochran <***@gmail.com>
---
port.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/port.c b/port.c
index ef2686d..91bb4e9 100644
--- a/port.c
+++ b/port.c
@@ -1666,12 +1666,18 @@ static void process_delay_resp(struct port *p, struct ptp_message *m)

clock_path_delay(p->clock, t3, t4c);

- if (p->logMinDelayReqInterval != rsp->hdr.logMessageInterval) {
- // TODO - validate the input.
- p->logMinDelayReqInterval = rsp->hdr.logMessageInterval;
- pr_notice("port %hu: minimum delay request interval 2^%d",
- portnum(p), p->logMinDelayReqInterval);
+ if (p->logMinDelayReqInterval == rsp->hdr.logMessageInterval) {
+ return;
+ }
+ if (rsp->hdr.logMessageInterval < -10 ||
+ rsp->hdr.logMessageInterval > 22) {
+ pr_debug("port %hu: ignore bogus delay request interval 2^%d",
+ portnum(p), rsp->hdr.logMessageInterval);
+ return;
}
+ p->logMinDelayReqInterval = rsp->hdr.logMessageInterval;
+ pr_notice("port %hu: minimum delay request interval 2^%d",
+ portnum(p), p->logMinDelayReqInterval);
}

static void process_follow_up(struct port *p, struct ptp_message *m)

[Miroslav Lichvar]

Post by Richard Cochran
Buggy or mis-configured masters can place bogus logMessageInterval values
in their delay response messages. This patch places reasonable limits on
the range of values that we will accept.

Both patches in the series look good to me. Just one question.

Post by Richard Cochran
+ if (rsp->hdr.logMessageInterval < -10 ||
+ rsp->hdr.logMessageInterval > 22) {
+ pr_debug("port %hu: ignore bogus delay request interval 2^%d",
+ portnum(p), rsp->hdr.logMessageInterval);
+ return;

Would it make sense to make it a warning message so the user can see
it and maybe think about reporting it to the vendor? To not spam the
syslog there could be a rate limit.

[Richard Cochran]

Post by Miroslav Lichvar
Would it make sense to make it a warning message so the user can see
it and maybe think about reporting it to the vendor? To not spam the
syslog there could be a rate limit.

Yeah, but we need rate limit support in the printing functions. There
are a couple of other messages (like fault on link down) that could
use rate limiting.

Care to send a patch?

Thanks,
Richard

------------------------------------------------------------------------------