Re: buggy check in netlink_mmap_sendmsg()

[Patrick McHardy <kaber@trash.net>]

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.