| Message ID | 20190104190000.91387-1-edumazet@google.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] ipv6: make icmp6_send() robust against null skb->dev | expand |
On 1/4/2019 11:00 AM, Eric Dumazet wrote: > syzbot was able to crash one host with the following stack trace : > > kasan: GPF could be caused by NULL-ptr deref or user memory access > general protection fault: 0000 [#1] PREEMPT SMP KASAN > CPU: 0 PID: 8625 Comm: syz-executor4 Not tainted 4.20.0+ #8 > RIP: 0010:dev_net include/linux/netdevice.h:2169 [inline] > RIP: 0010:icmp6_send+0x116/0x2d30 net/ipv6/icmp.c:426 > icmpv6_send > smack_socket_sock_rcv_skb > security_sock_rcv_skb > sk_filter_trim_cap > __sk_receive_skb > dccp_v6_do_rcv > release_sock > > This is because a RX packet found socket owned by user and > was stored into socket backlog. Before leaving RCU protected section, > skb->dev was cleared in __sk_receive_skb(). When socket backlog > was finally handled at release_sock() time, skb was fed to > smack_socket_sock_rcv_skb() then icmp6_send() > > We could fix the bug in smack_socket_sock_rcv_skb(), or simply > make icmp6_send() more robust against such possibility. The Smack patch would be a trivial check for skb->dev == NULL, in which case it wouldn't call icmp6_send(). Unless there's a timing issue, of course. If there are no known timing issues I would be happy to create a Smack patch to address this problem. Or, I'm happy with the patch below if you like it. > > In the future we might provide to icmp6_send() the net pointer > instead of infering it. > > Fixes: d66a8acbda92 ("Smack: Inform peer that IPv6 traffic has been blocked") > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Piotr Sawicki <p.sawicki2@partner.samsung.com> > Cc: Casey Schaufler <casey@schaufler-ca.com> > Reported-by: syzbot <syzkaller@googlegroups.com> > --- > net/ipv6/icmp.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c > index 5d7aa2c2770ca2b4981d2dd211c3cf0a79a6f9e2..bbcdfd2996926a78c3ea0b274adfa9b5f297efbc 100644 > --- a/net/ipv6/icmp.c > +++ b/net/ipv6/icmp.c > @@ -423,10 +423,10 @@ static int icmp6_iif(const struct sk_buff *skb) > static void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, > const struct in6_addr *force_saddr) > { > - struct net *net = dev_net(skb->dev); > struct inet6_dev *idev = NULL; > struct ipv6hdr *hdr = ipv6_hdr(skb); > struct sock *sk; > + struct net *net; > struct ipv6_pinfo *np; > const struct in6_addr *saddr = NULL; > struct dst_entry *dst; > @@ -437,12 +437,16 @@ static void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, > int iif = 0; > int addr_type = 0; > int len; > - u32 mark = IP6_REPLY_MARK(net, skb->mark); > + u32 mark; > > if ((u8 *)hdr < skb->head || > (skb_network_header(skb) + sizeof(*hdr)) > skb_tail_pointer(skb)) > return; > > + if (!skb->dev) > + return; > + net = dev_net(skb->dev); > + mark = IP6_REPLY_MARK(net, skb->mark); > /* > * Make sure we respect the rules > * i.e. RFC 1885 2.4(e)
On Fri, Jan 4, 2019 at 11:36 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > > On 1/4/2019 11:00 AM, Eric Dumazet wrote: > > syzbot was able to crash one host with the following stack trace : > > > > kasan: GPF could be caused by NULL-ptr deref or user memory access > > general protection fault: 0000 [#1] PREEMPT SMP KASAN > > CPU: 0 PID: 8625 Comm: syz-executor4 Not tainted 4.20.0+ #8 > > RIP: 0010:dev_net include/linux/netdevice.h:2169 [inline] > > RIP: 0010:icmp6_send+0x116/0x2d30 net/ipv6/icmp.c:426 > > icmpv6_send > > smack_socket_sock_rcv_skb > > security_sock_rcv_skb > > sk_filter_trim_cap > > __sk_receive_skb > > dccp_v6_do_rcv > > release_sock > > > > This is because a RX packet found socket owned by user and > > was stored into socket backlog. Before leaving RCU protected section, > > skb->dev was cleared in __sk_receive_skb(). When socket backlog > > was finally handled at release_sock() time, skb was fed to > > smack_socket_sock_rcv_skb() then icmp6_send() > > > > We could fix the bug in smack_socket_sock_rcv_skb(), or simply > > make icmp6_send() more robust against such possibility. > > The Smack patch would be a trivial check for skb->dev == NULL, > in which case it wouldn't call icmp6_send(). Unless there's a > timing issue, of course. If there are no known timing issues I > would be happy to create a Smack patch to address this problem. > > Or, I'm happy with the patch below if you like it. > Well, doing the check in icmp6_send() is more generic, this is the path I took, thanks ;) > > > > In the future we might provide to icmp6_send() the net pointer > > instead of infering it. > > > > Fixes: d66a8acbda92 ("Smack: Inform peer that IPv6 traffic has been blocked") > > Signed-off-by: Eric Dumazet <edumazet@google.com> > > Cc: Piotr Sawicki <p.sawicki2@partner.samsung.com> > > Cc: Casey Schaufler <casey@schaufler-ca.com> > > Reported-by: syzbot <syzkaller@googlegroups.com> > > --- > > net/ipv6/icmp.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c > > index 5d7aa2c2770ca2b4981d2dd211c3cf0a79a6f9e2..bbcdfd2996926a78c3ea0b274adfa9b5f297efbc 100644 > > --- a/net/ipv6/icmp.c > > +++ b/net/ipv6/icmp.c > > @@ -423,10 +423,10 @@ static int icmp6_iif(const struct sk_buff *skb) > > static void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, > > const struct in6_addr *force_saddr) > > { > > - struct net *net = dev_net(skb->dev); > > struct inet6_dev *idev = NULL; > > struct ipv6hdr *hdr = ipv6_hdr(skb); > > struct sock *sk; > > + struct net *net; > > struct ipv6_pinfo *np; > > const struct in6_addr *saddr = NULL; > > struct dst_entry *dst; > > @@ -437,12 +437,16 @@ static void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, > > int iif = 0; > > int addr_type = 0; > > int len; > > - u32 mark = IP6_REPLY_MARK(net, skb->mark); > > + u32 mark; > > > > if ((u8 *)hdr < skb->head || > > (skb_network_header(skb) + sizeof(*hdr)) > skb_tail_pointer(skb)) > > return; > > > > + if (!skb->dev) > > + return; > > + net = dev_net(skb->dev); > > + mark = IP6_REPLY_MARK(net, skb->mark); > > /* > > * Make sure we respect the rules > > * i.e. RFC 1885 2.4(e) >
On 1/4/2019 11:38 AM, Eric Dumazet wrote: > On Fri, Jan 4, 2019 at 11:36 AM Casey Schaufler <casey@schaufler-ca.com> wrote: >> On 1/4/2019 11:00 AM, Eric Dumazet wrote: >>> syzbot was able to crash one host with the following stack trace : >>> >>> kasan: GPF could be caused by NULL-ptr deref or user memory access >>> general protection fault: 0000 [#1] PREEMPT SMP KASAN >>> CPU: 0 PID: 8625 Comm: syz-executor4 Not tainted 4.20.0+ #8 >>> RIP: 0010:dev_net include/linux/netdevice.h:2169 [inline] >>> RIP: 0010:icmp6_send+0x116/0x2d30 net/ipv6/icmp.c:426 >>> icmpv6_send >>> smack_socket_sock_rcv_skb >>> security_sock_rcv_skb >>> sk_filter_trim_cap >>> __sk_receive_skb >>> dccp_v6_do_rcv >>> release_sock >>> >>> This is because a RX packet found socket owned by user and >>> was stored into socket backlog. Before leaving RCU protected section, >>> skb->dev was cleared in __sk_receive_skb(). When socket backlog >>> was finally handled at release_sock() time, skb was fed to >>> smack_socket_sock_rcv_skb() then icmp6_send() >>> >>> We could fix the bug in smack_socket_sock_rcv_skb(), or simply >>> make icmp6_send() more robust against such possibility. >> The Smack patch would be a trivial check for skb->dev == NULL, >> in which case it wouldn't call icmp6_send(). Unless there's a >> timing issue, of course. If there are no known timing issues I >> would be happy to create a Smack patch to address this problem. >> >> Or, I'm happy with the patch below if you like it. >> > Well, doing the check in icmp6_send() is more generic, this is the path I took, > thanks ;) > >>> In the future we might provide to icmp6_send() the net pointer >>> instead of infering it. >>> >>> Fixes: d66a8acbda92 ("Smack: Inform peer that IPv6 traffic has been blocked") >>> Signed-off-by: Eric Dumazet <edumazet@google.com> >>> Cc: Piotr Sawicki <p.sawicki2@partner.samsung.com> >>> Cc: Casey Schaufler <casey@schaufler-ca.com> >>> Reported-by: syzbot <syzkaller@googlegroups.com> OK, you can add my Acked-by: Casey Schaufler <casey@schaufler-ca.com> >>> --- >>> net/ipv6/icmp.c | 8 ++++++-- >>> 1 file changed, 6 insertions(+), 2 deletions(-) >>> >>> diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c >>> index 5d7aa2c2770ca2b4981d2dd211c3cf0a79a6f9e2..bbcdfd2996926a78c3ea0b274adfa9b5f297efbc 100644 >>> --- a/net/ipv6/icmp.c >>> +++ b/net/ipv6/icmp.c >>> @@ -423,10 +423,10 @@ static int icmp6_iif(const struct sk_buff *skb) >>> static void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, >>> const struct in6_addr *force_saddr) >>> { >>> - struct net *net = dev_net(skb->dev); >>> struct inet6_dev *idev = NULL; >>> struct ipv6hdr *hdr = ipv6_hdr(skb); >>> struct sock *sk; >>> + struct net *net; >>> struct ipv6_pinfo *np; >>> const struct in6_addr *saddr = NULL; >>> struct dst_entry *dst; >>> @@ -437,12 +437,16 @@ static void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, >>> int iif = 0; >>> int addr_type = 0; >>> int len; >>> - u32 mark = IP6_REPLY_MARK(net, skb->mark); >>> + u32 mark; >>> >>> if ((u8 *)hdr < skb->head || >>> (skb_network_header(skb) + sizeof(*hdr)) > skb_tail_pointer(skb)) >>> return; >>> >>> + if (!skb->dev) >>> + return; >>> + net = dev_net(skb->dev); >>> + mark = IP6_REPLY_MARK(net, skb->mark); >>> /* >>> * Make sure we respect the rules >>> * i.e. RFC 1885 2.4(e)
From: Eric Dumazet <edumazet@google.com> Date: Fri, 4 Jan 2019 11:00:00 -0800 > syzbot was able to crash one host with the following stack trace : ... > This is because a RX packet found socket owned by user and > was stored into socket backlog. Before leaving RCU protected section, > skb->dev was cleared in __sk_receive_skb(). When socket backlog > was finally handled at release_sock() time, skb was fed to > smack_socket_sock_rcv_skb() then icmp6_send() > > We could fix the bug in smack_socket_sock_rcv_skb(), or simply > make icmp6_send() more robust against such possibility. > > In the future we might provide to icmp6_send() the net pointer > instead of infering it. > > Fixes: d66a8acbda92 ("Smack: Inform peer that IPv6 traffic has been blocked") > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Piotr Sawicki <p.sawicki2@partner.samsung.com> > Cc: Casey Schaufler <casey@schaufler-ca.com> > Reported-by: syzbot <syzkaller@googlegroups.com> Applied and queued up for -stable, thanks.
dccp_v6_rcv() calls __sk_receive_skb() which calls sk_filter_trim_cap(). sk_filter_trim_cap() should return a value not equal to 0 and cause the skb to be dropped, since icmpv6_send() is called when smack_socket_sock_rcv_skb() returns -EACCES. So, the packet shouldn't be put into the backlog queue. How did it get there? Regards, Piotr On 1/4/19 8:00 PM, Eric Dumazet wrote: > syzbot was able to crash one host with the following stack trace : > > kasan: GPF could be caused by NULL-ptr deref or user memory access > general protection fault: 0000 [#1] PREEMPT SMP KASAN > CPU: 0 PID: 8625 Comm: syz-executor4 Not tainted 4.20.0+ #8 > RIP: 0010:dev_net include/linux/netdevice.h:2169 [inline] > RIP: 0010:icmp6_send+0x116/0x2d30 net/ipv6/icmp.c:426 > icmpv6_send > smack_socket_sock_rcv_skb > security_sock_rcv_skb > sk_filter_trim_cap > __sk_receive_skb > dccp_v6_do_rcv > release_sock > > This is because a RX packet found socket owned by user and > was stored into socket backlog. Before leaving RCU protected section, > skb->dev was cleared in __sk_receive_skb(). When socket backlog > was finally handled at release_sock() time, skb was fed to > smack_socket_sock_rcv_skb() then icmp6_send() > > We could fix the bug in smack_socket_sock_rcv_skb(), or simply > make icmp6_send() more robust against such possibility. > > In the future we might provide to icmp6_send() the net pointer > instead of infering it. > > Fixes: d66a8acbda92 ("Smack: Inform peer that IPv6 traffic has been blocked") > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Piotr Sawicki <p.sawicki2@partner.samsung.com> > Cc: Casey Schaufler <casey@schaufler-ca.com> > Reported-by: syzbot <syzkaller@googlegroups.com> > --- > net/ipv6/icmp.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c > index 5d7aa2c2770ca2b4981d2dd211c3cf0a79a6f9e2..bbcdfd2996926a78c3ea0b274adfa9b5f297efbc 100644 > --- a/net/ipv6/icmp.c > +++ b/net/ipv6/icmp.c > @@ -423,10 +423,10 @@ static int icmp6_iif(const struct sk_buff *skb) > static void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, > const struct in6_addr *force_saddr) > { > - struct net *net = dev_net(skb->dev); > struct inet6_dev *idev = NULL; > struct ipv6hdr *hdr = ipv6_hdr(skb); > struct sock *sk; > + struct net *net; > struct ipv6_pinfo *np; > const struct in6_addr *saddr = NULL; > struct dst_entry *dst; > @@ -437,12 +437,16 @@ static void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, > int iif = 0; > int addr_type = 0; > int len; > - u32 mark = IP6_REPLY_MARK(net, skb->mark); > + u32 mark; > > if ((u8 *)hdr < skb->head || > (skb_network_header(skb) + sizeof(*hdr)) > skb_tail_pointer(skb)) > return; > > + if (!skb->dev) > + return; > + net = dev_net(skb->dev); > + mark = IP6_REPLY_MARK(net, skb->mark); > /* > * Make sure we respect the rules > * i.e. RFC 1885 2.4(e)
On Tue, Jan 8, 2019 at 12:57 AM Piotr Sawicki <p.sawicki2@partner.samsung.com> wrote: > > dccp_v6_rcv() calls __sk_receive_skb() which calls sk_filter_trim_cap(). > > sk_filter_trim_cap() should return a value not equal to 0 and cause the skb to be dropped, since icmpv6_send() is called when smack_socket_sock_rcv_skb() returns -EACCES. > > So, the packet shouldn't be put into the backlog queue. > > How did it get there? > I do not believe crash involved a BPF filter at all (My changelog said nothing about sk_filter_trim_cap()) After packet is queued to backlog, the packet circulates, reaching the smack_socket_sock_rcv_skb() point. The stack trace shows only the 2nd phase of the packet, when the user process calls release_sock() > > Regards, > > Piotr > > > On 1/4/19 8:00 PM, Eric Dumazet wrote: > > syzbot was able to crash one host with the following stack trace : > > > > kasan: GPF could be caused by NULL-ptr deref or user memory access > > general protection fault: 0000 [#1] PREEMPT SMP KASAN > > CPU: 0 PID: 8625 Comm: syz-executor4 Not tainted 4.20.0+ #8 > > RIP: 0010:dev_net include/linux/netdevice.h:2169 [inline] > > RIP: 0010:icmp6_send+0x116/0x2d30 net/ipv6/icmp.c:426 > > icmpv6_send > > smack_socket_sock_rcv_skb > > security_sock_rcv_skb > > sk_filter_trim_cap > > __sk_receive_skb > > dccp_v6_do_rcv > > release_sock > > > > This is because a RX packet found socket owned by user and > > was stored into socket backlog. Before leaving RCU protected section, > > skb->dev was cleared in __sk_receive_skb(). When socket backlog > > was finally handled at release_sock() time, skb was fed to > > smack_socket_sock_rcv_skb() then icmp6_send() > > > > We could fix the bug in smack_socket_sock_rcv_skb(), or simply > > make icmp6_send() more robust against such possibility. > > > > In the future we might provide to icmp6_send() the net pointer > > instead of infering it. > > > > Fixes: d66a8acbda92 ("Smack: Inform peer that IPv6 traffic has been blocked") > > Signed-off-by: Eric Dumazet <edumazet@google.com> > > Cc: Piotr Sawicki <p.sawicki2@partner.samsung.com> > > Cc: Casey Schaufler <casey@schaufler-ca.com> > > Reported-by: syzbot <syzkaller@googlegroups.com> > > --- > > net/ipv6/icmp.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c > > index 5d7aa2c2770ca2b4981d2dd211c3cf0a79a6f9e2..bbcdfd2996926a78c3ea0b274adfa9b5f297efbc 100644 > > --- a/net/ipv6/icmp.c > > +++ b/net/ipv6/icmp.c > > @@ -423,10 +423,10 @@ static int icmp6_iif(const struct sk_buff *skb) > > static void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, > > const struct in6_addr *force_saddr) > > { > > - struct net *net = dev_net(skb->dev); > > struct inet6_dev *idev = NULL; > > struct ipv6hdr *hdr = ipv6_hdr(skb); > > struct sock *sk; > > + struct net *net; > > struct ipv6_pinfo *np; > > const struct in6_addr *saddr = NULL; > > struct dst_entry *dst; > > @@ -437,12 +437,16 @@ static void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, > > int iif = 0; > > int addr_type = 0; > > int len; > > - u32 mark = IP6_REPLY_MARK(net, skb->mark); > > + u32 mark; > > > > if ((u8 *)hdr < skb->head || > > (skb_network_header(skb) + sizeof(*hdr)) > skb_tail_pointer(skb)) > > return; > > > > + if (!skb->dev) > > + return; > > + net = dev_net(skb->dev); > > + mark = IP6_REPLY_MARK(net, skb->mark); > > /* > > * Make sure we respect the rules > > * i.e. RFC 1885 2.4(e)
On 1/8/19 10:21 AM, Eric Dumazet wrote: > On Tue, Jan 8, 2019 at 12:57 AM Piotr Sawicki > <p.sawicki2@partner.samsung.com> wrote: >> dccp_v6_rcv() calls __sk_receive_skb() which calls sk_filter_trim_cap(). >> >> sk_filter_trim_cap() should return a value not equal to 0 and cause the skb to be dropped, since icmpv6_send() is called when smack_socket_sock_rcv_skb() returns -EACCES. >> >> So, the packet shouldn't be put into the backlog queue. >> >> How did it get there? >> > I do not believe crash involved a BPF filter at all (My changelog said > nothing about sk_filter_trim_cap() Not only BPF but also the LSM subsystem is involved (in this case Smack). dccp_v6_rcv() __sk_receive_skb() sk_filter_trim_cap() security_sock_rcv_skb() smack_sock_rcv_skb() So, before putting this skb into the backlog queue, a network packet is checked against Smack rules. If Smack denies access, the packet is discarded. __sk_receive_skb() ... if (sk_filter_trim_cap <https://elixir.bootlin.com/linux/latest/ident/sk_filter_trim_cap>(sk, skb, trim_cap)) goto discard_and_relse; ... > After packet is queued to backlog, the packet circulates, reaching the > smack_socket_sock_rcv_skb() point. > > The stack trace shows only the 2nd phase of the packet, when the user > process calls release_sock() > > >> Regards, >> >> Piotr >> >> >> On 1/4/19 8:00 PM, Eric Dumazet wrote: >>> syzbot was able to crash one host with the following stack trace : >>> >>> kasan: GPF could be caused by NULL-ptr deref or user memory access >>> general protection fault: 0000 [#1] PREEMPT SMP KASAN >>> CPU: 0 PID: 8625 Comm: syz-executor4 Not tainted 4.20.0+ #8 >>> RIP: 0010:dev_net include/linux/netdevice.h:2169 [inline] >>> RIP: 0010:icmp6_send+0x116/0x2d30 net/ipv6/icmp.c:426 >>> icmpv6_send >>> smack_socket_sock_rcv_skb >>> security_sock_rcv_skb >>> sk_filter_trim_cap >>> __sk_receive_skb >>> dccp_v6_do_rcv >>> release_sock >>> >>> This is because a RX packet found socket owned by user and >>> was stored into socket backlog. Before leaving RCU protected section, >>> skb->dev was cleared in __sk_receive_skb(). When socket backlog >>> was finally handled at release_sock() time, skb was fed to >>> smack_socket_sock_rcv_skb() then icmp6_send() >>> >>> We could fix the bug in smack_socket_sock_rcv_skb(), or simply >>> make icmp6_send() more robust against such possibility. >>> >>> In the future we might provide to icmp6_send() the net pointer >>> instead of infering it. >>> >>> Fixes: d66a8acbda92 ("Smack: Inform peer that IPv6 traffic has been blocked") >>> Signed-off-by: Eric Dumazet <edumazet@google.com> >>> Cc: Piotr Sawicki <p.sawicki2@partner.samsung.com> >>> Cc: Casey Schaufler <casey@schaufler-ca.com> >>> Reported-by: syzbot <syzkaller@googlegroups.com> >>> --- >>> net/ipv6/icmp.c | 8 ++++++-- >>> 1 file changed, 6 insertions(+), 2 deletions(-) >>> >>> diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c >>> index 5d7aa2c2770ca2b4981d2dd211c3cf0a79a6f9e2..bbcdfd2996926a78c3ea0b274adfa9b5f297efbc 100644 >>> --- a/net/ipv6/icmp.c >>> +++ b/net/ipv6/icmp.c >>> @@ -423,10 +423,10 @@ static int icmp6_iif(const struct sk_buff *skb) >>> static void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, >>> const struct in6_addr *force_saddr) >>> { >>> - struct net *net = dev_net(skb->dev); >>> struct inet6_dev *idev = NULL; >>> struct ipv6hdr *hdr = ipv6_hdr(skb); >>> struct sock *sk; >>> + struct net *net; >>> struct ipv6_pinfo *np; >>> const struct in6_addr *saddr = NULL; >>> struct dst_entry *dst; >>> @@ -437,12 +437,16 @@ static void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, >>> int iif = 0; >>> int addr_type = 0; >>> int len; >>> - u32 mark = IP6_REPLY_MARK(net, skb->mark); >>> + u32 mark; >>> >>> if ((u8 *)hdr < skb->head || >>> (skb_network_header(skb) + sizeof(*hdr)) > skb_tail_pointer(skb)) >>> return; >>> >>> + if (!skb->dev) >>> + return; >>> + net = dev_net(skb->dev); >>> + mark = IP6_REPLY_MARK(net, skb->mark); >>> /* >>> * Make sure we respect the rules >>> * i.e. RFC 1885 2.4(e) >
On Tue, Jan 8, 2019 at 1:47 AM Piotr Sawicki <p.sawicki2@partner.samsung.com> wrote: > > > On 1/8/19 10:21 AM, Eric Dumazet wrote: > > On Tue, Jan 8, 2019 at 12:57 AM Piotr Sawicki > > <p.sawicki2@partner.samsung.com> wrote: > >> dccp_v6_rcv() calls __sk_receive_skb() which calls sk_filter_trim_cap(). > >> > >> sk_filter_trim_cap() should return a value not equal to 0 and cause the skb to be dropped, since icmpv6_send() is called when smack_socket_sock_rcv_skb() returns -EACCES. > >> > >> So, the packet shouldn't be put into the backlog queue. > >> > >> How did it get there? > >> > > I do not believe crash involved a BPF filter at all (My changelog said > > nothing about sk_filter_trim_cap() > > Not only BPF but also the LSM subsystem is involved (in this case Smack). > > dccp_v6_rcv() > __sk_receive_skb() > > sk_filter_trim_cap() > security_sock_rcv_skb() > smack_sock_rcv_skb() > > So, before putting this skb into the backlog queue, > > a network packet is checked against Smack rules. If Smack denies access, > > the packet is discarded. > > __sk_receive_skb() > ... > if (sk_filter_trim_cap <https://elixir.bootlin.com/linux/latest/ident/sk_filter_trim_cap>(sk, skb, trim_cap)) > goto discard_and_relse; ... > Crash did not involve sk_filter_trim_cap() here... Not sure what you are trying to say. > > After packet is queued to backlog, the packet circulates, reaching the > > smack_socket_sock_rcv_skb() point. > > > > The stack trace shows only the 2nd phase of the packet, when the user > > process calls release_sock() > > > > > >> Regards, > >> > >> Piotr > >> > >> > >> On 1/4/19 8:00 PM, Eric Dumazet wrote: > >>> syzbot was able to crash one host with the following stack trace : > >>> > >>> kasan: GPF could be caused by NULL-ptr deref or user memory access > >>> general protection fault: 0000 [#1] PREEMPT SMP KASAN > >>> CPU: 0 PID: 8625 Comm: syz-executor4 Not tainted 4.20.0+ #8 > >>> RIP: 0010:dev_net include/linux/netdevice.h:2169 [inline] > >>> RIP: 0010:icmp6_send+0x116/0x2d30 net/ipv6/icmp.c:426 > >>> icmpv6_send > >>> smack_socket_sock_rcv_skb > >>> security_sock_rcv_skb > >>> sk_filter_trim_cap > >>> __sk_receive_skb > >>> dccp_v6_do_rcv > >>> release_sock > >>> > >>> This is because a RX packet found socket owned by user and > >>> was stored into socket backlog. Before leaving RCU protected section, > >>> skb->dev was cleared in __sk_receive_skb(). When socket backlog > >>> was finally handled at release_sock() time, skb was fed to > >>> smack_socket_sock_rcv_skb() then icmp6_send() > >>> > >>> We could fix the bug in smack_socket_sock_rcv_skb(), or simply > >>> make icmp6_send() more robust against such possibility. > >>> > >>> In the future we might provide to icmp6_send() the net pointer > >>> instead of infering it. > >>> > >>> Fixes: d66a8acbda92 ("Smack: Inform peer that IPv6 traffic has been blocked") > >>> Signed-off-by: Eric Dumazet <edumazet@google.com> > >>> Cc: Piotr Sawicki <p.sawicki2@partner.samsung.com> > >>> Cc: Casey Schaufler <casey@schaufler-ca.com> > >>> Reported-by: syzbot <syzkaller@googlegroups.com> > >>> --- > >>> net/ipv6/icmp.c | 8 ++++++-- > >>> 1 file changed, 6 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c > >>> index 5d7aa2c2770ca2b4981d2dd211c3cf0a79a6f9e2..bbcdfd2996926a78c3ea0b274adfa9b5f297efbc 100644 > >>> --- a/net/ipv6/icmp.c > >>> +++ b/net/ipv6/icmp.c > >>> @@ -423,10 +423,10 @@ static int icmp6_iif(const struct sk_buff *skb) > >>> static void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, > >>> const struct in6_addr *force_saddr) > >>> { > >>> - struct net *net = dev_net(skb->dev); > >>> struct inet6_dev *idev = NULL; > >>> struct ipv6hdr *hdr = ipv6_hdr(skb); > >>> struct sock *sk; > >>> + struct net *net; > >>> struct ipv6_pinfo *np; > >>> const struct in6_addr *saddr = NULL; > >>> struct dst_entry *dst; > >>> @@ -437,12 +437,16 @@ static void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, > >>> int iif = 0; > >>> int addr_type = 0; > >>> int len; > >>> - u32 mark = IP6_REPLY_MARK(net, skb->mark); > >>> + u32 mark; > >>> > >>> if ((u8 *)hdr < skb->head || > >>> (skb_network_header(skb) + sizeof(*hdr)) > skb_tail_pointer(skb)) > >>> return; > >>> > >>> + if (!skb->dev) > >>> + return; > >>> + net = dev_net(skb->dev); > >>> + mark = IP6_REPLY_MARK(net, skb->mark); > >>> /* > >>> * Make sure we respect the rules > >>> * i.e. RFC 1885 2.4(e) > >
On 1/8/19 11:06 AM, Eric Dumazet wrote: > On Tue, Jan 8, 2019 at 1:47 AM Piotr Sawicki > <p.sawicki2@partner.samsung.com> wrote: >> >> On 1/8/19 10:21 AM, Eric Dumazet wrote: >>> On Tue, Jan 8, 2019 at 12:57 AM Piotr Sawicki >>> <p.sawicki2@partner.samsung.com> wrote: >>>> dccp_v6_rcv() calls __sk_receive_skb() which calls sk_filter_trim_cap(). >>>> >>>> sk_filter_trim_cap() should return a value not equal to 0 and cause the skb to be dropped, since icmpv6_send() is called when smack_socket_sock_rcv_skb() returns -EACCES. >>>> >>>> So, the packet shouldn't be put into the backlog queue. >>>> >>>> How did it get there? >>>> >>> I do not believe crash involved a BPF filter at all (My changelog said >>> nothing about sk_filter_trim_cap() >> Not only BPF but also the LSM subsystem is involved (in this case Smack). >> >> dccp_v6_rcv() >> __sk_receive_skb() >> >> sk_filter_trim_cap() >> security_sock_rcv_skb() >> smack_sock_rcv_skb() >> >> So, before putting this skb into the backlog queue, >> >> a network packet is checked against Smack rules. If Smack denies access, >> >> the packet is discarded. >> >> __sk_receive_skb() >> ... >> if (sk_filter_trim_cap <https://elixir.bootlin.com/linux/latest/ident/sk_filter_trim_cap>(sk, skb, trim_cap)) >> goto discard_and_relse; ... >> > Crash did not involve sk_filter_trim_cap() here... > > Not sure what you are trying to say. Are you sure that sk_filter_trim_cap() is not on the stack trace? Maybe I'm missing something. How should I read the below dump? kasan: GPF could be caused by NULL-ptr deref or user memory access general protection fault: 0000 [#1] PREEMPT SMP KASAN CPU: 0 PID: 8625 Comm: syz-executor4 Not tainted 4.20.0+ #8 RIP: 0010:dev_net include/linux/netdevice.h:2169 [inline] RIP: 0010:icmp6_send+0x116/0x2d30 net/ipv6/icmp.c:426 icmpv6_send smack_socket_sock_rcv_skb security_sock_rcv_skb sk_filter_trim_cap <---- here __sk_receive_skb dccp_v6_do_rcv release_sock >>> After packet is queued to backlog, the packet circulates, reaching the >>> smack_socket_sock_rcv_skb() point. >>> >>> The stack trace shows only the 2nd phase of the packet, when the user >>> process calls release_sock() >>> >>> >>>> Regards, >>>> >>>> Piotr >>>> >>>> >>>> On 1/4/19 8:00 PM, Eric Dumazet wrote: >>>>> syzbot was able to crash one host with the following stack trace : >>>>> >>>>> kasan: GPF could be caused by NULL-ptr deref or user memory access >>>>> general protection fault: 0000 [#1] PREEMPT SMP KASAN >>>>> CPU: 0 PID: 8625 Comm: syz-executor4 Not tainted 4.20.0+ #8 >>>>> RIP: 0010:dev_net include/linux/netdevice.h:2169 [inline] >>>>> RIP: 0010:icmp6_send+0x116/0x2d30 net/ipv6/icmp.c:426 >>>>> icmpv6_send >>>>> smack_socket_sock_rcv_skb >>>>> security_sock_rcv_skb >>>>> sk_filter_trim_cap >>>>> __sk_receive_skb >>>>> dccp_v6_do_rcv >>>>> release_sock >>>>> >>>>> This is because a RX packet found socket owned by user and >>>>> was stored into socket backlog. Before leaving RCU protected section, >>>>> skb->dev was cleared in __sk_receive_skb(). When socket backlog >>>>> was finally handled at release_sock() time, skb was fed to >>>>> smack_socket_sock_rcv_skb() then icmp6_send() >>>>> >>>>> We could fix the bug in smack_socket_sock_rcv_skb(), or simply >>>>> make icmp6_send() more robust against such possibility. >>>>> >>>>> In the future we might provide to icmp6_send() the net pointer >>>>> instead of infering it. >>>>> >>>>> Fixes: d66a8acbda92 ("Smack: Inform peer that IPv6 traffic has been blocked") >>>>> Signed-off-by: Eric Dumazet <edumazet@google.com> >>>>> Cc: Piotr Sawicki <p.sawicki2@partner.samsung.com> >>>>> Cc: Casey Schaufler <casey@schaufler-ca.com> >>>>> Reported-by: syzbot <syzkaller@googlegroups.com> >>>>> --- >>>>> net/ipv6/icmp.c | 8 ++++++-- >>>>> 1 file changed, 6 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c >>>>> index 5d7aa2c2770ca2b4981d2dd211c3cf0a79a6f9e2..bbcdfd2996926a78c3ea0b274adfa9b5f297efbc 100644 >>>>> --- a/net/ipv6/icmp.c >>>>> +++ b/net/ipv6/icmp.c >>>>> @@ -423,10 +423,10 @@ static int icmp6_iif(const struct sk_buff *skb) >>>>> static void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, >>>>> const struct in6_addr *force_saddr) >>>>> { >>>>> - struct net *net = dev_net(skb->dev); >>>>> struct inet6_dev *idev = NULL; >>>>> struct ipv6hdr *hdr = ipv6_hdr(skb); >>>>> struct sock *sk; >>>>> + struct net *net; >>>>> struct ipv6_pinfo *np; >>>>> const struct in6_addr *saddr = NULL; >>>>> struct dst_entry *dst; >>>>> @@ -437,12 +437,16 @@ static void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, >>>>> int iif = 0; >>>>> int addr_type = 0; >>>>> int len; >>>>> - u32 mark = IP6_REPLY_MARK(net, skb->mark); >>>>> + u32 mark; >>>>> >>>>> if ((u8 *)hdr < skb->head || >>>>> (skb_network_header(skb) + sizeof(*hdr)) > skb_tail_pointer(skb)) >>>>> return; >>>>> >>>>> + if (!skb->dev) >>>>> + return; >>>>> + net = dev_net(skb->dev); >>>>> + mark = IP6_REPLY_MARK(net, skb->mark); >>>>> /* >>>>> * Make sure we respect the rules >>>>> * i.e. RFC 1885 2.4(e) >
On Tue, Jan 8, 2019 at 2:36 AM Piotr Sawicki <p.sawicki2@partner.samsung.com> wrote: > > > On 1/8/19 11:06 AM, Eric Dumazet wrote: > > On Tue, Jan 8, 2019 at 1:47 AM Piotr Sawicki > > <p.sawicki2@partner.samsung.com> wrote: > >> > >> On 1/8/19 10:21 AM, Eric Dumazet wrote: > >>> On Tue, Jan 8, 2019 at 12:57 AM Piotr Sawicki > >>> <p.sawicki2@partner.samsung.com> wrote: > >>>> dccp_v6_rcv() calls __sk_receive_skb() which calls sk_filter_trim_cap(). > >>>> > >>>> sk_filter_trim_cap() should return a value not equal to 0 and cause the skb to be dropped, since icmpv6_send() is called when smack_socket_sock_rcv_skb() returns -EACCES. > >>>> > >>>> So, the packet shouldn't be put into the backlog queue. > >>>> > >>>> How did it get there? > >>>> > >>> I do not believe crash involved a BPF filter at all (My changelog said > >>> nothing about sk_filter_trim_cap() > >> Not only BPF but also the LSM subsystem is involved (in this case Smack). > >> > >> dccp_v6_rcv() > >> __sk_receive_skb() > >> > >> sk_filter_trim_cap() > >> security_sock_rcv_skb() > >> smack_sock_rcv_skb() > >> > >> So, before putting this skb into the backlog queue, > >> > >> a network packet is checked against Smack rules. If Smack denies access, > >> > >> the packet is discarded. > >> > >> __sk_receive_skb() > >> ... > >> if (sk_filter_trim_cap <https://elixir.bootlin.com/linux/latest/ident/sk_filter_trim_cap>(sk, skb, trim_cap)) > >> goto discard_and_relse; ... > >> > > Crash did not involve sk_filter_trim_cap() here... > > > > Not sure what you are trying to say. > > > Are you sure that sk_filter_trim_cap() is not on the stack trace? > > Maybe I'm missing something. How should I read the below dump? > Crash happens from smack_socket_sock_rcv_skb() calling icmpv6_send() You missed the fact that this stuff can be called twice per packet. First invocation might have allowed the packet being queued (into the backlog) Anything can happen when user owns the socket lock, including changing security parameters/filters. > kasan: GPF could be caused by NULL-ptr deref or user memory access > general protection fault: 0000 [#1] PREEMPT SMP KASAN > CPU: 0 PID: 8625 Comm: syz-executor4 Not tainted 4.20.0+ #8 > RIP: 0010:dev_net include/linux/netdevice.h:2169 [inline] > RIP: 0010:icmp6_send+0x116/0x2d30 net/ipv6/icmp.c:426 > icmpv6_send > smack_socket_sock_rcv_skb > security_sock_rcv_skb > sk_filter_trim_cap <---- here > __sk_receive_skb > dccp_v6_do_rcv > release_sock > > > >>> After packet is queued to backlog, the packet circulates, reaching the > >>> smack_socket_sock_rcv_skb() point. > >>> > >>> The stack trace shows only the 2nd phase of the packet, when the user > >>> process calls release_sock() > >>> > >>> > >>>> Regards, > >>>> > >>>> Piotr > >>>> > >>>> > >>>> On 1/4/19 8:00 PM, Eric Dumazet wrote: > >>>>> syzbot was able to crash one host with the following stack trace : > >>>>> > >>>>> kasan: GPF could be caused by NULL-ptr deref or user memory access > >>>>> general protection fault: 0000 [#1] PREEMPT SMP KASAN > >>>>> CPU: 0 PID: 8625 Comm: syz-executor4 Not tainted 4.20.0+ #8 > >>>>> RIP: 0010:dev_net include/linux/netdevice.h:2169 [inline] > >>>>> RIP: 0010:icmp6_send+0x116/0x2d30 net/ipv6/icmp.c:426 > >>>>> icmpv6_send > >>>>> smack_socket_sock_rcv_skb > >>>>> security_sock_rcv_skb > >>>>> sk_filter_trim_cap > >>>>> __sk_receive_skb > >>>>> dccp_v6_do_rcv > >>>>> release_sock > >>>>> > >>>>> This is because a RX packet found socket owned by user and > >>>>> was stored into socket backlog. Before leaving RCU protected section, > >>>>> skb->dev was cleared in __sk_receive_skb(). When socket backlog > >>>>> was finally handled at release_sock() time, skb was fed to > >>>>> smack_socket_sock_rcv_skb() then icmp6_send() > >>>>> > >>>>> We could fix the bug in smack_socket_sock_rcv_skb(), or simply > >>>>> make icmp6_send() more robust against such possibility. > >>>>> > >>>>> In the future we might provide to icmp6_send() the net pointer > >>>>> instead of infering it. > >>>>> > >>>>> Fixes: d66a8acbda92 ("Smack: Inform peer that IPv6 traffic has been blocked") > >>>>> Signed-off-by: Eric Dumazet <edumazet@google.com> > >>>>> Cc: Piotr Sawicki <p.sawicki2@partner.samsung.com> > >>>>> Cc: Casey Schaufler <casey@schaufler-ca.com> > >>>>> Reported-by: syzbot <syzkaller@googlegroups.com> > >>>>> --- > >>>>> net/ipv6/icmp.c | 8 ++++++-- > >>>>> 1 file changed, 6 insertions(+), 2 deletions(-) > >>>>> > >>>>> diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c > >>>>> index 5d7aa2c2770ca2b4981d2dd211c3cf0a79a6f9e2..bbcdfd2996926a78c3ea0b274adfa9b5f297efbc 100644 > >>>>> --- a/net/ipv6/icmp.c > >>>>> +++ b/net/ipv6/icmp.c > >>>>> @@ -423,10 +423,10 @@ static int icmp6_iif(const struct sk_buff *skb) > >>>>> static void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, > >>>>> const struct in6_addr *force_saddr) > >>>>> { > >>>>> - struct net *net = dev_net(skb->dev); > >>>>> struct inet6_dev *idev = NULL; > >>>>> struct ipv6hdr *hdr = ipv6_hdr(skb); > >>>>> struct sock *sk; > >>>>> + struct net *net; > >>>>> struct ipv6_pinfo *np; > >>>>> const struct in6_addr *saddr = NULL; > >>>>> struct dst_entry *dst; > >>>>> @@ -437,12 +437,16 @@ static void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, > >>>>> int iif = 0; > >>>>> int addr_type = 0; > >>>>> int len; > >>>>> - u32 mark = IP6_REPLY_MARK(net, skb->mark); > >>>>> + u32 mark; > >>>>> > >>>>> if ((u8 *)hdr < skb->head || > >>>>> (skb_network_header(skb) + sizeof(*hdr)) > skb_tail_pointer(skb)) > >>>>> return; > >>>>> > >>>>> + if (!skb->dev) > >>>>> + return; > >>>>> + net = dev_net(skb->dev); > >>>>> + mark = IP6_REPLY_MARK(net, skb->mark); > >>>>> /* > >>>>> * Make sure we respect the rules > >>>>> * i.e. RFC 1885 2.4(e) > >
On 1/8/19 11:48 AM, Eric Dumazet wrote: > On Tue, Jan 8, 2019 at 2:36 AM Piotr Sawicki > <p.sawicki2@partner.samsung.com> wrote: >> >> On 1/8/19 11:06 AM, Eric Dumazet wrote: >>> On Tue, Jan 8, 2019 at 1:47 AM Piotr Sawicki >>> <p.sawicki2@partner.samsung.com> wrote: >>>> On 1/8/19 10:21 AM, Eric Dumazet wrote: >>>>> On Tue, Jan 8, 2019 at 12:57 AM Piotr Sawicki >>>>> <p.sawicki2@partner.samsung.com> wrote: >>>>>> dccp_v6_rcv() calls __sk_receive_skb() which calls sk_filter_trim_cap(). >>>>>> >>>>>> sk_filter_trim_cap() should return a value not equal to 0 and cause the skb to be dropped, since icmpv6_send() is called when smack_socket_sock_rcv_skb() returns -EACCES. >>>>>> >>>>>> So, the packet shouldn't be put into the backlog queue. >>>>>> >>>>>> How did it get there? >>>>>> >>>>> I do not believe crash involved a BPF filter at all (My changelog said >>>>> nothing about sk_filter_trim_cap() >>>> Not only BPF but also the LSM subsystem is involved (in this case Smack). >>>> >>>> dccp_v6_rcv() >>>> __sk_receive_skb() >>>> >>>> sk_filter_trim_cap() >>>> security_sock_rcv_skb() >>>> smack_sock_rcv_skb() >>>> >>>> So, before putting this skb into the backlog queue, >>>> >>>> a network packet is checked against Smack rules. If Smack denies access, >>>> >>>> the packet is discarded. >>>> >>>> __sk_receive_skb() >>>> ... >>>> if (sk_filter_trim_cap <https://elixir.bootlin.com/linux/latest/ident/sk_filter_trim_cap>(sk, skb, trim_cap)) >>>> goto discard_and_relse; ... >>>> >>> Crash did not involve sk_filter_trim_cap() here... >>> >>> Not sure what you are trying to say. >> >> Are you sure that sk_filter_trim_cap() is not on the stack trace? >> >> Maybe I'm missing something. How should I read the below dump? >> > Crash happens from smack_socket_sock_rcv_skb() calling icmpv6_send() > > You missed the fact that this stuff can be called twice per packet. > > First invocation might have allowed the packet being queued (into the backlog) > > Anything can happen when user owns the socket lock, including changing > security parameters/filters. Yes I know. It looks like the Smack's security rule was changed during this process. Firstly the packet was allowed to be received and it was put into the backlog queue. Then, the rule was changed, and during the release phase LSM was called again for the same packet. But this time, Smack denied access and tried to send an ICMPv6 packet to inform a peer. I want to make sure if it is the root cause of this problem. Besides, what is the purpose of setting skb->dev to NULL in __sk_receive_skb() ? >> kasan: GPF could be caused by NULL-ptr deref or user memory access >> general protection fault: 0000 [#1] PREEMPT SMP KASAN >> CPU: 0 PID: 8625 Comm: syz-executor4 Not tainted 4.20.0+ #8 >> RIP: 0010:dev_net include/linux/netdevice.h:2169 [inline] >> RIP: 0010:icmp6_send+0x116/0x2d30 net/ipv6/icmp.c:426 >> icmpv6_send >> smack_socket_sock_rcv_skb >> security_sock_rcv_skb >> sk_filter_trim_cap <---- here >> __sk_receive_skb >> dccp_v6_do_rcv >> release_sock >> >> >>>>> After packet is queued to backlog, the packet circulates, reaching the >>>>> smack_socket_sock_rcv_skb() point. >>>>> >>>>> The stack trace shows only the 2nd phase of the packet, when the user >>>>> process calls release_sock() >>>>> >>>>> >>>>>> Regards, >>>>>> >>>>>> Piotr >>>>>> >>>>>> >>>>>> On 1/4/19 8:00 PM, Eric Dumazet wrote: >>>>>>> syzbot was able to crash one host with the following stack trace : >>>>>>> >>>>>>> kasan: GPF could be caused by NULL-ptr deref or user memory access >>>>>>> general protection fault: 0000 [#1] PREEMPT SMP KASAN >>>>>>> CPU: 0 PID: 8625 Comm: syz-executor4 Not tainted 4.20.0+ #8 >>>>>>> RIP: 0010:dev_net include/linux/netdevice.h:2169 [inline] >>>>>>> RIP: 0010:icmp6_send+0x116/0x2d30 net/ipv6/icmp.c:426 >>>>>>> icmpv6_send >>>>>>> smack_socket_sock_rcv_skb >>>>>>> security_sock_rcv_skb >>>>>>> sk_filter_trim_cap >>>>>>> __sk_receive_skb >>>>>>> dccp_v6_do_rcv >>>>>>> release_sock >>>>>>> >>>>>>> This is because a RX packet found socket owned by user and >>>>>>> was stored into socket backlog. Before leaving RCU protected section, >>>>>>> skb->dev was cleared in __sk_receive_skb(). When socket backlog >>>>>>> was finally handled at release_sock() time, skb was fed to >>>>>>> smack_socket_sock_rcv_skb() then icmp6_send() >>>>>>> >>>>>>> We could fix the bug in smack_socket_sock_rcv_skb(), or simply >>>>>>> make icmp6_send() more robust against such possibility. >>>>>>> >>>>>>> In the future we might provide to icmp6_send() the net pointer >>>>>>> instead of infering it. >>>>>>> >>>>>>> Fixes: d66a8acbda92 ("Smack: Inform peer that IPv6 traffic has been blocked") >>>>>>> Signed-off-by: Eric Dumazet <edumazet@google.com> >>>>>>> Cc: Piotr Sawicki <p.sawicki2@partner.samsung.com> >>>>>>> Cc: Casey Schaufler <casey@schaufler-ca.com> >>>>>>> Reported-by: syzbot <syzkaller@googlegroups.com> >>>>>>> --- >>>>>>> net/ipv6/icmp.c | 8 ++++++-- >>>>>>> 1 file changed, 6 insertions(+), 2 deletions(-) >>>>>>> >>>>>>> diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c >>>>>>> index 5d7aa2c2770ca2b4981d2dd211c3cf0a79a6f9e2..bbcdfd2996926a78c3ea0b274adfa9b5f297efbc 100644 >>>>>>> --- a/net/ipv6/icmp.c >>>>>>> +++ b/net/ipv6/icmp.c >>>>>>> @@ -423,10 +423,10 @@ static int icmp6_iif(const struct sk_buff *skb) >>>>>>> static void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, >>>>>>> const struct in6_addr *force_saddr) >>>>>>> { >>>>>>> - struct net *net = dev_net(skb->dev); >>>>>>> struct inet6_dev *idev = NULL; >>>>>>> struct ipv6hdr *hdr = ipv6_hdr(skb); >>>>>>> struct sock *sk; >>>>>>> + struct net *net; >>>>>>> struct ipv6_pinfo *np; >>>>>>> const struct in6_addr *saddr = NULL; >>>>>>> struct dst_entry *dst; >>>>>>> @@ -437,12 +437,16 @@ static void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, >>>>>>> int iif = 0; >>>>>>> int addr_type = 0; >>>>>>> int len; >>>>>>> - u32 mark = IP6_REPLY_MARK(net, skb->mark); >>>>>>> + u32 mark; >>>>>>> >>>>>>> if ((u8 *)hdr < skb->head || >>>>>>> (skb_network_header(skb) + sizeof(*hdr)) > skb_tail_pointer(skb)) >>>>>>> return; >>>>>>> >>>>>>> + if (!skb->dev) >>>>>>> + return; >>>>>>> + net = dev_net(skb->dev); >>>>>>> + mark = IP6_REPLY_MARK(net, skb->mark); >>>>>>> /* >>>>>>> * Make sure we respect the rules >>>>>>> * i.e. RFC 1885 2.4(e) >
On Tue, Jan 8, 2019 at 3:08 AM Piotr Sawicki <p.sawicki2@partner.samsung.com> wrote: > Yes I know. It looks like the Smack's security rule was changed during this process. > > Firstly the packet was allowed to be received and it was put into the backlog queue. Then, the > > rule was changed, and during the release phase LSM was called again for the same packet. > > But this time, Smack denied access and tried to send an ICMPv6 packet to inform a peer. > > I want to make sure if it is the root cause of this problem. > > > Besides, what is the purpose of setting skb->dev to NULL in __sk_receive_skb() ? We can not keep a pointer to the device, the device might be dismantled/freed before socket backlog can be processed. Input processing is using RCU, meaning no refcount is taken on the device.
On 1/8/19 12:14 PM, Eric Dumazet wrote: > On Tue, Jan 8, 2019 at 3:08 AM Piotr Sawicki > <p.sawicki2@partner.samsung.com> wrote: > >> Yes I know. It looks like the Smack's security rule was changed during this process. >> >> Firstly the packet was allowed to be received and it was put into the backlog queue. Then, the >> >> rule was changed, and during the release phase LSM was called again for the same packet. >> >> But this time, Smack denied access and tried to send an ICMPv6 packet to inform a peer. >> >> I want to make sure if it is the root cause of this problem. >> >> >> Besides, what is the purpose of setting skb->dev to NULL in __sk_receive_skb() ? > > We can not keep a pointer to the device, the device might be > dismantled/freed before socket backlog can be processed. > > Input processing is using RCU, meaning no refcount is taken on the device. Thanks for explanation.
diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c index 5d7aa2c2770ca2b4981d2dd211c3cf0a79a6f9e2..bbcdfd2996926a78c3ea0b274adfa9b5f297efbc 100644 --- a/net/ipv6/icmp.c +++ b/net/ipv6/icmp.c @@ -423,10 +423,10 @@ static int icmp6_iif(const struct sk_buff *skb) static void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, const struct in6_addr *force_saddr) { - struct net *net = dev_net(skb->dev); struct inet6_dev *idev = NULL; struct ipv6hdr *hdr = ipv6_hdr(skb); struct sock *sk; + struct net *net; struct ipv6_pinfo *np; const struct in6_addr *saddr = NULL; struct dst_entry *dst; @@ -437,12 +437,16 @@ static void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, int iif = 0; int addr_type = 0; int len; - u32 mark = IP6_REPLY_MARK(net, skb->mark); + u32 mark; if ((u8 *)hdr < skb->head || (skb_network_header(skb) + sizeof(*hdr)) > skb_tail_pointer(skb)) return; + if (!skb->dev) + return; + net = dev_net(skb->dev); + mark = IP6_REPLY_MARK(net, skb->mark); /* * Make sure we respect the rules * i.e. RFC 1885 2.4(e)
syzbot was able to crash one host with the following stack trace : kasan: GPF could be caused by NULL-ptr deref or user memory access general protection fault: 0000 [#1] PREEMPT SMP KASAN CPU: 0 PID: 8625 Comm: syz-executor4 Not tainted 4.20.0+ #8 RIP: 0010:dev_net include/linux/netdevice.h:2169 [inline] RIP: 0010:icmp6_send+0x116/0x2d30 net/ipv6/icmp.c:426 icmpv6_send smack_socket_sock_rcv_skb security_sock_rcv_skb sk_filter_trim_cap __sk_receive_skb dccp_v6_do_rcv release_sock This is because a RX packet found socket owned by user and was stored into socket backlog. Before leaving RCU protected section, skb->dev was cleared in __sk_receive_skb(). When socket backlog was finally handled at release_sock() time, skb was fed to smack_socket_sock_rcv_skb() then icmp6_send() We could fix the bug in smack_socket_sock_rcv_skb(), or simply make icmp6_send() more robust against such possibility. In the future we might provide to icmp6_send() the net pointer instead of infering it. Fixes: d66a8acbda92 ("Smack: Inform peer that IPv6 traffic has been blocked") Signed-off-by: Eric Dumazet <edumazet@google.com> Cc: Piotr Sawicki <p.sawicki2@partner.samsung.com> Cc: Casey Schaufler <casey@schaufler-ca.com> Reported-by: syzbot <syzkaller@googlegroups.com> --- net/ipv6/icmp.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)