Re: [PATCH net] net: hsr: fix mac_len checks

From: Vladimir Oltean
Date: Mon May 24 2021 - 17:29:49 EST

Next message: patchwork-bot+netdevbpf: "Re: [PATCH net] net: dsa: microchip: enable phy errata workaround on 9567"
Previous message: Jarkko Sakkinen: "Re: [PATCH v3] tpm2: add longer timeout for verify signature command"
In reply to: George McCollister: "[PATCH net] net: hsr: fix mac_len checks"
Next in thread: George McCollister: "Re: [PATCH net] net: hsr: fix mac_len checks"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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 <george.mccollister@xxxxxxxxx>
> ---

Tested-by: Vladimir Oltean <olteanv@xxxxxxxxx>

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).

Next message: patchwork-bot+netdevbpf: "Re: [PATCH net] net: dsa: microchip: enable phy errata workaround on 9567"
Previous message: Jarkko Sakkinen: "Re: [PATCH v3] tpm2: add longer timeout for verify signature command"
In reply to: George McCollister: "[PATCH net] net: hsr: fix mac_len checks"
Next in thread: George McCollister: "Re: [PATCH net] net: hsr: fix mac_len checks"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]