All of lore.kernel.org
 help / color / mirror / Atom feed
From: Leonid Yegoshin <Leonid.Yegoshin@imgtec.com>
To: James Hogan <james.hogan@imgtec.com>
Cc: <stable@vger.kernel.org>, Tom Herbert <therbert@google.com>,
	"Markos Chandras" <markos.chandras@imgtec.com>,
	Paul Burton <paul.burton@imgtec.com>, <linux-mips@linux-mips.org>,
	Ralf Baechle <ralf@linux-mips.org>
Subject: Re: [PATCH backport v3.15..v4.1 1/2] MIPS: uaccess: Take EVA into account in __copy_from_user()
Date: Mon, 4 Jan 2016 16:46:02 -0800	[thread overview]
Message-ID: <568B124A.7040305@imgtec.com> (raw)
In-Reply-To: <20160104222822.GJ17861@jhogan-linux.le.imgtec.org>

On 01/04/2016 02:28 PM, James Hogan wrote:
> Hi Leonid,
>
> On Mon, Jan 04, 2016 at 01:33:51PM -0800, Leonid Yegoshin wrote:
>> On 01/04/2016 12:29 PM, James Hogan wrote:
>>> Add the eva_kernel_access() check in __copy_from_user() like the one in
>>> copy_from_user().
> ...
>
>> Adding a user space check in __copy_from_user() kills the original
>> design.
> The original patch which did the same thing is already merged, so its a
> bit late to be arguing with it now.
>
> In any case, like other __ prefixed uaccess functions I believe the
> semantics are such that __copy_from_user() can be used instead of
> copy_from_user() to avoid multiple redundant access_ok() checks, since
> the caller can do it once before calling __copy_from_user().

... and it seems ridiculous that all net code uses copy_from*() besides 
one place which uses __copy_from_user_nocache() right after access_ok(). 
Note - there is no any saving because of splitting address verification 
into access_ok() from copy*() in this specific case.


>
> I have yet to see evidence or documentation suggesting that it was
> intended never to be used for kernel addresses, which would be
> inconsistent with copy_from_user and other __ uaccess functions which do
> handle them. Given the awkwardness of auditing whether some of these
> functions are ever called with kernel addresses, and the rate of code
> change in Linux, taking shortcuts with the semantics, even if possible
> to do at this moment, will only result in future code rot.

Well, there are cases then you know inside caller that address is kernel 
address space and wants just skip ds set/restore and access_ok(). But it 
is not a case of skb_do_copy_data_nocache().

- Leonid.

>
> Cheers
> James

WARNING: multiple messages have this Message-ID (diff)
From: Leonid Yegoshin <Leonid.Yegoshin@imgtec.com>
To: James Hogan <james.hogan@imgtec.com>
Cc: stable@vger.kernel.org, Tom Herbert <therbert@google.com>,
	Markos Chandras <markos.chandras@imgtec.com>,
	Paul Burton <paul.burton@imgtec.com>,
	linux-mips@linux-mips.org, Ralf Baechle <ralf@linux-mips.org>
Subject: Re: [PATCH backport v3.15..v4.1 1/2] MIPS: uaccess: Take EVA into account in __copy_from_user()
Date: Mon, 4 Jan 2016 16:46:02 -0800	[thread overview]
Message-ID: <568B124A.7040305@imgtec.com> (raw)
Message-ID: <20160105004602.L3MVqerF0MDKR2KUNAq8u_uacBI3K_8uNn450TEK448@z> (raw)
In-Reply-To: <20160104222822.GJ17861@jhogan-linux.le.imgtec.org>

On 01/04/2016 02:28 PM, James Hogan wrote:
> Hi Leonid,
>
> On Mon, Jan 04, 2016 at 01:33:51PM -0800, Leonid Yegoshin wrote:
>> On 01/04/2016 12:29 PM, James Hogan wrote:
>>> Add the eva_kernel_access() check in __copy_from_user() like the one in
>>> copy_from_user().
> ...
>
>> Adding a user space check in __copy_from_user() kills the original
>> design.
> The original patch which did the same thing is already merged, so its a
> bit late to be arguing with it now.
>
> In any case, like other __ prefixed uaccess functions I believe the
> semantics are such that __copy_from_user() can be used instead of
> copy_from_user() to avoid multiple redundant access_ok() checks, since
> the caller can do it once before calling __copy_from_user().

... and it seems ridiculous that all net code uses copy_from*() besides 
one place which uses __copy_from_user_nocache() right after access_ok(). 
Note - there is no any saving because of splitting address verification 
into access_ok() from copy*() in this specific case.


>
> I have yet to see evidence or documentation suggesting that it was
> intended never to be used for kernel addresses, which would be
> inconsistent with copy_from_user and other __ uaccess functions which do
> handle them. Given the awkwardness of auditing whether some of these
> functions are ever called with kernel addresses, and the rate of code
> change in Linux, taking shortcuts with the semantics, even if possible
> to do at this moment, will only result in future code rot.

Well, there are cases then you know inside caller that address is kernel 
address space and wants just skip ds set/restore and access_ok(). But it 
is not a case of skb_do_copy_data_nocache().

- Leonid.

>
> Cheers
> James

  reply	other threads:[~2016-01-05  0:46 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-01-04 20:29 [PATCH backport v3.15..v4.1 0/2] MIPS: uaccess: EVA fixes James Hogan
2016-01-04 20:29 ` James Hogan
2016-01-04 20:29 ` [PATCH backport v3.15..v4.1 1/2] MIPS: uaccess: Take EVA into account in __copy_from_user() James Hogan
2016-01-04 20:29   ` James Hogan
2016-01-04 21:33   ` Leonid Yegoshin
2016-01-04 21:33     ` Leonid Yegoshin
2016-01-04 22:28     ` James Hogan
2016-01-04 22:28       ` James Hogan
2016-01-05  0:46       ` Leonid Yegoshin [this message]
2016-01-05  0:46         ` Leonid Yegoshin
2016-01-04 20:29 ` [PATCH backport v3.15..v4.1 2/2] MIPS: uaccess: Take EVA into account in [__]clear_user James Hogan
2016-01-04 20:29   ` James Hogan
2016-01-15 17:52 ` [PATCH backport v3.15..v4.1 0/2] MIPS: uaccess: EVA fixes Luis Henriques
2016-01-15 17:52   ` Luis Henriques

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=568B124A.7040305@imgtec.com \
    --to=leonid.yegoshin@imgtec.com \
    --cc=james.hogan@imgtec.com \
    --cc=linux-mips@linux-mips.org \
    --cc=markos.chandras@imgtec.com \
    --cc=paul.burton@imgtec.com \
    --cc=ralf@linux-mips.org \
    --cc=stable@vger.kernel.org \
    --cc=therbert@google.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.