All of lore.kernel.org
 help / color / mirror / Atom feed
From: Will Deacon <will.deacon@arm.com>
To: Mark Rutland <mark.rutland@arm.com>
Cc: linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	kernel-hardening@lists.openwall.com,
	Catalin Marinas <catalin.marinas@arm.com>,
	Kees Cook <keescook@chromium.org>,
	Laura Abbott <labbott@redhat.com>
Subject: Re: [RFC PATCH 0/2] arm64: optional paranoid __{get,put}_user checks
Date: Fri, 27 Oct 2017 16:41:13 +0100	[thread overview]
Message-ID: <20171027154113.GA13737@arm.com> (raw)
In-Reply-To: <20171026090942.7041-1-mark.rutland@arm.com>

On Thu, Oct 26, 2017 at 10:09:40AM +0100, Mark Rutland wrote:
> Hi,
> 
> In Prague, Kees mentioned that it would be nice to have a mechanism to
> catch bad __{get,put}_user uses, such as the recent CVE-2017-5123 [1,2]
> issue with unsafe_put_user() in waitid().
> 
> These patches allow an optional access_ok() check to be dropped in
> arm64's __{get,put}_user() primitives. These will then BUG() if a bad
> user pointer is passed (which should only happen in the absence of an
> earlier access_ok() check).
> 
> The first patch rewrites the arm64 access_ok() check in C. This gives
> the compiler the visibility it needs to elide redundant access_ok()
> checks, so in the common case:
> 
>   get_user()
>     access_ok()
>     __get_user()
>       BUG_ON(!access_ok())
>       <uaccess asm>
> 
> ... the compiler can determine that the second access_ok() must return
> true, and can elide it along with the BUG_ON(), leaving:
> 
>   get_user()
>     access_ok()
>       __get_user()
>         <uaccess asm>
> 
> ... and thus this sanity check can have no cost in the common case.

Probably a stupid question, but why not just move the access_ok check
into __{get,put}_user and remove it from {get,put}_user? We can also
then move the uaccess_{enable,disable}_not_uao calls out from the __
variants so that we can implement user_access_{begin,end}.

Will

WARNING: multiple messages have this Message-ID (diff)
From: will.deacon@arm.com (Will Deacon)
To: linux-arm-kernel@lists.infradead.org
Subject: [RFC PATCH 0/2] arm64: optional paranoid __{get,put}_user checks
Date: Fri, 27 Oct 2017 16:41:13 +0100	[thread overview]
Message-ID: <20171027154113.GA13737@arm.com> (raw)
In-Reply-To: <20171026090942.7041-1-mark.rutland@arm.com>

On Thu, Oct 26, 2017 at 10:09:40AM +0100, Mark Rutland wrote:
> Hi,
> 
> In Prague, Kees mentioned that it would be nice to have a mechanism to
> catch bad __{get,put}_user uses, such as the recent CVE-2017-5123 [1,2]
> issue with unsafe_put_user() in waitid().
> 
> These patches allow an optional access_ok() check to be dropped in
> arm64's __{get,put}_user() primitives. These will then BUG() if a bad
> user pointer is passed (which should only happen in the absence of an
> earlier access_ok() check).
> 
> The first patch rewrites the arm64 access_ok() check in C. This gives
> the compiler the visibility it needs to elide redundant access_ok()
> checks, so in the common case:
> 
>   get_user()
>     access_ok()
>     __get_user()
>       BUG_ON(!access_ok())
>       <uaccess asm>
> 
> ... the compiler can determine that the second access_ok() must return
> true, and can elide it along with the BUG_ON(), leaving:
> 
>   get_user()
>     access_ok()
>       __get_user()
>         <uaccess asm>
> 
> ... and thus this sanity check can have no cost in the common case.

Probably a stupid question, but why not just move the access_ok check
into __{get,put}_user and remove it from {get,put}_user? We can also
then move the uaccess_{enable,disable}_not_uao calls out from the __
variants so that we can implement user_access_{begin,end}.

Will

WARNING: multiple messages have this Message-ID (diff)
From: Will Deacon <will.deacon@arm.com>
To: Mark Rutland <mark.rutland@arm.com>
Cc: linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	kernel-hardening@lists.openwall.com,
	Catalin Marinas <catalin.marinas@arm.com>,
	Kees Cook <keescook@chromium.org>,
	Laura Abbott <labbott@redhat.com>
Subject: [kernel-hardening] Re: [RFC PATCH 0/2] arm64: optional paranoid __{get,put}_user checks
Date: Fri, 27 Oct 2017 16:41:13 +0100	[thread overview]
Message-ID: <20171027154113.GA13737@arm.com> (raw)
In-Reply-To: <20171026090942.7041-1-mark.rutland@arm.com>

On Thu, Oct 26, 2017 at 10:09:40AM +0100, Mark Rutland wrote:
> Hi,
> 
> In Prague, Kees mentioned that it would be nice to have a mechanism to
> catch bad __{get,put}_user uses, such as the recent CVE-2017-5123 [1,2]
> issue with unsafe_put_user() in waitid().
> 
> These patches allow an optional access_ok() check to be dropped in
> arm64's __{get,put}_user() primitives. These will then BUG() if a bad
> user pointer is passed (which should only happen in the absence of an
> earlier access_ok() check).
> 
> The first patch rewrites the arm64 access_ok() check in C. This gives
> the compiler the visibility it needs to elide redundant access_ok()
> checks, so in the common case:
> 
>   get_user()
>     access_ok()
>     __get_user()
>       BUG_ON(!access_ok())
>       <uaccess asm>
> 
> ... the compiler can determine that the second access_ok() must return
> true, and can elide it along with the BUG_ON(), leaving:
> 
>   get_user()
>     access_ok()
>       __get_user()
>         <uaccess asm>
> 
> ... and thus this sanity check can have no cost in the common case.

Probably a stupid question, but why not just move the access_ok check
into __{get,put}_user and remove it from {get,put}_user? We can also
then move the uaccess_{enable,disable}_not_uao calls out from the __
variants so that we can implement user_access_{begin,end}.

Will

  parent reply	other threads:[~2017-10-27 15:41 UTC|newest]

Thread overview: 63+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-10-26  9:09 [RFC PATCH 0/2] arm64: optional paranoid __{get,put}_user checks Mark Rutland
2017-10-26  9:09 ` [kernel-hardening] " Mark Rutland
2017-10-26  9:09 ` Mark Rutland
2017-10-26  9:09 ` [RFC PATCH 1/2] arm64: write __range_ok() in C Mark Rutland
2017-10-26  9:09   ` [kernel-hardening] " Mark Rutland
2017-10-26  9:09   ` Mark Rutland
2017-11-16 15:28   ` Will Deacon
2017-11-16 15:28     ` [kernel-hardening] " Will Deacon
2017-11-16 15:28     ` Will Deacon
2017-11-20 12:22     ` Mark Rutland
2017-11-20 12:22       ` [kernel-hardening] " Mark Rutland
2017-11-20 12:22       ` Mark Rutland
2017-10-26  9:09 ` [RFC PATCH 2/2] arm64: allow paranoid __{get,put}user Mark Rutland
2017-10-26  9:09   ` [kernel-hardening] " Mark Rutland
2017-10-26  9:09   ` Mark Rutland
2017-10-27 15:41 ` Will Deacon [this message]
2017-10-27 15:41   ` [kernel-hardening] Re: [RFC PATCH 0/2] arm64: optional paranoid __{get,put}_user checks Will Deacon
2017-10-27 15:41   ` Will Deacon
2017-10-27 20:44   ` Mark Rutland
2017-10-27 20:44     ` [kernel-hardening] " Mark Rutland
2017-10-27 20:44     ` Mark Rutland
2017-10-28  8:47   ` Russell King - ARM Linux
2017-10-28  8:47     ` [kernel-hardening] " Russell King - ARM Linux
2017-10-28  8:47     ` Russell King - ARM Linux
2017-10-31 23:56 ` Laura Abbott
2017-10-31 23:56   ` [kernel-hardening] " Laura Abbott
2017-10-31 23:56   ` Laura Abbott
2017-11-01 12:05   ` Mark Rutland
2017-11-01 12:05     ` [kernel-hardening] " Mark Rutland
2017-11-01 12:05     ` Mark Rutland
2017-11-01 21:13     ` Laura Abbott
2017-11-01 21:13       ` [kernel-hardening] " Laura Abbott
2017-11-01 21:13       ` Laura Abbott
2017-11-01 22:28       ` Kees Cook
2017-11-01 22:28         ` [kernel-hardening] " Kees Cook
2017-11-01 22:28         ` Kees Cook
2017-11-01 23:05         ` Laura Abbott
2017-11-01 23:05           ` [kernel-hardening] " Laura Abbott
2017-11-01 23:05           ` Laura Abbott
2017-11-01 23:29           ` Kees Cook
2017-11-01 23:29             ` [kernel-hardening] " Kees Cook
2017-11-01 23:29             ` Kees Cook
2017-11-02  1:25             ` Laura Abbott
2017-11-02  1:25               ` [kernel-hardening] " Laura Abbott
2017-11-02  1:25               ` Laura Abbott
2017-11-03 23:04 ` [RFC PATCH 1/2] x86: Avoid multiple evaluations in __{get,put}_user_size Laura Abbott
2017-11-03 23:04   ` [kernel-hardening] " Laura Abbott
2017-11-03 23:04   ` [RFC PATCH 2/2] x86: Allow paranoid __{get,put}_user Laura Abbott
2017-11-03 23:04     ` [kernel-hardening] " Laura Abbott
2017-11-04  0:14     ` Kees Cook
2017-11-04  0:14       ` [kernel-hardening] " Kees Cook
2017-11-04  0:24       ` Al Viro
2017-11-04  0:24         ` [kernel-hardening] " Al Viro
2017-11-04  0:44         ` Al Viro
2017-11-04  0:44           ` [kernel-hardening] " Al Viro
2017-11-04  1:39         ` Kees Cook
2017-11-04  1:39           ` [kernel-hardening] " Kees Cook
2017-11-04  1:41           ` Kees Cook
2017-11-04  1:41             ` [kernel-hardening] " Kees Cook
2017-11-04  1:58         ` Mark Rutland
2017-11-04  1:58           ` [kernel-hardening] " Mark Rutland
2017-11-06 20:38       ` Laura Abbott
2017-11-06 20:38         ` [kernel-hardening] " Laura Abbott

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20171027154113.GA13737@arm.com \
    --to=will.deacon@arm.com \
    --cc=catalin.marinas@arm.com \
    --cc=keescook@chromium.org \
    --cc=kernel-hardening@lists.openwall.com \
    --cc=labbott@redhat.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.