Re: [PATCH net] net: hsr: avoid potential out-of-bound access in fill_frame_info()

[Stephan Wurm <stephan.wurm@a-eberle.de>]

Am 17. Jan 14:22 hat Eric Dumazet geschrieben:
>
> Thanks for the report !
>
> You could add instrumentation there so that we see packet content.
>
> I suspect mac_len was not properly set somewhere.
>
> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index 87bb3a91598ee96b825f7aaff53aafb32ffe4f95..b0068e23083416ba13794e3b152517afbe5125b7
> 100644
> --- a/net/hsr/hsr_forward.c
> +++ b/net/hsr/hsr_forward.c
> @@ -700,8 +700,10 @@ 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 (skb->mac_len < offsetofend(struct hsr_vlan_ethhdr,
> vlanhdr)) {
> +                       DO_ONCE_LITE(skb_dump, KERN_ERR, skb, true);
>                         return -EINVAL;
> +               }
>                 vlan_hdr = (struct hsr_vlan_ethhdr *)ethhdr;
>                 proto = vlan_hdr->vlanhdr.h_vlan_encapsulated_proto;
>         }

Thanks for your instrumentation patch.

I got the following output in kernel log when sending an icmp echo with
VLAN header:

kernel: prp0: entered promiscuous mode
kernel: skb len=46 headroom=2 headlen=46 tailroom=144
        mac=(2,14) net=(16,-1) trans=-1
        shinfo(txflags=0 nr_frags=0 gso(size=0 type=0 segs=0))
        csum(0x0 ip_summed=0 complete_sw=0 valid=0 level=0)
        hash(0x0 sw=0 l4=0) proto=0x0000 pkttype=0 iif=0
kernel: dev name=prp0 feat=0x0000000000007000
kernel: sk family=17 type=3 proto=0
kernel: skb headroom: 00000000: 0d 12
kernel: skb linear:   00000000: 00 d0 93 4a 2d 91 00 d0 93 53 9c cb 81 00 00 00
kernel: skb linear:   00000010: 08 00 45 00 00 1c 00 01 00 00 40 01 d4 a1 ac 10
kernel: skb linear:   00000020: 27 14 ac 10 27 0a 08 00 f7 ff 00 00 00 00
kernel: skb tailroom: 00000000: 00 01 00 06 20 03 00 25 3c 20 00 00 00 00 00 00
kernel: skb tailroom: 00000010: 00 00 00 00 00 00 00 00 00 00 00 00 00 01 00 3d
kernel: skb tailroom: 00000020: 00 00 00 00 67 8a 61 45 15 63 56 39 00 25 00 7f
kernel: skb tailroom: 00000030: f8 fe ff ff 7f 00 d0 93 ff fe 64 e8 8e 00 53 00
kernel: skb tailroom: 00000040: 14 0e 14 31 00 00 53 00 14 0e 14 29 00 00 00 00
kernel: skb tailroom: 00000050: 00 00 00 00 00 00 00 00 00 00 08 00 45 00 00 34
kernel: skb tailroom: 00000060: 24 fa 40 00 40 06 17 c8 7f 00 00 01 7f 00 00 01
kernel: skb tailroom: 00000070: aa 04 13 8c 94 1d a0 b2 77 d6 5f 8a 80 10 02 00
kernel: skb tailroom: 00000080: fe 28 00 00 01 01 08 0a 89 e9 8a f7 89 e9 8a f7
kernel: prp0: left promiscuous mode

Additionally, I have tried some own instrumentation:

diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index e87447d04033..66f4c0d2a03a 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -700,8 +700,12 @@ 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))
-			return -EINVAL;
+		if (skb->mac_len < offsetofend(struct hsr_vlan_ethhdr, vlanhdr)) {
+			netdev_warn(port->dev,
+					"Would drop VLAN frame due to %d < %d\n",
+					skb->mac_len, offsetofend(struct hsr_vlan_ethhdr, vlanhdr));
+			// return -EINVAL;
+		}
 		vlan_hdr = (struct hsr_vlan_ethhdr *)ethhdr;
 		proto = vlan_hdr->vlanhdr.h_vlan_encapsulated_proto;
 	}

This gave me the following output (one line per VLAN tagged frame):

kernel: prp0: Would drop VLAN frame due to 14 < 18
kernel: prp0: Would drop VLAN frame due to 14 < 18
kernel: prp0: Would drop VLAN frame due to 14 < 18
kernel: prp0: Would drop VLAN frame due to 14 < 18
kernel: prp0: Would drop VLAN frame due to 14 < 18
kernel: prp0: Would drop VLAN frame due to 14 < 18
kernel: prp0: Would drop VLAN frame due to 14 < 18

Hopefully this helps to identify some new pointers.


Maybe I should add that the embedded system uses a 32 bit controller
(NXP LS1021a)? This fact already led to some strange side effects with
addressing issues.


Best regards

Stephan