Bug 207597 - pppoe does not check dst addr of received packets
Summary: pppoe does not check dst addr of received packets
Status: RESOLVED CODE_FIX
Alias: None
Product: Drivers
Classification: Unclassified
Component: Network (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: drivers_network@kernel-bugs.osdl.org
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-05-06 10:32 UTC by David Balažic
Modified: 2021-03-25 18:13 UTC (History)
2 users (show)

See Also:
Kernel Version: 4.14.171
Tree: Mainline
Regression: No


Attachments
patch with debug output (1.53 KB, patch)
2020-05-06 10:32 UTC, David Balažic
Details | Diff
quick fix (412 bytes, patch)
2020-05-06 10:33 UTC, David Balažic
Details | Diff

Description David Balažic 2020-05-06 10:32:29 UTC
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
Comment 1 David Balažic 2020-05-06 10:33:43 UTC
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
Comment 2 David Balažic 2021-03-25 18:13:44 UTC
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;

Note You need to log in before you can comment on or make changes to this bug.