From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 06817C04EBF for ; Tue, 4 Dec 2018 08:43:00 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id AFA4520851 for ; Tue, 4 Dec 2018 08:42:59 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org AFA4520851 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ellerman.id.au Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725907AbeLDIm6 (ORCPT ); Tue, 4 Dec 2018 03:42:58 -0500 Received: from ozlabs.org ([203.11.71.1]:54587 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725747AbeLDIm5 (ORCPT ); Tue, 4 Dec 2018 03:42:57 -0500 Received: from authenticated.ozlabs.org (localhost [127.0.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPSA id 438Fk6451gz9s55; Tue, 4 Dec 2018 19:42:54 +1100 (AEDT) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ellerman.id.au From: Michael Ellerman To: Christophe Leroy , Kees Cook , Andrew Morton , Benjamin Herrenschmidt , Paul Mackerras Cc: linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-mm@kvack.org Subject: Re: [PATCH 1/2] mm: add probe_user_read() and probe_user_address() In-Reply-To: References: Date: Tue, 04 Dec 2018 19:42:52 +1100 Message-ID: <874lbtisxf.fsf@concordia.ellerman.id.au> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Christophe Leroy writes: > In the powerpc, there are several places implementing safe ^ code ? > access to user data. This is sometimes implemented using > probe_kernel_address() with additional access_ok() verification, > sometimes with get_user() enclosed in a pagefault_disable()/enable() > pair, etc... : > show_user_instructions() > bad_stack_expansion() > p9_hmi_special_emu() > fsl_pci_mcheck_exception() > read_user_stack_64() > read_user_stack_32() on PPC64 > read_user_stack_32() on PPC32 > power_pmu_bhrb_to() > > In the same spirit as probe_kernel_read() and probe_kernel_address(), > this patch adds probe_user_read() and probe_user_address(). > > probe_user_read() does the same as probe_kernel_read() but > first checks that it is really a user address. > > probe_user_address() is a shortcut to probe_user_read() ... > +#define probe_user_address(addr, retval) \ > + probe_user_read(&(retval), addr, sizeof(retval)) I realise you added probe_user_address() to mirror probe_kernel_address(), but I'd rather we just used probe_user_read() directly. The only advantage of probe_kernel_address() is that you don't have to mention retval twice. But the downsides are that it's not obvious that you're writing to retval (because the macro takes the address for you), and retval is evaluated twice (the latter is usually not a problem but it can be). eg, call sites like this are confusing: static int read_user_stack_64(unsigned long __user *ptr, unsigned long *ret) { ... if (!probe_user_address(ptr, *ret)) return 0; It's confusing because ret is a pointer, but then we dereference it before passing it to probe_user_address(), so it looks like we're just passing a value, but we're not. Compare to: if (!probe_user_read(ret, ptr, sizeof(*ret))) return 0; Which is entirely analogous to a call to memcpy() and involves no magic. I know there's lots of precedent here with get_user() etc. but that doesn't mean we have to follow that precedent blindly :) I guess perhaps we can add probe_user_address() but just not use it in the powerpc code, if other folks want it to exist. cheers