Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752651AbbGaBEF (ORCPT ); Thu, 30 Jul 2015 21:04:05 -0400 Received: from relay2-d.mail.gandi.net ([217.70.183.194]:57547 "EHLO relay2-d.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751153AbbGaBEC (ORCPT ); Thu, 30 Jul 2015 21:04:02 -0400 X-Greylist: delayed 24097 seconds by postgrey-1.27 at vger.kernel.org; Thu, 30 Jul 2015 21:04:01 EDT X-Originating-IP: 50.43.43.179 Date: Thu, 30 Jul 2015 18:03:44 -0700 From: Josh Triplett To: Kees Cook Cc: David Drysdale , Ingo Molnar , Linux API , Michael Kerrisk , Andrew Morton , Arnd Bergmann , Shuah Khan , Jonathan Corbet , Eric B Munson , Randy Dunlap , Andrea Arcangeli , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , Oleg Nesterov , Linus Torvalds , Greg Kroah-Hartman , Andy Lutomirski , Al Viro , Rusty Russell , Peter Zijlstra , Vivek Goyal , Alexei Starovoitov , David Herrmann , "Theodore Ts'o" , Milosz Tanski , Fam Zheng , Mathieu Desnoyers , "linux-doc@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Peter Zijlstra Subject: Re: [PATCHv2 1/1] Documentation: describe how to add a system call Message-ID: <20150731010344.GA8050@x> References: <1438242731-27756-1-git-send-email-drysdale@google.com> <1438242731-27756-2-git-send-email-drysdale@google.com> <20150730083831.GA22182@gmail.com> <20150730190434.GD16452@x> <20150731010234.GA7265@x> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150731010234.GA7265@x> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3135 Lines: 77 On Thu, Jul 30, 2015 at 06:02:34PM -0700, Josh Triplett wrote: > On Thu, Jul 30, 2015 at 01:03:43PM -0700, Kees Cook wrote: > > On Thu, Jul 30, 2015 at 12:04 PM, Josh Triplett wrote: > > > On Thu, Jul 30, 2015 at 11:21:54AM -0700, Kees Cook wrote: > > >> I like this, it's a good description of both options. I'm still biased > > >> about the approach: I prefer flags, since pointers to user structures > > >> complicate syscall filtering. ;) > > > > > > Seems like we should do two things to make that easier: > > > > > > 1) Create a standardized kernel mechanism for parameter-struct handling, > > > implementing the recommendations mentioned here. > > > > It's been suggested in the past that nlmsg is appropriate for such a > > thing, but I remain suspicious. :) > > Likewise. :) > > > > 2) Integrate into that mechanism a way to filter the resulting parameter > > > struct with BPF *after* it has been copied to kernel space (and thus > > > can no longer be tampered with). > > > > Yeah, this is a irritating part: the structures operated on are copied > > from userspace adhoc in each syscall. Doing argument checking would > > mean double copies initially, and perhaps teaching syscalls about > > optional "already copied" arguments or something as an optimization. > > No, double copies can't work for security reasons. Because otherwise > you could race the kernel from another thread, substituting different > values after the check and before the use. > > I think the right API looks *roughly* like this: > > int _copy_param_struct(size_t kernel_len, void *kernel_struct, size_t user_len, void __user *user_struct) > { > if (user_len > kernel_len) > return -EINVAL; > if (user_len && copy_from_user(kernel_struct, user_struct, user_len)) > return -EFAULT; > if (user_len < kernel_len) > memset(kernel_struct + user_len, 0, kernel_len - user_len); > return 0; > } > > #define copy_param_struct(kernel_struct, user_len, user_struct) _copy_param_struct( \ > sizeof(*kernel_struct) + BUILD_BUG_ON_ZERO(!__same_type(*kernel_struct, *user_struct)), \ > kernel_struct, user_len, user_struct) > > > Then the syscall looks like this: > > SYSCALL_DEFINEn(xyzzy, ..., ..., size_t user_params_len, struct xyzzy_params __user *user_params) Missed a couple of commas here (after the types and before the names). > { > int ret; > struct xyzzy_params params; > > ret = copy_param_struct(¶ms, user_params_len, user_params); > if (ret) > return ret; > ... > > > And you could then hook copy_params_struct to add arbitrary additional > syscall parameter validation. Bonus if there's some way to make the > copy and validation occur before the syscall is ever invoked, rather > than inside the syscall, but that would require adding fancier syscall > definition mechanisms that autogenerate such code. > > - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/