linux-kernel - RE: [PATCH] MIPS: Use __copy_{to,from}_user() for emulated FP loads/stores

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: <20191229190123.ju24cz7thuvybejs@lantea.localdomain>
Date:   Sun, 29 Dec 2019 11:01:23 -0800
From:   Paul Burton <paulburton@...nel.org>
To:     "Maciej W. Rozycki" <macro@...ux-mips.org>
Cc:     David Laight <David.Laight@...lab.com>,
        "linux-mips@...r.kernel.org" <linux-mips@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: RE: [PATCH] MIPS: Use __copy_{to,from}_user() for emulated FP
 loads/stores

Hi Maciej,

On Thu, Dec 26, 2019 at 03:01:06AM +0000, Maciej W. Rozycki wrote:
> On Wed, 4 Dec 2019, David Laight wrote:
> > > We used to have separate get_user_unaligned() & put_user_unaligned()
> > > which would suggest that it's expected that get_user() & put_user()
> > > require their accesses be aligned, but they were removed by commit
> > > 3170d8d226c2 ("kill {__,}{get,put}_user_unaligned()") in v4.13.
> > > 
> > > But perhaps we should just take the second AdEL exception & recover via
> > > the fixups table. We definitely don't right now... Needs further
> > > investigation...
> > 
> > get/put_user can fault because the user page is absent (etc).
> > So there must be code to 'expect' a fault on those instructions.
> 
>  As I recall we only emulate unaligned accesses with a subset of integer 
> load/store instructions (and then only if TIF_FIXADE is set, which is the 
> default), and never with FP load/store instructions.  Consequently I see 
> no point in doing this in the FP emulator either and I think these ought 
> to just send SIGBUS instead.  Otherwise you'll end up with user code that 
> works differently depending on whether the FP hardware is real or 
> emulated, which is really bad.

That might simplify things here, but it's incorrect. I'm fairly certain
the intent is that emulate_load_store_insn() handles all non-FP loads &
stores (though looking at it we're missing some instructions added in
r6). More importantly though we've been emulating FP loads & stores
since v3.10 which introduced the change alongside microMIPS support in
commit 102cedc32a6e ("MIPS: microMIPS: Floating point support."). The
commit contains no description of why, and I'm not aware of any reason
microMIPS specifically would need this so I suspect that commit bundled
this change for no good reason...

It's also worth noting that some hardware will handle unaligned FP
loads/stores, which means having the emulator reject them will result in
more of a visible difference to userland. ie. on some hardware they'll
work just fine, but on some you'd get SIGBUS. So I do think emulating
them makes some sense - just as for non-FP loads & stores it lets
userland not care whether the hardware will handle them, so long as it's
not performance critical code. If we knew that had never been used then
perhaps we could enforce the alignment requirement (and maybe that's
what you recall doing), but since we've been emulating them for the past
6 years it's too late for that now.

Thanks,
    Paul