Bug 184141 - ppp: Kernel PPPoE sends bad echo-req magic number on big endian machines
Summary: ppp: Kernel PPPoE sends bad echo-req magic number on big endian machines
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: sparc64 (show other bugs)
Version: unspecified
Hardware: Any Any
: --- Affects Some People
Assignee: Ed Maste
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-21 13:40 UTC by James
Modified: 2019-08-14 13:16 UTC (History)
3 users (show)

See Also:
koobs: mfc-stable11?
koobs: mfc-stable12?


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description James 2013-11-21 13:40:00 UTC
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.
Comment 1 John-Mark Gurney 2013-11-21 17:25:30 UTC
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."
Comment 2 John-Mark Gurney freebsd_committer 2013-11-21 17:25:45 UTC
Responsible Changed
From-To: freebsd-sparc64->jmg

I'll take this since I plan on committing it...
Comment 3 Michael Moll freebsd_committer 2015-11-29 20:11:14 UTC
What's the status here? :)
Comment 4 Ed Maste freebsd_committer 2017-11-22 03:42:04 UTC
jmg do you plan to commit this?
Comment 5 Eitan Adler freebsd_committer freebsd_triage 2018-05-28 19:46:11 UTC
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.
Comment 6 Eugene Grosbein freebsd_committer 2018-07-28 08:31:26 UTC
Reset assignee after 5 years of inactivity.
Comment 7 commit-hook freebsd_committer 2019-08-01 13:43:35 UTC
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
Comment 8 commit-hook freebsd_committer 2019-08-14 13:15:13 UTC
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
Comment 9 commit-hook freebsd_committer 2019-08-14 13:16:15 UTC
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