Bug 207597

Created attachment 288953 [details]
patch with debug output

driver/net/ppp/pppoe.c   

Discovered in:    linux-4.14.171 (OpenWRT 19.07.2)
Still present in: linux-5.6.10  (judging by code review)

WHAT:
The pppoe code ignores the destination address of incoming PPPOE(D) packets.
Therefore it sometimes acts on a packet belonging to other that own PPP session.

EXAMPLE:
For example it takes a PADT packet meant to other connection and terminates own connection (when it shouldn't).
In practice it means PPPOE connections sporadically disconnect (when a "stray" PADT packet arrives on the ethernet interface).

DISCOVERY:

OpenWRT forum discussion:
"PPPoE disconnects every few hours"
https://forum.openwrt.org/t/pppoe-disconnects-every-few-hours/61239

linux-ppp mail list:
"PPPoE Modem hangup after random time - how to debug?"
https://marc.info/?l=linux-ppp&m=158757752621773&w=2


COMMENT(S):
Here [1] is Dianne Skoll's (rp-ppoe , on their mail list) take on it:

---quote-start---
> What is the angle from the standards point of view? Should/must the
> dst addr be checked?

I would say yes, although the standard does not explicitly spell this out.
It says this:

   This packet may be sent anytime after a session is established to
   indicate that a PPPoE session has been terminated.  It may be sent by
   either the Host or the Access Concentrator.  The DESTINATION_ADDR
   field is a unicast Ethernet address, the CODE field is set to 0xa7
   and the SESSION_ID MUST be set to indicate which session is to be
   terminated.  No TAGs are required.

Since is says that the DESTINATION_ADDR field is a unicast Ethernet
address, I think that's highly suggestive that the receiver of a PADT
should ignore it if the destination address doesn't match its interface
address.  I cannot see any rationale for accepting a PADT that is *not*
directed towards our interface, because it may be for a completely
unrelated session.  The standard also says [Sec 4]:

   The SESSION_ID field is sixteen bits.  It is an unsigned value in
   network byte order.  It's value is defined below for Discovery
   packets.  The value is fixed for a given PPP session and, in fact,
   defines a PPP session along with the Ethernet SOURCE_ADDR and
   DESTINATION_ADDR.

> IOW: why does the rp-ppoe code do this? (following the standard, or
> fixing a user reported problem, something else?)

I did it that way because the standard says that all three of these
items are needed to define a session:  Session ID, SOURCE_ADDR *and*
DESTINATION_ADDR.  If the DESTINATION_ADDR does not match, then it's
by definition a different session.

Therefore, IMO, this is a bug in the Linux kernel.
---quote-end---


PATCH:
A quick fix (attached, changes only handling of pppoed PADT packets) is to compare the dst addr to the current net device addr and ignore the packet if not matching.

A proper fix would be to record the local ends address (similar as the remote address is recorded, in include/uapi/linux/if_pppox.h struct pppoe_addr.remote) and compare to it for each received packet.
That would also require changes in the user space (ppp.samba.org and possibly others) IINM.


[1] https://dianne.skoll.ca/pipermail/rp-pppoe/2020q2/000575.html