[PATCH net 0/2] pfcp: fix two bugs in the receive path

[PATCH net 0/2] pfcp: fix two bugs in the receive path

[Haishuang Yan]

This series fixes two bugs in pfcp_encap_recv(), both introduced when
the receive path started filling in PFCP tunnel metadata.

Patch 1 fixes a metadata_dst leak when iptunnel_pull_header() fails
after tun_dst has been allocated.

Patch 2 makes sure the SEID is in the linear area before it is read, so
that a short packet with the S flag set cannot make the driver read
beyond the end of the packet data.

Haishuang Yan (2):
  pfcp: fix metadata_dst leak in pfcp_encap_recv()
  pfcp: make sure the SEID is linear before reading it

 drivers/net/pfcp.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH net 1/2] pfcp: fix metadata_dst leak in pfcp_encap_recv()

[Haishuang Yan]

pfcp_encap_recv() allocates tun_dst with udp_tun_rx_dst() but only
attaches it to the skb after iptunnel_pull_header() has succeeded.
When iptunnel_pull_header() fails, the code jumps to the drop label,
which frees the skb but not tun_dst, so the metadata_dst is leaked.

iptunnel_pull_header() can fail for a GSO skb that is cloned, when
skb_unclone() cannot allocate a new header with GFP_ATOMIC.

The dst cannot simply be attached earlier, since skb_scrub_packet() in
iptunnel_pull_header() drops it for cross-netns devices. Release it
explicitly on the error paths instead. The !md check can never be true,
but route it through the same label for consistency.

Fixes: 6dd514f48110 ("pfcp: always set pfcp metadata")
Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
---
 drivers/net/pfcp.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/pfcp.c b/drivers/net/pfcp.c
index e1cca779d2ec..1d14baf27c64 100644
--- a/drivers/net/pfcp.c
+++ b/drivers/net/pfcp.c
@@ -74,7 +74,7 @@ static int pfcp_encap_recv(struct sock *sk, struct sk_buff *skb)
 
 	md = ip_tunnel_info_opts(&tun_dst->u.tun_info);
 	if (unlikely(!md))
-		goto drop;
+		goto drop_dst;
 
 	if (unparsed->flags & PFCP_SEID_FLAG)
 		pfcp_session_recv(pfcp, skb, md);
@@ -87,7 +87,7 @@ static int pfcp_encap_recv(struct sock *sk, struct sk_buff *skb)
 	if (unlikely(iptunnel_pull_header(skb, PFCP_HLEN, skb->protocol,
 					  !net_eq(sock_net(sk),
 					  dev_net(pfcp->dev)))))
-		goto drop;
+		goto drop_dst;
 
 	skb_dst_set(skb, (struct dst_entry *)tun_dst);
 
@@ -98,6 +98,8 @@ static int pfcp_encap_recv(struct sock *sk, struct sk_buff *skb)
 	gro_cells_receive(&pfcp->gro_cells, skb);
 
 	return 0;
+drop_dst:
+	dst_release(&tun_dst->dst);
 drop:
 	kfree_skb(skb);
 	return 0;
-- 
2.43.0

[PATCH net 2/2] pfcp: make sure the SEID is linear before reading it

[Haishuang Yan]

pfcp_encap_recv() only makes sure that the UDP header and the 4 byte
PFCP header are in the linear area. When the S flag is set,
pfcp_session_recv() then reads the 8 byte SEID that follows, which is
not covered by the pskb_may_pull() check.

A short PFCP packet with the S flag set, or one whose session header
lies in a fragment, therefore makes pfcp_session_recv() read beyond the
end of the packet data, and whatever it finds there is stored in the
tunnel metadata that flower later classifies on.

Pull up to the end of the SEID before reading it, and drop packets
that are too short to contain it. Reload the header pointer afterwards
since pskb_may_pull() may reallocate the skb head.

Use offsetofend() rather than sizeof(struct pfcphdr_session): the
structure is not packed, so its size is 16 bytes because of the
alignment of the __be64 member, while the header on the wire is only 12
bytes, and valid session messages would be dropped.

Fixes: 6dd514f48110 ("pfcp: always set pfcp metadata")
Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
---
 drivers/net/pfcp.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/pfcp.c b/drivers/net/pfcp.c
index 1d14baf27c64..b5ebe6871cd9 100644
--- a/drivers/net/pfcp.c
+++ b/drivers/net/pfcp.c
@@ -65,6 +65,13 @@ static int pfcp_encap_recv(struct sock *sk, struct sk_buff *skb)
 		goto drop;
 
 	unparsed = pfcp_hdr(skb);
+	if (unparsed->flags & PFCP_SEID_FLAG) {
+		if (unlikely(!pskb_may_pull(skb, PFCP_HLEN +
+					    offsetofend(struct pfcphdr_session,
+							seid))))
+			goto drop;
+		unparsed = pfcp_hdr(skb);
+	}
 
 	ip_tunnel_flags_zero(flags);
 	tun_dst = udp_tun_rx_dst(skb, sk->sk_family, flags, 0,
-- 
2.43.0

Re: [PATCH net 0/2] pfcp: fix two bugs in the receive path

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.