Bug 184141
| Summary: | ppp: Kernel PPPoE sends bad echo-req magic number on big endian machines | ||
|---|---|---|---|
| Product: | Base System | Reporter: | James <jwm-freebsd> |
| Component: | sparc64 | Assignee: | Ed Maste <emaste> |
| Status: | Closed FIXED | ||
| Severity: | Affects Some People | CC: | emaste, mmoll, net |
| Priority: | --- | Flags: | koobs:
mfc-stable12+
koobs: mfc-stable11+ |
| Version: | Unspecified | ||
| Hardware: | Any | ||
| OS: | Any | ||
James wrote this message on Thu, Nov 21, 2013 at 13:35 +0000:
>
> >Number: 184141
> >Category: sparc64
> >Synopsis: Kernel PPPoE sends bad echo-req magic number on big endian machines
> >Confidential: no
> >Severity: non-critical
> >Priority: low
> >Responsible: freebsd-sparc64
> >State: open
> >Quarter:
> >Keywords:
> >Date-Required:
> >Class: sw-bug
> >Submitter-Id: current-users
> >Arrival-Date: Thu Nov 21 13:40:00 UTC 2013
> >Closed-Date:
> >Last-Modified:
> >Originator: James
> >Release: none
> >Organization:
> >Environment:
> n/a
> >Description:
> Recently, I found an OpenBSD bug where pppoe(4) sends echo-req packets with bad magic numbers. The following tcpdump trace below shows the bad echo-req packet with 00-00-00-00:
>
> Nov 1 09:24:59 smyslov /bsd: pppoe0: lcp output <echo-req id=0xea len=8 00-00-00-00>
> Nov 1 09:24:59 smyslov /bsd: pppoe0 (8864) state=3, session=0x3d22 output -> 00:90:1a:a3:7e:12, len=16
> Nov 1 09:24:59 smyslov /bsd: pppoe0: lcp input(opened): <term-req id=0xbf len=4 00-00-00-...-00-00-00>
> Nov 1 09:24:59 smyslov /bsd: pppoe0: lcp opened->stopping
>
> According to RFC 1661, "A Magic-Number of zero is illegal and MUST always be Nak'd".
>
> OpenBSD and FreeBSD share the code responsible for in-kernel PPPoE negotiation (sys/net/if_spppsubr.c):
>
> else if (sp->pp_phase >= PHASE_AUTHENTICATE) {
> unsigned long nmagic = htonl (sp->lcp.magic);
> sp->lcp.echoid = ++sp->pp_seq;
> sppp_cp_send (sp, PPP_LCP, ECHO_REQ,
> sp->lcp.echoid, 4, &nmagic);
> }
>
> The problem here is that sparc64 is big endian and 64 bit, and passing the address using '&nmagic' causes sppp_cp_send to see only zeros in the 'first' four bytes (most significant bytes).
>
> Small test case showing this:
>
> $ cat bigendian.c
> #include <stdio.h>
> #include <stdint.h>
>
> int main()
> {
> int j;
> uint64_t i = 0x11335577;
> uint8_t * p = &i;
>
> for( j = 0; j < sizeof(unsigned long); j++)
> printf("%02x ", p[j]);
>
> printf("\n");
>
> return 0;
> }
>
> $ cc -o be bigendian.c
> bigendian.c: In function 'main':
> bigendian.c:8: warning: initialization from incompatible pointer type
> $ ./be
> 00 00 00 00 11 33 55 77
>
> >How-To-Repeat:
> Problem eventually occurs during any PPPoE connection.
> >Fix:
> Changing nmagic to an int32_t fixes this problem:
>
> $ diff /usr/src/sys/net/if_spppsubr.c.orig /usr/src/sys/net/if_spppsubr.c
> 4595c4595
> < unsigned long nmagic = htonl (sp->lcp.magic);
> ---
> > int32_t nmagic = htonl (sp->lcp.magic);
>
> This is also what NetBSD does:
>
> https://github.com/jsonn/src/blob/trunk/sys/net/if_spppsubr.c#L4793
Would you mind if we used uint32_t instead? That is what htonl is
declared to return, and matches the signedness of the previous type
(unsigned)...
Have you tested this change and verified it worked? If so, I'll
commit it...
Thanks for bring this bug up..
--
John-Mark Gurney Voice: +1 415 225 5579
"All that I will do, has been done, All that I have, has not."
Responsible Changed From-To: freebsd-sparc64->jmg I'll take this since I plan on committing it... What's the status here? :) jmg do you plan to commit this? batch change: For bugs that match the following - Status Is In progress AND - Untouched since 2018-01-01. AND - Affects Base System OR Documentation DO: Reset to open status. Note: I did a quick pass but if you are getting this email it might be worthwhile to double check to see if this bug ought to be closed. Reset assignee after 5 years of inactivity. A commit references this bug: Author: emaste Date: Thu Aug 1 13:42:59 UTC 2019 New revision: 350497 URL: https://svnweb.freebsd.org/changeset/base/350497 Log: ppp: correct echo-req magic number on big endian archs The magic number is a 32-bit quantity; use uint32_t to match hton's return type and avoid sending zeros (upper 32 bits) on big-endian architectures. PR: 184141 MFC after: 1 week Sponsored by: The FreeBSD Foundation Changes: head/sys/net/if_spppsubr.c A commit references this bug: Author: emaste Date: Wed Aug 14 13:14:48 UTC 2019 New revision: 351025 URL: https://svnweb.freebsd.org/changeset/base/351025 Log: MFC r350497: ppp: correct echo-req magic number on big endian archs The magic number is a 32-bit quantity; use uint32_t to match hton's return type and avoid sending zeros (upper 32 bits) on big-endian architectures. PR: 184141 Sponsored by: The FreeBSD Foundation Changes: stable/12/sys/net/if_spppsubr.c A commit references this bug: Author: emaste Date: Wed Aug 14 13:15:39 UTC 2019 New revision: 351026 URL: https://svnweb.freebsd.org/changeset/base/351026 Log: MFC r350497: ppp: correct echo-req magic number on big endian archs The magic number is a 32-bit quantity; use uint32_t to match hton's return type and avoid sending zeros (upper 32 bits) on big-endian architectures. PR: 184141 Sponsored by: The FreeBSD Foundation Changes: _U stable/11/ stable/11/sys/net/if_spppsubr.c ^Triage: Track MFC (merge) flags/status. This also made it to releng/12.1 |
Recently, I found an OpenBSD bug where pppoe(4) sends echo-req packets with bad magic numbers. The following tcpdump trace below shows the bad echo-req packet with 00-00-00-00: Nov 1 09:24:59 smyslov /bsd: pppoe0: lcp output <echo-req id=0xea len=8 00-00-00-00> Nov 1 09:24:59 smyslov /bsd: pppoe0 (8864) state=3, session=0x3d22 output -> 00:90:1a:a3:7e:12, len=16 Nov 1 09:24:59 smyslov /bsd: pppoe0: lcp input(opened): <term-req id=0xbf len=4 00-00-00-...-00-00-00> Nov 1 09:24:59 smyslov /bsd: pppoe0: lcp opened->stopping According to RFC 1661, "A Magic-Number of zero is illegal and MUST always be Nak'd". OpenBSD and FreeBSD share the code responsible for in-kernel PPPoE negotiation (sys/net/if_spppsubr.c): else if (sp->pp_phase >= PHASE_AUTHENTICATE) { unsigned long nmagic = htonl (sp->lcp.magic); sp->lcp.echoid = ++sp->pp_seq; sppp_cp_send (sp, PPP_LCP, ECHO_REQ, sp->lcp.echoid, 4, &nmagic); } The problem here is that sparc64 is big endian and 64 bit, and passing the address using '&nmagic' causes sppp_cp_send to see only zeros in the 'first' four bytes (most significant bytes). Small test case showing this: $ cat bigendian.c #include <stdio.h> #include <stdint.h> int main() { int j; uint64_t i = 0x11335577; uint8_t * p = &i; for( j = 0; j < sizeof(unsigned long); j++) printf("%02x ", p[j]); printf("\n"); return 0; } $ cc -o be bigendian.c bigendian.c: In function 'main': bigendian.c:8: warning: initialization from incompatible pointer type $ ./be 00 00 00 00 11 33 55 77 Fix: Changing nmagic to an int32_t fixes this problem: $ diff /usr/src/sys/net/if_spppsubr.c.orig /usr/src/sys/net/if_spppsubr.c 4595c4595 < unsigned long nmagic = htonl (sp->lcp.magic); --- > int32_t nmagic = htonl (sp->lcp.magic); This is also what NetBSD does: https://github.com/jsonn/src/blob/trunk/sys/net/if_spppsubr.c#L4793 How-To-Repeat: Problem eventually occurs during any PPPoE connection.