linuxptp-devel

Re: [Linuxptp-devel] ptp4l wrongly takes padding bytes as TLV?

[Vincent Li X <vin...@er...>]

Hi Erez,
In that case, we take padding as TLV if I read code correctly. Now it's
basically ok, only because the padding bytes can be maximum two bytes (64 -
14 - 4 FCS - 44 (smallest PTP PDU?)), we would not enter below while-loop as
TLV struct is of size 4.
But how about if future comes new smaller PTP PDU? And I don't know for
UDP/IP/IPv6.

static int suffix_post_recv(struct ptp_message *msg, int len)
{
	...
	while (len >= sizeof(struct TLV)) {

Thanks,
Vincent

-----Original Message-----
From: Geva, Erez <ere...@si...> 
Sent: Thursday, January 31, 2019 5:22 PM
To: Jiri Benc <jb...@re...>
Cc: lin...@li...
Subject: Re: [Linuxptp-devel] ptp4l wrongly takes padding bytes as TLV?

As you feel, I don't care on the Ethernet frame.
Nor do I think we should try to fix it or criticize it.
If the packet pass the Linux kernel and received by the PTP daemon, we
should not care.

For me, the only question is, if the Ethernet frame does have padding and
the PTP frame is proper.
Is there a problem?
Is the PTP messaged parsed in a correct manner?

Erez
________________________________________
From: Jiri Benc [jb...@re...]
Sent: 31 January 2019 17:03
To: Richard Cochran
Cc: Mats Bergman H; Richard Jönsson; Lin...@li...
Subject: Re: [Linuxptp-devel] ptp4l wrongly takes padding bytes as TLV?

On Thu, 31 Jan 2019 07:41:38 -0800, Richard Cochran wrote:
> FWIW, Wireshark shows "Bad FCS" for this frame.  Please fix it at the 
> sender.

To be fair, this is just an artifact of Wireshark guessing wrong on the
packet structure. AFAIK there's no indication of the frames having FCS or
not in pcap. Wireshark has to guess and when it sees the packet being
6 bytes longer than the payload and 64 bytes in total, it's natural to guess
it's 2 bytes padding and 4 bytes FCS to satisfy the Ethernet minimum length
requirements.

While in fact, I expect that the FCS got stripped and the frame was
68 bytes. The real FCS was most likely correct.

Seeing the padding bytes not being zero, I cannot resist wondering what part
of its memory is the sender leaking. Could the leak be used to gather some
interesting data? ;-)

In any case, this behavior is wrong on several levels. And with the likely
security issue present, I don't think it's worth the time to consider this
hardware seriously.

 Jiri


_______________________________________________
Linuxptp-devel mailing list
Lin...@li...
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


_______________________________________________
Linuxptp-devel mailing list
Lin...@li...
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

Re: [Linuxptp-devel] ptp4l wrongly takes padding bytes as TLV?

[Geva, E. <ere...@si...>]

Since padding is permitable in Ethernet protocol, not limited by size or value of padding.

Depend on having only 2 bytes, based on the minimum PTP PDU size and the fact that most driver do not pad more then 64 bytes.

In  my opinion it sound shaky and prone to be bug in future implementations.

But, I do respect other opinions. -:)

Erez

________________________________________
From: Vincent Li X [vin...@er...]
Sent: 31 January 2019 18:14
To: Geva, Erez (ext) (PD PA CI R&D 3); Jiri Benc
Cc: lin...@li...
Subject: RE: [Linuxptp-devel] ptp4l wrongly takes padding bytes as TLV?

Hi Erez,
In that case, we take padding as TLV if I read code correctly. Now it's
basically ok, only because the padding bytes can be maximum two bytes (64 -
14 - 4 FCS - 44 (smallest PTP PDU?)), we would not enter below while-loop as
TLV struct is of size 4.
But how about if future comes new smaller PTP PDU? And I don't know for
UDP/IP/IPv6.

static int suffix_post_recv(struct ptp_message *msg, int len)
{
        ...
        while (len >= sizeof(struct TLV)) {

Thanks,
Vincent

-----Original Message-----
From: Geva, Erez <ere...@si...>
Sent: Thursday, January 31, 2019 5:22 PM
To: Jiri Benc <jb...@re...>
Cc: lin...@li...
Subject: Re: [Linuxptp-devel] ptp4l wrongly takes padding bytes as TLV?

As you feel, I don't care on the Ethernet frame.
Nor do I think we should try to fix it or criticize it.
If the packet pass the Linux kernel and received by the PTP daemon, we
should not care.

For me, the only question is, if the Ethernet frame does have padding and
the PTP frame is proper.
Is there a problem?
Is the PTP messaged parsed in a correct manner?

Erez
________________________________________
From: Jiri Benc [jb...@re...]
Sent: 31 January 2019 17:03
To: Richard Cochran
Cc: Mats Bergman H; Richard Jönsson; Lin...@li...
Subject: Re: [Linuxptp-devel] ptp4l wrongly takes padding bytes as TLV?

On Thu, 31 Jan 2019 07:41:38 -0800, Richard Cochran wrote:
> FWIW, Wireshark shows "Bad FCS" for this frame.  Please fix it at the
> sender.

To be fair, this is just an artifact of Wireshark guessing wrong on the
packet structure. AFAIK there's no indication of the frames having FCS or
not in pcap. Wireshark has to guess and when it sees the packet being
6 bytes longer than the payload and 64 bytes in total, it's natural to guess
it's 2 bytes padding and 4 bytes FCS to satisfy the Ethernet minimum length
requirements.

While in fact, I expect that the FCS got stripped and the frame was
68 bytes. The real FCS was most likely correct.

Seeing the padding bytes not being zero, I cannot resist wondering what part
of its memory is the sender leaking. Could the leak be used to gather some
interesting data? ;-)

In any case, this behavior is wrong on several levels. And with the likely
security issue present, I don't think it's worth the time to consider this
hardware seriously.

 Jiri


_______________________________________________
Linuxptp-devel mailing list
Lin...@li...
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


_______________________________________________
Linuxptp-devel mailing list
Lin...@li...
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

Re: [Linuxptp-devel] ptp4l wrongly takes padding bytes as TLV?

[Jiri B. <jb...@re...>]

On Thu, 31 Jan 2019 16:28:30 +0000, Vincent Li X wrote:
> we might also need to check again m->header.messageLength is bigger than
> cnt.

This might not be a bad idea; if the packet length is inconsistent with
the PTP or 802.3 standard, a warning can be emitted and the packet
dropped and not processed further. This would make linuxptp more robust
against randomly malformed packets and would give some indication to
users that the other end or intermediate network equipment is broken.

 Jiri

Re: [Linuxptp-devel] ptp4l wrongly takes padding bytes as TLV?

[Rafaël C. <fu...@vi...>]

Hello,

Since I see the subject is ethernet padding I would like to mention what
we use together with PTP:

A timestamping card which adds 9 bytes of VSS-monitoring trailer to the
ethernet packet.

Depending on the exact value of the timestamp, wireshark can misdetect
it and show a "wrong" FCS plus 5 bytes padding, see
https://github.com/boundary/wireshark/blob/master/epan/dissectors/packet-vssmonitoring.c#L139

I'm attaching a small capture where you can see traffic from 2 NICs, one
which adds a trailer to all incoming packets and one which does not.
I am anonymizing the MAC vendor for the timestamping card.

On 31/01/2019 19:24, Jiri Benc wrote:
> On Thu, 31 Jan 2019 16:28:30 +0000, Vincent Li X wrote:
>> we might also need to check again m->header.messageLength is bigger than
>> cnt.
> 
> This might not be a bad idea; if the packet length is inconsistent with
> the PTP or 802.3 standard, a warning can be emitted and the packet
> dropped and not processed further. This would make linuxptp more robust
> against randomly malformed packets and would give some indication to
> users that the other end or intermediate network equipment is broken.
> 
>  Jiri

Re: [Linuxptp-devel] ptp4l wrongly takes padding bytes as TLV?

[Richard C. <ric...@gm...>]

On Thu, Jan 31, 2019 at 04:28:30PM +0000, Vincent Li X wrote:
> we might also need to check again m->header.messageLength is bigger than
> cnt.

What?  We already have

	if (cnt < pdulen)
		return -EBADMSG;

in msg_post_recv();

Or did you mean something else?

Thanks,
Richard

Re: [Linuxptp-devel] ptp4l wrongly takes padding bytes as TLV?

[Miroslav L. <mli...@re...>]

On Thu, Jan 31, 2019 at 04:28:30PM +0000, Vincent Li X wrote:
> But we still think it's more safe to use header.messageLength instead of
> socket count, 
> Msg.c
> 	err = suffix_post_recv(m, cnt - pdulen);
> ==>
> 	err = suffix_post_recv(m, m->header.messageLength - pdulen);

I'm not sure that is more safe. If the field had a large value, it
might enable reading of uninitialized data, possibly even past the
buffer. A better way is to check the length in each transport specific
code and either remove the padding or drop the packet if the transport
doesn't allow padding.

-- 
Miroslav Lichvar

Re: [Linuxptp-devel] ptp4l wrongly takes padding bytes as TLV?

[Vincent Li X <vin...@er...>]

Sorry Miroslav again for missing your msg again in Junk box!
How about the combination with the length-check mentioned in my last msg?


-----Original Message-----
From: Miroslav Lichvar <mli...@re...> 
Sent: Friday, February 1, 2019 9:19 AM
To: Vincent Li X <vin...@er...>
Cc: Jiri Benc <jb...@re...>; Richard Cochran
<ric...@gm...>; Mats Bergman H <mat...@er...>;
Richard Jönsson <ric...@er...>;
Lin...@li...
Subject: Re: [Linuxptp-devel] ptp4l wrongly takes padding bytes as TLV?

On Thu, Jan 31, 2019 at 04:28:30PM +0000, Vincent Li X wrote:
> But we still think it's more safe to use header.messageLength instead 
> of socket count, Msg.c
> 	err = suffix_post_recv(m, cnt - pdulen); ==>
> 	err = suffix_post_recv(m, m->header.messageLength - pdulen);

I'm not sure that is more safe. If the field had a large value, it might
enable reading of uninitialized data, possibly even past the buffer. A
better way is to check the length in each transport specific code and either
remove the padding or drop the packet if the transport doesn't allow
padding.

--
Miroslav Lichvar