On Tue, Jan 8, 2019 at 2:36 AM Piotr Sawicki
<p.sawic...@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.sawic...@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.sawic...@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 <eduma...@google.com>
> >>>>> Cc: Piotr Sawicki <p.sawic...@partner.samsung.com>
> >>>>> Cc: Casey Schaufler <ca...@schaufler-ca.com>
> >>>>> Reported-by: syzbot <syzkal...@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)
> >
