Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934631AbdC3PcU (ORCPT ); Thu, 30 Mar 2017 11:32:20 -0400 Received: from zeniv.linux.org.uk ([195.92.253.2]:47518 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933959AbdC3PcR (ORCPT ); Thu, 30 Mar 2017 11:32:17 -0400 Date: Thu, 30 Mar 2017 16:31:59 +0100 From: Al Viro To: Linus Torvalds Cc: Vineet Gupta , "linux-arch@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Richard Henderson , Russell King , Will Deacon , Haavard Skinnemoen , Steven Miao , Jesper Nilsson , Mark Salter , Yoshinori Sato , Richard Kuo , Tony Luck , Geert Uytterhoeven , James Hogan , Michal Simek , David Howells , Ley Foon Tan , Jonas Bonn , Helge Deller , Martin Schwidefsky , Ralf Baechle , Benjamin Herrenschmidt , Chen Liqin , "David S. Miller" , Chris Metcalf , Richard Weinberger , Guan Xuetao , Thomas Gleixner , Chris Zankel Subject: Re: [RFC][CFT][PATCHSET v1] uaccess unification Message-ID: <20170330153159.GP29622@ZenIV.linux.org.uk> References: <20170329055706.GH29622@ZenIV.linux.org.uk> <3399faa9-795e-39db-42f5-7d1e10bbff9c@synopsys.com> <20170329202939.GI29622@ZenIV.linux.org.uk> <20170329210322.GJ29622@ZenIV.linux.org.uk> <20170329230934.GK29622@ZenIV.linux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.7.1 (2016-10-04) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2770 Lines: 57 On Wed, Mar 29, 2017 at 04:43:05PM -0700, Linus Torvalds wrote: > > As for __copy_in_user()... I'm not sure we want to keep it in the long run - > > I agree, it's probably not worth it at all. > > In fact, I suspect none of the "__copy_.*_user()" versions are worth > it, and we should strive to remove them. > > There aren't even that many users, and they _have_ caused security > issues when people have had some path that hasn't checked the range. Actually, looking through those users shows some very odd places: for example, sctp_setsockopt_bindx() does /* Check the user passed a healthy pointer. */ if (unlikely(!access_ok(VERIFY_READ, addrs, addrs_size))) return -EFAULT; /* Alloc space for the address array in kernel memory. */ kaddrs = kmalloc(addrs_size, GFP_USER | __GFP_NOWARN); if (unlikely(!kaddrs)) return -ENOMEM; if (__copy_from_user(kaddrs, addrs, addrs_size)) { kfree(kaddrs); return -EFAULT; } The obvious question is "why not memdup_user()?" and rationale looks fishy: * We don't use copy_from_user() for optimization: we first do the * sanity checks (buffer size -fast- and access check-healthy * pointer); if all of those succeed, then we can alloc the memory * (expensive operation) needed to copy the data to kernel. Then we do * the copying without checking the user space area * (__copy_from_user()). plus that: sctp: use GFP_USER for user-controlled kmalloc Dmitry Vyukov reported that the user could trigger a kernel warning by using a large len value for getsockopt SCTP_GET_LOCAL_ADDRS, as that value directly affects the value used as a kmalloc() parameter. This patch thus switches the allocation flags from all user-controllable kmalloc size to GFP_USER to put some more restrictions on it and also disables the warn, as they are not necessary. First of all, access_ok() for sanity checks on size is BS - on some architectures it's constant 1 and on *all* architectures it allows a lot more than what kmalloc() will. So it won't stop a malicious program from getting to kmalloc() and wasting its cycles and it's not much help with buggy ones. __GFP_NOWARN part is more interesting, but... the quoted commit misses memdup_user() calls in other setsockopt cases on the same sctp. So if we care about that one, we probably should care about the rest of them as well, and I doubt that open-coding each is a good solution. Something like kvmemdup_user(), perhaps? I.e. quiet fallback to vmalloc on large sizes/in case when kmalloc barfs, with kvfree on the freeing side? Or just a flat-out check for some reasonably upper limit on optlen in the very beginning?