linux-kernel - Re: [PATCH 2/2] x86: Only do a single page fault for copy_from_user

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 3 Oct 2014 06:53:59 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	Andi Kleen <ak@...ux.intel.com>
Cc:	Peter Zijlstra <peterz@...radead.org>,
	Andi Kleen <andi@...stfloor.org>, dave@...1.net,
	linux-kernel@...r.kernel.org, eranian@...gle.com, x86@...nel.org,
	torvalds@...ux-foundation.org,
	Vitaly Mayatskikh <v.mayatskih@...il.com>
Subject: Re: [PATCH 2/2] x86: Only do a single page fault for
 copy_from_user_nmi


* Andi Kleen <ak@...ux.intel.com> wrote:

> > For now, changing the semantics of the function seems like a 
> > sure way to fail in the future though.
> 
> I doubt it. Nearly nobody uses the exact return value 
> semantics.
> 
> (iirc it's mostly write() and some bizarre code in mount)
> 
> In fact it's a regular mistake to assume it returns -errno.
> 
> > > In theory we could also duplicate the whole copy_*_ path 
> > > for cases where the caller doesn't care about the exact 
> > > bytes. But that seems overkill for just this issue, and I'm 
> > > not sure anyone else cares about how fast this is. The 
> > > simpler check works as well for now.
> > 
> > So I don't get that code, but why not fix it in general? 
> > Taking two faults seems silly.
> 
> It's really complicated to reconstruct the exact bytes, as an 
> optimized memcpy is very complicated and has a lot of corner 
> cases.
> 
> I tried it originally when writing the original copy function, 
> but failed. That is why people came up later with this 
> two-fault scheme.
> 
> I think two fault is fine for most cases, just not for NMIs.

Slapping an ugly in_nmi() check into a generic memcpy routine, 
especially if it changes semantics, is asking for trouble.
It is a non-starter and I'm NAK-ing it.

There are cleaner ways to solve this problem - PeterZ offered 
one, but there are other options as well, such as:

 - removing exact-bytes semantics explicitly from almost all 
   cases and offering a separate (and more expensive, in the 
   faulting case) memcpy variant for write() and other code that 
   absolutely must know the number of copied bytes.

 - or adding a special no-bytes-copied memcpy variant that the 
   NMI code could use.

Use one of these or outline that they cannot possibly work.

It might be more work for you, but it gives us a cleaner and more 
maintainable kernel. The problem is that you should know this 
general principle already, instead you are wasting maintainer 
bandwidth via arguing in favor of ugly hacks again and again...

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/