linux-kernel - Re: [PATCH v14] x86, mce: Add memcpy

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]

Message-ID: <20160313092505.GA11079@gmail.com>
Date:	Sun, 13 Mar 2016 10:25:05 +0100
From:	Ingo Molnar <mingo@...nel.org>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Tony Luck <tony.luck@...il.com>,
	Mika Penttilä <mika.penttila@...tfour.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Borislav Petkov <bp@...en8.de>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	"H. Peter Anvin" <hpa@...or.com>,
	Andy Lutomirski <luto@...capital.net>,
	"Williams, Dan J" <dan.j.williams@...el.com>
Subject: Re: [PATCH v14] x86, mce: Add memcpy_mcsafe()

* Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> On Sat, Mar 12, 2016 at 9:16 AM, Ingo Molnar <mingo@...nel.org> wrote:
> >
> > Please use the copy_*_user() memory copying API semantics, which are: return
> > negative code (-EFAULT) on error, 0 on success.
> 
> Those are the get_user/put_user() semantics.
> 
> copy_*_user() has those annoying "bytes left uncopied" return values
> that I really wouldn't encourage anybody else use unless they really
> have to. [...]

Indeed, and I even grepped for it because I tend to get it wrong ... I suck!

> [...]  There are (good) reasons why it's done that way, but it's still not 
> something that should be aped without the same kind of major reasons.

Yeah, so IIRC the main reason being security and the ability to know which bits of 
a possibly uninitialized area are not yet copied on.

But with primary memcpy_mcsafe() usage being in-kernel (MM)IO space management I 
don't think we have the 'uninitialized' problem in nearly as marked a fashion, and 
we very much have the (largely uncompensated) cost of more complex assembly.

Nevertheless, wrt. error handling, the bit I feel very strongly about is the 
following:

We have the existing 'mempcy API check for failure' pattern of:

	if (put_user()) {
		... it didn't succeed ...
	}

	if (copy_from_user()) {
		... it didn't succeed ...
	}

... which should be kept IMHO:

	if (memcpy_mcsafe()) {
		... it didn't succeed ...
	}

... and what I most disagree with most is one of the suggestions in the original 
mail I replied to:

    >> 2) Reverse the return values.

    (== return 1 on success, 0 on failure)

... that's crazy IMHO and reverses the pattern of almost every memcpy-with-check 
(or syscall-success) API we have!

So I still think we should have the regular '0 on success, negative error value on 
failure' semantics - but I'd not be hugely against a '0 on success, bytes left 
uncopied on failure' variant either in principle, except that I think it unduly 
complicates the assembly side to recover the bytes.

The '0/1' current implementation is really neither of these but still fits the 
common error checking pattern, but it's still much better than the '1/0' 
suggestion which is crazy!

So I think we should move from 0/1 to 0/-EFAULT for now, and maybe (maybe) move to 
a 0/bytes_left return code in the future, if there comes a strong usecase. Most 
error checks will have to be converted in that case anyway to make use of the 
return code, so it's not like we introduce an extra cost here by going to 
0/-EFAULT.

Also, by going to 0/-EFAULT we have the option to also allow other negative error 
codes, which would allow the distinction of #MC traps from regular page fault 
traps for example. (a driver might want to log them differently.)

Thanks,

	Ingo