Re: [PATCH net] net: hsr: avoid potential out-of-bound access in fill_frame_info()
From: Stephan Wurm <hidden>
Date: 2025-01-22 10:28:04
Subsystem:
hsr network protocol, networking [general], the rest · Maintainers:
"David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Linus Torvalds
Am 21. Jan 16:35 hat Eric Dumazet geschrieben:
quoted hunk ↗ jump to hunk
On Tue, Jan 21, 2025 at 4:15 PM Stephan Wurm [off-list ref] wrote:quoted
I did some additional experiments.equoted
First, I was able to get v6.13 running on the system, although it did not fix my issue. Then I played around with VLAN interfaces. I created an explicit VLAN interface on top of the prp interface. In that case the VLAN header gets transparently attached to the tx frames and forwarding through the interface layers works as expected. It was even possible to get my application working on top of the vlan interface, but it resulted in two VLAN headers - the inner from the application, the outer from the vlan interface. So when sending vlan tagged frames directly from an application through a prp interface the mac_len field does not get updated, even though the VLAN protocol header is properly detected; when sending frames through an explicit vlan interface, the mac_len seems to be properly parsed into the skb. Now I am running out of ideas how to proceed. For the time being I would locally revert this fix, to make my application working again. But I can support in testing proposed solutions.If mac_len can not be used, we need yet another pskb_may_pull() I am unsure why hsr_get_node() is working, since it also uses skb->mac_len Please test the following patch :diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c index 87bb3a91598ee96b825f7aaff53aafb32ffe4f9..8942592130c151f2c948308e1ae16a6736822d5100644--- a/net/hsr/hsr_forward.c +++ b/net/hsr/hsr_forward.c@@ -700,9 +700,11 @@ static int fill_frame_info(struct hsr_frame_info *frame, frame->is_vlan = true; if (frame->is_vlan) { - if (skb->mac_len < offsetofend(struct hsr_vlan_ethhdr, vlanhdr)) + if (pskb_may_pull(skb, + skb_mac_offset(skb) + + offsetofend(struct hsr_vlan_ethhdr, vlanhdr))) return -EINVAL; - vlan_hdr = (struct hsr_vlan_ethhdr *)ethhdr; + vlan_hdr = (struct hsr_vlan_ethhdr *)skb_mac_header(skb); proto = vlan_hdr->vlanhdr.h_vlan_encapsulated_proto; }
Thank you very much! With this patch (slightly modified) everything works as expected now :) I only needed to invert the logic in the if clause, as otherwise all proper VLAN frames were dropped from PRP. Here is the modified version:
diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index 87bb3a91598e..3491627bbaf4 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c@@ -700,9 +700,11 @@ static int fill_frame_info(struct hsr_frame_info *frame, frame->is_vlan = true; if (frame->is_vlan) { - if (skb->mac_len < offsetofend(struct hsr_vlan_ethhdr, vlanhdr)) + if (!pskb_may_pull(skb, + skb_mac_offset(skb) + + offsetofend(struct hsr_vlan_ethhdr, vlanhdr))) return -EINVAL; - vlan_hdr = (struct hsr_vlan_ethhdr *)ethhdr; + vlan_hdr = (struct hsr_vlan_ethhdr *)skb_mac_header(skb); proto = vlan_hdr->vlanhdr.h_vlan_encapsulated_proto; }