From mboxrd@z Thu Jan 1 00:00:00 1970 From: Patrick McHardy Subject: Re: buggy check in netlink_mmap_sendmsg() Date: Fri, 19 Jul 2013 17:38:46 +0200 Message-ID: <20130719153846.GB14764@macbook.localnet> References: <20130714093619.GH4165@ZenIV.linux.org.uk> <20130718111358.GA27488@macbook.localnet> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: netdev@vger.kernel.org To: Al Viro Return-path: Received: from stinky.trash.net ([213.144.137.162]:59873 "EHLO stinky.trash.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752615Ab3GSPit (ORCPT ); Fri, 19 Jul 2013 11:38:49 -0400 Content-Disposition: inline In-Reply-To: <20130718111358.GA27488@macbook.localnet> Sender: netdev-owner@vger.kernel.org List-ID: 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.