Commit 2e9f60932a2c ("net: hsr: check skb can contain struct hsr_ethhdr
in fill_frame_info") added the following which resulted in -EINVAL
always being returned:
if (skb->mac_len < sizeof(struct hsr_ethhdr))
return -EINVAL;
mac_len was not being set correctly so this check completely broke
HSR/PRP since it was always 14, not 20.
Set mac_len correctly and modify the mac_len checks to test in the
correct places since sometimes it is legitimately 14.
Fixes: 2e9f60932a2c ("net: hsr: check skb can contain struct hsr_ethhdr in fill_frame_info")
Signed-off-by: George McCollister <[email protected]>
---
net/hsr/hsr_device.c | 2 ++
net/hsr/hsr_forward.c | 30 +++++++++++++++++++++---------
net/hsr/hsr_forward.h | 8 ++++----
net/hsr/hsr_main.h | 4 ++--
net/hsr/hsr_slave.c | 11 +++++------
5 files changed, 34 insertions(+), 21 deletions(-)
diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
index bfcdc75fc01e..26c32407f029 100644
--- a/net/hsr/hsr_device.c
+++ b/net/hsr/hsr_device.c
@@ -218,6 +218,7 @@ static netdev_tx_t hsr_dev_xmit(struct sk_buff *skb, struct net_device *dev)
if (master) {
skb->dev = master->dev;
skb_reset_mac_header(skb);
+ skb_reset_mac_len(skb);
hsr_forward_skb(skb, master);
} else {
atomic_long_inc(&dev->tx_dropped);
@@ -259,6 +260,7 @@ static struct sk_buff *hsr_init_skb(struct hsr_port *master)
goto out;
skb_reset_mac_header(skb);
+ skb_reset_mac_len(skb);
skb_reset_network_header(skb);
skb_reset_transport_header(skb);
diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index 6852e9bccf5b..ceb8afb2a62f 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -474,8 +474,8 @@ static void handle_std_frame(struct sk_buff *skb,
}
}
-void hsr_fill_frame_info(__be16 proto, struct sk_buff *skb,
- struct hsr_frame_info *frame)
+int hsr_fill_frame_info(__be16 proto, struct sk_buff *skb,
+ struct hsr_frame_info *frame)
{
struct hsr_port *port = frame->port_rcv;
struct hsr_priv *hsr = port->hsr;
@@ -483,20 +483,26 @@ void hsr_fill_frame_info(__be16 proto, struct sk_buff *skb,
/* HSRv0 supervisory frames double as a tag so treat them as tagged. */
if ((!hsr->prot_version && proto == htons(ETH_P_PRP)) ||
proto == htons(ETH_P_HSR)) {
+ /* Check if skb contains hsr_ethhdr */
+ if (skb->mac_len < sizeof(struct hsr_ethhdr))
+ return -EINVAL;
+
/* HSR tagged frame :- Data or Supervision */
frame->skb_std = NULL;
frame->skb_prp = NULL;
frame->skb_hsr = skb;
frame->sequence_nr = hsr_get_skb_sequence_nr(skb);
- return;
+ return 0;
}
/* Standard frame or PRP from master port */
handle_std_frame(skb, frame);
+
+ return 0;
}
-void prp_fill_frame_info(__be16 proto, struct sk_buff *skb,
- struct hsr_frame_info *frame)
+int prp_fill_frame_info(__be16 proto, struct sk_buff *skb,
+ struct hsr_frame_info *frame)
{
/* Supervision frame */
struct prp_rct *rct = skb_get_PRP_rct(skb);
@@ -507,9 +513,11 @@ void prp_fill_frame_info(__be16 proto, struct sk_buff *skb,
frame->skb_std = NULL;
frame->skb_prp = skb;
frame->sequence_nr = prp_get_skb_sequence_nr(rct);
- return;
+ return 0;
}
handle_std_frame(skb, frame);
+
+ return 0;
}
static int fill_frame_info(struct hsr_frame_info *frame,
@@ -519,9 +527,10 @@ static int fill_frame_info(struct hsr_frame_info *frame,
struct hsr_vlan_ethhdr *vlan_hdr;
struct ethhdr *ethhdr;
__be16 proto;
+ int ret;
- /* Check if skb contains hsr_ethhdr */
- if (skb->mac_len < sizeof(struct hsr_ethhdr))
+ /* Check if skb contains ethhdr */
+ if (skb->mac_len < sizeof(struct ethhdr))
return -EINVAL;
memset(frame, 0, sizeof(*frame));
@@ -548,7 +557,10 @@ static int fill_frame_info(struct hsr_frame_info *frame,
frame->is_from_san = false;
frame->port_rcv = port;
- hsr->proto_ops->fill_frame_info(proto, skb, frame);
+ ret = hsr->proto_ops->fill_frame_info(proto, skb, frame);
+ if (ret)
+ return ret;
+
check_local_dest(port->hsr, skb, frame);
return 0;
diff --git a/net/hsr/hsr_forward.h b/net/hsr/hsr_forward.h
index b6acaafa83fc..206636750b30 100644
--- a/net/hsr/hsr_forward.h
+++ b/net/hsr/hsr_forward.h
@@ -24,8 +24,8 @@ struct sk_buff *prp_get_untagged_frame(struct hsr_frame_info *frame,
struct hsr_port *port);
bool prp_drop_frame(struct hsr_frame_info *frame, struct hsr_port *port);
bool hsr_drop_frame(struct hsr_frame_info *frame, struct hsr_port *port);
-void prp_fill_frame_info(__be16 proto, struct sk_buff *skb,
- struct hsr_frame_info *frame);
-void hsr_fill_frame_info(__be16 proto, struct sk_buff *skb,
- struct hsr_frame_info *frame);
+int prp_fill_frame_info(__be16 proto, struct sk_buff *skb,
+ struct hsr_frame_info *frame);
+int hsr_fill_frame_info(__be16 proto, struct sk_buff *skb,
+ struct hsr_frame_info *frame);
#endif /* __HSR_FORWARD_H */
diff --git a/net/hsr/hsr_main.h b/net/hsr/hsr_main.h
index 8f264672b70b..53d1f7a82463 100644
--- a/net/hsr/hsr_main.h
+++ b/net/hsr/hsr_main.h
@@ -186,8 +186,8 @@ struct hsr_proto_ops {
struct hsr_port *port);
struct sk_buff * (*create_tagged_frame)(struct hsr_frame_info *frame,
struct hsr_port *port);
- void (*fill_frame_info)(__be16 proto, struct sk_buff *skb,
- struct hsr_frame_info *frame);
+ int (*fill_frame_info)(__be16 proto, struct sk_buff *skb,
+ struct hsr_frame_info *frame);
bool (*invalid_dan_ingress_frame)(__be16 protocol);
void (*update_san_info)(struct hsr_node *node, bool is_sup);
};
diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c
index c5227d42faf5..b70e6bbf6021 100644
--- a/net/hsr/hsr_slave.c
+++ b/net/hsr/hsr_slave.c
@@ -60,12 +60,11 @@ static rx_handler_result_t hsr_handle_frame(struct sk_buff **pskb)
goto finish_pass;
skb_push(skb, ETH_HLEN);
-
- if (skb_mac_header(skb) != skb->data) {
- WARN_ONCE(1, "%s:%d: Malformed frame at source port %s)\n",
- __func__, __LINE__, port->dev->name);
- goto finish_consume;
- }
+ skb_reset_mac_header(skb);
+ if ((!hsr->prot_version && protocol == htons(ETH_P_PRP)) ||
+ protocol == htons(ETH_P_HSR))
+ skb_set_network_header(skb, ETH_HLEN + HSR_HLEN);
+ skb_reset_mac_len(skb);
hsr_forward_skb(skb, port);
--
2.11.0
Hi George,
On Mon, May 24, 2021 at 01:50:54PM -0500, George McCollister wrote:
> Commit 2e9f60932a2c ("net: hsr: check skb can contain struct hsr_ethhdr
> in fill_frame_info") added the following which resulted in -EINVAL
> always being returned:
> if (skb->mac_len < sizeof(struct hsr_ethhdr))
> return -EINVAL;
>
> mac_len was not being set correctly so this check completely broke
> HSR/PRP since it was always 14, not 20.
>
> Set mac_len correctly and modify the mac_len checks to test in the
> correct places since sometimes it is legitimately 14.
>
> Fixes: 2e9f60932a2c ("net: hsr: check skb can contain struct hsr_ethhdr in fill_frame_info")
> Signed-off-by: George McCollister <[email protected]>
> ---
Tested-by: Vladimir Oltean <[email protected]>
pinging works properly now.
> net/hsr/hsr_device.c | 2 ++
> net/hsr/hsr_forward.c | 30 +++++++++++++++++++++---------
> net/hsr/hsr_forward.h | 8 ++++----
> net/hsr/hsr_main.h | 4 ++--
> net/hsr/hsr_slave.c | 11 +++++------
> 5 files changed, 34 insertions(+), 21 deletions(-)
>
> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> index bfcdc75fc01e..26c32407f029 100644
> --- a/net/hsr/hsr_device.c
> +++ b/net/hsr/hsr_device.c
> @@ -218,6 +218,7 @@ static netdev_tx_t hsr_dev_xmit(struct sk_buff *skb, struct net_device *dev)
> if (master) {
> skb->dev = master->dev;
> skb_reset_mac_header(skb);
> + skb_reset_mac_len(skb);
> hsr_forward_skb(skb, master);
> } else {
> atomic_long_inc(&dev->tx_dropped);
> @@ -259,6 +260,7 @@ static struct sk_buff *hsr_init_skb(struct hsr_port *master)
> goto out;
>
> skb_reset_mac_header(skb);
> + skb_reset_mac_len(skb);
> skb_reset_network_header(skb);
> skb_reset_transport_header(skb);
>
> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index 6852e9bccf5b..ceb8afb2a62f 100644
> --- a/net/hsr/hsr_forward.c
> +++ b/net/hsr/hsr_forward.c
> @@ -474,8 +474,8 @@ static void handle_std_frame(struct sk_buff *skb,
> }
> }
>
> -void hsr_fill_frame_info(__be16 proto, struct sk_buff *skb,
> - struct hsr_frame_info *frame)
> +int hsr_fill_frame_info(__be16 proto, struct sk_buff *skb,
> + struct hsr_frame_info *frame)
> {
> struct hsr_port *port = frame->port_rcv;
> struct hsr_priv *hsr = port->hsr;
> @@ -483,20 +483,26 @@ void hsr_fill_frame_info(__be16 proto, struct sk_buff *skb,
> /* HSRv0 supervisory frames double as a tag so treat them as tagged. */
> if ((!hsr->prot_version && proto == htons(ETH_P_PRP)) ||
> proto == htons(ETH_P_HSR)) {
> + /* Check if skb contains hsr_ethhdr */
> + if (skb->mac_len < sizeof(struct hsr_ethhdr))
> + return -EINVAL;
> +
> /* HSR tagged frame :- Data or Supervision */
> frame->skb_std = NULL;
> frame->skb_prp = NULL;
> frame->skb_hsr = skb;
> frame->sequence_nr = hsr_get_skb_sequence_nr(skb);
> - return;
> + return 0;
> }
>
> /* Standard frame or PRP from master port */
> handle_std_frame(skb, frame);
> +
> + return 0;
> }
>
> -void prp_fill_frame_info(__be16 proto, struct sk_buff *skb,
> - struct hsr_frame_info *frame)
> +int prp_fill_frame_info(__be16 proto, struct sk_buff *skb,
> + struct hsr_frame_info *frame)
> {
> /* Supervision frame */
> struct prp_rct *rct = skb_get_PRP_rct(skb);
> @@ -507,9 +513,11 @@ void prp_fill_frame_info(__be16 proto, struct sk_buff *skb,
> frame->skb_std = NULL;
> frame->skb_prp = skb;
> frame->sequence_nr = prp_get_skb_sequence_nr(rct);
> - return;
> + return 0;
> }
> handle_std_frame(skb, frame);
> +
> + return 0;
> }
>
> static int fill_frame_info(struct hsr_frame_info *frame,
> @@ -519,9 +527,10 @@ static int fill_frame_info(struct hsr_frame_info *frame,
> struct hsr_vlan_ethhdr *vlan_hdr;
> struct ethhdr *ethhdr;
> __be16 proto;
> + int ret;
>
> - /* Check if skb contains hsr_ethhdr */
> - if (skb->mac_len < sizeof(struct hsr_ethhdr))
> + /* Check if skb contains ethhdr */
> + if (skb->mac_len < sizeof(struct ethhdr))
> return -EINVAL;
>
> memset(frame, 0, sizeof(*frame));
> @@ -548,7 +557,10 @@ static int fill_frame_info(struct hsr_frame_info *frame,
>
> frame->is_from_san = false;
> frame->port_rcv = port;
> - hsr->proto_ops->fill_frame_info(proto, skb, frame);
> + ret = hsr->proto_ops->fill_frame_info(proto, skb, frame);
Nitpick: hsr uses "res", not "ret".
> + if (ret)
> + return ret;
> +
> check_local_dest(port->hsr, skb, frame);
>
> return 0;
> diff --git a/net/hsr/hsr_forward.h b/net/hsr/hsr_forward.h
> index b6acaafa83fc..206636750b30 100644
> --- a/net/hsr/hsr_forward.h
> +++ b/net/hsr/hsr_forward.h
> @@ -24,8 +24,8 @@ struct sk_buff *prp_get_untagged_frame(struct hsr_frame_info *frame,
> struct hsr_port *port);
> bool prp_drop_frame(struct hsr_frame_info *frame, struct hsr_port *port);
> bool hsr_drop_frame(struct hsr_frame_info *frame, struct hsr_port *port);
> -void prp_fill_frame_info(__be16 proto, struct sk_buff *skb,
> - struct hsr_frame_info *frame);
> -void hsr_fill_frame_info(__be16 proto, struct sk_buff *skb,
> - struct hsr_frame_info *frame);
> +int prp_fill_frame_info(__be16 proto, struct sk_buff *skb,
> + struct hsr_frame_info *frame);
> +int hsr_fill_frame_info(__be16 proto, struct sk_buff *skb,
> + struct hsr_frame_info *frame);
> #endif /* __HSR_FORWARD_H */
> diff --git a/net/hsr/hsr_main.h b/net/hsr/hsr_main.h
> index 8f264672b70b..53d1f7a82463 100644
> --- a/net/hsr/hsr_main.h
> +++ b/net/hsr/hsr_main.h
> @@ -186,8 +186,8 @@ struct hsr_proto_ops {
> struct hsr_port *port);
> struct sk_buff * (*create_tagged_frame)(struct hsr_frame_info *frame,
> struct hsr_port *port);
> - void (*fill_frame_info)(__be16 proto, struct sk_buff *skb,
> - struct hsr_frame_info *frame);
> + int (*fill_frame_info)(__be16 proto, struct sk_buff *skb,
> + struct hsr_frame_info *frame);
> bool (*invalid_dan_ingress_frame)(__be16 protocol);
> void (*update_san_info)(struct hsr_node *node, bool is_sup);
> };
> diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c
> index c5227d42faf5..b70e6bbf6021 100644
> --- a/net/hsr/hsr_slave.c
> +++ b/net/hsr/hsr_slave.c
> @@ -60,12 +60,11 @@ static rx_handler_result_t hsr_handle_frame(struct sk_buff **pskb)
> goto finish_pass;
>
> skb_push(skb, ETH_HLEN);
> -
> - if (skb_mac_header(skb) != skb->data) {
> - WARN_ONCE(1, "%s:%d: Malformed frame at source port %s)\n",
> - __func__, __LINE__, port->dev->name);
> - goto finish_consume;
> - }
> + skb_reset_mac_header(skb);
> + if ((!hsr->prot_version && protocol == htons(ETH_P_PRP)) ||
> + protocol == htons(ETH_P_HSR))
> + skb_set_network_header(skb, ETH_HLEN + HSR_HLEN);
> + skb_reset_mac_len(skb);
>
> hsr_forward_skb(skb, port);
>
> --
> 2.11.0
>
I admit that I went through both patches and I still don't understand
what is the code path that the original commit 2e9f60932a2c ("net: hsr:
check skb can contain struct hsr_ethhdr in fill_frame_info") is
protecting against. I ran the C reproducer linked by syzbot too and I
did not reproduce it (I did not compile with the linked clang though).
Hello:
This patch was applied to netdev/net.git (refs/heads/master):
On Mon, 24 May 2021 13:50:54 -0500 you wrote:
> Commit 2e9f60932a2c ("net: hsr: check skb can contain struct hsr_ethhdr
> in fill_frame_info") added the following which resulted in -EINVAL
> always being returned:
> if (skb->mac_len < sizeof(struct hsr_ethhdr))
> return -EINVAL;
>
> mac_len was not being set correctly so this check completely broke
> HSR/PRP since it was always 14, not 20.
>
> [...]
Here is the summary with links:
- [net] net: hsr: fix mac_len checks
https://git.kernel.org/netdev/net/c/48b491a5cc74
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
On Mon, May 24, 2021 at 4:29 PM Vladimir Oltean <[email protected]> wrote:
[snip]
> > + ret = hsr->proto_ops->fill_frame_info(proto, skb, frame);
>
> Nitpick: hsr uses "res", not "ret".
>
Oops. I'll try to pay more attention to what is used in the existing
code next time.
[snip]
>
> I admit that I went through both patches and I still don't understand
> what is the code path that the original commit 2e9f60932a2c ("net: hsr:
> check skb can contain struct hsr_ethhdr in fill_frame_info") is
> protecting against. I ran the C reproducer linked by syzbot too and I
> did not reproduce it (I did not compile with the linked clang though).
I think it's complaining if you access more than mac_len bytes from
the pointer returned by skb_mac_header() but I'm not familiar with
this bot.
Thanks for testing the patch.
-George