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

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

[Eric Dumazet]

syzbot is able to feed a packet with 14 bytes, pretending
it is a vlan one.

Since fill_frame_info() is relying on skb->mac_len already,
extend the check to cover this case.

BUG: KMSAN: uninit-value in fill_frame_info net/hsr/hsr_forward.c:709 [inline]
 BUG: KMSAN: uninit-value in hsr_forward_skb+0x9ee/0x3b10 net/hsr/hsr_forward.c:724
  fill_frame_info net/hsr/hsr_forward.c:709 [inline]
  hsr_forward_skb+0x9ee/0x3b10 net/hsr/hsr_forward.c:724
  hsr_dev_xmit+0x2f0/0x350 net/hsr/hsr_device.c:235
  __netdev_start_xmit include/linux/netdevice.h:5002 [inline]
  netdev_start_xmit include/linux/netdevice.h:5011 [inline]
  xmit_one net/core/dev.c:3590 [inline]
  dev_hard_start_xmit+0x247/0xa20 net/core/dev.c:3606
  __dev_queue_xmit+0x366a/0x57d0 net/core/dev.c:4434
  dev_queue_xmit include/linux/netdevice.h:3168 [inline]
  packet_xmit+0x9c/0x6c0 net/packet/af_packet.c:276
  packet_snd net/packet/af_packet.c:3146 [inline]
  packet_sendmsg+0x91ae/0xa6f0 net/packet/af_packet.c:3178
  sock_sendmsg_nosec net/socket.c:711 [inline]
  __sock_sendmsg+0x30f/0x380 net/socket.c:726
  __sys_sendto+0x594/0x750 net/socket.c:2197
  __do_sys_sendto net/socket.c:2204 [inline]
  __se_sys_sendto net/socket.c:2200 [inline]
  __x64_sys_sendto+0x125/0x1d0 net/socket.c:2200
  x64_sys_call+0x346a/0x3c30 arch/x86/include/generated/asm/syscalls_64.h:45
  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
  do_syscall_64+0xcd/0x1e0 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

Uninit was created at:
  slab_post_alloc_hook mm/slub.c:4091 [inline]
  slab_alloc_node mm/slub.c:4134 [inline]
  kmem_cache_alloc_node_noprof+0x6bf/0xb80 mm/slub.c:4186
  kmalloc_reserve+0x13d/0x4a0 net/core/skbuff.c:587
  __alloc_skb+0x363/0x7b0 net/core/skbuff.c:678
  alloc_skb include/linux/skbuff.h:1323 [inline]
  alloc_skb_with_frags+0xc8/0xd00 net/core/skbuff.c:6612
  sock_alloc_send_pskb+0xa81/0xbf0 net/core/sock.c:2881
  packet_alloc_skb net/packet/af_packet.c:2995 [inline]
  packet_snd net/packet/af_packet.c:3089 [inline]
  packet_sendmsg+0x74c6/0xa6f0 net/packet/af_packet.c:3178
  sock_sendmsg_nosec net/socket.c:711 [inline]
  __sock_sendmsg+0x30f/0x380 net/socket.c:726
  __sys_sendto+0x594/0x750 net/socket.c:2197
  __do_sys_sendto net/socket.c:2204 [inline]
  __se_sys_sendto net/socket.c:2200 [inline]
  __x64_sys_sendto+0x125/0x1d0 net/socket.c:2200
  x64_sys_call+0x346a/0x3c30 arch/x86/include/generated/asm/syscalls_64.h:45
  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
  do_syscall_64+0xcd/0x1e0 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

Fixes: 48b491a5cc74 ("net: hsr: fix mac_len checks")
Reported-by: syzbot+671e2853f9851d039551@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/netdev/6745dc7f.050a0220.21d33d.0018.GAE@google.com/T/#u
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: WingMan Kwok <w-kwok2@ti.com>
Cc: Murali Karicheri <m-karicheri2@ti.com>
Cc: MD Danish Anwar <danishanwar@ti.com>
Cc: Jiri Pirko <jiri@nvidia.com>
Cc: George McCollister <george.mccollister@gmail.com>
---
 net/hsr/hsr_forward.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index aa6acebc7c1ef6547451329fa23dafdc5443a5ba..87bb3a91598ee96b825f7aaff53aafb32ffe4f95 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -700,6 +700,8 @@ 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;
 		vlan_hdr = (struct hsr_vlan_ethhdr *)ethhdr;
 		proto = vlan_hdr->vlanhdr.h_vlan_encapsulated_proto;
 	}
-- 
2.47.0.338.g60cca15819-goog

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 26 Nov 2024 14:43:44 +0000 you wrote:
> syzbot is able to feed a packet with 14 bytes, pretending
> it is a vlan one.
> 
> Since fill_frame_info() is relying on skb->mac_len already,
> extend the check to cover this case.
> 
> BUG: KMSAN: uninit-value in fill_frame_info net/hsr/hsr_forward.c:709 [inline]
>  BUG: KMSAN: uninit-value in hsr_forward_skb+0x9ee/0x3b10 net/hsr/hsr_forward.c:724
>   fill_frame_info net/hsr/hsr_forward.c:709 [inline]
>   hsr_forward_skb+0x9ee/0x3b10 net/hsr/hsr_forward.c:724
>   hsr_dev_xmit+0x2f0/0x350 net/hsr/hsr_device.c:235
>   __netdev_start_xmit include/linux/netdevice.h:5002 [inline]
>   netdev_start_xmit include/linux/netdevice.h:5011 [inline]
>   xmit_one net/core/dev.c:3590 [inline]
>   dev_hard_start_xmit+0x247/0xa20 net/core/dev.c:3606
>   __dev_queue_xmit+0x366a/0x57d0 net/core/dev.c:4434
>   dev_queue_xmit include/linux/netdevice.h:3168 [inline]
>   packet_xmit+0x9c/0x6c0 net/packet/af_packet.c:276
>   packet_snd net/packet/af_packet.c:3146 [inline]
>   packet_sendmsg+0x91ae/0xa6f0 net/packet/af_packet.c:3178
>   sock_sendmsg_nosec net/socket.c:711 [inline]
>   __sock_sendmsg+0x30f/0x380 net/socket.c:726
>   __sys_sendto+0x594/0x750 net/socket.c:2197
>   __do_sys_sendto net/socket.c:2204 [inline]
>   __se_sys_sendto net/socket.c:2200 [inline]
>   __x64_sys_sendto+0x125/0x1d0 net/socket.c:2200
>   x64_sys_call+0x346a/0x3c30 arch/x86/include/generated/asm/syscalls_64.h:45
>   do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>   do_syscall_64+0xcd/0x1e0 arch/x86/entry/common.c:83
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 
> [...]

Here is the summary with links:
  - [net] net: hsr: avoid potential out-of-bound access in fill_frame_info()
    https://git.kernel.org/netdev/net/c/b9653d19e556

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[Stephan Wurm]

Hello Eric,

Am 26. Nov 14:43 hat Eric Dumazet geschrieben:
> syzbot is able to feed a packet with 14 bytes, pretending
> it is a vlan one.
>
> Since fill_frame_info() is relying on skb->mac_len already,
> extend the check to cover this case.
thanks for addressing this szybot finding.

Unfortunately, this seems to cause issues with VLAN tagged frames being
dropped from a PRP interface.

My setup consists of a custom embedded system equipped with v6.6 kernel,
recently updated from v6.6.62 to v6.6.69. In order to gain support for
VLAN tagged messages on top of PRP, I have applied first patch of the
series (see msgid 20241106091710.3308519-2-danishanwar@ti.com) that is
currently integrated with v6.13.

Now I want to send GOOSE messages (L2 broadcast messages with VLAN
header, including id=0 and QoS information) via the PRP interface.
With v6.6.62 this works as expected, with v6.6.69 the functionality
stopped again, with all VLAN-tagged frames being dropped from the PRP
interface.

By reverting this fix locally, I was able to restore the desired
functionality. But I do not iyet understand, why this fix breaks
sending of VLAN tagged frames in general.

Do you already know about this side effect?
Can you guide me to narrow down this issue?


Best regards
Stephan

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

[Eric Dumazet]

On Fri, Jan 17, 2025 at 12:30 PM Stephan Wurm <stephan.wurm@a-eberle.de> wrote:
>
> Hello Eric,
>
> Am 26. Nov 14:43 hat Eric Dumazet geschrieben:
> > syzbot is able to feed a packet with 14 bytes, pretending
> > it is a vlan one.
> >
> > Since fill_frame_info() is relying on skb->mac_len already,
> > extend the check to cover this case.
> thanks for addressing this szybot finding.
>
> Unfortunately, this seems to cause issues with VLAN tagged frames being
> dropped from a PRP interface.
>
> My setup consists of a custom embedded system equipped with v6.6 kernel,
> recently updated from v6.6.62 to v6.6.69. In order to gain support for
> VLAN tagged messages on top of PRP, I have applied first patch of the
> series (see msgid 20241106091710.3308519-2-danishanwar@ti.com) that is
> currently integrated with v6.13.
>
> Now I want to send GOOSE messages (L2 broadcast messages with VLAN
> header, including id=0 and QoS information) via the PRP interface.
> With v6.6.62 this works as expected, with v6.6.69 the functionality
> stopped again, with all VLAN-tagged frames being dropped from the PRP
> interface.
>
> By reverting this fix locally, I was able to restore the desired
> functionality. But I do not iyet understand, why this fix breaks
> sending of VLAN tagged frames in general.
>
> Do you already know about this side effect?
> Can you guide me to narrow down this issue?
>

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;
        }

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

[Stephan Wurm]

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

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

[Eric Dumazet]

On Fri, Jan 17, 2025 at 3:16 PM Stephan Wurm <stephan.wurm@a-eberle.de> wrote:
>
> 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
>

Yup, mac_len is incorrect, and the network header is also wrong.

Please give us a stack trace, because at least one caller of
hsr_forward() needs to be VLAN ready.

diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index 87bb3a91598ee96b825f7aaff53aafb32ffe4f95..6f65a535c7fcd740cef81e718323e86fd1eef832
100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -700,8 +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 (skb->mac_len < offsetofend(struct hsr_vlan_ethhdr,
vlanhdr)) {
+                       DO_ONCE_LITE(skb_dump, KERN_ERR, skb, true);
+                       WARN_ON_ONCE(1);
                        return -EINVAL;
+               }
                vlan_hdr = (struct hsr_vlan_ethhdr *)ethhdr;
                proto = vlan_hdr->vlanhdr.h_vlan_encapsulated_proto;
        }

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

[Eric Dumazet]

On Fri, Jan 17, 2025 at 7:14 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Fri, Jan 17, 2025 at 3:16 PM Stephan Wurm <stephan.wurm@a-eberle.de> wrote:
> >
> > 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
> >
>
> Yup, mac_len is incorrect, and the network header is also wrong.
>
> Please give us a stack trace, because at least one caller of
> hsr_forward() needs to be VLAN ready.
>
> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index 87bb3a91598ee96b825f7aaff53aafb32ffe4f95..6f65a535c7fcd740cef81e718323e86fd1eef832
> 100644
> --- a/net/hsr/hsr_forward.c
> +++ b/net/hsr/hsr_forward.c
> @@ -700,8 +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 (skb->mac_len < offsetofend(struct hsr_vlan_ethhdr,
> vlanhdr)) {
> +                       DO_ONCE_LITE(skb_dump, KERN_ERR, skb, true);
> +                       WARN_ON_ONCE(1);
>                         return -EINVAL;
> +               }
>                 vlan_hdr = (struct hsr_vlan_ethhdr *)ethhdr;
>                 proto = vlan_hdr->vlanhdr.h_vlan_encapsulated_proto;
>         }

BTW, also please cherry-pick this commit from linux-6.10

commit 4308811ba90118ae1b71a95fee79ab7dada6400c
Author: Eric Dumazet <edumazet@google.com>
Date:   Sun Apr 7 08:06:06 2024 +0000

    net: display more skb fields in skb_dump()

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

[Stephan Wurm]

Am 17. Jan 19:18 hat Eric Dumazet geschrieben:
> On Fri, Jan 17, 2025 at 7:14 PM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Fri, Jan 17, 2025 at 3:16 PM Stephan Wurm <stephan.wurm@a-eberle.de> wrote:
> > >
> > > 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
> > >
> >
> > Yup, mac_len is incorrect, and the network header is also wrong.
> >
> > Please give us a stack trace, because at least one caller of
> > hsr_forward() needs to be VLAN ready.
> >
> > diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> > index 87bb3a91598ee96b825f7aaff53aafb32ffe4f95..6f65a535c7fcd740cef81e718323e86fd1eef832
> > 100644
> > --- a/net/hsr/hsr_forward.c
> > +++ b/net/hsr/hsr_forward.c
> > @@ -700,8 +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 (skb->mac_len < offsetofend(struct hsr_vlan_ethhdr,
> > vlanhdr)) {
> > +                       DO_ONCE_LITE(skb_dump, KERN_ERR, skb, true);
> > +                       WARN_ON_ONCE(1);
> >                         return -EINVAL;
> > +               }
> >                 vlan_hdr = (struct hsr_vlan_ethhdr *)ethhdr;
> >                 proto = vlan_hdr->vlanhdr.h_vlan_encapsulated_proto;
> >         }
>
> BTW, also please cherry-pick this commit from linux-6.10
>
> commit 4308811ba90118ae1b71a95fee79ab7dada6400c
> Author: Eric Dumazet <edumazet@google.com>
> Date:   Sun Apr 7 08:06:06 2024 +0000
>
>     net: display more skb fields in skb_dump()

Applying the new instrumentation gives me the following stack trace:

kernel: skb len=170 headroom=2 headlen=170 tailroom=20
        mac=(2,14) mac_len=14 net=(16,-1) trans=-1
        shinfo(txflags=0 nr_frags=0 gso(size=0 type=0 segs=0))
        csum(0x0 start=0 offset=0 ip_summed=0 complete_sw=0 valid=0 level=0)
        hash(0x0 sw=0 l4=0) proto=0x0000 pkttype=0 iif=0
        priority=0x0 mark=0x0 alloc_cpu=0 vlan_all=0x0
        encapsulation=0 inner(proto=0x0000, mac=0, net=0, trans=0)
kernel: dev name=prp0 feat=0x0000000000007000
kernel: sk family=17 type=3 proto=0
kernel: skb headroom: 00000000: 74 00
kernel: skb linear:   00000000: 01 0c cd 01 00 01 00 d0 93 53 9c cb 81 00 80 00
kernel: skb linear:   00000010: 88 b8 00 01 00 98 00 00 00 00 61 81 8d 80 16 52
kernel: skb linear:   00000020: 45 47 44 4e 43 54 52 4c 2f 4c 4c 4e 30 24 47 4f
kernel: skb linear:   00000030: 24 47 6f 43 62 81 01 14 82 16 52 45 47 44 4e 43
kernel: skb linear:   00000040: 54 52 4c 2f 4c 4c 4e 30 24 44 73 47 6f 6f 73 65
kernel: skb linear:   00000050: 83 07 47 6f 49 64 65 6e 74 84 08 67 8d f5 93 7e
kernel: skb linear:   00000060: 76 c8 00 85 01 01 86 01 00 87 01 00 88 01 01 89
kernel: skb linear:   00000070: 01 00 8a 01 02 ab 33 a2 15 83 01 00 84 03 03 00
kernel: skb linear:   00000080: 00 91 08 67 8d f5 92 77 4b c6 1f 83 01 00 a2 1a
kernel: skb linear:   00000090: a2 06 85 01 00 83 01 00 84 03 03 00 00 91 08 67
kernel: skb linear:   000000a0: 8d f5 92 77 4b c6 1f 83 01 00
kernel: skb tailroom: 00000000: 80 18 02 00 fe 4e 00 00 01 01 08 0a 4f fd 5e d1
kernel: skb tailroom: 00000010: 4f fd 5e cd
kernel: ------------[ cut here ]------------
kernel: WARNING: CPU: 0 PID: 751 at /net/hsr/hsr_forward.c:605 fill_frame_info+0x180/0x19c
kernel: Modules linked in:
kernel: CPU: 0 PID: 751 Comm: reg61850 Not tainted 6.6.69-ga7a5cc0c39f0 #1
kernel: Hardware name: Freescale LS1021A
kernel:  unwind_backtrace from show_stack+0x10/0x14
kernel:  show_stack from dump_stack_lvl+0x40/0x4c
kernel:  dump_stack_lvl from __warn+0x94/0xc0
kernel:  __warn from warn_slowpath_fmt+0x1b4/0x1bc
kernel:  warn_slowpath_fmt from fill_frame_info+0x180/0x19c
kernel:  fill_frame_info from hsr_forward_skb+0x54/0x118
kernel:  hsr_forward_skb from hsr_dev_xmit+0x60/0xc4
kernel:  hsr_dev_xmit from dev_hard_start_xmit+0xa0/0xe4
kernel:  dev_hard_start_xmit from __dev_queue_xmit+0x144/0x5e8
kernel:  __dev_queue_xmit from packet_snd+0x5c0/0x784
kernel:  packet_snd from sock_write_iter+0xa0/0x10c
kernel:  sock_write_iter from vfs_write+0x3ac/0x41c
kernel:  vfs_write from ksys_write+0xbc/0xf0
kernel:  ksys_write from ret_fast_syscall+0x0/0x4c
kernel: Exception stack(0xc0d8dfa8 to 0xc0d8dff0)
kernel: dfa0:                   000000aa 73058e53 00000012 73058e53 000000aa 00000000
kernel: dfc0: 000000aa 73058e53 00000012 00000004 6ebf9940 0000000a 00000000 00000000
kernel: dfe0: 00000004 6ebf90f8 766a17ad 7661e5e6
kernel: ---[ end trace 0000000000000000 ]---

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

[Eric Dumazet]

On Mon, Jan 20, 2025 at 8:32 AM Stephan Wurm <stephan.wurm@a-eberle.de> wrote:
>
> Am 17. Jan 19:18 hat Eric Dumazet geschrieben:
> > On Fri, Jan 17, 2025 at 7:14 PM Eric Dumazet <edumazet@google.com> wrote:
> > >
> > > On Fri, Jan 17, 2025 at 3:16 PM Stephan Wurm <stephan.wurm@a-eberle.de> wrote:
> > > >
> > > > 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
> > > >
> > >
> > > Yup, mac_len is incorrect, and the network header is also wrong.
> > >
> > > Please give us a stack trace, because at least one caller of
> > > hsr_forward() needs to be VLAN ready.
> > >
> > > diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> > > index 87bb3a91598ee96b825f7aaff53aafb32ffe4f95..6f65a535c7fcd740cef81e718323e86fd1eef832
> > > 100644
> > > --- a/net/hsr/hsr_forward.c
> > > +++ b/net/hsr/hsr_forward.c
> > > @@ -700,8 +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 (skb->mac_len < offsetofend(struct hsr_vlan_ethhdr,
> > > vlanhdr)) {
> > > +                       DO_ONCE_LITE(skb_dump, KERN_ERR, skb, true);
> > > +                       WARN_ON_ONCE(1);
> > >                         return -EINVAL;
> > > +               }
> > >                 vlan_hdr = (struct hsr_vlan_ethhdr *)ethhdr;
> > >                 proto = vlan_hdr->vlanhdr.h_vlan_encapsulated_proto;
> > >         }
> >
> > BTW, also please cherry-pick this commit from linux-6.10
> >
> > commit 4308811ba90118ae1b71a95fee79ab7dada6400c
> > Author: Eric Dumazet <edumazet@google.com>
> > Date:   Sun Apr 7 08:06:06 2024 +0000
> >
> >     net: display more skb fields in skb_dump()
>
> Applying the new instrumentation gives me the following stack trace:
>
> kernel: skb len=170 headroom=2 headlen=170 tailroom=20
>         mac=(2,14) mac_len=14 net=(16,-1) trans=-1
>         shinfo(txflags=0 nr_frags=0 gso(size=0 type=0 segs=0))
>         csum(0x0 start=0 offset=0 ip_summed=0 complete_sw=0 valid=0 level=0)
>         hash(0x0 sw=0 l4=0) proto=0x0000 pkttype=0 iif=0
>         priority=0x0 mark=0x0 alloc_cpu=0 vlan_all=0x0
>         encapsulation=0 inner(proto=0x0000, mac=0, net=0, trans=0)
> kernel: dev name=prp0 feat=0x0000000000007000
> kernel: sk family=17 type=3 proto=0
> kernel: skb headroom: 00000000: 74 00
> kernel: skb linear:   00000000: 01 0c cd 01 00 01 00 d0 93 53 9c cb 81 00 80 00
> kernel: skb linear:   00000010: 88 b8 00 01 00 98 00 00 00 00 61 81 8d 80 16 52
> kernel: skb linear:   00000020: 45 47 44 4e 43 54 52 4c 2f 4c 4c 4e 30 24 47 4f
> kernel: skb linear:   00000030: 24 47 6f 43 62 81 01 14 82 16 52 45 47 44 4e 43
> kernel: skb linear:   00000040: 54 52 4c 2f 4c 4c 4e 30 24 44 73 47 6f 6f 73 65
> kernel: skb linear:   00000050: 83 07 47 6f 49 64 65 6e 74 84 08 67 8d f5 93 7e
> kernel: skb linear:   00000060: 76 c8 00 85 01 01 86 01 00 87 01 00 88 01 01 89
> kernel: skb linear:   00000070: 01 00 8a 01 02 ab 33 a2 15 83 01 00 84 03 03 00
> kernel: skb linear:   00000080: 00 91 08 67 8d f5 92 77 4b c6 1f 83 01 00 a2 1a
> kernel: skb linear:   00000090: a2 06 85 01 00 83 01 00 84 03 03 00 00 91 08 67
> kernel: skb linear:   000000a0: 8d f5 92 77 4b c6 1f 83 01 00
> kernel: skb tailroom: 00000000: 80 18 02 00 fe 4e 00 00 01 01 08 0a 4f fd 5e d1
> kernel: skb tailroom: 00000010: 4f fd 5e cd
> kernel: ------------[ cut here ]------------
> kernel: WARNING: CPU: 0 PID: 751 at /net/hsr/hsr_forward.c:605 fill_frame_info+0x180/0x19c
> kernel: Modules linked in:
> kernel: CPU: 0 PID: 751 Comm: reg61850 Not tainted 6.6.69-ga7a5cc0c39f0 #1
> kernel: Hardware name: Freescale LS1021A
> kernel:  unwind_backtrace from show_stack+0x10/0x14
> kernel:  show_stack from dump_stack_lvl+0x40/0x4c
> kernel:  dump_stack_lvl from __warn+0x94/0xc0
> kernel:  __warn from warn_slowpath_fmt+0x1b4/0x1bc
> kernel:  warn_slowpath_fmt from fill_frame_info+0x180/0x19c
> kernel:  fill_frame_info from hsr_forward_skb+0x54/0x118
> kernel:  hsr_forward_skb from hsr_dev_xmit+0x60/0xc4
> kernel:  hsr_dev_xmit from dev_hard_start_xmit+0xa0/0xe4
> kernel:  dev_hard_start_xmit from __dev_queue_xmit+0x144/0x5e8
> kernel:  __dev_queue_xmit from packet_snd+0x5c0/0x784
> kernel:  packet_snd from sock_write_iter+0xa0/0x10c
> kernel:  sock_write_iter from vfs_write+0x3ac/0x41c
> kernel:  vfs_write from ksys_write+0xbc/0xf0
> kernel:  ksys_write from ret_fast_syscall+0x0/0x4c
> kernel: Exception stack(0xc0d8dfa8 to 0xc0d8dff0)
> kernel: dfa0:                   000000aa 73058e53 00000012 73058e53 000000aa 00000000
> kernel: dfc0: 000000aa 73058e53 00000012 00000004 6ebf9940 0000000a 00000000 00000000
> kernel: dfe0: 00000004 6ebf90f8 766a17ad 7661e5e6
> kernel: ---[ end trace 0000000000000000 ]---

Thanks.

So hsr_forward() can be used in tx path, not forwarding as its name
would imply... what a mess.

It looks like it might be an af_packet issue, or an application bug.

packet_parse_headers() should really for this vlan packet set the
network header correctly.

Otherwise, I do not see how hsr could use mac_len at all.

I am afraid commit 48b491a5cc74333c ("net: hsr: fix mac_len checks")
was not good enough.

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

[Stephan Wurm]

Am 20. Jan 13:24 hat Eric Dumazet geschrieben:
> On Mon, Jan 20, 2025 at 8:32 AM Stephan Wurm <stephan.wurm@a-eberle.de> wrote:
> >
> > Applying the new instrumentation gives me the following stack trace:
> >
> > kernel: skb len=170 headroom=2 headlen=170 tailroom=20
> >         mac=(2,14) mac_len=14 net=(16,-1) trans=-1
> >         shinfo(txflags=0 nr_frags=0 gso(size=0 type=0 segs=0))
> >         csum(0x0 start=0 offset=0 ip_summed=0 complete_sw=0 valid=0 level=0)
> >         hash(0x0 sw=0 l4=0) proto=0x0000 pkttype=0 iif=0
> >         priority=0x0 mark=0x0 alloc_cpu=0 vlan_all=0x0
> >         encapsulation=0 inner(proto=0x0000, mac=0, net=0, trans=0)
> > kernel: dev name=prp0 feat=0x0000000000007000
> > kernel: sk family=17 type=3 proto=0
> > kernel: skb headroom: 00000000: 74 00
> > kernel: skb linear:   00000000: 01 0c cd 01 00 01 00 d0 93 53 9c cb 81 00 80 00
> > kernel: skb linear:   00000010: 88 b8 00 01 00 98 00 00 00 00 61 81 8d 80 16 52
> > kernel: skb linear:   00000020: 45 47 44 4e 43 54 52 4c 2f 4c 4c 4e 30 24 47 4f
> > kernel: skb linear:   00000030: 24 47 6f 43 62 81 01 14 82 16 52 45 47 44 4e 43
> > kernel: skb linear:   00000040: 54 52 4c 2f 4c 4c 4e 30 24 44 73 47 6f 6f 73 65
> > kernel: skb linear:   00000050: 83 07 47 6f 49 64 65 6e 74 84 08 67 8d f5 93 7e
> > kernel: skb linear:   00000060: 76 c8 00 85 01 01 86 01 00 87 01 00 88 01 01 89
> > kernel: skb linear:   00000070: 01 00 8a 01 02 ab 33 a2 15 83 01 00 84 03 03 00
> > kernel: skb linear:   00000080: 00 91 08 67 8d f5 92 77 4b c6 1f 83 01 00 a2 1a
> > kernel: skb linear:   00000090: a2 06 85 01 00 83 01 00 84 03 03 00 00 91 08 67
> > kernel: skb linear:   000000a0: 8d f5 92 77 4b c6 1f 83 01 00
> > kernel: skb tailroom: 00000000: 80 18 02 00 fe 4e 00 00 01 01 08 0a 4f fd 5e d1
> > kernel: skb tailroom: 00000010: 4f fd 5e cd
> > kernel: ------------[ cut here ]------------
> > kernel: WARNING: CPU: 0 PID: 751 at /net/hsr/hsr_forward.c:605 fill_frame_info+0x180/0x19c
> > kernel: Modules linked in:
> > kernel: CPU: 0 PID: 751 Comm: reg61850 Not tainted 6.6.69-ga7a5cc0c39f0 #1
> > kernel: Hardware name: Freescale LS1021A
> > kernel:  unwind_backtrace from show_stack+0x10/0x14
> > kernel:  show_stack from dump_stack_lvl+0x40/0x4c
> > kernel:  dump_stack_lvl from __warn+0x94/0xc0
> > kernel:  __warn from warn_slowpath_fmt+0x1b4/0x1bc
> > kernel:  warn_slowpath_fmt from fill_frame_info+0x180/0x19c
> > kernel:  fill_frame_info from hsr_forward_skb+0x54/0x118
> > kernel:  hsr_forward_skb from hsr_dev_xmit+0x60/0xc4
> > kernel:  hsr_dev_xmit from dev_hard_start_xmit+0xa0/0xe4
> > kernel:  dev_hard_start_xmit from __dev_queue_xmit+0x144/0x5e8
> > kernel:  __dev_queue_xmit from packet_snd+0x5c0/0x784
> > kernel:  packet_snd from sock_write_iter+0xa0/0x10c
> > kernel:  sock_write_iter from vfs_write+0x3ac/0x41c
> > kernel:  vfs_write from ksys_write+0xbc/0xf0
> > kernel:  ksys_write from ret_fast_syscall+0x0/0x4c
> > kernel: Exception stack(0xc0d8dfa8 to 0xc0d8dff0)
> > kernel: dfa0:                   000000aa 73058e53 00000012 73058e53 000000aa 00000000
> > kernel: dfc0: 000000aa 73058e53 00000012 00000004 6ebf9940 0000000a 00000000 00000000
> > kernel: dfe0: 00000004 6ebf90f8 766a17ad 7661e5e6
> > kernel: ---[ end trace 0000000000000000 ]---
>
> Thanks.
>
> So hsr_forward() can be used in tx path, not forwarding as its name
> would imply... what a mess.
>
> It looks like it might be an af_packet issue, or an application bug.
>
> packet_parse_headers() should really for this vlan packet set the
> network header correctly.
>
> Otherwise, I do not see how hsr could use mac_len at all.
>
> I am afraid commit 48b491a5cc74333c ("net: hsr: fix mac_len checks")
> was not good enough.

I did some additional experiments.

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.

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

[Eric Dumazet]

On Tue, Jan 21, 2025 at 4:15 PM Stephan Wurm <stephan.wurm@a-eberle.de> wrote:

> I did some additional experiments.
>
> 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..8942592130c151f2c948308e1ae16a6736822d5
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;
        }

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

[Stephan Wurm]

Am 21. Jan 16:35 hat Eric Dumazet geschrieben:
> On Tue, Jan 21, 2025 at 4:15 PM Stephan Wurm <stephan.wurm@a-eberle.de> wrote:
>
> > I did some additional experiments.
> e
> > 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..8942592130c151f2c948308e1ae16a6736822d5
> 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;
>         }

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;
 	}

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

[Eric Dumazet]

On Wed, Jan 22, 2025 at 11:27 AM Stephan Wurm <stephan.wurm@a-eberle.de> wrote:
>
> Am 21. Jan 16:35 hat Eric Dumazet geschrieben:
> > On Tue, Jan 21, 2025 at 4:15 PM Stephan Wurm <stephan.wurm@a-eberle.de> wrote:
> >
> > > I did some additional experiments.
> > e
> > > 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..8942592130c151f2c948308e1ae16a6736822d5
> > 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;
> >         }
>
> 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.

Yes, of course.

>
> 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;
>         }

Thanks, I am going to submit an official patch then.