| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 8 Jan 2019 02:48:25 -0800
From: Eric Dumazet <edumazet@...gle.com>
To: p.sawicki2@...tner.samsung.com
Cc: David Miller <davem@...emloft.net>,
netdev <netdev@...r.kernel.org>,
Eric Dumazet <eric.dumazet@...il.com>,
Casey Schaufler <casey@...aufler-ca.com>,
syzbot <syzkaller@...glegroups.com>
Subject: Re: [PATCH net] ipv6: make icmp6_send() robust against null skb->dev
On Tue, Jan 8, 2019 at 2:36 AM Piotr Sawicki
<p.sawicki2@...tner.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@...tner.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@...tner.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@...gle.com>
> >>>>> Cc: Piotr Sawicki <p.sawicki2@...tner.samsung.com>
> >>>>> Cc: Casey Schaufler <casey@...aufler-ca.com>
> >>>>> Reported-by: syzbot <syzkaller@...glegroups.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)
> >
Powered by blists - more mailing lists