netdev - Re: [PATCH net-next] nfnetlink_queue: enable PID info retrieval

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]

Date:	Fri, 10 Jun 2016 01:22:16 +0200
From:	Daniel Borkmann <daniel@...earbox.net>
To:	Florian Westphal <fw@...len.de>,
	Saeed Mahameed <saeedm@...lanox.com>
CC:	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
	netfilter-devel@...r.kernel.org,
	Yevgeny Petrilin <yevgenyp@...lanox.com>,
	Andre Melkoumian <andre@...lanox.com>,
	Matthew Finlay <matt@...lanox.com>,
	Pablo Neira Ayuso <pablo@...filter.org>,
	Patrick McHardy <kaber@...sh.net>,
	Jozsef Kadlecsik <kadlec@...ckhole.kfki.hu>,
	john.fastabend@...il.com, daniel.wagner@...-carit.de,
	hannes@...essinduktion.org, tj@...nel.org, viro@...iv.linux.org.uk
Subject: Re: [PATCH net-next] nfnetlink_queue: enable PID info retrieval

On 06/10/2016 12:21 AM, Daniel Borkmann wrote:
> On 06/09/2016 11:35 PM, Florian Westphal wrote:
>> Saeed Mahameed <saeedm@...lanox.com> wrote:
>>> index a1bd161..67de200 100644
>>> --- a/net/socket.c
>>> +++ b/net/socket.c
>>> @@ -382,6 +382,7 @@ struct file *sock_alloc_file(struct socket *sock, int flags, const char *dname)
>>>       }
>>>
>>>       sock->file = file;
>>> +    file->f_owner.sock_pid  = find_get_pid(task_pid_nr(current));
>>>       file->f_flags = O_RDWR | (flags & O_NONBLOCK);
>>>       file->private_data = sock;
>>>       return file;
>>
>> This looks like this leaks sock_pid reference...?
>>
>> (find_get_pid -> get_pid -> atomic_inc() , I don't see a put_pid in the
>>   patch)
>>
>> Can't comment further than this since I'm not familiar with vfs; e.g.
>> I can't say if fown_struct is right place or not, or if this approach
>> even works when creating process has exited after fork, etc.
>
> Or ... if you xmit the fd via unix domain socket to a different process
> and initial owner terminates, which should give you invalid information
> then; afaik, this would just increase struct file's refcnt and hand out
> an unused fdnum ( get_unused_fd_flags() + fd_install(), etc).
> For extending 'struct fown_struct', you probably also need to Cc fs folks.

[ Cc'ing John, Daniel, et al ]

Btw, while I just looked at scm_detach_fds(), I think commits ...

  * 48a87cc26c13 ("net: netprio: fd passed in SCM_RIGHTS datagram not set correctly")
  * d84295067fc7 ("net: net_cls: fd passed in SCM_RIGHTS datagram not set correctly")

... might not be correct, maybe I'm missing something ...? Lets say process A
has a socket fd that it sends via SCM_RIGHTS to process B. Process A was the
one that called sk_alloc() originally. Now in scm_detach_fds() we install a new
fd for process B pointing to the same sock (file's private_data) and above commits
update the cached socket cgroup data for net_cls/net_prio to the new process B.
So, if process A for example still sends data over that socket, skbs will then
wrongly match on B's cgroup membership instead of A's, no?

Thanks,
Daniel