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
Created attachment 288955 [details] quick fix The same patch but without printk, just the code note: this is a quickfix, probably not a correct solution
This was fied in May 2020, for kernel versions 4.19, 4.14 (and 4.9, 4.4): From: Guillaume Nault <gnault@redhat.com> [ Upstream commit b8c158395119be62294da73646a3953c29ac974b ] We don't want to disconnect a session because of a stray PADT arriving while the interface is in promiscuous mode. Furthermore, multicast and broadcast packets make no sense here, so only PACKET_HOST is accepted. Reported-by: David Balažic <xerces9@gmail.com> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Signed-off-by: Guillaume Nault <gnault@redhat.com> Signed-off-by: David S. Miller <davem@davemloft.net> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- drivers/net/ppp/pppoe.c | 3 +++ 1 file changed, 3 insertions(+) --- a/drivers/net/ppp/pppoe.c +++ b/drivers/net/ppp/pppoe.c @@ -497,6 +497,9 @@ static int pppoe_disc_rcv(struct sk_buff if (!skb) goto out; + if (skb->pkt_type != PACKET_HOST) + goto abort; + if (!pskb_may_pull(skb, sizeof(struct pppoe_hdr))) goto abort;