netdev - Re: buggy check in netlink_mmap

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, 19 Jul 2013 17:38:46 +0200
From:	Patrick McHardy <kaber@...sh.net>
To:	Al Viro <viro@...IV.linux.org.uk>
Cc:	netdev@...r.kernel.org
Subject: Re: buggy check in netlink_mmap_sendmsg()

On Thu, Jul 18, 2013 at 01:13:58PM +0200, Patrick McHardy wrote:
> On Sun, Jul 14, 2013 at 10:36:19AM +0100, Al Viro wrote:
> > This
> >         /* Netlink messages are validated by the receiver before processing.
> >          * In order to avoid userspace changing the contents of the message
> >          * after validation, the socket and the ring may only be used by a
> >          * single process, otherwise we fall back to copying.
> >          */
> >         if (atomic_long_read(&sk->sk_socket->file->f_count) > 2 ||  
> >             atomic_read(&nlk->mapped) > 1)
> >                 excl = false;
> > looks very odd.  For one thing, descriptor table may be shared, with
> > one thread calling sendmsg() (which gives f_count equal to 2), while
> > another calls mmap() just as the first one gets past that check.
> 
> Another thread calling mmap() should be fine since validation, processing
> and mmap() all happen under the pg_vec_lock mutex.
> 
> > Moreover, we might very well have the damn thing mmapped, then clone(2)
> > creating another thread that shares address space, but not the descriptor
> > table.  Child closes the socket descriptor it got, then parent does
> > sendmsg(2) (f_count == 2, again, since this time descriptor table isn't
> > shared and sendmsg(2) doesn't grab a reference and we have 1 from descriptor
> > table and 1 from mapping).  Again, the child has it mapped and can play
> > with it as it wishes...
> 
> This is unfortunately not my area of expertise. Let me look into how we
> can prevent this.

>From what I can tell, the second check should catch the second case you
describe. If the address space is shared, dup_mmap() will invoke
netlink_mmap_ops->open, so nlk->mmaped will be > 1. Basically the intention
was to have the nlk->mmaped check catch all cases of shared address spaces,
and have the f_count check prevent socket descriptor passing using
AF_UNIX between unrelated processes.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html