[Linuxptp-devel] ptp4l and OnTime Labs 1588 Browser

[Rohrer Hansjoerg]

Hello

If I use the OnTime Labs IEEEE1588-2008 Browser V1.5 together with ptp4l V1.2 I got lots
of "bad message" reports in the log of the ptp4l. The browser is not able to show the ordinary clock info
of the ptp4l device.
Wireshark shows that ptp4l answers the management requests with "Error message (WRONG_LENGTH)".
Any hints what is wrong?

Best regards
Hansjörg

__________________________________________________


Hansjoerg Rohrer
Deputy CTO
Engineering & Design
***@mobatime.com
Phone direct: [phone]

Moser-Baer AG
Spitalstrasse 7
3454 Sumiswald
Switzerland

Phone: [phone]
Fax: [phone]

www.mobatime.com <http://www.mobatime.com>
www.mobatec.ch <http://www.mobatec.ch>
[cid:moser-baer_75years7c4e5b]

__________________________________________________

Confidentiality:
The information contained in this e-mail and in any attached files is confidential and/or legally privileged. If you are not the intended recipient, please contact the sender and delete this e-mail. Any unauthorised copying or distribution of the information contained in this e-mail and/or in any attached file is prohibited. The sender and/or the sending company do not accept liability for the incorrect and/or incomplete transmission of the information, nor for any delay or interruption of the transmission, nor for the damages arising from the use of or reliance on the information unless mandatory law provides otherwise. E-mails may be interfered with, may contain computer viruses or other defects. The sender and/or the sending company give no warranties and do not accept liability in relation to these matters, unless mandatory law provides otherwise. Thank you for your cooperation.

[Richard Cochran]

Post by Rohrer Hansjoerg
Any hints what is wrong?

Can you please post a short pcap file that shows the message exchange?

Thanks,
Richard

[Richard Cochran]

Post by Rohrer Hansjoerg
Any hints what is wrong?

(Thanks for the pcap file, sent off list.)

Your management tool is sending GET messages along with values, but
with the values all set to zero. Of course, this makes no sense, but I
checked the 1588 errata site, and it seems that Interpretation
Response #29 actually is saying to do this.

http://standards.ieee.org/findstds/interps/1588-2008.html

[ Notice that the question specifically asks about what to put into
the GET requests, but the Interpretation Response does not answer
that part of the question. It is silly for a GET message to send
values. They must be too ashamed to say it directly. ]

So, we are going to have to fix it. Geoff, can you take care of this?

If not, I'll have a patch ready in a few days.

Thanks,
Richard

[Dale Smith]

Ah yes.

In clock.c around line 787 there is this comment:

/*
The correct length according to the management ID is checked
in tlv.c, but management TLVs with empty bodies are also
received successfully to support GETs and CMDs. At this
point the TLV either has the correct length or length 2.
*/

And then the code goes on to reject any messages where the length != 2.

There is no allowance for messages that have the correct length.

I've just locally commented out those tests. I haven't submitted a patch
because I'm not sure what is the best approach to fix this. The comment
implies that tlv.c has already checked for the correct length, so at this
point in the code, we don't need to check the length at all.

-Dale

Hello****
** **
If I use the OnTime Labs IEEEE1588-2008 Browser V1.5 together with ptp4l
V1.2 I got lots****
of “bad message” reports in the log of the ptp4l. The browser is not able
to show the ordinary clock info****
of the ptp4l device.****
Wireshark shows that ptp4l answers the management requests with “Error
message (WRONG_LENGTH)”.****
Any hints what is wrong?****
** **
Best regards****
Hansjörg****
__________________________________________________
Hansjoerg Rohrer
Deputy CTO
Engineering & Design
Phone direct: [phone]
Moser-Baer AG
Spitalstrasse 7
3454 Sumiswald
Switzerland
Phone: [phone] Fax: [phone]
www.mobatime.com
www.mobatec.ch
__________________________________________________
*** ** The information contained in this e-mail and in any attached files
is confidential and/or legally privileged. If you are not the intended
recipient, please contact the sender and delete this e-mail. Any
unauthorised copying or distribution of the information contained in this
e-mail and/or in any attached file is prohibited. The sender and/or the
sending company do not accept liability for the incorrect and/or incomplete
transmission of the information, nor for any delay or interruption of the
transmission, nor for the damages arising from the use of or reliance on
the information unless mandatory law provides otherwise. E-mails may be
interfered with, may contain computer viruses or other defects. The sender
and/or the sending company give no warranties and do not accept liability
in relation to these matters, unless mandatory law provides otherwise.
Thank you for your cooperation.
------------------------------------------------------------------------------
Build for Windows Store.
http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Richard Cochran]

Post by Dale Smith
Ah yes.
/*
The correct length according to the management ID is checked
in tlv.c, but management TLVs with empty bodies are also
received successfully to support GETs and CMDs. At this
point the TLV either has the correct length or length 2.
*/
And then the code goes on to reject any messages where the length != 2.
There is no allowance for messages that have the correct length.

For COMMANDs, the only correct length is 2.

For GETs, the only *logical* length is 2, but, as I just found out,
the offical interpretation is that the GET sends a payload of
data. The contents of the data are not specified, however. It makes no
sense, but we will have to play along. Sending all zeroes, as
Hansjoerg's tool does, is probably the most reasonable thing to do.

The code currently is rejecting Hansjoerg's messages in two different
places, AFAICT, in clock.c as mentioned, and in tlv.c, depending on
the ID of the GET message.

Thanks,
Richard

[Geoff Salmon]

Post by Richard Cochran

Post by Dale Smith
/*
The correct length according to the management ID is checked
in tlv.c, but management TLVs with empty bodies are also
received successfully to support GETs and CMDs. At this
point the TLV either has the correct length or length 2.
*/
And then the code goes on to reject any messages where the length != 2.

...

Post by Richard Cochran
The code currently is rejecting Hansjoerg's messages in two different
places, AFAICT, in clock.c as mentioned, and in tlv.c, depending on
the ID of the GET message.

The tlv.c code doesn't know the action (GET, SET, etc.) so it should
accept GET messages with payloads as long as the size of the payload
matches both the management id and the field lengths that may be in the
payload. If these zero payloads are the correct length, I think the
check in clock.c that Dale identified is the only thing preventing the
GETs from working.

I'll try to make and test a patch this evening.

- Geoff

[Richard Cochran]

Post by Geoff Salmon

Post by Richard Cochran

Post by Dale Smith
/*
The correct length according to the management ID is checked
in tlv.c, but management TLVs with empty bodies are also
received successfully to support GETs and CMDs. At this
point the TLV either has the correct length or length 2.
*/
And then the code goes on to reject any messages where the length != 2.

...

Post by Richard Cochran
The code currently is rejecting Hansjoerg's messages in two different
places, AFAICT, in clock.c as mentioned, and in tlv.c, depending on
the ID of the GET message.

The tlv.c code doesn't know the action (GET, SET, etc.) so it should
accept GET messages with payloads as long as the size of the payload
matches both the management id and the field lengths that may be in the
payload. If these zero payloads are the correct length, I think the
check in clock.c that Dale identified is the only thing preventing the
GETs from working.

But Hansjoerg reports seeing "bad message", and this must come from
mgt_post_recv's -EBADMSG return after the bad_length: label.

Also, this is consistent with the pcap file he sent me. I'll forward
it to you, too.

Post by Geoff Salmon
I'll try to make and test a patch this evening.

Great.

Thanks,
Richard

[Richard Cochran]

Post by Geoff Salmon
The tlv.c code doesn't know the action (GET, SET, etc.) so it should
accept GET messages with payloads as long as the size of the payload
matches both the management id and the field lengths that may be in the
payload. If these zero payloads are the correct length, I think the

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Post by Geoff Salmon
check in clock.c that Dale identified is the only thing preventing the
GETs from working.

I loaded the GET CLOCK_DESCRIPTION message from the pcap file into a
test file (hacked pmc.c) in order to step thru. Here is where our code
throws an error.

#0 mgt_post_recv (m=0x8055050, data_len=26, extra=0xbffff08c) at tlv.c:170

170 if (extra_len + sizeof(m->id) != m->length)

(gdb) p extra_len = 22
(gdb) p sizeof(m->id) = 2
(gdb) p m->length = 28

So the packet is really wrong, I think. It looks like your careful
checking code has revealed a bug in OnTime's program. I'll have to
double check it, though.

Thanks,
Richard

[Richard Cochran]

Post by Richard Cochran
170 if (extra_len + sizeof(m->id) != m->length)
(gdb) p extra_len = 22
(gdb) p sizeof(m->id) = 2
(gdb) p m->length = 28

Yep, the OnTime program has a bug, and we have a bug, too.

Our bug is that we expect GET messages to have no payload. That is
easily fixed, but I think we should accept both full GET payloads and
empty ones.

OnTime's bug is that they don't know how to add:

| Field | Len | Type |
|-----------------------+-----+----------------------------|
| clockType | 2 | |
| physicalLayerProtocol | 1 | PTPText |
| physicalAddressLength | 2 | UInteger16 |
| physicalAddress | 0 | |
| protocolAddress | 4 | Enumeration16 + UInteger16 |
| manufacturerIdentity | 3 | |
| reserved | 1 | |
| productDescription | 1 | PTPText |
| revisionData | 1 | PTPText |
| userDescription | 1 | PTPText |
| profileIdentity | 6 | |
|-----------------------+-----+----------------------------|
| TOTAL | 22 | |

Our code correctly figures that the TLV length should be 24, and not
28 as advertised in the packet.


Thanks,
Richard

[Geoff Salmon]

Post by Richard Cochran
Our code correctly figures that the TLV length should be 24, and not
28 as advertised in the packet.

Huh, that's annoying. My guess is they're making the 4 PTPText fields
each be 2 bytes instead of 1 in a (misguided?) effort to keep fields
aligned. The 1 byte reserved field in the CLOCK_DESCRIPTION message and
the padding at the end of TLVs hint at trying to align things, but
alignment goes right out the window for fields after PTPTexts in my
opinion. I don't see anything in the spec about rounding up the size of
PTPTexts.

- Geoff

[Richard Cochran]

Post by Geoff Salmon
Huh, that's annoying. My guess is they're making the 4 PTPText
fields each be 2 bytes instead of 1 in a (misguided?) effort to keep
fields aligned. The 1 byte reserved field in the CLOCK_DESCRIPTION
message and the padding at the end of TLVs hint at trying to align
things, but alignment goes right out the window for fields after
PTPTexts in my opinion. I don't see anything in the spec about
rounding up the size of PTPTexts.

Or maybe they think a zero length PTPText has one zero byte in the
textField. But any way you look at it, the OnTime TLV is wrong.

Hansjoerg, perhaps you might file a bug report against OnTime? You
might even get a response, but don't hold your breath.

There are two bugs here, one in ptpt4l, and one in the OnTime
browser. The ptp4l bug is fixed by Goeff's patch. If you just want to
work around the OnTime bug, you can use the following hack.

Thanks,
Richard

---
diff --git a/tlv.c b/tlv.c
index ce72e4b..239455b 100644
--- a/tlv.c
+++ b/tlv.c
@@ -172,7 +172,7 @@ static int mgt_post_recv(struct management_tlv *m, uint16_t data_len,
}
return 0;
bad_length:
- return -EBADMSG;
+ return 0;
}

static void mgt_pre_send(struct management_tlv *m, struct tlv_extra *extra)

[Richard Cochran]

Post by Richard Cochran
Our code correctly figures that the TLV length should be 24, and not
28 as advertised in the packet.

Goeff,

I noticed another potential problem with the CLOCK_DESCRIPTION
parsing. In mgt_post_recv we have the following.

cd->productDescription = (struct PTPText *) buf;
buf += sizeof(struct PTPText) + cd->productDescription->length;
cd->revisionData = (struct PTPText *) buf;
buf += sizeof(struct PTPText) + cd->revisionData->length;
cd->userDescription = (struct PTPText *) buf;
buf += sizeof(struct PTPText) + cd->userDescription->length;

Don't we need a check that (buf + cd->xyz->length) does not overflow
the buffer?

Thanks,
Richard

[Geoff Salmon]

Post by Richard Cochran
Don't we need a check that (buf + cd->xyz->length) does not overflow
the buffer?

Yeah, looks like there's nothing in the CLOCK_DESCRIPTION case
preventing buf from being set past the end of the TLV and possibly past
the end of the buffer, so those ->length reads could be invalid. Even
worse, the earlier flip16 calls could be writing past the end of the buffer.

I'm not sure what I was thinking when I wrote it. I probably thought
reading off the end of the buffer wasn't harmful, and previously the
flip16 calls would be writing to the struct in the tlv_extra struct and
not into the buffer itself.

Anyways, I have a patch to add the checks, but haven't had a chance to
test it yet. Will send when it's ready.

- Geoff

[Richard Cochran]

Post by Geoff Salmon
Anyways, I have a patch to add the checks, but haven't had a chance
to test it yet. Will send when it's ready.

Okay, and please check the other cases, too. I think they are alright,
since they only set pointers, without dereferencing them, but double
checking never hurts.

Thanks,
Richard

[Richard Cochran]

Post by Geoff Salmon
Anyways, I have a patch to add the checks, but haven't had a chance
to test it yet. Will send when it's ready.

Geoff,

I would like to get a new release ready. Can you post this patch, even
if not fully tested?

If you don't have the time, then I can go ahead and fix this myself.

Thanks,
Richard

[Geoff Salmon]

Avoid reading, or worse byte swapping values, past the end
of the msg buffer.
---
tlv.c | 43 +++++++++++++++++++++++++++++--------------
1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/tlv.c b/tlv.c
index 35a3aa0..5dfb82d 100644
--- a/tlv.c
+++ b/tlv.c
@@ -64,48 +64,63 @@ static int mgt_post_recv(struct management_tlv *m, uint16_t data_len,
struct mgmt_clock_description *cd;
int extra_len = 0;
uint8_t *buf;
+ uint8_t *buf_end;
uint16_t u16;
+
switch (m->id) {
case CLOCK_DESCRIPTION:
cd = &extra->cd;
buf = m->data;
+ buf_end = buf + data_len;
+
+ /* A macro for assigning a pointer from the current
+ buf position, ensuring there is enough data for
+ the pointer's type, and advancing the buf pointer */
+#define ASSIGN_BUF_PTR(ptr) \
+ do { \
+ if (buf_end - buf < sizeof(*(ptr))) \
+ goto bad_length; \
+ (ptr) = (void *) buf; \
+ buf += sizeof(*(ptr)); \
+ } while (0)

- cd->clockType = (UInteger16 *) buf;
+ ASSIGN_BUF_PTR(cd->clockType);
flip16(cd->clockType);
- buf += sizeof(*cd->clockType);

- cd->physicalLayerProtocol = (struct PTPText *) buf;
- buf += sizeof(struct PTPText);
+ ASSIGN_BUF_PTR(cd->physicalLayerProtocol);
buf += cd->physicalLayerProtocol->length;

- cd->physicalAddress = (struct PhysicalAddress *) buf;
+ ASSIGN_BUF_PTR(cd->physicalAddress);
u16 = flip16(&cd->physicalAddress->length);
if (u16 > TRANSPORT_ADDR_LEN)
goto bad_length;
- buf += sizeof(struct PhysicalAddress) + u16;
+ buf += u16;

- cd->protocolAddress = (struct PortAddress *) buf;
+ ASSIGN_BUF_PTR(cd->protocolAddress);
flip16(&cd->protocolAddress->networkProtocol);
u16 = flip16(&cd->protocolAddress->addressLength);
if (u16 > TRANSPORT_ADDR_LEN)
goto bad_length;
- buf += sizeof(struct PortAddress) + u16;
+ buf += u16;

cd->manufacturerIdentity = buf;
buf += OUI_LEN + 1;

- cd->productDescription = (struct PTPText *) buf;
- buf += sizeof(struct PTPText) + cd->productDescription->length;
- cd->revisionData = (struct PTPText *) buf;
- buf += sizeof(struct PTPText) + cd->revisionData->length;
- cd->userDescription = (struct PTPText *) buf;
- buf += sizeof(struct PTPText) + cd->userDescription->length;
+ ASSIGN_BUF_PTR(cd->productDescription);
+ buf += cd->productDescription->length;
+ ASSIGN_BUF_PTR(cd->revisionData);
+ buf += cd->revisionData->length;
+ ASSIGN_BUF_PTR(cd->userDescription);
+ buf += cd->userDescription->length;

cd->profileIdentity = buf;
buf += PROFILE_ID_LEN;
extra_len = buf - m->data;
+#undef ASSIGN_BUF_PTR
break;
case USER_DESCRIPTION:
+ if (data_len < sizeof(struct PTPText))
+ goto bad_length;
extra->cd.userDescription = (struct PTPText *) m->data;
extra_len = sizeof(struct PTPText);
extra_len += extra->cd.userDescription->length;

[Geoff Salmon]

Sorry for the delay on this one. Never got around to testing it properly.

- Geoff

Post by Geoff Salmon
Avoid reading, or worse byte swapping values, past the end
of the msg buffer.
---
tlv.c | 43 +++++++++++++++++++++++++++++--------------
1 file changed, 29 insertions(+), 14 deletions(-)
diff --git a/tlv.c b/tlv.c
index 35a3aa0..5dfb82d 100644
--- a/tlv.c
+++ b/tlv.c
@@ -64,48 +64,63 @@ static int mgt_post_recv(struct management_tlv *m, uint16_t data_len,
struct mgmt_clock_description *cd;
int extra_len = 0;
uint8_t *buf;
+ uint8_t *buf_end;
uint16_t u16;
+
switch (m->id) {
cd = &extra->cd;
buf = m->data;
+ buf_end = buf + data_len;
+
+ /* A macro for assigning a pointer from the current
+ buf position, ensuring there is enough data for
+ the pointer's type, and advancing the buf pointer */
+#define ASSIGN_BUF_PTR(ptr) \
+ do { \
+ if (buf_end - buf < sizeof(*(ptr))) \
+ goto bad_length; \
+ (ptr) = (void *) buf; \
+ buf += sizeof(*(ptr)); \
+ } while (0)
- cd->clockType = (UInteger16 *) buf;
+ ASSIGN_BUF_PTR(cd->clockType);
flip16(cd->clockType);
- buf += sizeof(*cd->clockType);
- cd->physicalLayerProtocol = (struct PTPText *) buf;
- buf += sizeof(struct PTPText);
+ ASSIGN_BUF_PTR(cd->physicalLayerProtocol);
buf += cd->physicalLayerProtocol->length;
- cd->physicalAddress = (struct PhysicalAddress *) buf;
+ ASSIGN_BUF_PTR(cd->physicalAddress);
u16 = flip16(&cd->physicalAddress->length);
if (u16 > TRANSPORT_ADDR_LEN)
goto bad_length;
- buf += sizeof(struct PhysicalAddress) + u16;
+ buf += u16;
- cd->protocolAddress = (struct PortAddress *) buf;
+ ASSIGN_BUF_PTR(cd->protocolAddress);
flip16(&cd->protocolAddress->networkProtocol);
u16 = flip16(&cd->protocolAddress->addressLength);
if (u16 > TRANSPORT_ADDR_LEN)
goto bad_length;
- buf += sizeof(struct PortAddress) + u16;
+ buf += u16;
cd->manufacturerIdentity = buf;
buf += OUI_LEN + 1;
- cd->productDescription = (struct PTPText *) buf;
- buf += sizeof(struct PTPText) + cd->productDescription->length;
- cd->revisionData = (struct PTPText *) buf;
- buf += sizeof(struct PTPText) + cd->revisionData->length;
- cd->userDescription = (struct PTPText *) buf;
- buf += sizeof(struct PTPText) + cd->userDescription->length;
+ ASSIGN_BUF_PTR(cd->productDescription);
+ buf += cd->productDescription->length;
+ ASSIGN_BUF_PTR(cd->revisionData);
+ buf += cd->revisionData->length;
+ ASSIGN_BUF_PTR(cd->userDescription);
+ buf += cd->userDescription->length;
cd->profileIdentity = buf;
buf += PROFILE_ID_LEN;
extra_len = buf - m->data;
+#undef ASSIGN_BUF_PTR
break;
+ if (data_len < sizeof(struct PTPText))
+ goto bad_length;
extra->cd.userDescription = (struct PTPText *) m->data;
extra_len = sizeof(struct PTPText);
extra_len += extra->cd.userDescription->length;

[Richard Cochran]

Post by Geoff Salmon
Sorry for the delay on this one. Never got around to testing it properly.

Thats okay. I will solve this in a slightly different way, since I
found an issue with this patch (see below). It did help me to focus on
this passage, though.

Post by Geoff Salmon

Post by Geoff Salmon
Avoid reading, or worse byte swapping values, past the end
of the msg buffer.
---
tlv.c | 43 +++++++++++++++++++++++++++++--------------
1 file changed, 29 insertions(+), 14 deletions(-)
diff --git a/tlv.c b/tlv.c
index 35a3aa0..5dfb82d 100644
--- a/tlv.c
+++ b/tlv.c
@@ -64,48 +64,63 @@ static int mgt_post_recv(struct management_tlv *m, uint16_t data_len,
struct mgmt_clock_description *cd;
int extra_len = 0;
uint8_t *buf;
+ uint8_t *buf_end;
uint16_t u16;
+
switch (m->id) {
cd = &extra->cd;
buf = m->data;
+ buf_end = buf + data_len;
+
+ /* A macro for assigning a pointer from the current
+ buf position, ensuring there is enough data for
+ the pointer's type, and advancing the buf pointer */
+#define ASSIGN_BUF_PTR(ptr) \
+ do { \
+ if (buf_end - buf < sizeof(*(ptr))) \

This is an unsigned comparison, and so the logic fails
whenever (buf > buf_end).

Post by Geoff Salmon

Post by Geoff Salmon
+ goto bad_length; \
+ (ptr) = (void *) buf; \
+ buf += sizeof(*(ptr)); \
+ } while (0)

...

Post by Geoff Salmon

Post by Geoff Salmon
+ ASSIGN_BUF_PTR(cd->productDescription);
+ buf += cd->productDescription->length;

Could happen here, for example.

Post by Geoff Salmon

Post by Geoff Salmon
+ ASSIGN_BUF_PTR(cd->revisionData);
+ buf += cd->revisionData->length;
+ ASSIGN_BUF_PTR(cd->userDescription);
+ buf += cd->userDescription->length;

Thanks,
Richard

[Keller, Jacob E]

Post by Richard Cochran

Post by Geoff Salmon
Anyways, I have a patch to add the checks, but haven't had a chance
to test it yet. Will send when it's ready.

Geoff,
I would like to get a new release ready. Can you post this patch, even
if not fully tested?
If you don't have the time, then I can go ahead and fix this myself.
Thanks,
Richard
------------------------------------------------------------------------------
See everything from the browser to the database with AppDynamics
Get end-to-end visibility with application monitoring from AppDynamics
Isolate bottlenecks and diagnose root cause in seconds.
Start your free trial of AppDynamics Pro today!
http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

Will this release include the two RFC patch series you recently pos

[Richard Cochran]

Will this release include the two RFC patch series you recently posted?

Yes, it will include everything in current git master.

b74e467 pmc: release the message cache on exit.

Thanks,
Richard