netdev - Re: memory leak in unix_create1/copy_process/security_prepare

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6581fd3754b79_95e63208f@john.notmuch>
Date: Tue, 19 Dec 2023 12:29:43 -0800
From: John Fastabend <john.fastabend@...il.com>
To: John Fastabend <john.fastabend@...il.com>, 
 Kuniyuki Iwashima <kuniyu@...zon.com>, 
 xrivendell7@...il.com
Cc: alexander@...alicyn.com, 
 bpf@...r.kernel.org, 
 daan.j.demeyer@...il.com, 
 davem@...emloft.net, 
 dhowells@...hat.com, 
 edumazet@...gle.com, 
 john.fastabend@...il.com, 
 kuba@...nel.org, 
 kuniyu@...zon.com, 
 linux-kernel@...r.kernel.org, 
 netdev@...r.kernel.org, 
 pabeni@...hat.com
Subject: Re: memory leak in unix_create1/copy_process/security_prepare_creds

John Fastabend wrote:
> Kuniyuki Iwashima wrote:
> > From: xingwei lee <xrivendell7@...il.com>
> > Date: Tue, 19 Dec 2023 17:12:25 +0800
> > > Hello I found a bug in net/af_unix in the lastest upstream linux
> > > 6.7.rc5 and comfired in lastest net/net-next/bpf/bpf-next tree.
> > > Titled "TITLE: memory leak in unix_create1” and I also upload the
> > > repro.c and repro.txt.
> > > 
> > > If you fix this issue, please add the following tag to the commit:
> > > Reported-by: xingwei Lee <xrivendell7@...il.com>
> > 
> > Thanks for reporting!
> > 
> > It seems 8866730aed510 forgot to add sock_put().
> > I've confirmed that the diff below silenced kmemleak but will check
> > more before posting a patch.
> 
> Did it really silence the memleak?

Yes reverting the patch fixed the issue for me.

> 
> > 
> > ---8<---
> > diff --git a/net/unix/unix_bpf.c b/net/unix/unix_bpf.c
> > index 7ea7c3a0d0d0..32daba9e7f8b 100644
> > --- a/net/unix/unix_bpf.c
> > +++ b/net/unix/unix_bpf.c
> > @@ -164,6 +164,7 @@ int unix_stream_bpf_update_proto(struct sock *sk, struct sk_psock *psock, bool r
> >  	if (restore) {
> >  		sk->sk_write_space = psock->saved_write_space;
> >  		sock_replace_proto(sk, psock->sk_proto);
> > +		sock_put(psock->sk_pair);
> >  		return 0;
> 
> The reason the sock_put is not in this routine but in the sk_psock_destory
> is because we need to wait a RCU grace period for any pending queued
> BPF sends to also be flushed.

So we need a different fix here. I'll look into it.

> 
> >  	}
> >  
> > ---8<---
> > 
> > Thanks!
> > 
> > 
> 
> I'm also trying to understand how this adds up to
> unix_stream_bpf_update_proto() issue. The reproduce has a map_create
> followed by two map_delete() calls. I can't see how the unix socket
> ever got added to the BPF map and the deletes should be empty?
> 
> > > 
> > > lastest net tree: 979e90173af8d2f52f671d988189aab98c6d1be6
> > > Kernel config: https://syzkaller.appspot.com/text?tag=KernelConfig&x=8c4e4700f1727d30
> > > 
> > > in the lastest net tree, the crash like:
> > > Linux syzkaller 6.7.0-rc5-00172-g979e90173af8 #4 SMP PREEMPT_DYNAMIC
> > > Tue Dec 19 11:03:58 HKT 2023 x86_4
> > > 
> > > TITLE: memory leak in security_prepare_creds
> > >    [<ffffffff8129291a>] copy_process+0x6aa/0x25c0 kernel/fork.c:2366
> > >    [<ffffffff812949db>] kernel_clone+0x11b/0x690 kernel/fork.c:2907
> > >    [<ffffffff81294fcc>] __do_sys_clone+0x7c/0xb0 kernel/fork.c:3050
> > >    [<ffffffff84b70dcf>] do_syscall_x64 arch/x86/entry/common.c:52 [inline]
> > >    [<ffffffff84b70dcf>] do_syscall_64+0x3f/0x110 arch/x86/entry/common.c:83
> > >    [<ffffffff84c0008b>] entry_SYSCALL_64_after_hwframe+0x63/0x6b
> 
> ...
> 
> > > uint64_t r[1] = {0xffffffffffffffff};
> > > 
> > > void execute_one(void) {
> > >  intptr_t res = 0;
> > >  syscall(__NR_socketpair, /*domain=*/1ul, /*type=*/1ul, /*proto=*/0,
> > >          /*fds=*/0x20000000ul);
> > >  *(uint32_t*)0x200000c0 = 0x12;
> > >  *(uint32_t*)0x200000c4 = 2;
> > >  *(uint32_t*)0x200000c8 = 4;
> > >  *(uint32_t*)0x200000cc = 1;
> > >  *(uint32_t*)0x200000d0 = 0;
> > >  *(uint32_t*)0x200000d4 = -1;
> > >  *(uint32_t*)0x200000d8 = 0;
> 
> 
> > >  memset((void*)0x200000dc, 0, 16);
> > >  *(uint32_t*)0x200000ec = 0;
> > >  *(uint32_t*)0x200000f0 = -1;
> > >  *(uint32_t*)0x200000f4 = 0;
> > >  *(uint32_t*)0x200000f8 = 0;
> > >  *(uint32_t*)0x200000fc = 0;
> > >  *(uint64_t*)0x20000100 = 0;
> > >  res = syscall(__NR_bpf, /*cmd=*/0ul, /*arg=*/0x200000c0ul, /*size=*/0x48ul);
> 
> mapfd = map_create( bpf_attr { SOCKHASH, 1 entry, 0 flags, ...} )
> 
> > >  if (res != -1) r[0] = res;
> > >  *(uint32_t*)0x200003c0 = r[0];
> > >  *(uint64_t*)0x200003c8 = 0x20000040;
> > >  *(uint64_t*)0x200003d0 = 0x20000000;
> > >  *(uint64_t*)0x200003d8 = 0;
> > >  syscall(__NR_bpf, /*cmd=*/2ul, /*arg=*/0x200003c0ul, /*size=*/0x20ul);
> 
> map_delete(mapfd, key=0x20000040, value=0x20000000, flags = 0)
> 
> > >  *(uint32_t*)0x200003c0 = r[0];
> > >  *(uint64_t*)0x200003c8 = 0x20000040;
> > >  *(uint64_t*)0x200003d0 = 0x20000000;
> > >  *(uint64_t*)0x200003d8 = 0;
> > >  syscall(__NR_bpf, /*cmd=*/2ul, /*arg=*/0x200003c0ul, /*size=*/0x20ul);
> 
> map_delete(mapfd, key=0x20000040, value=0x20000000, flags = 0)

cmd=2 is update so this splat makes more sense.

> 
> so same as repro.txt below makes sense. But, if the sockets are
> never added to the sockhash then we never touched the proto from
> BPF side. And both of these deletes should return errors.
> 
> > > }
> > > int main(void) {
> > >  syscall(__NR_mmap, /*addr=*/0x1ffff000ul, /*len=*/0x1000ul, /*prot=*/0ul,
> > >          /*flags=*/0x32ul, /*fd=*/-1, /*offset=*/0ul);
> > >  syscall(__NR_mmap, /*addr=*/0x20000000ul, /*len=*/0x1000000ul, /*prot=*/7ul,
> > >          /*flags=*/0x32ul, /*fd=*/-1, /*offset=*/0ul);
> > >  syscall(__NR_mmap, /*addr=*/0x21000000ul, /*len=*/0x1000ul, /*prot=*/0ul,
> > >          /*flags=*/0x32ul, /*fd=*/-1, /*offset=*/0ul);
> > >  setup_leak();
> > >  loop();
> > >  return 0;
> > > }
> > > 
> > > 
> > > 
> > > =* repro.txt =*
> > > socketpair(0x1, 0x1, 0x0, &(0x7f0000000000))
> > > r0 = bpf$MAP_CREATE(0x0, &(0x7f00000000c0)=@...e={0x12, 0x2, 0x4, 0x1}, 0x48)
> > > bpf$MAP_DELETE_ELEM(0x2, &(0x7f00000003c0)={r0, &(0x7f0000000040),
> > > 0x20000000}, 0x20)
> > > bpf$MAP_DELETE_ELEM(0x2, &(0x7f00000003c0)={r0, &(0x7f0000000040),
> > > 0x20000000}, 0x20)
> 
> So not making sense to me how we got to blaming the proto delete
> logic here. It doesn't look like we ever added the psock and
> configured the proto?

Not sure why these is delete cmd=0x2.

#define ___BPF_FUNC_MAPPER(FN, ctx...)                  \
        FN(unspec, 0, ##ctx)                            \
        FN(map_lookup_elem, 1, ##ctx)                   \            
        FN(map_update_elem, 2, ##ctx)                   \


> 
> Did a bisect really blame the mentioned patch? I can likely try here
> as well.

Seems so.

> 
> Thanks,
> John