buggy check in netlink_mmap_sendmsg()

buggy check in netlink_mmap_sendmsg()

[Al Viro]

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.
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...

Re: buggy check in netlink_mmap_sendmsg()

[Patrick McHardy]

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.

Re: buggy check in netlink_mmap_sendmsg()

[Patrick McHardy]

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.

Re: buggy check in netlink_mmap_sendmsg()

[Al Viro]

On Fri, Jul 19, 2013 at 05:38:46PM +0200, Patrick McHardy wrote:

> >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

See the check for CLONE_VM in copy_mm(); we do not call dup_mm() (which would
be calling dup_mmap()) in that case.

Re: buggy check in netlink_mmap_sendmsg()

[Al Viro]

On Fri, Jul 19, 2013 at 05:38:46PM +0200, Patrick McHardy wrote:
> 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.

Sorry, no.  Thread A: calls sendmsg(), gets past the check, loses CPU1 on
e.g. preempt.  Thread B on CPU2 calls mmap(2), returns to userland (having
dropped the mutex, of course) and proceeds to play with the mapped area.
Thread A regains CPU and moves on past that if (....), excl still being true.
See the problem?