* [PATCH net] ipv6: make icmp6_send() robust against null skb->dev
@ 2019-01-04 19:00 ` Eric Dumazet
2019-01-04 19:36 ` Casey Schaufler
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Eric Dumazet @ 2019-01-04 19:00 UTC (permalink / raw)
To: David S . Miller
Cc: netdev, Eric Dumazet, Eric Dumazet, Piotr Sawicki,
Casey Schaufler, syzbot
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)
--
2.20.1.97.g81188d93c3-goog
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH net] ipv6: make icmp6_send() robust against null skb->dev
2019-01-04 19:00 ` [PATCH net] ipv6: make icmp6_send() robust against null skb->dev Eric Dumazet
@ 2019-01-04 19:36 ` Casey Schaufler
2019-01-04 19:38 ` Eric Dumazet
2019-01-04 21:40 ` David Miller
2019-01-08 8:57 ` Piotr Sawicki
2 siblings, 1 reply; 14+ messages in thread
From: Casey Schaufler @ 2019-01-04 19:36 UTC (permalink / raw)
To: Eric Dumazet, David S . Miller
Cc: netdev, Eric Dumazet, Piotr Sawicki, syzbot
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)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] ipv6: make icmp6_send() robust against null skb->dev
2019-01-04 19:36 ` Casey Schaufler
@ 2019-01-04 19:38 ` Eric Dumazet
2019-01-04 19:48 ` Casey Schaufler
0 siblings, 1 reply; 14+ messages in thread
From: Eric Dumazet @ 2019-01-04 19:38 UTC (permalink / raw)
To: Casey Schaufler; +Cc: David Miller, netdev, Eric Dumazet, p.sawicki2, syzbot
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)
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] ipv6: make icmp6_send() robust against null skb->dev
2019-01-04 19:38 ` Eric Dumazet
@ 2019-01-04 19:48 ` Casey Schaufler
0 siblings, 0 replies; 14+ messages in thread
From: Casey Schaufler @ 2019-01-04 19:48 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, netdev, Eric Dumazet, p.sawicki2, syzbot, Casey Schaufler
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)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] ipv6: make icmp6_send() robust against null skb->dev
2019-01-04 19:00 ` [PATCH net] ipv6: make icmp6_send() robust against null skb->dev Eric Dumazet
2019-01-04 19:36 ` Casey Schaufler
@ 2019-01-04 21:40 ` David Miller
2019-01-08 8:57 ` Piotr Sawicki
2 siblings, 0 replies; 14+ messages in thread
From: David Miller @ 2019-01-04 21:40 UTC (permalink / raw)
To: edumazet; +Cc: netdev, eric.dumazet, p.sawicki2, casey, syzkaller
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] ipv6: make icmp6_send() robust against null skb->dev
2019-01-04 19:00 ` [PATCH net] ipv6: make icmp6_send() robust against null skb->dev Eric Dumazet
2019-01-04 19:36 ` Casey Schaufler
2019-01-04 21:40 ` David Miller
@ 2019-01-08 8:57 ` Piotr Sawicki
2019-01-08 9:21 ` Eric Dumazet
2 siblings, 1 reply; 14+ messages in thread
From: Piotr Sawicki @ 2019-01-08 8:57 UTC (permalink / raw)
To: Eric Dumazet, David S . Miller
Cc: netdev, Eric Dumazet, Casey Schaufler, syzbot
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)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] ipv6: make icmp6_send() robust against null skb->dev
2019-01-08 8:57 ` Piotr Sawicki
@ 2019-01-08 9:21 ` Eric Dumazet
2019-01-08 9:46 ` Piotr Sawicki
0 siblings, 1 reply; 14+ messages in thread
From: Eric Dumazet @ 2019-01-08 9:21 UTC (permalink / raw)
To: p.sawicki2; +Cc: David Miller, netdev, Eric Dumazet, Casey Schaufler, syzbot
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)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] ipv6: make icmp6_send() robust against null skb->dev
2019-01-08 9:21 ` Eric Dumazet
@ 2019-01-08 9:46 ` Piotr Sawicki
2019-01-08 10:06 ` Eric Dumazet
0 siblings, 1 reply; 14+ messages in thread
From: Piotr Sawicki @ 2019-01-08 9:46 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, Eric Dumazet, Casey Schaufler, syzbot
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)
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] ipv6: make icmp6_send() robust against null skb->dev
2019-01-08 9:46 ` Piotr Sawicki
@ 2019-01-08 10:06 ` Eric Dumazet
2019-01-08 10:36 ` Piotr Sawicki
0 siblings, 1 reply; 14+ messages in thread
From: Eric Dumazet @ 2019-01-08 10:06 UTC (permalink / raw)
To: p.sawicki2; +Cc: David Miller, netdev, Eric Dumazet, Casey Schaufler, syzbot
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)
> >
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] ipv6: make icmp6_send() robust against null skb->dev
2019-01-08 10:06 ` Eric Dumazet
@ 2019-01-08 10:36 ` Piotr Sawicki
2019-01-08 10:48 ` Eric Dumazet
0 siblings, 1 reply; 14+ messages in thread
From: Piotr Sawicki @ 2019-01-08 10:36 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, Eric Dumazet, Casey Schaufler, syzbot
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)
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] ipv6: make icmp6_send() robust against null skb->dev
2019-01-08 10:36 ` Piotr Sawicki
@ 2019-01-08 10:48 ` Eric Dumazet
2019-01-08 11:08 ` Piotr Sawicki
0 siblings, 1 reply; 14+ messages in thread
From: Eric Dumazet @ 2019-01-08 10:48 UTC (permalink / raw)
To: p.sawicki2; +Cc: David Miller, netdev, Eric Dumazet, Casey Schaufler, syzbot
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)
> >
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] ipv6: make icmp6_send() robust against null skb->dev
2019-01-08 10:48 ` Eric Dumazet
@ 2019-01-08 11:08 ` Piotr Sawicki
2019-01-08 11:14 ` Eric Dumazet
0 siblings, 1 reply; 14+ messages in thread
From: Piotr Sawicki @ 2019-01-08 11:08 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, Eric Dumazet, Casey Schaufler, syzbot
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)
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] ipv6: make icmp6_send() robust against null skb->dev
2019-01-08 11:08 ` Piotr Sawicki
@ 2019-01-08 11:14 ` Eric Dumazet
2019-01-08 11:41 ` Piotr Sawicki
0 siblings, 1 reply; 14+ messages in thread
From: Eric Dumazet @ 2019-01-08 11:14 UTC (permalink / raw)
To: p.sawicki2; +Cc: David Miller, netdev, Eric Dumazet, Casey Schaufler, syzbot
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net] ipv6: make icmp6_send() robust against null skb->dev
2019-01-08 11:14 ` Eric Dumazet
@ 2019-01-08 11:41 ` Piotr Sawicki
0 siblings, 0 replies; 14+ messages in thread
From: Piotr Sawicki @ 2019-01-08 11:41 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, Eric Dumazet, Casey Schaufler, syzbot
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-01-08 11:41 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20190104190052epcas5p31f95a311e681146143408f99ffea78ac@epcas5p3.samsung.com>
2019-01-04 19:00 ` [PATCH net] ipv6: make icmp6_send() robust against null skb->dev Eric Dumazet
2019-01-04 19:36 ` Casey Schaufler
2019-01-04 19:38 ` Eric Dumazet
2019-01-04 19:48 ` Casey Schaufler
2019-01-04 21:40 ` David Miller
2019-01-08 8:57 ` Piotr Sawicki
2019-01-08 9:21 ` Eric Dumazet
2019-01-08 9:46 ` Piotr Sawicki
2019-01-08 10:06 ` Eric Dumazet
2019-01-08 10:36 ` Piotr Sawicki
2019-01-08 10:48 ` Eric Dumazet
2019-01-08 11:08 ` Piotr Sawicki
2019-01-08 11:14 ` Eric Dumazet
2019-01-08 11:41 ` Piotr Sawicki
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.