[x86] copy_from{to}_user question

[x86] copy_from{to}_user question

[yalin wang]

hi x86 maintainers,

i have a question about copy_from{to}_user() function,
i find on other platforms like arm/ arm64 /hexagon,
all copy_from{to}_user function only check source address for
copy_from and only check to address for copy_to user function,
never check both source and dest together,

but on x86 platform, i see copy_from{to}_user use a generic function
named copy_user_generic_unrolled() in arch/x86/lib/copy_user_64.S,

it check source and dest address no matter it is copy_from user or
copy_to_user ,  is it correct? 
for copy_from_user i think only need check source address is enough,
if check both address, may hide some kernel BUG, if the kernel address
is not valid, because the fix up code will fix it and kernel will
not panic in this situation.

another problems is that in ./fs/proc/kcore.c ,
read_kcore() function:


if (kern_addr_valid(start)) {
          unsigned long n;
  
          n = copy_to_user(buffer, (char *)start, tsz);
          /*                                                                                                                                                                                               
          ¦* We cannot distinguish between fault on source
          ¦* and fault on destination. When this happens
          ¦* we clear too and hope it will trigger the
          ¦* EFAULT again.
          ¦*/ 
          if (n) { 
                  if (clear_user(buffer + tsz - n,
                                          n)) 
                          return -EFAULT;
          }   
  } else {
          if (clear_user(buffer, tsz))
                  return -EFAULT;
  }

it relies on copy_to_user() can fault on both user and kernel address,
it is not true on arm / arm64 /hexgon platforms, maybe some other platforms,
i don’t check all platform code.
and this code may result in kernel panic on these platforms.

i think x86’s copy_from{to}_user code need to change like other platforms.
or am i missing something ?

Thanks

Re: [x86] copy_from{to}_user question

[Borislav Petkov]

On Wed, Aug 12, 2015 at 05:01:14PM +0800, yalin wang wrote:
> hi x86 maintainers,
> 
> i have a question about copy_from{to}_user() function,
> i find on other platforms like arm/ arm64 /hexagon,
> all copy_from{to}_user function only check source address for
> copy_from and only check to address for copy_to user function,
> never check both source and dest together,
> 
> but on x86 platform, i see copy_from{to}_user use a generic function
> named copy_user_generic_unrolled() in arch/x86/lib/copy_user_64.S,

That one is the fallback and used only on machines which don't set
X86_FEATURE_REP_GOOD or X86_FEATURE_ERMS. Basically old P4 and K7 and
early K8s.

> it check source and dest address no matter it is copy_from user or
> copy_to_user ,  is it correct? 
> for copy_from_user i think only need check source address is enough,

How else would we be able to use the same function in copy_to and
copy_from variants?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [x86] copy_from{to}_user question

[yalin wang]

> On Aug 12, 2015, at 18:07, Borislav Petkov <bp@suse.de> wrote:
> 
> On Wed, Aug 12, 2015 at 05:01:14PM +0800, yalin wang wrote:
>> hi x86 maintainers,
>> 
>> i have a question about copy_from{to}_user() function,
>> i find on other platforms like arm/ arm64 /hexagon,
>> all copy_from{to}_user function only check source address for
>> copy_from and only check to address for copy_to user function,
>> never check both source and dest together,
>> 
>> but on x86 platform, i see copy_from{to}_user use a generic function
>> named copy_user_generic_unrolled() in arch/x86/lib/copy_user_64.S,
> 
> That one is the fallback and used only on machines which don't set
> X86_FEATURE_REP_GOOD or X86_FEATURE_ERMS. Basically old P4 and K7 and
> early K8s.
> 
i see, generically, it use 3 function for different processors,

static __always_inline __must_check unsigned long
copy_user_generic(void *to, const void *from, unsigned len)
{
	unsigned ret;

	/*
	 * If CPU has ERMS feature, use copy_user_enhanced_fast_string.
	 * Otherwise, if CPU has rep_good feature, use copy_user_generic_string.
	 * Otherwise, use copy_user_generic_unrolled.
	 */
	alternative_call_2(copy_user_generic_unrolled,
			 copy_user_generic_string,
			 X86_FEATURE_REP_GOOD,
			 copy_user_enhanced_fast_string,
			 X86_FEATURE_ERMS,
			 ASM_OUTPUT2("=a" (ret), "=D" (to), "=S" (from),
				     "=d" (len)),
			 "1" (to), "2" (from), "3" (len)
			 : "memory", "rcx", "r8", "r9", "r10", "r11");
	return ret;
}

>> it check source and dest address no matter it is copy_from user or
>> copy_to_user ,  is it correct? 
>> for copy_from_user i think only need check source address is enough,
> 
> How else would we be able to use the same function in copy_to and
> copy_from variants?

for 3 methods implemented here, i think can implemented by add one more function parameter,
like this:
#define COPY_FROM 0
#define COPY_TO 1
#define COPY_IN 2
copy_user_generic(void *to, const void *from, unsigned len, int type)

we store type into one fix register, for example r12 ,
then in fix up code, we can know the exception is caused by copy_from
copy_to or copy_in user function by check r12 value(0 , 1 ,2 value), then if 
it is copy_from, we only allow read fault, if the exception is write fault, panic() .

the same rules also apply to copy_to / copy_in function .

is it possible to change it like this ?

Thanks

Re: [x86] copy_from{to}_user question

[Borislav Petkov]

On Thu, Aug 13, 2015 at 06:04:54PM +0800, yalin wang wrote:
> we store type into one fix register, for example r12 ,
> then in fix up code, we can know the exception is caused by copy_from
> copy_to or copy_in user function by check r12 value(0 , 1 ,2 value), then if 
> it is copy_from, we only allow read fault, if the exception is write fault, panic() .
> 
> the same rules also apply to copy_to / copy_in function .
> 
> is it possible to change it like this ?

... and we'll do all that jumping through hoops to fix what actual,
real-life problem exactly?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [x86] copy_from{to}_user question

[yalin wang]

> On Aug 14, 2015, at 00:43, Borislav Petkov <bp@suse.de> wrote:
> 
> On Thu, Aug 13, 2015 at 06:04:54PM +0800, yalin wang wrote:
>> we store type into one fix register, for example r12 ,
>> then in fix up code, we can know the exception is caused by copy_from
>> copy_to or copy_in user function by check r12 value(0 , 1 ,2 value), then if 
>> it is copy_from, we only allow read fault, if the exception is write fault, panic() .
>> 
>> the same rules also apply to copy_to / copy_in function .
>> 
>> is it possible to change it like this ?
> 
> ... and we'll do all that jumping through hoops to fix what actual,
> real-life problem exactly?
i just want the x86 copy_from{to,in}_user() function have 
the same behaviour as other platforms.
and can disclose potential BUGs in kernel, if do like this.

Thanks.

Re: [x86] copy_from{to}_user question

[Borislav Petkov]

On Mon, Aug 17, 2015 at 11:27:01AM +0800, yalin wang wrote:
> i just want the x86 copy_from{to,in}_user() function have 
> the same behaviour as other platforms.

Back to the original question from 2 mails ago:

How else would we be able to use the same function in copy_to and
copy_from variants?

> and can disclose potential BUGs in kernel, if do like this.

Back to my other question:

Do you have any real life examples where you can trigger such bugs or is
this only "potential"?

IOW, what I *think* you're trying to do sounds to me like unnecessary
complication with no apparent gain *at* *all*. So show me why you want
to do it: code it up, trigger a bug and show me why your version is
better. No "but but it might be a good idea", no "potentially maybe",
none of that maybe stuff. Write it, send it with instructions how
someone else can apply it and trigger the issue. Ok?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--

Re: [x86] copy_from{to}_user question

[yalin wang]

> On Aug 17, 2015, at 12:16, Borislav Petkov <bp@suse.de> wrote:
> 
> On Mon, Aug 17, 2015 at 11:27:01AM +0800, yalin wang wrote:
>> i just want the x86 copy_from{to,in}_user() function have 
>> the same behaviour as other platforms.
> 
> Back to the original question from 2 mails ago:
> 
> How else would we be able to use the same function in copy_to and
> copy_from variants?
> 
>> and can disclose potential BUGs in kernel, if do like this.
> 
> Back to my other question:
> 
> Do you have any real life examples where you can trigger such bugs or is
> this only "potential"?
> 
> IOW, what I *think* you're trying to do sounds to me like unnecessary
> complication with no apparent gain *at* *all*. So show me why you want
> to do it: code it up, trigger a bug and show me why your version is
> better. No "but but it might be a good idea", no "potentially maybe",
> none of that maybe stuff. Write it, send it with instructions how
> someone else can apply it and trigger the issue. Ok?
> 
> 
i will loop you in a patch mail, for kcore driver rely on x86 copy_to_user()
feature like this , but not true on other platforms .

Re: [x86] copy_from{to}_user question

[H. Peter Anvin]

On 08/16/2015 09:16 PM, Borislav Petkov wrote:
> On Mon, Aug 17, 2015 at 11:27:01AM +0800, yalin wang wrote:
>> i just want the x86 copy_from{to,in}_user() function have 
>> the same behaviour as other platforms.
> 
> Back to the original question from 2 mails ago:
> 
> How else would we be able to use the same function in copy_to and
> copy_from variants?
> 
>> and can disclose potential BUGs in kernel, if do like this.
> 
> Back to my other question:
> 
> Do you have any real life examples where you can trigger such bugs or is
> this only "potential"?
> 
> IOW, what I *think* you're trying to do sounds to me like unnecessary
> complication with no apparent gain *at* *all*. So show me why you want
> to do it: code it up, trigger a bug and show me why your version is
> better. No "but but it might be a good idea", no "potentially maybe",
> none of that maybe stuff. Write it, send it with instructions how
> someone else can apply it and trigger the issue. Ok?
> 

There is a valid reason to do this, which is that currently
copy_{to,from}_user() effectively bypass SMAP as they don't verify that
the kernel pointer is actually a kernel pointer.

The /proc/kcore issue is a completely different ball of wax, however.

	-hpa

Re: [x86] copy_from{to}_user question

[Borislav Petkov]

On Thu, Aug 20, 2015 at 11:22:43AM -0700, H. Peter Anvin wrote:
> There is a valid reason to do this, which is that currently
> copy_{to,from}_user() effectively bypass SMAP as they don't verify that
> the kernel pointer is actually a kernel pointer.

Well, we do STAC before we copy but SMAP is checking for supervisor
access to *user* data. But you say "kernel pointers" which is supervisor
data. What am I missing?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--

Re: [x86] copy_from{to}_user question

[H. Peter Anvin]

On 08/20/2015 09:35 PM, Borislav Petkov wrote:
> On Thu, Aug 20, 2015 at 11:22:43AM -0700, H. Peter Anvin wrote:
>> There is a valid reason to do this, which is that currently
>> copy_{to,from}_user() effectively bypass SMAP as they don't verify that
>> the kernel pointer is actually a kernel pointer.
> 
> Well, we do STAC before we copy but SMAP is checking for supervisor
> access to *user* data. But you say "kernel pointers" which is supervisor
> data. What am I missing?
> 

What I'm saying is that we do do STAC, which *disables* SMAP.  We have
to do that because one pointer is known to be a user space pointer.

However, we currently don't verify that the *other* pointer is kernel
space, which it is supposed to be (if not, we should be using
copy_in_user).  We have to do this manually since we have to STAC which
means SMAP doesn't do anything at all.  I believe it would be a good
idea to add such checks (and they would even benefit non-SMAP hardware.)

	-hpa

Re: [x86] copy_from{to}_user question

[Borislav Petkov]

On Fri, Aug 21, 2015 at 02:06:16PM -0700, H. Peter Anvin wrote:
> What I'm saying is that we do do STAC, which *disables* SMAP.  We have
> to do that because one pointer is known to be a user space pointer.
> 
> However, we currently don't verify that the *other* pointer is kernel
> space, which it is supposed to be (if not, we should be using
> copy_in_user).  We have to do this manually since we have to STAC which
> means SMAP doesn't do anything at all.  I believe it would be a good
> idea to add such checks (and they would even benefit non-SMAP hardware.)

Ah, ok, so we're on the same page.

And yep, Linus gave the probe_kernel_read() suggestion in another mail.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--

Re: [x86] copy_from{to}_user question

[yalin wang]

> On Aug 22, 2015, at 17:05, Borislav Petkov <bp@suse.de> wrote:
> 
> On Fri, Aug 21, 2015 at 02:06:16PM -0700, H. Peter Anvin wrote:
>> What I'm saying is that we do do STAC, which *disables* SMAP.  We have
>> to do that because one pointer is known to be a user space pointer.
>> 
>> However, we currently don't verify that the *other* pointer is kernel
>> space, which it is supposed to be (if not, we should be using
>> copy_in_user).  We have to do this manually since we have to STAC which
>> means SMAP doesn't do anything at all.  I believe it would be a good
>> idea to add such checks (and they would even benefit non-SMAP hardware.)
> 
> Ah, ok, so we're on the same page.
> 
> And yep, Linus gave the probe_kernel_read() suggestion in another mail.
> 
i am not clear about what is STAC / SMAP ?
could you give me a link for understanding ?

Linus suggest to use probe_kernel_read() , but also said it is 
not efficient to use it, because we need copy the data 2 times by this method.

my patch suggests to use copy_in_user() ,
but seems not a generic(portable) function on all architectures.

Thanks

Re: [x86] copy_from{to}_user question

[Jeff Epler]

On Mon, Aug 24, 2015 at 03:52:11PM +0800, yalin wang wrote:
> i am not clear about what is STAC / SMAP ?
> could you give me a link for understanding ?

the first item I found by googling was
https://lwn.net/Articles/517251/

Jeff