From patchwork Fri Apr 4 08:18:24 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Borislav Petkov X-Patchwork-Id: 336874 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from huckleberry.canonical.com (huckleberry.canonical.com [91.189.94.19]) by ozlabs.org (Postfix) with ESMTP id 39F4314007E for ; Fri, 4 Apr 2014 19:18:46 +1100 (EST) Received: from localhost ([127.0.0.1] helo=huckleberry.canonical.com) by huckleberry.canonical.com with esmtp (Exim 4.76) (envelope-from ) id 1WVzKn-000187-Si; Fri, 04 Apr 2014 08:18:45 +0000 Received: from mail.skyhub.de ([78.46.96.112]) by huckleberry.canonical.com with esmtp (Exim 4.76) (envelope-from ) id 1WVzKi-00017s-CG for fwts-devel@lists.ubuntu.com; Fri, 04 Apr 2014 08:18:40 +0000 X-Virus-Scanned: Nedap ESD1 at mail.skyhub.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alien8.de; s=alien8; t=1396599520; bh=ABW6awC1I2f9TX4EvkK9zqfXUDRdrEQdCKFcSvyDCuQ=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:In-Reply-To; b=allMs3SZ2ZOqa7hv3Hh5lPBtN4OqGavmVYuirS QawFjyD65DAzcNY75TGpUM+mgR8gRzKyC7pu+xDz5Z2mFtzUppUGTRrUSYS/U8K7Ew2 UdvMUqONKxnlSBPXNlc4JYG7uDA4FbhhVO+SqVJDCKvcjeCHsNVavkfKAM11foEBfM= Received: from mail.skyhub.de ([127.0.0.1]) by localhost (door.skyhub.de [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id MHmMwVPAziuq; Fri, 4 Apr 2014 10:18:39 +0200 (CEST) Received: from liondog.tnic (p5DDC7525.dip0.t-ipconnect.de [93.220.117.37]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.skyhub.de (SuperMail on ZX Spectrum 128k) with ESMTPSA id B7C251DA234; Fri, 4 Apr 2014 10:18:38 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alien8.de; s=alien8; t=1396599519; bh=ABW6awC1I2f9TX4EvkK9zqfXUDRdrEQdCKFcSvyDCuQ=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:In-Reply-To; b=eawF7tAupwZpR3wh6UVNtTFWtI/+zboNIJVEvg zIDEnllFUB5Pk1ZGmQIjNdvUOnqlIZRb8yhb2mPAtzH9sfXHm/3tBOX+5BNKiHZbLV2 DksTaM3VylTVmVEeK4hZPbD4jbrVJE0cQhDrJ65++y4m7ulfilKTx+NKLylWML0564= Received: by liondog.tnic (Postfix, from userid 1000) id C98F5100B7A; Fri, 4 Apr 2014 10:18:24 +0200 (CEST) Date: Fri, 4 Apr 2014 10:18:24 +0200 From: Borislav Petkov To: Matt Fleming Subject: Re: [PATCH 4/4] efi_runtime: Do not pass user addresses to firmware Message-ID: <20140404081824.GE5047@pd.tnic> References: <1396535003-28253-1-git-send-email-matt@console-pimps.org> <1396535003-28253-5-git-send-email-matt@console-pimps.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1396535003-28253-5-git-send-email-matt@console-pimps.org> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: Matt Fleming , fwts-devel@lists.ubuntu.com X-BeenThere: fwts-devel@lists.ubuntu.com X-Mailman-Version: 2.1.14 Precedence: list List-Id: Firmware Test Suite Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: fwts-devel-bounces@lists.ubuntu.com Sender: fwts-devel-bounces@lists.ubuntu.com On Thu, Apr 03, 2014 at 03:23:23PM +0100, Matt Fleming wrote: > From: Matt Fleming > > Currently there's some inconsistency with how arguments are passed to > the firmware from the efi_runtime driver. Some values have the standard > get_user()/put_user() calls, others do not. > > Passing userspace pointers directly to the firmware is a bug because > those addresses may fault. And if they are going to fault we'd like to > know about it in the kernel rather than at some later time when > executing in firmware context. > > Furthermore, beginning with v3.14 of the kernel the current tests > actually cause the kernel to crash because firmware calls are now done > with their own, entirely separate, page tables - no user addresses are > mapped during execution of runtime services. > > This change doesn't require predication on a particular kernel version > because the efi_runtime should really have always done this copying > to/from userspace for every argument of the runtime services. > > This patch is heavily based on one from Borislav. > > Cc: Borislav Petkov > Signed-off-by: Matt Fleming > --- > efi_runtime/efi_runtime.c | 239 +++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 215 insertions(+), 24 deletions(-) > > diff --git a/efi_runtime/efi_runtime.c b/efi_runtime/efi_runtime.c > index ffe107341470..d90d24eb151b 100644 > --- a/efi_runtime/efi_runtime.c > +++ b/efi_runtime/efi_runtime.c > @@ -24,7 +24,7 @@ > #include > #include > #include > - > +#include > #include > > #include "efi_runtime.h" > @@ -100,6 +100,110 @@ static void convert_to_guid(efi_guid_t *vendor, EFI_GUID *vendor_guid) > vendor_guid->Data4[i] = vendor->b[i+8]; > } > > +/* > + * Count the bytes in 'str', including the terminating NULL. > + * > + * Note this function returns the number of *bytes*, not the number of > + * ucs2 characters. > + */ > +static inline size_t __strsize(uint16_t *str) > +{ > + uint16_t *s = str; > + size_t len; > + > + if (!str) > + return 0; > + > + /* Include terminating NULL */ > + len = sizeof(uint16_t); > + > + while (*s++ != 0) > + len += sizeof(uint16_t); > + > + return len; > +} > + > +/* > + * Copy a ucs2 string from user space into a newly allocated kernel > + * buffer. > + * > + * We take an explicit number of bytes to copy, and therefore do not > + * make any assumptions about 'src' (such as it being a valid string). > + */ > +static inline int > +get_ucs2_len(uint16_t **dst, uint16_t __user *src, size_t len) Just to show what I mean when we were talking on IRC: Since the caller needs to free *dst, the comment over the function should state that, the very least. Then, we probably could call those functions something like copy_uc2_to_user() and so on, to state that we're shuffling data to and fro userspace and have the get/put_ucs2 names for allocating/freeing buffers. I'm not crazy about this version either (ontop of this patch) but let me put it out there, just in case: diff --git a/efi_runtime/efi_runtime.c b/efi_runtime/efi_runtime.c index d90d24eb151b..82613df86962 100644 --- a/efi_runtime/efi_runtime.c +++ b/efi_runtime/efi_runtime.c @@ -129,9 +129,11 @@ static inline size_t __strsize(uint16_t *str) * * We take an explicit number of bytes to copy, and therefore do not * make any assumptions about 'src' (such as it being a valid string). + * + * After caller is done with *dst, it should do put_ucs2(dst); */ static inline int -get_ucs2_len(uint16_t **dst, uint16_t __user *src, size_t len) +copy_ucs2_from_user_len(uint16_t **dst, uint16_t __user *src, size_t len) { if (!src) { *dst = NULL; @@ -159,7 +161,7 @@ get_ucs2_len(uint16_t **dst, uint16_t __user *src, size_t len) * * If a non-zero value is returned, the caller MUST NOT access 'dst'. */ -static inline int get_ucs2(uint16_t **dst, uint16_t __user *src) +static inline int copy_ucs2_from_user(uint16_t **dst, uint16_t __user *src) { size_t len; @@ -167,7 +169,7 @@ static inline int get_ucs2(uint16_t **dst, uint16_t __user *src) return -EFAULT; len = __strsize(src); - return get_ucs2_len(dst, src, len); + return copy_ucs2_from_user_len(dst, src, len); } /* @@ -176,7 +178,7 @@ static inline int get_ucs2(uint16_t **dst, uint16_t __user *src) * 'len' specifies the number of bytes to copy. */ static inline int -put_ucs2_len(uint16_t *src, uint16_t __user *dst, size_t len) +copy_ucs2_to_user_len(uint16_t *src, uint16_t __user *dst, size_t len) { if (!src) return 0; @@ -193,7 +195,7 @@ put_ucs2_len(uint16_t *src, uint16_t __user *dst, size_t len) * We calculate the number of bytes to write from the ucs2 string 'src', * including the terminating NUL. */ -static inline int put_ucs2(uint16_t *src, uint16_t __user *dst) +static inline int copy_ucs2_to_user(uint16_t *src, uint16_t __user *dst) { size_t len; @@ -201,7 +203,12 @@ static inline int put_ucs2(uint16_t *src, uint16_t __user *dst) return -EFAULT; len = __strsize(src); - return put_ucs2_len(src, dst, len); + return copy_ucs2_to_user_len(src, dst, len); +} + +static inline void put_ucs2(uint16_t *name) +{ + kfree(name); } static long efi_runtime_get_variable(unsigned long arg) @@ -225,19 +232,19 @@ static long efi_runtime_get_variable(unsigned long arg) convert_from_guid(&vendor, &vendor_guid); - rv = get_ucs2(&name, pgetvariable->VariableName); + rv = copy_ucs2_from_user(&name, pgetvariable->VariableName); if (rv) return rv; data = kmalloc(datasize, GFP_KERNEL); if (!data) { - kfree(name); + put_ucs2(name); return -ENOMEM; } status = efi.get_variable(name, &vendor, &attr, &datasize, data); - kfree(name); + put_ucs2(name); rv = copy_to_user(pgetvariable->Data, data, datasize); kfree(data); @@ -281,20 +288,20 @@ static long efi_runtime_set_variable(unsigned long arg) convert_from_guid(&vendor, &vendor_guid); - rv = get_ucs2(&name, psetvariable->VariableName); + rv = copy_ucs2_from_user(&name, psetvariable->VariableName); if (rv) return rv; data = kmalloc(datasize, GFP_KERNEL); if (copy_from_user(data, psetvariable->Data, datasize)) { - kfree(name); + put_ucs2(name); return -EFAULT; } status = efi.set_variable(name, &vendor, attr, datasize, data); kfree(data); - kfree(name); + put_ucs2(name); if (put_user(status, psetvariable->status)) return -EFAULT; @@ -424,14 +431,16 @@ static long efi_runtime_get_nextvariablename(unsigned long arg) convert_from_guid(&vendor, &vendor_guid); - rv = get_ucs2_len(&name, pgetnextvariablename->VariableName, 1024); + rv = copy_ucs2_from_user_len(&name, pgetnextvariablename->VariableName, + 1024); if (rv) return rv; status = efi.get_next_variable(&name_size, name, &vendor); - rv = put_ucs2_len(name, pgetnextvariablename->VariableName, name_size); - kfree(name); + rv = copy_ucs2_to_user_len(name, pgetnextvariablename->VariableName, + name_size); + put_ucs2(name); if (rv) return -EFAULT;